bug/issue-type

[fivetran-catfritz]

PR Overview

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

This PR will result in the following new package version:

• v0.15.0
• full refresh needed due to affected incremental model

Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:

• Updated the jira__daily_issue_field_history model to make sure issue_type values are correctly joined into the downstream issue models. This applied only if issue type is leveraged within the issue_field_history_columns variable.

• Reorganized some of the logic in jira__daily_issue_field_history so that the for loops run less often and used nested conditionals.

PR Checklist

Basic Validation

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

• dbt compile
• dbt run –full-refresh
• dbt run
• dbt test
• dbt run –vars (if applicable)

Before marking this PR as "ready for review" the following have been applied:

• The appropriate issue has been linked and tagged
• You are assigned to the corresponding issue and this PR
• BuildKite integration tests are passing

Detailed Validation

Please acknowledge that the following validation checks have been performed prior to marking this PR as "ready for review":

• You have validated these changes and assure this PR will address the respective Issue/Feature.
• You are reasonably confident these changes will not impact any other components of this package or any dependent packages.
• You have provided details below around the validation steps performed to gain confidence in these changes.

• See internal ticket for link to validation worksheet.

Standard Updates

Please acknowledge that your PR contains the following standard updates:

• Package versioning has been appropriately indexed in the following locations:
  • indexed within dbt_project.yml
  • indexed within integration_tests/dbt_project.yml
• CHANGELOG has individual entries for each respective change in this PR
• README updates have been applied (if applicable)
• DECISIONLOG updates have been updated (if applicable)
• Appropriate yml documentation has been added (if applicable)

dbt Docs

Please acknowledge that after the above were all completed the below were applied to your branch:

• docs were regenerated (unless this PR does not include any code or yml updates)

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

💃

[fivetran-joemarkiewicz]

@fivetran-catfritz I just reviewed and really like your updates! I think the improvements you made to the conditional and for loop logic really help make this model more robust and scalable. I also was able to validate the changes locally as well as using your validation guide. Lastly, knowing that the customer confirmed it works on their end makes me comfortable with these changes. However, before approving I did have one question regarding the need for the else statements. The one comment should also apply to all other conditional updates.

Finally, this is a somewhat similar update to the components update we made a while ago. Do you know if there are any other custom fields that may need a similar update in the future? The updates of those would be out of scope for this ticket, but generally curious if we should create tickets for those proactively.

[fivetran-joemarkiewicz]

Is there a reason the else is essentially doing nothing here? Should the previous elif be an else instead?

[fivetran-joemarkiewicz]

It seems this behavior exists for all the other conditionals in this PR. What are your thoughts on the else presence and if we should change the final elif to an else or if the else is not needed at all?

[fivetran-joemarkiewicz]

Or is this specific and dependent on each conditional use case in the model?

[fivetran-catfritz]

I had included them because I thought not having them was causing an error I was getting at the time, but I just tried taking all the elses out, and there are no errors, so yes I agree they're pointless and have removed!

[fivetran-catfritz]

@fivetran-joemarkiewicz Thanks for reviewing! I have pushed the updates to remove the elses. As for other custom fields, the customer did run with a ton of the columns listed in the issue (see the dbt project configs), and said there weren't any other affected fields.

However now that I think of this more, I'm still a little uncertain about what they are supposed to display. I tested out some additional fields like resolution, priority and project, and they display their id rather than a name. However, I updated the issue_type to show the name rather than id. Is this actually what it's supposed to be?

[fivetran-joemarkiewicz]

However now that I think of this more, I'm still a little uncertain about what they are supposed to display. I tested out some additional fields like resolution, priority and project, and they display their id rather than a name. However, I updated the issue_type to show the name rather than id. Is this actually what it's supposed to be?

@fivetran-catfritz yup this is what I was leaning towards with my probing question around if other custom fields need a similar update. The ultimate goal of this jira__daily_issue_field_history model is to display the latest field history (of those selected) value for an issue on a given day. This model is primarily generated by the stg_jira__issue_field_history model as well as a few others. The end model consolidates, pivots, and creates a slowly changing dimension from that staging model. In that staging model the values for some of the fields are foreign keys to tables which the connector syncs. This is what we saw originally with components and then status, and now what you are seeing with issue_type. I was curious if this is also the case for other field types that have their own tables (ie. priority, project, resolution) and that does seem to be the case. With this, I would recommend opening a FR on this repo to expand the logic we have for components, status, and issue type to include the other fields with a similar FK relationship.

You can see that this is not the case for fields that do not have their own tables synced via the connector. Here is an example of my including a field that has a FK (shows as and ID) and one that does not. Notice how summary is not an ID but the actual value since there is no raw summary table. Ideally, we could create an end model that includes the value as opposed to the ID for the fields with their own tables. However, that is out of scope for this ticket and something we should handle in a future update.

[fivetran-catfritz]

Thanks @fivetran-joemarkiewicz that makes sense now. I found this issue already exists, so I'll add comments with more detail of what we found this round.

I would say this is ready for re-review now too!

[fivetran-joemarkiewicz]

@fivetran-catfritz thanks for making the updates and contributing to the FR to expand the package to perform the same updates here for the other field types. This looks good to go from my point of view!