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 new solve time metrics #123

Conversation

fivetran-avinash
Copy link
Contributor

@fivetran-avinash fivetran-avinash commented Nov 7, 2023

PR Overview

This PR will address the following Issue/Feature: #119

This PR will result in the following new package version: v0.12.1

We are adding new fields, but they should not impact existing customers and their data.

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

🚀 Feature Updates 🚀

  • Added solve_time_in_calendar_minutes and solve_time_in_business_minutes to our zendesk__ticket_metrics model, which calculates calendar and business minutes for when the ticket was in the 'new', 'open', 'hold', or 'pending' status.

🔎 Under the Hood 🔎

  • Updates to the seed files and seed file configurations for the package integration tests to align with changes introduced by the PR on dbt_zendesk_source in applying the dbt_utils.star macro PR #42.

PR Checklist

Basic Validation

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

  • dbt run –full-refresh && dbt test
  • [NA] 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:
(Sharing via Slack)

zendesk_integration_tests:
+quote_columns: "{{ true if target.type == 'redshift' else false }}"

Choose a reason for hiding this comment

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

why is there a condition on redshift?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears to be an artifact of our CircleCI integration. I'm not sure if we still need it but at the moment it isn't really blocking anything, so we'll discuss it internally. Thanks for spotting!

@@ -117,6 +117,8 @@ with ticket_historical_status as (
else 0 end as agent_wait_time_in_minutes,
case when ticket_status in ('new', 'open', 'hold') then scheduled_minutes
else 0 end as requester_wait_time_in_minutes,
case when ticket_status in ('new', 'open', 'hold', 'pending') then scheduled_minutes
else 0 end as solve_time_in_minutes,

Choose a reason for hiding this comment

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

looks good!

@@ -12,6 +12,8 @@ with ticket_historical_status as (
else 0 end as agent_wait_time_in_minutes,
case when status in ('new', 'open', 'hold') then status_duration_calendar_minutes
else 0 end as requester_wait_time_in_minutes,
case when status in ('new', 'open', 'hold', 'pending') then status_duration_calendar_minutes
else 0 end as solve_time_in_minutes,

Choose a reason for hiding this comment

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

can we go over why this is defined twice in two separate places? Is there an opportunity for us to follow DRY principles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @fivetran-jessicacherny ! These metrics are being constructed for two different models here--with schedules enabled in Zendesk, and without schedules.

  • The int_zendesk__ticket_work_time_calendar.solve_time_in_minutes field is when schedules are disabled (as I believe they are internally). This is the field being brought downstream into our internal models.
  • The int_zendesk__ticket_work_time_business.solve_time_in_minutes field is when schedules are enabled (as they are for many other customers). This field is not currently being brought downstream internally.

Let me know if that clears it up!

Choose a reason for hiding this comment

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

definitely does! thank you @fivetran-avinash!

@fivetran-avinash fivetran-avinash self-assigned this Nov 15, 2023
@fivetran-avinash fivetran-avinash linked an issue Nov 15, 2023 that may be closed by this pull request
4 tasks
@fivetran-avinash fivetran-avinash marked this pull request as ready for review November 20, 2023 16:16
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.

These changes look good to me! I do have a comment regarding the package-lock.yml being removed and possibly making these breaking changes. Other than that, this is good to go!

integration_tests/package-lock.yml Outdated Show resolved Hide resolved
integration_tests/requirements.txt Outdated Show resolved Hide resolved
package-lock.yml Outdated Show resolved Hide resolved
packages.yml Outdated Show resolved Hide resolved
packages.yml Outdated
Comment on lines 2 to 7
- git: https://github.com/fivetran/dbt_zendesk_source.git
revision: feature/dbt-utils-star
warn-unpinned: false
# - package: fivetran/zendesk_source
# version: [">=0.9.0", "<1.0.0"]

Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to switch before merge

CHANGELOG.md Outdated Show resolved Hide resolved
@fivetran-joemarkiewicz fivetran-joemarkiewicz changed the base branch from main to release/v0.12.1 November 27, 2023 22:43
@fivetran-joemarkiewicz fivetran-joemarkiewicz changed the base branch from release/v0.12.1 to release/v0.13.0 November 27, 2023 22:49
@fivetran-joemarkiewicz fivetran-joemarkiewicz mentioned this pull request Nov 27, 2023
6 tasks
@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit 0d216c6 into release/v0.13.0 Nov 28, 2023
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.

[Feature] New hours to solve measure
3 participants