move cent conversions upstream to source package #101

fivetran-reneeli · 2024-12-18T19:34:29Z

PR Overview

This PR will address the following Issue/Feature: #100

This PR will result in the following new package version: v0.16.0

Breaking change as customers using only the staging models will experience changes in values in currency-related fields.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Breaking Changes

This package assumes that amount-based fields, which as raw values are represented in the smallest denomination in Stripe, are cent-based. This PR shifts the existing conversion from cents to dollars to further upstream. Previously, currency-related fields were converted in downstream models, but now have been converted directly in staging models. Since currency-related fields now have different values, this is a breaking change.

To disable this default conversion, refer to the README on disabling the stripe__amount_divide variable.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

See validation link in internal ticket

If you had to summarize this PR in an emoji, which would it be?

💃

… run script

fivetran-joemarkiewicz

@fivetran-reneeli thanks for working through this PR. I have a few comments and requests. In particular, I also would like to discuss some more why we only divided by 100 in a few particular places in previous versions of this package. I want to make sure we aren't missing anything before we apply an update like this.

fivetran-joemarkiewicz · 2025-01-06T16:33:48Z

CHANGELOG.md

+## Breaking Changes
+- This package assumes that amount-based fields, which as raw values are represented in the smallest denomination in Stripe, are cent-based. This PR shifts the existing conversion from cents to dollars to further upstream. Previously, currency-related fields were converted in downstream models, but now have been converted directly in staging models. Since currency-related fields now have different values, this is a breaking change. 
+
+To disable this default conversion, refer to the [README]((https://github.com/fivetran/dbt_stripe/blob/main/README.md#disabling-cent-to-dollar-conversion)) on disabling the `stripe__amount_divide` variable.


See notes from the source PR to make similar updates here.

Additionally, we should highlight each field in the end models which are seeing this update. This will drastically change the outputs if customers were not previously aware. We should make this obvious to customers.

Would this change the outputs if the values of the fields remain the same? For example, total_sales in customer_overview would still retain the same value, just the division occurs farther upstream. I can make a callout for the fields where the code has changed though. Which, by the way, you make a good point in that the division didn't occur across all end models originally. Will circle back on that in the other comment.

fivetran-joemarkiewicz · 2025-01-06T16:34:02Z

README.md

+#### Disabling Cent to Dollar Conversion
+
+Amount-based fields, such as `amount` and `net`, are typically displayed in the smallest denomination (e.g., cents for USD). By default, these values are automatically converted to dollars by dividing by `100.0`. To disable this conversion and retain the values in their smallest denomination, set the `stripe__amount_divide` variable to `False` as shown below:
+
+```yml
+vars:
+    stripe__amount_divide: False
+```


See source PR notes to make the same updates here.

README.md

fivetran-joemarkiewicz · 2025-01-06T16:35:43Z

packages.yml

+#   version: [">=0.13.0", "<0.14.0"]
+
+  - git: https://github.com/fivetran/dbt_stripe_source.git
+    revision: feature/standardize_cent_conversions
+    warn-unpinned: false


Reminder to swap before merge

fivetran-joemarkiewicz · 2025-01-06T16:48:46Z

models/stripe__customer_overview.sql

-      coalesce(transactions_grouped.total_gross_transaction_amount/100.0, 0) as total_gross_transaction_amount,
-      coalesce(transactions_grouped.total_fees/100.0, 0) as total_fees,
-      coalesce(transactions_grouped.total_net_transaction_amount/100.0, 0) as total_net_transaction_amount,
+      coalesce(transactions_grouped.total_sales, 0) as total_sales,


Why did we only divide by 100 for the int_stripe__account_daily and stripe__customer_overview models? We didn't do this function in the other (arguably more popular) end models such as balance transaction. Are we missing something here? Should we consider retaining the smallest unit by default but instead allow the customer to enable it if they want?

Oh hmm. I did some digging back into the commit histories of the models. I think this is product of when each model was introduced. For instance, the int_stripe__account_daily was created with the /100 in place. But some of the older models never had it so my guess is there wasn't a retroactive update applied to those.

Are you saying to set the conversion to False by default? I guess that would make more sense from an impact perspective if more fields weren't ever converted than there were. That way, most of the models wouldn't be impacted than if the conversion was set to True.

If that is the case, this means we keep the changes made to int_stripe__account_daily and stripe__customer_overview, but I will update the documentation and run script accordingly.

Thanks for the investigation here! This is really interesting and a bit confusing why we would apply this divide by 100 approach for a select few models, but not others.

Given this I would request we take the following approach. We have the default state of the stripe__convert_values variable be false. We then apply an addition conditional to these end models with the divide by 100 to only divide by 100 if the variable is false. This way we can ensure this update will be backwards compatible and not cause major breaking changes. So these end models would look like the following.

{{ 'coalesce(transactions_grouped.total_refunds/100.0, 0)' if not var('stripe__convert_values', false) else coalesce(transactions_grouped.total_refunds, 0)' }} as total_refunds,

Updated and added a note to the CHANGELOG.

fivetran-joemarkiewicz

@fivetran-reneeli thanks for these edits, a few more comments in the review. Thanks!

fivetran-joemarkiewicz · 2025-01-10T18:34:21Z

models/stripe__customer_overview.sql

-      coalesce(transactions_grouped.total_gross_transaction_amount/100.0, 0) as total_gross_transaction_amount,
-      coalesce(transactions_grouped.total_fees/100.0, 0) as total_fees,
-      coalesce(transactions_grouped.total_net_transaction_amount/100.0, 0) as total_net_transaction_amount,
+      coalesce(transactions_grouped.total_sales, 0) as total_sales,


Thanks for the investigation here! This is really interesting and a bit confusing why we would apply this divide by 100 approach for a select few models, but not others.

Given this I would request we take the following approach. We have the default state of the stripe__convert_values variable be false. We then apply an addition conditional to these end models with the divide by 100 to only divide by 100 if the variable is false. This way we can ensure this update will be backwards compatible and not cause major breaking changes. So these end models would look like the following.

{{ 'coalesce(transactions_grouped.total_refunds/100.0, 0)' if not var('stripe__convert_values', false) else coalesce(transactions_grouped.total_refunds, 0)' }} as total_refunds,

fivetran-joemarkiewicz · 2025-01-10T18:34:55Z

README.md

+
+```yml
+vars:
+    stripe__convert_values: False


Based on my other comment, let's change this default to false and ensure the docs show how to change this to true by default.

fivetran-joemarkiewicz · 2025-01-10T18:36:59Z

CHANGELOG.md

@@ -1,3 +1,16 @@
+# dbt_stripe v0.16.0


We'll need to update this based on the new variable being set to false now. We can likely remove the breaking change impacts but mention that the value of this variable will change in the future and they should be made aware, but if they want to set the variable to true then they should know the impact.

downgraded the version to 0.15.2 and made changes to the wording

fivetran-reneeli · 2025-01-14T20:12:57Z

@fivetran-joemarkiewicz ready for re-review! I made the /100 disabled by default, with the exception of where I added the backward compatibility. I also added a consistency test to customer_overview and switched the run script to test for stripe__convert_values: true. And updated the relevant documentation.

fivetran-reneeli · 2025-01-15T19:43:48Z

models/stripe__customer_overview.sql

Discussed during a team huddle recently and team brought up a good point about how this may be misleading if a customer did intend to turn off the convert_values but instead still this model includes the division because of the inverse logic. So this may not be the best approach. I can workshop another strategy

fivetran-joemarkiewicz

Generally this is looking really good! I have a few comments below before approving

fivetran-joemarkiewicz · 2025-01-15T17:41:36Z

CHANGELOG.md

@@ -1,6 +1,27 @@
-# dbt_stripe version.version
+# dbt_stripe v0.15.2


Please make the same update suggestions from the source PR here as well.

fivetran-joemarkiewicz · 2025-01-15T17:42:26Z

README.md

@@ -74,7 +75,7 @@ Include the following stripe package version in your `packages.yml` file:
 ```yaml
 packages:
  - package: fivetran/stripe
-    version: [">=0.15.0", "<0.16.0"]
+    version: [">=0.16.0", "<0.17.0"]


This updated isn't necessary anymore now that this is a patch update

fivetran-joemarkiewicz · 2025-01-15T17:55:02Z

models/docs.md

@@ -1,2 +1,6 @@
 {% docs source_relation -%} The source where this data was pulled from. If you are making use of the `union_schemas` variable, this will be the source schema. If you are making use of the `union_databases` variable, this will be the source database. If you are not unioning together multiple sources, this will be an empty string.
 {%- enddocs %}
+
+{% docs convert_values -%}


This actually isn't necessary since this exists in the source. So you can remove this.

fivetran-joemarkiewicz · 2025-01-15T19:51:02Z

integration_tests/tests/consistency/consistency_customer_overview.sql

+        select
+            {% for col in cols %}
+                {% if not loop.first %}, {% endif %}
+                floor(sum({{ col }})) as summed_{{ col }} -- floor and sum is to keep consistency between dev and prod aggs


When using our internal data I'm running into a test failure with this consistency test. Can you investigate why this test is failing.

Good idea to test with that schema-- it came down to incorrectly placed logic in the customer_overview model, I fixed it and the validation tests now all pass with that schema.

move cent conversions upstream to source package

fa5eba9

fivetran-reneeli linked an issue Dec 19, 2024 that may be closed by this pull request

[Feature] Standardize and move all cent conversions to the staging models #100

Open

4 tasks

fivetran-reneeli added 3 commits December 20, 2024 00:11

add changelog, up version to breaking

fea9677

update readmes

0848bbc

add blanket divide variable, add docs to readme and changelog, add to…

31384e3

… run script

fivetran-joemarkiewicz requested changes Jan 6, 2025

View reviewed changes

fivetran-reneeli linked an issue Jan 8, 2025 that may be closed by this pull request

[Documentation] Move badges below H1 heading #99

Open

4 tasks

update documentation and variable name

56c24d7

fivetran-joemarkiewicz assigned fivetran-reneeli Jan 10, 2025

fivetran-joemarkiewicz requested changes Jan 10, 2025

View reviewed changes

fivetran-reneeli added 5 commits January 14, 2025 11:48

backwards compat

6189002

update documentation now that is disabled by default

148c077

update run script and downgrade initial version

0826b48

readme

28a4415

schema

9a321ea

fivetran-reneeli linked an issue Jan 14, 2025 that may be closed by this pull request

[Feature] multi currency support #102

Open

4 tasks

fivetran-reneeli added 2 commits January 14, 2025 13:48

add consistency test for customer_overview

5e5b3fa

Merge branch 'main' into feature/standardize_cent_conversions

15ba391

fivetran-reneeli requested a review from fivetran-joemarkiewicz January 14, 2025 20:10

changelog

c197b93

fivetran-reneeli commented Jan 15, 2025

View reviewed changes

fivetran-joemarkiewicz requested changes Jan 15, 2025

View reviewed changes

fivetran-reneeli added 4 commits January 16, 2025 11:01

downgrade version

46c6490

updates

8839e92

schema and rm incorrect logic in customer overview

85b2a0a

downgrade deps

032c6ac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move cent conversions upstream to source package #101

move cent conversions upstream to source package #101

fivetran-reneeli commented Dec 18, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jan 6, 2025

fivetran-joemarkiewicz Jan 6, 2025

fivetran-reneeli Jan 8, 2025

fivetran-joemarkiewicz Jan 6, 2025

fivetran-joemarkiewicz Jan 6, 2025

fivetran-joemarkiewicz Jan 6, 2025

fivetran-reneeli Jan 8, 2025

fivetran-joemarkiewicz Jan 10, 2025

fivetran-reneeli Jan 14, 2025

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jan 10, 2025

fivetran-joemarkiewicz Jan 10, 2025

fivetran-reneeli Jan 14, 2025

fivetran-joemarkiewicz Jan 10, 2025

fivetran-reneeli Jan 14, 2025

fivetran-reneeli commented Jan 14, 2025

fivetran-reneeli Jan 15, 2025

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jan 15, 2025

fivetran-reneeli Jan 16, 2025

fivetran-joemarkiewicz Jan 15, 2025

fivetran-reneeli Jan 16, 2025

fivetran-joemarkiewicz Jan 15, 2025

fivetran-reneeli Jan 16, 2025

fivetran-joemarkiewicz Jan 15, 2025

fivetran-reneeli Jan 16, 2025

		@@ -1,6 +1,27 @@
		# dbt_stripe version.version
		# dbt_stripe v0.15.2

move cent conversions upstream to source package #101

Are you sure you want to change the base?

move cent conversions upstream to source package #101

Conversation

fivetran-reneeli commented Dec 18, 2024 • edited Loading

PR Overview

Breaking Changes

PR Checklist

Basic Validation

Detailed Validation

If you had to summarize this PR in an emoji, which would it be?

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-reneeli commented Jan 14, 2025

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-reneeli commented Dec 18, 2024 •

edited

Loading