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.

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

inventory level model #46

Merged
merged 10 commits into from
Jan 9, 2023

Conversation

fivetran-jamie
Copy link
Contributor

@fivetran-jamie fivetran-jamie commented Jan 4, 2023

Are you a current Fivetran customer?

fivetran made PR

What change(s) does this PR introduce?

  • adds inventory level model and an intermediate model supporting it
  • removed duplicative/deprecated columns from order_lines (just to avoid failures for now, will come back to this model in earnest later)
  • enhances product model with new tables (I thought product and inventory_level would overlap more in the DAG, but this didn't really happen. consider this an early start on the "enhancing existing models" task)

Did you update the CHANGELOG?

  • Yes
  • nope!! will save it for the documentation task

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • No (please provide an explanation as to how the change is non-breaking below.)

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)

  • Yes

Is this PR in response to a previously created Bug or Feature Request

  • Yes, Issue/Feature [link bug/feature number here]
  • No but inventory modeling has come up plenty with customers

How did you test the PR changes?

  • Buildkite
  • Local (please provide additional testing details below)

Select which warehouse(s) were used to test the PR

  • BigQuery
  • Redshift
  • Snowflake
  • Postgres
  • Databricks
  • Other (provide details below)

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.

@fivetran-joemarkiewicz fivetran-joemarkiewicz added type:enhancement New functionality or enhancement update_type:models Primary focus requires model updates labels Jan 5, 2023
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

Hey @fivetran-jamie these end models are looking great!! I really like the enhancement we are providing in them and think customers will get a lot of value from them! I have a few change requests and suggestions that should be addressed before this will be good to approve!

  • Address inline comments
  • Document the end models in the yml if possible. It will make for easier review and understanding of the models.
  • I believe we discussed this and it didn't make sense in the package at the moment - Is there a way we could get a daily inventory model? I could see the inventory levels model being killer if it showed a by day break down. I built one at my last job at it was a fan favorite. That would be awesome if we could do, but I imagine that will be pretty complex. Maybe more of a good community post than a model to start out (I think we chatted about that as well).

models/shopify.yml Outdated Show resolved Hide resolved
models/shopify__inventory_levels.sql Show resolved Hide resolved
models/shopify__products.sql Outdated Show resolved Hide resolved
models/shopify__products.sql Outdated Show resolved Hide resolved
models/shopify__products.sql Outdated Show resolved Hide resolved
packages.yml Outdated
@@ -3,7 +3,7 @@ packages:
# version: [">=0.7.0", "<0.8.0"]

- git: https://github.com/fivetran/dbt_shopify_source.git
revision: feature/package-revamp
revision: feature/revamp/transform-enhancements
Copy link
Contributor

Choose a reason for hiding this comment

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

Obligatory reminder to update before merge

@fivetran-jamie
Copy link
Contributor Author

@fivetran-joemarkiewicz yeah so re: daily inventory -- available inventory quantity is only stored as-of-today. a daily model would be possible by taking a snapshot of this model each day. i think it would be worth highlighting that pathway in the README or somewhere, in addition to a community/blog post

@fivetran-joemarkiewicz
Copy link
Contributor

@fivetran-joemarkiewicz yeah so re: daily inventory -- available inventory quantity is only stored as-of-today. a daily model would be possible by taking a snapshot of this model each day. i think it would be worth highlighting that pathway in the README or somewhere, in addition to a community/blog post

Ahhh yes this conversation is coming back to me! Yeah let's do the community/blog post after the fact. Thanks for reminding me!

fivetran-jamie and others added 3 commits January 5, 2023 15:02
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

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

@fivetran-jamie I have a few final questions about som To Do comments, but I believe these are probably for different updates to be completed outside of this PR.

If that is the case, the updates you made look great and I will approve this PR 😄

Comment on lines +19 to +21

todo - not filtering out _fivetran_deleted in staging models. when joining these tables together in the transform package, bring in _fivetran_deleted as is_<foreign key table>_deleted
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this piece is 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note for later -- this is about no longer filtering out soft-deleted records and bringing in the is_Deleted fields (similar to the hubspot PR sheri just worked on)

shopify__using_order_line_refund: false # true by default
shopify__using_refund: false # true by default
```
## Step 4: TODO - timezone converting, maybe should be optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be a separate update? If so, I can will skip over for this review.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just left it for now since i removed the section about the using__* variables and didn't wanna update the step numbers yet

dbt_project.yml Outdated
Comment on lines 13 to 16
int_shopify__inventory_level__aggregates:
+materialized: table # just for testing. remove later
int_shopify__products_with_aggregates:
+materialized: table # just for testing. remove later
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 switch before merge

@fivetran-jamie fivetran-jamie merged commit 228f89e into feature/package-revamp Jan 9, 2023
@fivetran-jamie fivetran-jamie deleted the feature/revamp/inventory-model branch January 9, 2023 18:13
@fivetran-jamie fivetran-jamie mentioned this pull request Jan 31, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New functionality or enhancement update_type:models Primary focus requires model updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants