Feature/pinterest v5 api support

[fivetran-reneeli]

PR Overview

This PR will address the following Issue/Feature:
#24
This PR will result in the following new package version: v0.8.0

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

• Following Pinterest Ads deprecating the v4 API on June 30, 2023 in place of v5, the Pinterest Ads Fivetran connector now leverages the Pinterest v5 API. The following fields have been deprecated/ introduced:
  | Model | Removed | New |
  |---|---|---|
  | stg_pinterest_ads__advertiser_history | billing_profile_status, billing_type, merchant_id, status | owner_username, permissions |
  | stg_pinterest_ads__campaign_history | | default_ad_group_budget_in_micro_currency, is_automated_campaign, is_campaign_budget_optimization, is_flexible_daily_budgets |
  | stg_pinterest_ads__ad_group_history | | pacing_delivery_type, placement_group, summary_status, advertiser_id |
  | stg_pinterest_ads__pin_promotion_history | | advertiser_id |

• In the v5 upgrade, advertiser_id has been replaced by ad_account_id and is a net new field in ad_group_history and pin_promotion_history. However, to keep our Pinterest Ads package standard with our other ad packages, we have kept it as advertiser_id.

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
• [n/a] 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
• [ n/a] 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.

Removed/ introduced new fields and updated the ymls, but we need to watch out for downstream impacts.

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
• [ n/a] README updates have been applied (if applicable)
• [ n/a] 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-reneeli]

holding off on docs until PR review is complete!

[fivetran-reneeli]

Btw, in the v5, advertiser_id has been replaced by ad_account_id. However, to keep our Pinterest Ads package standard with our other ad packages and to avoid another PR in ad reporting, I kept it as advertiser_id, renaming it from ad_account_id in the staging models.

Also, keep in mind we don't have the new data, therefore I haven't replaced the seed data used for integration testing. Also, this means that the new fields are being brought in as null when running this. Also since advertiser_id is represented as ad_account_id and we don't have that field in our test data, advertiser_id is null to when running this.

[fivetran-joemarkiewicz]

Hey @fivetran-reneeli this is definitely a tricky update to apply when we do not have data to support the assumption of the API update. However, during my review it doesn't look like these changes particularly match the release notes we are seeing on our release notes for Pinterest Ads.

In particular some things I noticed that didn't match the release notes:

• In your changes you included the ad_account_id field to every model. However, per the release notes it seems that this is a new field only in the ad_group_history model. Was there a reason this was added to every model in place of the advertiser_id?
• Similarly, it seems that ad_account_id was not included in the ad_group_history model. Was there a reason for this omission?
• I noticed you added all the new fields to the staging models. Are these really needed? Our initial build of the package didn't include all the fields from the source, only the necessary ones we deemed relevant to the downstream models. If they do in fact make sense to include then I am open to adding them. But I worry that with our limited understanding of these new fields it may not make sense to include them.
• The field removals are definitely something we want to consider. It looks like you got all of those covered for the impacted fields in the advertiser_history table! ✅

Ultimately, I would like us to consider the above questions. Would you be able to take another pass at this PR and make any necessary updates to this model and the downstream ones? I would rather we not include all the new fields but instead put priority on removing the deprecated fields and adding any necessary new fields (like the ad_account_id for the ad_group_history).

[fivetran-reneeli]

@fivetran-joemarkiewicz please see my replies below!

In particular some things I noticed that didn't match the release notes:

In your changes you included the ad_account_id field to every model. However, per the release notes it seems that this is a new field only in the ad_group_history model. Was there a reason this was added to every model in place of the advertiser_id?

So there was some confusion over this. According to the pinterest v5 docs, it reads as advertiser_id is getting replaced by ad_account_id across several endpoints, including ads, ad groups, advertiser, and campaigns. But I suppose our connector is only swapping it out for ad_group_history then?

Similarly, it seems that ad_account_id was not included in the ad_group_history model. Was there a reason for this omission?

Ah, I overlooked this, will add it in.

I noticed you added all the new fields to the staging models. Are these really needed? Our initial build of the package didn't include all the fields from the source, only the necessary ones we deemed relevant to the downstream models. If they do in fact make sense to include then I am open to adding them. But I worry that with our limited understanding of these new fields it may not make sense to include them.

I don't believe I added all the new fields, just the ones I thought were relevant. For example, I chose not to include bid_strategy_type in ad_group_history. I didn't bring any fields into the transforms though, just removed deprecated ones.

But yeah, the new fields I added are just brought into staging, for the ones I thought may be relevant. I'm also ok with just not having them entirely given we don't have info on them.

[fivetran-joemarkiewicz]

Thanks for the detailed responses @fivetran-reneeli!

I am comfortable with keeping the new fields in the staging model and comfortable with them not being included in the transform.

On the topic of the ad_account_id, I thought Calvin mentioned that they did not take this into account (outside of the ad group table) in order to make the migration easier. Can we confirm this with him? It would be a lot easier to just get data for this to fully validate... I may need to bite the bullet to serve an ad so we can get data 😞

[fivetran-reneeli]

On the topic of the ad_account_id, I thought Calvin mentioned that they did not take this into account (outside of the ad group table) in order to make the migration easier. Can we confirm this with him? It would be a lot easier to just get data for this to fully validate... I may need to bite the bullet to serve an ad so we can get data 😞

You know, actually I think he did say that, I just misunderstood. Let me follow up in our thread to confirm-- don't need to run an ad!

[fivetran-reneeli]

Hi @fivetran-joemarkiewicz , I switched back relevant models to use advertiser_id

[fivetran-joemarkiewicz]

@fivetran-reneeli thanks for making the requested updates! I do have a few final questions and suggestions to apply before this is all good to go. Let me know if you have questions!

[fivetran-joemarkiewicz]

Is this still true? Maybe it would be worthwhile to include how we renamed ad_account_id for the pin promotion history and ad group models to be advertiser_id to be inline with the other naming conventions of the field across the schema/package.

[fivetran-reneeli]

Good point, forgot to update this

[fivetran-joemarkiewicz]

Small note, was advertiser_id replaced in the ad_group_history and pin_promotion_history? I was under the impression it didn't even exist previously and was just added as ad_account_id? If that is the case, can you reword this to be in line with this understanding.

[fivetran-reneeli]

Hey @fivetran-joemarkiewicz I believe I updated this snippet, but I added more detail just now regardless

[fivetran-reneeli]

Updated documentation as well as the PR template above.

[fivetran-joemarkiewicz]

@fivetran-reneeli thanks for making the appropriate updates! This looks almost good to go. I only have one remaining note here. However, that is a fairly small note. Once that is updated this will be good to go. Therefore, I a comfortable approving this PR!

[fivetran-joemarkiewicz]

@fivetran-reneeli I actually just realized that the README has not been updated to bump the version. Please be sure to update this before kicking off the release process.

[fivetran-reneeli]

My bad, I didn't respond to your comment but I made the changelog is clearer! And thanks for catching the readme version-- updated