-
Notifications
You must be signed in to change notification settings - Fork 40
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
inventory level model #46
Conversation
There was a problem hiding this 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).
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 |
There was a problem hiding this comment.
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-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! |
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
There was a problem hiding this 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 😄
|
||
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 |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
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
int_shopify__inventory_level__aggregates: | ||
+materialized: table # just for testing. remove later | ||
int_shopify__products_with_aggregates: | ||
+materialized: table # just for testing. remove later |
There was a problem hiding this comment.
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
Are you a current Fivetran customer?
fivetran made PR
What change(s) does this PR introduce?
Did you update the CHANGELOG?
Does this PR introduce a breaking change?
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)
Is this PR in response to a previously created Bug or Feature Request
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
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.