Releases/v0.12.latest #82

fivetran-jamie · 2024-06-07T20:43:29Z

PR Overview

This PR will address the following Issue/Feature:

[Feature] Shopify test table warning dbt_shopify_source#83 by way of including Update shopify__daily_shop model to include new 'delayed' status from Shopify #81

See upstream source PR for additional issue addressed

This PR will result in the following new package version:

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

🪲 Bug Fixes 🪛

Added support for a new delayed fulfillment event status from Shopify. This produces a new count_fulfillment_delayed field in the shopify__daily_shop model.

Contributors

@shreveasaurus PR #81

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 tests added

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

⏳

Update code to include new 'delayed' status from Shopify

Update shopify__daily_shop model to include new 'delayed' status from Shopify

fivetran-jamie · 2024-06-07T20:56:36Z

packages.yml

-  version: [">=0.12.0", "<0.13.0"]
+# - package: fivetran/shopify_source
+#   version: [">=0.12.0", "<0.13.0"]
+  - git: https://github.com/fivetran/dbt_shopify_source.git


will update prior to merging

fivetran-joemarkiewicz

@fivetran-jamie this PR looks good to me! There are a few minor comments and questions, but nothin that would block this from being approved. Please address these comments before initiating the release review, but this is good from my end otherwise!

fivetran-joemarkiewicz · 2024-06-10T13:27:23Z

integration_tests/tests/integrity/vertical_integrity_daily_shop_fulfillment.sql

+        count(distinct case when status = 'delivered' then fulfillment_id end) as count_fulfillment_delivered
+
+    from source
+    where happened_at > '2023-01-01' and happened_at < '2024-01-01'


What is the reason for this filter? My only concern with this filter is if you run the integrity validation on a data set that doesn't have data for this period then it won't validate the results effectively. Is there a concern for removing this filter and running the validation for all records?

Same comment for the other integrity tests

ah this was just me avoiding live-data messing things up, will adjust to be wider

fivetran-joemarkiewicz · 2024-06-10T13:29:18Z

integration_tests/tests/consistency/consistency_daily_shop.sql

+        count(count_customers) as count_customers,
+        sum(order_adjusted_total) as order_adjusted_total,
+        sum(count_abandoned_checkouts) as count_abandoned_checkouts,
+        sum(count_fulfillment_attempted_delivery) as count_fulfillment_attempted_delivery,


This validation must have the shopify_using_fulfillment_event variable defined as true (default: false) in order for the validation to succeed.

Would you either be able to put a conditional in here to ensure the validation runs as expected regardless of the variable being true/false. Or leave a comment to callout that the shopify_using_fulfillment_event variable must be true in order for this to succeed.

ah i'll add a conditional

fivetran-joemarkiewicz · 2024-06-10T13:36:34Z

integration_tests/tests/integrity/zontal_integrity_daily_shop_orders.sql

+        order_metrics.count_orders as order_count_orders,
+        daily_shop_metrics.order_adjusted_total as daily_shop_order_adjusted_total,
+        order_metrics.order_adjusted_total as order_order_adjusted_total


We should add a coalesce here because if there are no orders at the source then the above ctes will produce null records here. To ensure the where clause at the bottom is always accurate it would be best to treat a null record as a 0.

Suggested change

order_metrics.count_orders as order_count_orders,

daily_shop_metrics.order_adjusted_total as daily_shop_order_adjusted_total,

order_metrics.order_adjusted_total as order_order_adjusted_total

coalesce(order_metrics.count_orders, 0) as order_count_orders,

daily_shop_metrics.order_adjusted_total as daily_shop_order_adjusted_total,

coalesce(order_metrics.order_adjusted_total, 0) as order_order_adjusted_total

added myself

fivetran-joemarkiewicz · 2024-06-10T13:37:36Z

integration_tests/tests/integrity/zontal_integrity_daily_shop_orders.sql

I am assuming we named this "zontal_*" because the naming was too long for the warehouses. If so, I am okay with this approach but can we update our Slab documentation on validation tests to highlight this naming convention and be sure to share the standard with the team.

lol no i can update -- i was just using slang for "horizontal" for funzies

