fix(int_zendesk__schedule_spine): offset holiday start and end time too

[cth84]

Please provide your name and company
Craig Homan - Beam Benefits

Link the issue/feature request which this PR is meant to address

#117

Detail what changes this PR introduces and how this addresses the issue/feature request linked above.

We now offset the start / end times as well for a holiday day, as well.

This now allows to better align to the schedule start / end times which have already been offset.

How did you validate the changes introduced within this PR?

I overwrote int_zendesk__schedule_spine in our local project, with these updates, and got the results we expected to see.

Obviously want someone to take a 'wider' look at it, but wanted to offer a jump start since I had been investigating it on our side and saw a potential fix for our narrowed situation and temporarily overwriting the model in our project worked.

Which warehouse did you use to develop these changes?

BigQuery

Did you update the CHANGELOG?

• Yes

Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)

• Yes

Provide an emoji that best describes your current mood

☕

Feedback

We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.

PR Template

• Community Pull Request Template (default)

• Maintainer Pull Request Template (to be used by maintainers)

[fivetran-reneeli]

Could you explain the change you're making here? Im curious about the second to minute switch as well as the 1440 - offset_minutes_to_add

[cth84]

I changed from second to minute to account for the fact that the offset is stored in minutes (the alternative could be to keep it in seconds, 86400 and then multiple offset_minutes by 60). Figured since it was a hard coded time to add, hardcoding it to 1440 and the not having to multiply the offset minutes made more sense.

1440 - offset_minutes_to_add is mimicking the logic applied to the start/time, which subtracts the offset so we can compare the two set of times:

potentially adding the _to_add to the offset_minutes and then subtracting it in that statement is a bit confusing, especially since our value is -240, currently. Most definitely open to a rename here.

[fivetran-reneeli]

Awesome, thanks for explaining!

[fivetran-avinash]

Hi @cth84 , thanks again for submitting your proposed PR to fix this issue!

After some investigation, I ran into a few issues with your logic.

1. The conversions to UTC time weren't quite aligning with the offset_minutes_to_add provided for each time_zone. I think you were on the right track though--I saw you trying to align it with the other utc conversion measurements. Here's what my logic provided, let me know if it makes sense or you think it should be something else?

 cast({{ dbt.dateadd("minute", "0 - offset_minutes_to_add", "schedule_holiday.holiday_start_date_at") }} as {{ dbt.type_timestamp() }}) as holiday_start_date_at,
       cast({{ dbt.dateadd("minute", "0 - offset_minutes_to_add", "schedule_holiday.holiday_end_date_at") }} as {{ dbt.type_timestamp() }}) as holiday_end_date_at,

2. However, after further investigation, I kept running into issues with getting this logic to work for every case. I think the problem is that downstream there are several CTEs that are dependent on comparisons between holiday schedules and regular schedules. For example, we have to compare minutes_from_sunday_start to start_time_utc, but it's dependent on a datediff between holiday_week_start_utc and holiday_start_date_at, and only one is in UTC format.

At the moment, we haven't deployed that full logic downstream in that model. If we are doing UTC conversions, we might have to.

But when I attempted to convert everything into UTC downstream, I think that was also causing cascading issues because there are some schedules running ahead of UTC and behind, as converting to UTC changes all the weekly holiday schedules to not start at midnight.

3. So I'm not sure if time zone conversion makes the most sense to fix this issue, because we'd have a ton of schedules starting at random times during the day. Let me know your thoughts. We could potentially adjust this spine to be more encompassing for all time zone cases. But if it is, it's likely a bit heftier than the fix you proposed.

4. If we're aiming for simplicity, I'm wondering if adjusting our join logic in the holiday_start_end_times model is the simplest route forward to provide the proper filtering on holiday dates. That way we keep all the holiday dates in general date format, and then we convert the schedule_holiday.holiday_start_date_at to compare to the valid_from/valid_to UTC fields. Something like

    inner join calculate_schedules
        on calculate_schedules.schedule_id = schedule_holiday.schedule_id
        and cast({{ dbt.dateadd("minute", "0 - offset_minutes_to_add", "schedule_holiday.holiday_start_date_at") }} as {{ dbt.type_timestamp() }}) >= calculate_schedules.valid_from 
        and cast({{ dbt.dateadd("minute", "0 - offset_minutes_to_add", "schedule_holiday.holiday_end_date_at") }} as {{ dbt.type_timestamp() }}) < calculate_schedules.valid_until

Might work. Testing this particular join didn't work, but I think if we figure out what filters to compare to valid_from/valid_to dates, this could get us closer to properly filtering out the dates that are holidays.

I think you have a better feel for Zendesk than me, so would love your thoughts! I will keep investigating and try to figure out a best solve.

[fivetran-joemarkiewicz]

@cth84 thanks so much for working with us to develop this bigfix to take the UTC offset into consideration when working with the Holiday and Schedules. We previously had not considered the logic would clash in this way, your issue and corresponding PR helped bring this to light for us and I believe we have found a viable solution within PR #126.

While we are not going to exactly take the approach you have provided here, I do believe there are components that we may leverage in the PR before merging to main. Additionally, I would like to include you as a contributor on the repo as you have helped us so much to understand the nuances of Zendesk! As such, I will merge this PR into the release branch and will make the respective edits in the base branch. Please let me know if you have any concerns.

Thanks 😄