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

Update filter in filtered reply times CTE in int_zendesk__reply_time_combined #134

Closed
wants to merge 12 commits into from

Conversation

fivetran-reneeli
Copy link
Contributor

@fivetran-reneeli fivetran-reneeli commented Jan 18, 2024

PR Overview

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

It was discovered that some tickets were not persisting through the final sla policies model, and mistakenly filtered out at the int_zendesk__reply_time_combined model.

This PR will result in the following new package version:

v0.13.2

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

Bug Fixes

  • Updated the int_zendesk__reply_time_combined model to account for when a ticket is first replied to outside SLA schedules or not yet replied to.

Under the Hood

  • Adjusted the filter used in the filtered_reply_times CTE for business hours to account for the aforementioned cases, as these records were mistakenly being filtered out previously.

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) && dbt test

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

  • tested with internal data, validations linked in height ticket

Detailed Validation

Please share any and all of your validation steps:

  • Confirmed the missing tickets were showing up prior to the int_zendesk__reply_time_combined model, and confirmed they are in fact expected to exist in the end models and not be filtered out. Then adjusted the filters in the filtered reply times CTE, ran the package, and confirmed that the tickets persisted in the end sla policies model.

If you had to summarize this PR in an emoji, which would it be?

📆

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-reneeli great job on working on this tricky bugfix! I do have a few questions included in my review that I would like you to take a look at before this will be ready to be approved. Let me know if you have any questions!

CHANGELOG.md Outdated Show resolved Hide resolved
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-reneeli thanks again for working through this PR. This is looking really good, I just have a few final questions and suggestions before this should be good to approve.

Let me know if you want to chat about any of these final comments in more detail.

CHANGELOG.md Outdated Show resolved Hide resolved
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.

Thanks for applying these changes @fivetran-reneeli! This looks good to go from my end. Be sure to regen the docs and then this is ready for release review.

@fivetran-joemarkiewicz
Copy link
Contributor

Consolidating this into PR #136

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