fivetran-avinash · 2024-06-11T18:58:12Z

CHANGELOG.md

@@ -1,3 +1,13 @@
+# dbt_shopify v0.12.1
+
+[PR #81](https://github.com/fivetran/dbt_shopify/pull/81) includes the following updates: 


The latest PR is #82, so I'd change this from #81 to #82 or do what you did in the source package (call out the specific change that was applied in #81 on the bug fixes line).

removed this line and added the PRs to the exact change line items

fivetran-avinash · 2024-06-11T18:58:44Z

CHANGELOG.md

+
+## 🪲 Bug Fixes 🪛
+- Added support for a new `delayed` fulfillment event status from Shopify. This produces a new `count_fulfillment_delayed` field in the `shopify__daily_shop` model.
+


I'd add your consistency/integrity tests as Under the Hood and that can be the #82 changes.

This reverts commit 9dc9de3.

shreveasaurus and others added 7 commits May 30, 2024 10:28

Update shopify__daily_shop.sql

df5e496

Update code to include new 'delayed' status from Shopify

Bump package version

89f0b09

Update CHANGELOG.md

f271d18

Bump project version

1baf570

Update link to PR in changelog

946f7b4

Merge pull request #81 from shreveasaurus/shreveasaurus-patch-1

7a4acbd

Update shopify__daily_shop model to include new 'delayed' status from Shopify

testing

ea6bbb1

fivetran-jamie self-assigned this Jun 7, 2024

fivetran-jamie commented Jun 7, 2024

View reviewed changes

validation tests

577d6dc

fivetran-jamie mentioned this pull request Jun 7, 2024

Releases/v0.12.latest fivetran/dbt_shopify_source#87

Merged

7 tasks

docs

7bcf0de

fivetran-joemarkiewicz self-requested a review June 10, 2024 13:11

fivetran-joemarkiewicz approved these changes Jun 10, 2024

View reviewed changes

fivetran-jamie added 2 commits June 11, 2024 11:28

joe feedback

c6a1690

change package ref

67157bb

fivetran-avinash reviewed Jun 11, 2024

View reviewed changes

avinash feedback

92a83ad

fivetran-avinash approved these changes Jun 11, 2024

View reviewed changes

fivetran-jamie merged commit 9dc9de3 into main Jun 11, 2024
9 checks passed

ranjeetmqat added a commit to MarqueeBrands/dbt_marquee_shopify that referenced this pull request Jun 19, 2024

Revert "Releases/v0.12.latest (fivetran#82)"

4d6c833

This reverts commit 9dc9de3.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Releases/v0.12.latest #82

Releases/v0.12.latest #82

fivetran-jamie commented Jun 7, 2024 •

edited

Loading

fivetran-jamie Jun 7, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Jun 10, 2024

fivetran-joemarkiewicz Jun 10, 2024

fivetran-jamie Jun 11, 2024

fivetran-joemarkiewicz Jun 10, 2024

fivetran-joemarkiewicz Jun 10, 2024

fivetran-jamie Jun 10, 2024

fivetran-joemarkiewicz Jun 10, 2024

fivetran-jamie Jun 11, 2024

fivetran-joemarkiewicz Jun 10, 2024

fivetran-jamie Jun 10, 2024

fivetran-avinash Jun 11, 2024

fivetran-jamie Jun 11, 2024

fivetran-avinash Jun 11, 2024

fivetran-jamie Jun 11, 2024

		@@ -1,3 +1,13 @@
		# dbt_shopify v0.12.1

		[PR #81](https://github.com/fivetran/dbt_shopify/pull/81) includes the following updates:


		## 🪲 Bug Fixes 🪛
		- Added support for a new `delayed` fulfillment event status from Shopify. This produces a new `count_fulfillment_delayed` field in the `shopify__daily_shop` model.

Releases/v0.12.latest #82

Releases/v0.12.latest #82

Conversation

fivetran-jamie commented Jun 7, 2024 • edited Loading

PR Overview

🪲 Bug Fixes 🪛

Contributors

PR Checklist

Basic Validation

Detailed Validation

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

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

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-jamie commented Jun 7, 2024 •

edited

Loading