convert cent based fields to major currency unit based #87

fivetran-reneeli · 2024-12-18T18:43:20Z

PR Overview

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

This PR will result in the following new package version: v0.13.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 Change

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 applying these updates. I have a few comments below before approval. Let me know if you would like to discuss in more detail.

models/stg_stripe__balance_transaction.sql

CHANGELOG.md

README.md

CHANGELOG.md

models/stg_stripe.yml

README.md

fivetran-joemarkiewicz

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

macros/convert_values.sql

CHANGELOG.md

models/docs.md

macros/convert_values.sql

fivetran-joemarkiewicz

A few remaining comments before approval. Let me know if you have any questions.

CHANGELOG.md

models/docs.md

macros/convert_values.sql

README.md

Co-authored-by: Joe Markiewicz <[email protected]>

fivetran-reneeli

Thanks @fivetran-joemarkiewicz , changes applied!

README.md

models/docs.md

macros/convert_values.sql

fivetran-joemarkiewicz

@fivetran-reneeli a few more change suggestions and notes before approval. The largest change is the request to make the macro more dynamic via the adapter.dispatch and then namespace references. Also, this will need to be a breaking change. See my notes below.

CHANGELOG.md

README.md

integration_tests/dbt_project.yml

fivetran-joemarkiewicz · 2025-01-17T21:55:26Z

macros/convert_values.sql

+{% macro convert_values(field_name, divide_by=100.0, divide_var=var('stripe__convert_values',false), alias=None) %}
+    {% if divide_var %}
+        {{ field_name }} / {{ divide_by }} as {{ alias if alias else field_name }}
+    {% else %}
+        {{ field_name }} as {{ alias if alias else field_name }}
+    {% endif %}
+{% endmacro %}


I apologize for not catching this earlier, but we should add the adapter dispatch here in case users ever need to override this while also allowing there to not be any issues if customers have macros with the same name in their own projects (which is likely with a macro named convert_values).

Can you update this to resemble the following structure seen here. Essentially adding the dispatch and then having the default version.

updated the macro, let me know if you have any questions

models/stg_stripe__credit_note.sql

Co-authored-by: Joe Markiewicz <[email protected]>

fivetran-joemarkiewicz

LGTM!

fivetran-jamie · 2025-01-22T20:37:57Z

models/stg_stripe.yml

@@ -14,7 +14,7 @@ models:
        tests:
          - not_null
      - name: amount
-        description: Gross amount of the transaction, in cents.
+        description: Gross amount of the transaction. "{{ doc('convert_values') }}"


I've never thought of using doc blocks in this way, neat!

fivetran-jamie

LGTM

fivetran-reneeli added 5 commits December 19, 2024 02:42

convert cent based fields to major currency unit based

8e3c320

add changelog and version up to breaking change

f676339

update readmes

11b4843

docs

9865584

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

677b5f2

… run script

fivetran-joemarkiewicz requested changes Jan 6, 2025

View reviewed changes

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

[Documentation] Move badges below H1 heading #86

Closed

4 tasks

fivetran-reneeli added 3 commits January 7, 2025 14:51

update stg.yml documentation

9ea9eb3

add convert_value macro, change variable name

ebb88e3

update docs

41a2cf3

fivetran-joemarkiewicz requested changes Jan 10, 2025

View reviewed changes

macros/convert_values.sql Outdated Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

models/docs.md Outdated Show resolved Hide resolved

fivetran-reneeli added 3 commits January 13, 2025 18:20

update convert macro

ffee358

update relevant documentation now that default is false

4fe8270

schema and docs

5af2ed5

fivetran-reneeli commented Jan 13, 2025

View reviewed changes

macros/convert_values.sql Show resolved Hide resolved

fivetran-reneeli added 2 commits January 14, 2025 11:49

downgrade version

816f94a

reamde

d4a391d

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

fivetran-reneeli added 3 commits January 14, 2025 15:14

Merge branch 'main' into feature/standardize_cent_conversions

4d8fe6d

changelog

e1354bd

schema

d481009

fivetran-joemarkiewicz assigned fivetran-reneeli Jan 15, 2025

fivetran-joemarkiewicz requested changes Jan 15, 2025

View reviewed changes

fivetran-joemarkiewicz reviewed Jan 15, 2025

View reviewed changes

README.md Show resolved Hide resolved

fivetran-reneeli and others added 6 commits January 16, 2025 11:01

updates

27a7b5e

Update CHANGELOG.md

a150a3d

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

ce47548

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

dc657b9

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

5010d7a

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

c4a8bae

Co-authored-by: Joe Markiewicz <[email protected]>

update macro

77ab32c

fivetran-reneeli commented Jan 16, 2025

View reviewed changes

README.md Show resolved Hide resolved

models/docs.md Outdated Show resolved Hide resolved

macros/convert_values.sql Show resolved Hide resolved

schema

254019e

fivetran-joemarkiewicz requested changes Jan 17, 2025

View reviewed changes

fivetran-reneeli and others added 11 commits January 21, 2025 12:28

Update CHANGELOG.md

dd223d6

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

ba36b57

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

e4de4f4

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

94e2852

Co-authored-by: Joe Markiewicz <[email protected]>

Update integration_tests/dbt_project.yml

1ed1537

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

305ec7d

Co-authored-by: Joe Markiewicz <[email protected]>

Update CHANGELOG.md

c1e285f

Co-authored-by: Joe Markiewicz <[email protected]>

updates

3970a6a

versioning and docs

92a784d

fix

2854b49

put back aliases and fix some docs, regen docs

79658db

fivetran-reneeli requested a review from fivetran-joemarkiewicz January 22, 2025 15:45

update macro and docs

9a3ad8d

fivetran-joemarkiewicz approved these changes Jan 22, 2025

View reviewed changes

fivetran-jamie reviewed Jan 22, 2025

View reviewed changes

fivetran-jamie approved these changes Jan 22, 2025

View reviewed changes

fivetran-reneeli merged commit d41ea2c into main Jan 22, 2025
8 checks passed

fivetran-reneeli deleted the feature/standardize_cent_conversions branch January 22, 2025 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert cent based fields to major currency unit based #87

convert cent based fields to major currency unit based #87

fivetran-reneeli commented Dec 18, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz left a comment

fivetran-reneeli left a comment •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jan 17, 2025

fivetran-reneeli Jan 21, 2025

fivetran-reneeli Jan 21, 2025

fivetran-joemarkiewicz left a comment

fivetran-jamie Jan 22, 2025

fivetran-jamie left a comment

convert cent based fields to major currency unit based #87

convert cent based fields to major currency unit based #87

Conversation

fivetran-reneeli commented Dec 18, 2024 • edited Loading

PR Overview

Breaking Change

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

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-reneeli left a comment • edited Loading

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz Jan 17, 2025

Choose a reason for hiding this comment

fivetran-reneeli Jan 21, 2025

Choose a reason for hiding this comment

fivetran-reneeli Jan 21, 2025

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-jamie Jan 22, 2025

Choose a reason for hiding this comment

fivetran-jamie left a comment

Choose a reason for hiding this comment

fivetran-reneeli commented Dec 18, 2024 •

edited

Loading

fivetran-reneeli left a comment •

edited

Loading