Bug/sprint history missing #62
Conversation
There was a problem hiding this comment.
@troyschuetrumpf-elation thanks so much for opening this PR!! 🏅
There are just a few small edits I feel we need to make around the status filter and ensuring we are using the field_id instead of the field_name for those records. Once you make those edits we should be good to merge!
Let me know if you have any questions 😄
| @@ -28,8 +28,8 @@ pivot_out as ( | |||
| select | |||
| valid_starting_on, | |||
| issue_id, | |||
| max(case when lower(field_id) = 'status' then field_value end) as status, | |||
| max(case when lower(field_name) = 'sprint' then field_value end) as sprint -- As sprint is a custom field, we aggregate on the field_name. | |||
| max(case when lower(field_name) = 'status' then field_value end) as status, | |||
There was a problem hiding this comment.
I actually believe we would want to retain this to be the field_id rather than the field_name for status.
Similar to the below comment, status will always have an ID as status within Jira data. However, the name is mutable and can be changed by the user. I think it would be best to keep status linked to the ID.
| where lower(field_name) in | ||
| ('status' | ||
| ,'sprint' |
There was a problem hiding this comment.
I believe we still want to have the filter look for the field_id for status as the ID within the field table for status will always be status. The name is mutable and can change. Therefore, I think we should keep status linked to field_id.
sprint on the other-hand I feel we should 100% link to field_name.
| where lower(field_name) in | |
| ('status' | |
| ,'sprint' | |
| where lower(field_id) = 'status' | |
| or lower(field_name) in ('sprint' |
There was a problem hiding this comment.
Make sense, Thanks for the review! Reverted status to be checked against field_id as recommended
There was a problem hiding this comment.
I did this as a seperate commit (instead of accepting the change) in order to update models/intermediate/field_history/int_jira__pivot_daily_field_history.sql at the same time
There was a problem hiding this comment.
Thanks so much for applying those changes @troyschuetrumpf-elation 🙌
I made a quick commit to update the CHANGELOG to include relevant links to this PR and your profile for contribution recognition. This PR looks good to go now. I tested the changes and we are seeing the sprint and status records being replicated correctly.
We greatly appreciate your eagle eyes for catching this bug and for working with us to apply the fix. These packages are made better by contributors like yourself. Thank you for contributing back to the community! 🏅
Pull Request
Are you a current Fivetran customer?
Troy Schuetrumpf, VP Platform Engineering, Elation Health
What change(s) does this PR introduce?
Bug introduced in 8.0. This PR resolves the introduced bug and updates the
statusselection to use field_name (I verified that this is how this columns ID and Name are represented in jira)Did you update the CHANGELOG?
Does this PR introduce a breaking change?
This will cause the status column of
jira__daily_issue_field_historyto be populated with data again. No column names or functionality changed.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)
Is this PR in response to a previously created Bug or Feature Request
[Bug] Sprint column not correctly mapped #61
How did you test the PR changes?
Updated our dbt project that is built on fivetran jira data to use the updated models, run against our dataset and observed sprint column populated with data.
Select which warehouse(s) were used to test the PR
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.