Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add var to persist former-agents' roles for SLA metrics #43

Merged
merged 10 commits into from
Jan 10, 2024

Conversation

fivetran-jamie
Copy link
Contributor

@fivetran-jamie fivetran-jamie commented Dec 5, 2023

PR Overview

This PR will address the following Issue/Feature:
fivetran/dbt_zendesk#120 and #40

This PR will result in the following new package version:

v0.10.1 -- by default nothing will change for folks. they'll need to explicitly set the internal user criteria var

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

dbt_zendesk_source v0.10.1

PR #43 introduces the following updates:

Feature Updates

  • Added the internal_user_criteria variable, which can be used to mark internal users whose USER.role may have changed from agent to end-user after they left your organization. This variable accepts SQL that may reference any non-custom field in USER, and it will be wrapped in a case when statement in the stg_zendesk__user model.
    • Example usage:
# dbt_project.yml
vars:
  zendesk_source:
    internal_user_criteria: "lower(email) like '%@fivetran.com' or external_id = '12345' or name in ('Garrett', 'Alfredo')" # can reference any non-custom field in USER
  • Output: In stg_zendesk__user, users who match your criteria and have a role of end-user will have their role switched to agent. This will ensure that downstream SLA metrics are appropriately calculated.

Under the Hood

  • Updated the way we dynamically disable sources. Previously, we used a custom meta.is_enabled flag, but, since we added this, dbt-core introduced a native config.enabled attribute. We have opted to use the dbt-native config instead.
  • Updated the pull request templates.
  • Included auto-releaser GitHub Actions workflow to automate future releases.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

  • dbt run –full-refresh && dbt test
  • dbt run (if incremental models are present)

Before marking this PR as "ready for review" the following have been applied:

  • The appropriate issue has been linked, tagged, and properly assigned
  • All necessary documentation and version upgrades have been applied
  • docs were regenerated (unless this PR does not include any code or yml updates)
  • BuildKite integration tests are passing
  • Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Will validate internally with @jackiexsun

@fivetran-jamie fivetran-jamie self-assigned this Dec 5, 2023
CHANGELOG.md Outdated
```yml
# dbt_project.yml
vars:
zendesk__internal_user_criteria: "lower(email) like '%@fivetran.com' or external_id = 12345 or name in ('Garrett', 'Alfredo')" # can reference any non-custom field in USER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omg love this example!

@fivetran-jamie fivetran-jamie marked this pull request as ready for review December 7, 2023 18:51
### Add passthrough columns
This package includes all source columns defined in the staging models. However, the `stg_zendesk__ticket` model allows for additional columns to be added using a pass-through column variable. This is extremely useful if you'd like to include custom fields to the package.
```yml
vars:
zendesk__ticket_passthrough_columns: [account_custom_field_1, account_custom_field_2]
```

### Mark Former Internal Users as Agents
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder to myself to add this to the transform README (no release needed there tho) after approval

@fivetran-jamie fivetran-jamie changed the title docs Add var to persist former-agents' roles for SLA metrics Dec 7, 2023
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-jamie just a few comments around the changes. Once you reply to them and make any updates then let me know for re-review and I can check again and approve.

CHANGELOG.md Outdated
[PR #43](https://github.com/fivetran/dbt_zendesk_source/pull/43) introduces the following updates:

## Feature Updates
- Added the `internal_user_criteria` variable, which can be used to mark internal users whose `USER.role` may have changed from `agent` to `end-user` after they left your organization.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be relevant to include here that this is used as a case when clause in the staging model. Just so users know the context of the variable. The example shows this well, but I feel a bit of an explanation to this would be helpful to someone new to the concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a brief note!

README.md Outdated
### Mark Former Internal Users as Agents
If a team member leaves your organization and their internal account is deactivated, their `USER.role` will switch from `agent` or `admin` to `end-user`. This will skew historical ticket SLA metrics, as we calculate reply times and other metrics based on `agent` or `admin` activity only.

To persist the integrity of historical ticket SLAs and mark these former team members as agents, provide the `internal_user_criteria` variable with a SQL clause to identify them, based on fields in the `USER` table:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include a similar update to one I suggested in the CHANGELOG here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a little something

email,
name,
organization_id,
{% if var('internal_user_criteria', false) -%}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that false would work in this context. I thought it would need to be [] (or something like that). Are we sure this works on all dbt versions supported by the package? It may be worthwhile to test this on dbt 1.3.0 just to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just ran it on dbt-bigquery==1.3.0 with and without providing internal_user_criteria: "lower(email) like '%@fivetran.com'" and it works as expected 😄

yeah i think it works similarly to how we provide false (or true i suppose) as the default value for using_<possibly missing source table> variables

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-jamie these changes look good to me. I just have one small request to update the example snippet before moving the a release review as I ran into a small issue when trying to copy/paste the snippet provided.

Thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
fivetran-jamie and others added 3 commits January 9, 2024 12:04
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
@fivetran-jamie fivetran-jamie merged commit 75d9621 into main Jan 10, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants