Skip to content
This repository was archived by the owner on Jan 20, 2026. It is now read-only.

Add geo reports#44

Merged
fivetran-jamie merged 24 commits into
mainfrom
feature-update/add-geo-reports
Apr 29, 2025
Merged

Add geo reports#44
fivetran-jamie merged 24 commits into
mainfrom
feature-update/add-geo-reports

Conversation

@fivetran-jamie
Copy link
Copy Markdown
Contributor

@fivetran-jamie fivetran-jamie commented Apr 23, 2025

PR Overview

Package version introduced in this PR:
v0.9.0

This PR addresses the following Issue/Feature(s):

GA-926126

Summary of changes:

Adds source tables necessary to create country and region reports downstream

Submission Checklist

  • Alignment meeting with the reviewer (if needed)
    • Timeline and validation requirements discussed
  • Provide validation details:
    • Validation Steps: See transform PR
    • Testing Instructions: Will share dataset to use in Height
    • Focus Areas: Confirming that variables work as intended

see transform PR for more details, but confirming that the enable/disable variables work:
False produces 16 models
image

and 20 tests
image

True produces 24 models
image

and 24 tests
image

Changelog

  • Draft changelog for PR
  • Final changelog for release review

@fivetran-jamie fivetran-jamie self-assigned this Apr 23, 2025
@fivetran-jamie fivetran-jamie marked this pull request as ready for review April 24, 2025 16:55
Copy link
Copy Markdown
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with just a few small suggestions

Comment thread models/stg_facebook_ads.yml Outdated
Comment thread models/stg_facebook_ads.yml Outdated
Comment thread models/stg_facebook_ads.yml Outdated
Comment thread models/stg_facebook_ads.yml Outdated
_fivetran_id as country_id,
country,
date as date_day,
cast(account_id as {{ dbt.type_bigint() }}) as account_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No necessary to change for this PR since I know this decision was made in other models and we need to conform with the approach. However, it's interesting we make the decision to cast these IDs as integers. Usually we cast as string to be safe.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought so too

fivetran-jamie and others added 3 commits April 28, 2025 13:17
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Comment thread models/src_facebook_ads.yml Outdated
description: Currency associated with account.
- name: timezone_name
description: Timezone associated with account.
- name: business_country_code
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a duplicate of lines 31-32, you can remove this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Comment on lines +286 to +287
- name: inline_link_click_ctr
description: '{{ doc("inline_link_click_ctr") }}'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see this field in the macro either.

Comment on lines +274 to +281
- name: cost_per_inline_link_click
description: '{{ doc("cost_per_inline_link_click") }}'
- name: cpc
description: '{{ doc("cpc") }}'
- name: cpm
description: '{{ doc("cpm") }}'
- name: ctr
description: '{{ doc("ctr") }}'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see these fields in the macro, should these be removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're not, but I thought it was easy enough to document them for users in case they want to pass them in. I prefer to leave them in unless you have a strong opinion otherwise

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

Comment thread CHANGELOG.md Outdated
**9 total changes • 0 possible breaking changes**
| **Data Model** | **Change type** | **Old name** | **New name** | **Notes** |
| ---------------- | --------------- | ------------ | ------------ | --------- |
| stg_facebook_ads__demographics_country | New Model | | | Uses `demographics_country` source table |
Copy link
Copy Markdown
Contributor

@fivetran-avinash fivetran-avinash Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request to add links to the DAG for all Data Model column entries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Comment thread README.md Outdated

To connect your multiple schema/database sources to the package models, follow the steps outlined in the [Union Data Defined Sources Configuration](https://github.com/fivetran/dbt_fivetran_utils/tree/releases/v0.4.latest#union_data-source) section of the Fivetran Utils documentation for the union_data macro. This will ensure a proper configuration and correct visualization of connections in the DAG.

#### Eanble or Disable Country and Region Reports
Copy link
Copy Markdown
Contributor

@fivetran-avinash fivetran-avinash Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#### Eanble or Disable Country and Region Reports
#### Enable or Disable Country and Region Reports

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusting myself directly

Comment on lines +311 to +316
- name: inline
description: '{{ doc("inline") }}'
- name: _7_d_click
description: '{{ doc("_7_d_click") }}'
- name: _1_d_view
description: '{{ doc("_1_d_view") }}'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see these fields in the macro, should they be removed?

Comment on lines +338 to +345
- name: cost_per_inline_link_click
description: '{{ doc("cost_per_inline_link_click") }}'
- name: cpc
description: '{{ doc("cpc") }}'
- name: cpm
description: '{{ doc("cpm") }}'
- name: ctr
description: '{{ doc("ctr") }}'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see these fields in the macro, should they be removed?

description: '{{ doc("frequency") }}'
- name: spend
description: '{{ doc("spend") }}'
- name: inline_link_click_ctr
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see this field either.

description: '{{ doc("conversions") }}'
- name: _fivetran_synced
description: '{{ doc("_fivetran_synced") }}'
- name: inline
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see this field in the corresponding macro.

Comment thread CHANGELOG.md Outdated

## Feature Updates:
- Added the `facebook_ads__using_demographics_country` and `facebook_ads__using_demographics_region` variables, which can be used to enable or disable the above transformations related to the `demographics_country`/`demograhics_country_actions` and `demographics_region`/`demographics_region_actions` tables.
- These variables are dynamically set for Fivetran Quickstart users, but **false** by default otherwise. See [README](https://github.com/fivetran/dbt_facebook_ads_source?tab=readme-ov-file#enable-or-disable-country-and-region-reports) for more details.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- These variables are dynamically set for Fivetran Quickstart users, but **false** by default otherwise. See [README](https://github.com/fivetran/dbt_facebook_ads_source?tab=readme-ov-file#enable-or-disable-country-and-region-reports) for more details.
- These variables are dynamically set for Fivetran Quickstart users, but **false** by default otherwise. See [README](https://github.com/fivetran/dbt_facebook_ads_source?tab=readme-ov-file#enable-or-disable-country-and-region-reports) for more details on how to enable these models, particularly if you are using dbt Core.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusting myself directly

Copy link
Copy Markdown
Contributor

@fivetran-avinash fivetran-avinash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fivetran-jamie Thanks for applying the updates, lgtm!

@fivetran-jamie fivetran-jamie merged commit 69ecea0 into main Apr 29, 2025
8 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants