-
Notifications
You must be signed in to change notification settings - Fork 29
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
bugfix/sla-investigations #136
Conversation
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fivetran-joemarkiewicz this looks great and nice job detailing all of it in depth in the changelog! The validations are really helpful as well. I want to comb through this one more time with a fresh brain tomorrow morning, but here are some notes I caught this first time around.
models/sla_policy/reply_time/int_zendesk__reply_time_business_hours.sql
Outdated
Show resolved
Hide resolved
Hey @fivetran-joemarkiewicz I was able to finish my review! This looks pretty good and the validations were very thorough 👍 . One suggestion for the changelog is it would help with navigating the changes if each bullet point was set up like 'Updated x model for the y metric / z logic', just so it's easier to track. |
Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>
Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>
Thanks for the suggestions @fivetran-reneeli! I just applied them and regenerated the docs. Re-requesting review. Let me know if there is anything else 😄 |
Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! just caught a small typo in the changelog but otherwise good to go
Thanks so much @fivetran-reneeli! I really appreciate the review and great notes for improving the documentation. Moving this forward in the release process! 🎉 |
PR Overview
This PR will address the following Issue/Feature: Issue #142, Issue #139, Issue #135, Issue #131, Issue #116
This PR will result in the following new package version:
v0.14.0
While this is not technically a breaking change. There are a number of changes in this update that will result in new and changed records. I would rather we make this breaking to highlight the large number of changes so users are aware of what changes will be applied and the impacts before upgrading.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Bug Fixes
zendesk__sla_policies
metric forsla_elapsed_time
to be reported in minutes to the second as opposed to just the nearest rounded minute. This ensures more accurate reporting.int_zendesk__reply_time_combined
model to additionally account for the following business hour scenarios as they were erroneously being filtered out in previous versions of the package:int_zendesk__ticket_schedules
model to more accurately select the active default schedule.sla_breach_at
within thezendesk__sla_policies
and upstream models for reply time SLAs. It was found this field was inconsistent with the actual breach/achieve time of an SLA. The overhaul should now ensure reply time SLA is accurate to either be the time of the SLA breach or achieve event.zendesk__sla_policies
model.Documentation Updates
first_reply_time
SLAs.PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
Please see the height ticket for validations
If you had to summarize this PR in an emoji, which would it be?
📆