Feature/enable null urls #25
Conversation
fivetran-catfritz
changed the base branch from
main
to
MagicBot/dbt-utils-cross-db-migration
There was a problem hiding this comment.
Hey @fivetran-catfritz, great job on the PR!
I made a few in-line comments and have an additional few here:
-
I think it may be worth it to update the
DECISIONLOG.mdfile as it refers to the UTM report currently and it's supposed to be renamed to the URL report - decisions re the model should still be relevant though. However, if this is something you feel is out of scope, you can just create an issue for it so that we're documenting it for the future. -
I believe that these updates to the models will indeed warrant a regeneration of docs.
models/facebook.yml
Outdated
| {{ 'False' | as_bool }} | ||
| {%- else -%} | ||
| {{ 'True' | as_bool }} | ||
| {%- endif -%} |
There was a problem hiding this comment.
I see what you're doing here, and I think you're definitely on the right track. This could potentially be simplified and you can check out apple_search_ads_source and twitter ads source. With these examples, you could use an 'or' statement rather than an 'and' statement to accomplish the same thing here!
There was a problem hiding this comment.
Oh I think I get it now after reading below comment! Ok I will update. 👍
There was a problem hiding this comment.
Actually Ok I remember now--I initially tried to do something like that but got tripped up because I have to pass false if the variable is true, whereas in the example it would use true if one of the variables was true right? I will noodle on this more however. Though now I'm 2nd guessing if I should have used something more like "disable null filter" = True...
There was a problem hiding this comment.
Suggest to use only ad_reporting__using_null_url_filter default is True.
CHANGELOG.md
Outdated
| [PR #25](https://github.com/fivetran/dbt_facebook_ads/pull/25) includes the following changes: | ||
| ## 🎉 Features 🎉 | ||
| - Added ability for a user to allow records having nulls in url fields to be included in the `facebook_ads__url_report` model. This is done by setting one of the variables below to `True` in your `dbt_project.yml` file. | ||
| - Note that using the variable `allow_ad_reporting_null_urls` will allow records with null urls for ALL Fivetran ad packages included in your project. |
There was a problem hiding this comment.
I think depending on what you answer to the below question re: the intended variable interpretation, it would be valuable to highlight it here. Like what is the expectation from the customer re: declaring the variables. Can they declare allow_ad_reporting_null_urls as true but declare facebook_ads__url_report as false?
There was a problem hiding this comment.
They technically can, though I guess the way I want it to work is that the allow_ad_reporting_null_urls would win since I'm using an or between the two variable versions, and nulls would be allowed everywhere. My thought was either you could allow it on individual packages or all of them at once since the default is false.
For example, if someone wanted all them to have nulls except facebook for whatever reason, they would allow on each of the other packages individually and either set facebook variable to false or not set it at all. Up for debate on the best approach, but this is what I went with.
CHANGELOG.md
Outdated
| ``` | ||
| - Updated README with this information. | ||
| ## 🚘 Under the Hood 🚘 | ||
| - Disabled the `not_null` test for `facebook_ads__url_report` when null urls are allowed. |
There was a problem hiding this comment.
Good job on including this!
There was a problem hiding this comment.
I know you like consistency, so I just moved the PR references to the end of each section like below:
| - Disabled the `not_null` test for `facebook_ads__url_report` when null urls are allowed. | |
| - Disabled the `not_null` test for `facebook_ads__url_report` when null urls are allowed [[#25](https://github.com/fivetran/dbt_facebook_ads/pull/25)]. |
models/facebook_ads__url_report.sql
Outdated
| where creatives.url is not null | ||
| on report.account_id = accounts.account_id | ||
|
|
||
| {% if (var('allow_facebook_ads_null_urls', False)==False and var('allow_ad_reporting_null_urls', False)==False) %} |
There was a problem hiding this comment.
| {% if (var('allow_facebook_ads_null_urls', False)==False and var('allow_ad_reporting_null_urls', False)==False) %} | |
| {% if var('ad_reporting__using_null_url_filter', True) %} |
There was a problem hiding this comment.
Hey @fivetran-catfritz! Thanks for the productive session this morning - there are mostly some minor updates here that were held over from the previous version of the PR. But the tests and the model updates make sense to me!
Co-authored-by: Sheri Nguyen <94874400+fivetran-sheringuyen@users.noreply.github.com>
Co-authored-by: Sheri Nguyen <94874400+fivetran-sheringuyen@users.noreply.github.com>
Are you a current Fivetran customer?
Fivetran created PR.
What change(s) does this PR introduce?
ad_reporting__url_reportmodel.not_nulltest forfacebook_ads__url_reportwhen null urls are allowedDid you update the CHANGELOG?
Does this PR introduce a breaking change?
Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)
Is this PR in response to a previously created Bug or Feature Request
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
🍝
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.