-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add conversions to Linkedin Ads Transform package #36
Add conversions to Linkedin Ads Transform package #36
Conversation
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 GREAT! just doc comments really
{% for field in additional_exclude_fields %} | ||
{% do except_fields.append(field) %} | ||
{% endfor %} |
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 like we can remove this part? is additional_exclude_fields
supposed to be used somewhere?
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.
@fivetran-jamie Made a slight modification to the code, let me know if this makes more sense.
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.
@fivetran-jamie Ready for re-review!
{% for field in additional_exclude_fields %} | ||
{% do except_fields.append(field) %} | ||
{% endfor %} |
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.
@fivetran-jamie Made a slight modification to the code, let me know if this makes more sense.
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 great to me! I'll take on merging this in this/next sprint
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.
@fivetran-jamie and @fivetran-avinash great work on these PRs! I only have the one callout to update the README section for the new conversion variable, but all other stress tests I performed look good! Once that final update is applied this should be good to go.
@@ -1,3 +1,3 @@ | |||
packages: | |||
- package: fivetran/linkedin_source | |||
version: [">=0.8.0", "<0.9.0"] | |||
version: [">=0.9.0", "<0.10.0"] |
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.
Reminder to hold off releasing this package until the source package is live on the dbt hub.
PR Overview
This PR will address the following Issue/Feature: Adding conversion support
This PR will result in the following new package version:
Although no breaking changes are introduced, the PR is significantly robust enough to require a version bump.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Feature Updates: Conversion Support!
We have added more robust support for conversions in our data models by doing the following:
conversion_value_by_local_currency
field to each_report
end model, representing the value of conversions that occurred on each day for each campaign/campaign_group/creative/url/account.linkedin_ads__conversion_fields
variable to pass through and transform additional conversion value metrics into their aggregate sums.dbt_project.yml
to bring in the most used conversion fieldsexternal_website_conversions
andone_click_leads
by default.total_conversions
metric in our end models to track all conversions being brought in by thelinkedin_ads_conversion_fields
variable.Documentation Update
linkedin_ads__conversion_fields
in the README.linkedin_ads__conversion_fields
variable.Under the Hood
persist_pass_through_columns()
macro in which we can includecoalesces
and properly check between conversion field values and the existing passthrough column.integration_tests
for the Linkedin transformation models.linkedin_ad_analytics_by_creative_data
seed file with relevant conversion fields for more robust testing.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:
Validation tests passing! Mostly mirroring Jamie's validations in Facebook.
If you had to summarize this PR in an emoji, which would it be?
🥼