feature/add_custom_action_conversions#61
Merged
fivetran-savage merged 9 commits intomainfrom Dec 22, 2025
Merged
Conversation
fivetran-jamie
requested changes
Dec 18, 2025
Contributor
fivetran-jamie
left a comment
fivetran-jamie
left a comment
There was a problem hiding this comment.
Looks great! Left some suggestions - biggest thing is we'll want to add conversions_value fields as well
CHANGELOG.md
Outdated
|
|
||
| | Data Model(s) | Change type | Old | New | Notes | | ||
| | ---------- | ----------- | -------- | -------- | ----- | | ||
| | [facebook_ads__account_report](https://fivetran.github.io/dbt_facebook_ads/#!/model/model.facebook_ads.facebook_ads__account_report) | New Columns | | `onsite_conversion_purchase_conversions`, `onsite_conversion_lead_grouped_conversions`, `offsite_conversion_fb_pixel_purchase_conversions`, `offsite_conversion_fb_pixel_lead_conversions`, `offsite_conversion_fb_pixel_custom_conversions` | Adds individual conversion metrics for each default action type configured in the package. | |
Contributor
There was a problem hiding this comment.
I like the way you did Reddit Ads, where each field had its own row. The Data Models column can just read "All report models" or something like that
Contributor
There was a problem hiding this comment.
If we do that, then I'd say the # of total changes should = the # of new fields per model
CHANGELOG.md
Outdated
| | [facebook_ads__region_report](https://fivetran.github.io/dbt_facebook_ads/#!/model/model.facebook_ads.facebook_ads__region_report) | New Columns | | `onsite_conversion_purchase_conversions`, `onsite_conversion_lead_grouped_conversions`, `offsite_conversion_fb_pixel_purchase_conversions`, `offsite_conversion_fb_pixel_lead_conversions`, `offsite_conversion_fb_pixel_custom_conversions` | Adds individual conversion metrics for each default action type configured in the package. | | ||
| | [int_facebook_ads__conversions](https://fivetran.github.io/dbt_facebook_ads/#!/model/model.facebook_ads.int_facebook_ads__conversions) | New Columns | | `onsite_conversion_purchase_conversions`, `onsite_conversion_lead_grouped_conversions`, `offsite_conversion_fb_pixel_purchase_conversions`, `offsite_conversion_fb_pixel_lead_conversions`, `offsite_conversion_fb_pixel_custom_conversions` | Adds individual conversion metrics for each default action type configured in the package. | | ||
|
|
||
| The default action types included are `onsite_conversion.purchase`, `onsite_conversion.lead_grouped`, `offsite_conversion.fb_pixel_purchase`, `offsite_conversion.fb_pixel_lead`, and `offsite_conversion.fb_pixel_custom`. You can add additional custom action types by configuring the `facebook_ads__conversion_action_types` variable. See the README for details on how to configure custom action types. |
Contributor
There was a problem hiding this comment.
Suggested change
| The default action types included are `onsite_conversion.purchase`, `onsite_conversion.lead_grouped`, `offsite_conversion.fb_pixel_purchase`, `offsite_conversion.fb_pixel_lead`, and `offsite_conversion.fb_pixel_custom`. You can add additional custom action types by configuring the `facebook_ads__conversion_action_types` variable. See the README for details on how to configure custom action types. | |
| The default action types included are `onsite_conversion.purchase`, `onsite_conversion.lead_grouped`, `offsite_conversion.fb_pixel_purchase`, `offsite_conversion.fb_pixel_lead`, and `offsite_conversion.fb_pixel_custom`. You can add alternative conversion actions by configuring the `facebook_ads__conversion_action_types` variable. See the [README](https://github.com/fivetran/dbt_facebook_ads?tab=readme-ov-file#configuring-conversion-action-types) for details on how to configure custom action types. |
fivetran-savage
added
docs:ready
Contributor
fivetran-jamie
left a comment
fivetran-jamie
left a comment
There was a problem hiding this comment.
Looks great - some suggestions about the changelog and just consolidating for loops! Will approve after those are adjusted
CHANGELOG.md
Outdated
| | Data Model(s) | Change type | Old | New | Notes | | ||
| | ---------- | ----------- | -------- | -------- | ----- | | ||
| | All facebook_ads__*_report models | New Columns | | `onsite_conversion_purchase_conversions`, `onsite_conversion_lead_grouped_conversions`, `offsite_conversion_fb_pixel_purchase_conversions`, `offsite_conversion_fb_pixel_lead_conversions`, `offsite_conversion_fb_pixel_custom_conversions` | Add individual conversion metrics for each default action type configured in the package. | | ||
| | All facebook_ads__*_report models except [facebook_ads__country_report](https://fivetran.github.io/dbt_facebook_ads/#!/model/model.facebook_ads.facebook_ads__country_report) and [facebook_ads__region_report](https://fivetran.github.io/dbt_facebook_ads/#!/model/model.facebook_ads.facebook_ads__region_report) | New Columns | | `onsite_conversion_purchase_conversions_value`, `onsite_conversion_lead_grouped_conversions_value`, `offsite_conversion_fb_pixel_purchase_conversions_value`, `offsite_conversion_fb_pixel_lead_conversions_value`, `offsite_conversion_fb_pixel_custom_conversions_value` | Add individual conversion metrics for each default action type configured in the package. | |
Contributor
There was a problem hiding this comment.
Suggested change
| | All facebook_ads__*_report models except [facebook_ads__country_report](https://fivetran.github.io/dbt_facebook_ads/#!/model/model.facebook_ads.facebook_ads__country_report) and [facebook_ads__region_report](https://fivetran.github.io/dbt_facebook_ads/#!/model/model.facebook_ads.facebook_ads__region_report) | New Columns | | `onsite_conversion_purchase_conversions_value`, `onsite_conversion_lead_grouped_conversions_value`, `offsite_conversion_fb_pixel_purchase_conversions_value`, `offsite_conversion_fb_pixel_lead_conversions_value`, `offsite_conversion_fb_pixel_custom_conversions_value` | Add individual conversion metrics for each default action type configured in the package. | | |
| | All non-geographical facebook_ads__*_report models | New Columns | | `onsite_conversion_purchase_conversions_value`, `onsite_conversion_lead_grouped_conversions_value`, `offsite_conversion_fb_pixel_purchase_conversions_value`, `offsite_conversion_fb_pixel_lead_conversions_value`, `offsite_conversion_fb_pixel_custom_conversions_value` | Add individual conversion metrics for each default action type configured in the package. | |
Seems a little cleaner?
Contributor
Author
There was a problem hiding this comment.
Awesome, thanks.
Comment on lines
+43
to
+51
| {% for action_type in var('facebook_ads__conversion_action_types') %} | ||
| , sum(coalesce(conversion_report.{{ facebook_action_slug(action_type) }}_conversions, 0)) | ||
| as {{ facebook_action_slug(action_type) }}_conversions | ||
| {% endfor %} | ||
|
|
||
| {% for action_type in var('facebook_ads__conversion_action_types') %} | ||
| , sum(coalesce(conversion_report.{{ facebook_action_slug(action_type) }}_conversions_value, 0)) | ||
| as {{ facebook_action_slug(action_type) }}_conversions_value | ||
| {% endfor %} |
Contributor
There was a problem hiding this comment.
Suggested change
| {% for action_type in var('facebook_ads__conversion_action_types') %} | |
| , sum(coalesce(conversion_report.{{ facebook_action_slug(action_type) }}_conversions, 0)) | |
| as {{ facebook_action_slug(action_type) }}_conversions | |
| {% endfor %} | |
| {% for action_type in var('facebook_ads__conversion_action_types') %} | |
| , sum(coalesce(conversion_report.{{ facebook_action_slug(action_type) }}_conversions_value, 0)) | |
| as {{ facebook_action_slug(action_type) }}_conversions_value | |
| {% endfor %} | |
| {% for action_type in var('facebook_ads__conversion_action_types') %} | |
| , sum(coalesce(conversion_report.{{ facebook_action_slug(action_type) }}_conversions, 0)) | |
| as {{ facebook_action_slug(action_type) }}_conversions | |
| , sum(coalesce(conversion_report.{{ facebook_action_slug(action_type) }}_conversions_value, 0)) | |
| as {{ facebook_action_slug(action_type) }}_conversions_value | |
| {% endfor %} |
Since these are the same for loop, could you consolidate?
models/facebook_ads__ad_report.sql
Outdated
| as {{ facebook_action_slug(action_type) }}_conversions | ||
| {% endfor %} | ||
|
|
||
| {% for action_type in var('facebook_ads__conversion_action_types') %} |
Contributor
There was a problem hiding this comment.
same request to consolidate the for loops
| as {{ facebook_action_slug(action_type) }}_conversions | ||
| {% endfor %} | ||
|
|
||
| {% for action_type in var('facebook_ads__conversion_action_types') %} |
Contributor
There was a problem hiding this comment.
same request to consolidate the for loops
| as {{ facebook_action_slug(action_type) }}_conversions | ||
| {% endfor %} | ||
|
|
||
| {% for action_type in var('facebook_ads__conversion_action_types') %} |
Contributor
There was a problem hiding this comment.
same request to consolidate the for loops
models/facebook_ads__url_report.sql
Outdated
| as {{ facebook_action_slug(action_type) }}_conversions | ||
| {% endfor %} | ||
|
|
||
| {% for action_type in var('facebook_ads__conversion_action_types') %} |
Contributor
There was a problem hiding this comment.
same request to consolidate the for loops
| ,{{ facebook_action_slug(action_type) }}_conversions | ||
| {% endfor %} | ||
|
|
||
| {% for action_type in var('facebook_ads__conversion_action_types') %} |
Contributor
There was a problem hiding this comment.
same request to consolidate the for loops
Contributor
fivetran-jamie
left a comment
fivetran-jamie
left a comment
There was a problem hiding this comment.
Looks great! Just 2 final suggestions in the changelog only and then we're good to go
CHANGELOG.md
Outdated
Comment on lines
+10
to
+19
| | All facebook_ads__*_report models | New Column | | `onsite_conversion_purchase_conversions` | Total attributed conversions for onsite_conversion_purchase action type. | | ||
| | All facebook_ads__*_report models | New Column | | `onsite_conversion_lead_grouped_conversions` | Total attributed conversions for onsite_conversion_lead_grouped action type. | | ||
| | All facebook_ads__*_report models | New Column | | `offsite_conversion_fb_pixel_purchase_conversions` | Total attributed conversions for offsite_conversion_fb_pixel_purchase action type. | | ||
| | All facebook_ads__*_report models | New Column | | `offsite_conversion_fb_pixel_lead_conversions` | Total attributed conversions for offsite_conversion_fb_pixel_lead action type. | | ||
| | All facebook_ads__*_report models | New Column | | `offsite_conversion_fb_pixel_custom_conversions` | Total attributed conversions for offsite_conversion_fb_pixel_custom action type. | | ||
| | All non-geographical facebook_ads__*_report models | New Column | | `onsite_conversion_purchase_conversions_value` | Total attributed conversions_value for onsite_conversion_purchase action type. | | ||
| | All non-geographical facebook_ads__*_report models | New Column | | `onsite_conversion_lead_grouped_conversions_value` | Total attributed conversions_value for onsite_conversion_lead_grouped action type. | | ||
| | All non-geographical facebook_ads__*_report models | New Column | | `offsite_conversion_fb_pixel_purchase_conversions_value` | Total attributed conversions_value for offsite_conversion_fb_pixel_purchase action type. | | ||
| | All non-geographical facebook_ads__*_report models | New Column | | `ooffsite_conversion_fb_pixel_lead_conversions_value` | Total attributed conversions_value for offsite_conversion_fb_pixel_lead action type. | | ||
| | All non-geographical facebook_ads__*_report models | New Column | | `offsite_conversion_fb_pixel_custom_conversions_value` | Total attributed conversions_value for offsite_conversion_fb_pixel_custom action type. | |
Contributor
There was a problem hiding this comment.
For the field descriptions could you use the definitions you have in the facebook.yml? Those ones convey the actual events a bit more clearly
CHANGELOG.md
Outdated
| [PR #60](https://github.com/fivetran/dbt_facebook_ads/pull/60) includes the following updates: | ||
|
|
||
| ## Schema/Data Change | ||
| **35 total changes • 0 possible breaking changes** |
Contributor
There was a problem hiding this comment.
Suggested change
| **35 total changes • 0 possible breaking changes** | |
| **10 total changes • 0 possible breaking changes** |
This might make a little more sense since there are 10 rows/new fields
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
facebook_ads__conversion_action_typestofacebook_ads__account_report,facebook_ads__ad_report,facebook_ads__ad_set_report,facebook_ads__campaign_report,facebook_ads__country_report,facebook_ads__region_report, andint_facebook_ads__conversionsfacebook_action_slugmacro to generate slugified column names from action type configurations.Submission Checklist
Changelog