Closed fivetran-sheringuyen closed 2 years ago
This PR will actually also close out Issue #12
Hey @fivetran-joemarkiewicz, I've just committed the PR fixes you've requested, let me know if you want to discuss further!
Regarding your question:
What is your stance on renaming the models that were moved from dbt_facebook_ads_creativehistory to stg instead of int_?
- Since I could see people using them as staging models, I wonder if it would make more sense to name them as such, but obviously keep the logic in the transformation package.
- Otherwise, what are your thoughts on making them final models that users can reference. Simply because I see these models being leveraged on their own, and it may be confusing to name them as intermediate. Thoughts?
I agree that people could definitely use them so I think it would make sense to create them as their own final models. With the one model (*_url_tags
) that we've migrated over in this update, I've made it into a final model and added appropriate testing for it in facebook_ads.yml
.
int_facebook_ads__creative_history
, however, should be kept as an intermediate model -- it actually takes *_url_tags
and prepares it for the utm_report
. While we've transformed the data to fit for our specific modeling needs, these transformations may not scale across the user base.
Additionally, as per our conversation last week, I believe we spoke about keeping
- package: fivetran/facebook_ads_creative_history
version: [">=0.4.0", "<0.5.0"]
in the packages.yml
, so I updated this as well. Let me know if your thoughts have changed on this front.
Running comments before merge:
Pull Request Are you a current Fivetran customer?
What change(s) does this PR introduce?
facebook_ads__ad_report
that reportsspend
,clicks
andimpressions
at the ad level.facebook_ads__ad_adapter
model tofacebook_ads__utm_report
to more accurately reflect what is included in the report; this report now also filters for only records that have valid URLs.README
updates for easier navigation and use of the package.dbt_facebook_ads_creative_history.stg_facebook_ads__url_tag
directly into this package via the use of theget_url_tag_query
macro as an intermediate model calledint_facebook_ads__url_tags
.facebook_ads__creative_history_prep
model toint_facebook_ads__creative_history
to conform with new styling standards.Did you update the CHANGELOG?
Does this PR introduce a breaking change?
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
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
:dancer: **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](https://www.surveymonkey.com/r/DQ7K7WW) on our existing dbt packages or what you'd like to see next.