-
Notifications
You must be signed in to change notification settings - Fork 7
feature/country-region-reports #74
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
Conversation
fivetran-jamie
left a comment
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! left mostly doc-related comments
Co-authored-by: Jamie Rodriguez <65564846+fivetran-jamie@users.noreply.github.com>
fivetran-jamie
left a comment
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.
LGTM!
CHANGELOG.md
Outdated
| [PR #74](https://github.com/fivetran/dbt_linkedin_source/pull/74) includes the following updates: | ||
|
|
||
| ## Schema Changes | ||
| **6 total changes • 0 possible breaking changes |
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.
To properly bold:
| **6 total changes • 0 possible breaking changes | |
| **6 total changes • 0 possible breaking changes** |
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.
Updated
CHANGELOG.md
Outdated
| **6 total changes • 0 possible breaking changes | ||
| | Data Model | Change Type | Old Name | New Name | Notes | | ||
| |---------------------------------------------------|-------------|----------|-------------------------------------------|-------------------------------------------------------------------| | ||
| | stg_tiktok_ads__geo_tmp | New temp model | | | Temp model added for `geo`. | |
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.
Similar comment to add the links to the DAG in the Data Model columns.
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.
Updated
CHANGELOG.md
Outdated
| **6 total changes • 0 possible breaking changes | ||
| | Data Model | Change Type | Old Name | New Name | Notes | | ||
| |---------------------------------------------------|-------------|----------|-------------------------------------------|-------------------------------------------------------------------| | ||
| | stg_tiktok_ads__geo_tmp | New temp model | | | Temp model added for `geo`. | |
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.
Need to update the data model.
| | stg_tiktok_ads__geo_tmp | New temp model | | | Temp model added for `geo`. | | |
| | stg_linkedin_ads__geo_tmp | New temp model | | | Temp model added for `geo`. | |
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.
Updated
CHANGELOG.md
Outdated
| | Data Model | Change Type | Old Name | New Name | Notes | | ||
| |---------------------------------------------------|-------------|----------|-------------------------------------------|-------------------------------------------------------------------| | ||
| | stg_tiktok_ads__geo_tmp | New temp model | | | Temp model added for `geo`. | | ||
| | stg_tiktok_ads__geo | New staging model | | | Staging model added for `geo`. | |
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.
Data model needs to be updated.
| | stg_tiktok_ads__geo | New staging model | | | Staging model added for `geo`. | | |
| | stg_linkedin_ads__geo | New staging model | | | Staging model added for `geo`. | |
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.
Updated
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.
Should we call out what this macro does in an Under the Hood CHANGELOG section?
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.
Doesn't hurt to include, added an entry.
models/src_linkedin.yml
Outdated
| - name: external_website_conversions | ||
| description: Number of conversions that occurred on an external website as a result of the ad. | ||
| - name: one_click_leads | ||
| description: Number of one-click leads generated from the ad campaign. | ||
| - name: external_website_conversions | ||
| description: The actions taken on your website that you've defined as valuable to your business after clicking on LinkedIn ads. | ||
| - name: one_click_leads | ||
| description: Leads submitted after clicking on LinkedIn ads. |
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.
These columns are duplicated. I think the second set of columns have better descriptions, so I'd opt to keep those, but will leave up to you!
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.
Unsure how this happened. Removed.
models/src_linkedin.yml
Outdated
| - name: external_website_conversions | ||
| description: Number of conversions that occurred on an external website as a result of the ad. | ||
| - name: one_click_leads | ||
| description: Number of one-click leads generated from the ad campaign. | ||
| - name: external_website_conversions | ||
| description: The actions taken on your website that you've defined as valuable to your business after clicking on LinkedIn ads. | ||
| - name: one_click_leads | ||
| description: Leads submitted after clicking on LinkedIn ads. |
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.
These columns are duplicated. I think the second set of columns have better descriptions, so I'd opt to keep those, but will leave up to you!
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.
Unsure how this happened. Removed.
models/stg_linkedin.yml
Outdated
| - name: external_website_conversions | ||
| description: Number of conversions that occurred on an external website as a result of the ad. | ||
| - name: one_click_leads | ||
| description: Number of one-click leads generated from the ad campaign. | ||
| - name: external_website_conversions | ||
| description: The actions taken on your website that you've defined as valuable to your business after clicking on LinkedIn ads. | ||
| - name: one_click_leads | ||
| description: Leads submitted after clicking on LinkedIn ads. |
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.
Same duplicate column issue, similar recommendation to use the second set.
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.
Unsure how this happened. Removed.
models/stg_linkedin.yml
Outdated
| description: The value generated by your conversions, displayed in your LinkedIn account's local currency. | ||
| - name: cost | ||
| description: The cost of the ads in the local currency or USD. | ||
| - name: external_website_conversions |
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.
Same duplicate column issue, similar recommendation to use the second set.
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.
Unsure how this happened. Removed.
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.
We will want to apply line 139 with the new support for the new tables added I think, with a link to this upcoming release.
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.
This was mainly just to call out the backwards compatibility at that point in time. It's not entirely necessary to include this release in that section since this is a net new model. However, I did update the section to callout the downstream components which the variable is used compatibile.
fivetran-avinash
left a comment
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-joemarkiewicz Almost there! A few questions, comments and suggestions before release.
fivetran-avinash
left a comment
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.
lgtm
PR Overview
Package version introduced in this PR:
v0.11.0This PR addresses the following Issue/Feature(s): Internal ticket
Summary of changes:
Adds new staging models for the
geo,monthly_ad_analytics_by_member_country, andmonthly_ad_analytics_by_member_regionsources.Submission Checklist
For all above validations and testing please see internal ticket. Additionally, docs have not be generated and will be once PR is near approval.
Changelog