-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feature that added ad squad & campaign in the ad report #33
base: main
Are you sure you want to change the base?
Conversation
…campaign-and-ad-squad-to-ad-report
…ad-to-ad-report Feature: Add ad_squad and campaign to ad report
…hub.com/fivetran/dbt_snapchat_ads into feature/add-ad-squad-campaign-ad-report
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 with a few small notes
CHANGELOG.md
Outdated
- Fields added include: | ||
- `ad_squad_id` | ||
- `ad_squad_name` | ||
- `campaign_id` | ||
- `campaign_name` |
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.
Can you consolidate this into the one bullet above. "Fields added include:" doesn't need to be a separate bullet. But I would like the separate fields to be their own sub bullets still.
@@ -1,5 +1,5 @@ | |||
name: 'snapchat_ads' | |||
version: '0.6.2' | |||
version: '0.8.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.
Yikes, looks like we missed updating this in the last release.
integration_tests/dbt_project.yml
Outdated
profile: 'integration_tests' | ||
config-version: 2 | ||
|
||
vars: | ||
vars: |
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.
Whitespace removal
vars: | |
vars: |
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.
While you're making a new test, can you also update the consistency tests to ensure the conversions is included moving forward.
PR Overview
This PR will address the following Issue/Feature: [#31] contribution via @JeremyDOwens
This PR will result in the following new package version: 0.8.0
Added new fields, leading to a schema change.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
This release introduces the following updates:
Breaking Changes
snapchat_ads__ad_report
model, so that they can eventually be incorporated in thedbt_ad_reporting
package for thead_reporting__ad_report
model. (#33)ad_squad_id
ad_squad_name
campaign_id
campaign_name
Under The Hood
ad_squad_id
andcampaign_id
insnapchat.yml
forsnapchat_ads__ad_report
andsnapchat_ads__url_report
to more closely align with other ad reporting packages. (#33)snapchat_ads__ad_report
maintains the same row grain count forad_id
anddate_day
betweens the source and end models. (#33)Documentation
snapchat.yml
with new above field additions. (#33)Contributors
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:
Validated that these fields were brought through to the end model, as well as ensured new tests passed
If you had to summarize this PR in an emoji, which would it be?
🦑