Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

update uniqueness test #74

Merged
merged 9 commits into from
Apr 17, 2024
Merged

update uniqueness test #74

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c6a1ae0
update uniqueness test
fivetran-reneeli Apr 9, 2024
ff16898
changelog
fivetran-reneeli Apr 9, 2024
e787d29
fix version
fivetran-reneeli Apr 9, 2024
668a851
rm breaking change
fivetran-reneeli Apr 15, 2024
c3821d8
rm unique test from unique invoice line item id, and add more to the …
fivetran-reneeli Apr 16, 2024
856c775
changelog
fivetran-reneeli Apr 16, 2024
02b4c77
docs
fivetran-reneeli Apr 16, 2024
fcaf58b
new schema
fivetran-reneeli Apr 16, 2024
b147150
src yml updates
fivetran-reneeli Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# dbt_stripe_source v0.11.1
[PR [#74](https://github.com/fivetran/dbt_stripe_source/pull/74)] includes the following updates:

## 🚨 Breaking Changes 🚨
- Updates the `unique_invoice_line_item_id` uniqueness test in `stg_stripe__invoice_line_item` to include `invoice_id`. This is because `unique_invoice_line_item_id` (`unique_id` in the raw source `invoice_line_item` table) was part of an older version of Stripe that was included in the new version to help migrate internal references. See the Stripe [API update](https://stripe.com/docs/upgrades#2019-12-03) for more information. The Fivetran connector persists this in order to resolve the pagination break issue for invoice line items that was introduced by the [API update](https://stripe.com/docs/upgrades#2019-12-03).
Copy link
Contributor

Choose a reason for hiding this comment

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

Request to update this based on feedback from the test updates.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, is there a reason this is listed as a breaking change? If it is a breaking change, we will need to bump the version to v0.12.0 and also make a breaking change downstream in the dbt_stripe package.

I am not entirely sure a breaking change is needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes update based on above! And true, I changed this to a bugfix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder to make the appropriate updates here based on the other comment.


# dbt_stripe_source v0.11.0
[PR #72](https://github.com/fivetran/dbt_stripe_source/pull/72) includes the following updates:

Expand Down
2 changes: 1 addition & 1 deletion dbt_project.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
config-version: 2
name: 'stripe_source'
version: '0.11.0'
version: '0.11.1'
require-dbt-version: [">=1.3.0", "<2.0.0"]

models:
Expand Down
2 changes: 1 addition & 1 deletion integration_tests/dbt_project.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
config-version: 2

name: 'stripe_source_integration_tests'
version: '0.11.0'
version: '0.11.1'


profile: 'integration_tests'
Expand Down
7 changes: 5 additions & 2 deletions models/stg_stripe.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,11 @@ models:
- invoice_line_item_id
- invoice_id
- source_relation
- dbt_utils.unique_combination_of_columns:
combination_of_columns:
- unique_invoice_line_item_id
- invoice_id
- source_relation
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Wouldn't it make more sense to include unique_invoice_line_item_id in the unique combination of columns test above as opposed to creating a new test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, do we even need to include this in the test? It seems the original unique combination of columns test is passing just fine. Since the unique_id is an artifact of an older Stripe API, should we just remove the test on that field altogether? It seems like testing on invoice_id and invoice_line_id is sufficient. What are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see, keep the existing unique test on the combination of invoice_id and invoice_line_id, just remove the unique test on unique_invoice_line_item_id. The hesitancy I have with that is it then diverges from the ERD.

image

Given that, a good compromise could be your first suggestion, adding unique_invoice_line_item_id to the existing test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I would not create a new test in this release. I would prefer we remove the failing one and then either not test the unique_id anymore or include it in the existing test.

This may be a hot take, but I would actually recommend we don't include the unique_id at all and we just remove the test. From your investigation we have found that the unique_id is an artifact from a release from January 2020. We also can see the unique combo test on invoice_id and invoice_line_id is working as expected. I would worry that adding the unique_id to that test would not actually be an accurate representation of the uniqueness of the table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per internal discussion with team, will remove the uniqueness test from unique_id entirely. Will see if any changes are needed on the connector end.

columns:
- name: invoice_line_item_id
description: Unique identifier for the object. Note that the same line item can be shown across different invoices, so this value can appear multiple times.
Expand Down Expand Up @@ -519,8 +524,6 @@ models:
description: A string identifying the type of the source of this line item, either an invoice item or a subscription.
- name: unique_invoice_line_item_id
description: A unique id generated for old invoice line item ID's from a past version of the API
tests:
- unique
- name: period_start
description: Start of the usage period during which invoice items were added to this invoice.
- name: period_end
Expand Down