-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
[DFC Orders] Backorder stock controlled products #12888
Conversation
We want to trigger the backordering for any linked product now. So let's do that check early and then select the variants in the background. It means less data passed to the job and less space for race conditions.
We aggregate quantities over the whole order cycle to account for cancelations and order adjustments by admins.
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.
Great, a few minor comments but it looks good to me so far 👍
@order = order | ||
@linked_variants = linked_variants | ||
@linked_variants = order.variants |
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.
This now lists all variants associated with an order, whether they're linked or not. Hmm, regardless we probably only want to know about variants that had errors. I'll wait to see where the PR goes..
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, we don't actually know what users want. We have no data on that. In the pilots, I think, all variants will be linked. Looking at errors could be good but also another source for errors. I think that we should wait until this actually happens and what people say about it.
# We are only supporting producer stock at the moment. | ||
variant = item.variant | ||
variant.semantic_links.present? && | ||
(variant.on_demand == false || variant.on_hand&.negative?) |
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 wonder if the model (or a module like VariantStock) should be deciding what is backorderable? Perhaps it can even be a scope.
app/jobs/backorder_job.rb
Outdated
needed_quantity = -1 * variant.on_hand | ||
# We look at linked variants which are either stock controlled or | ||
# are on demand with negative stock. | ||
def backorder_items(order) |
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.
The method sounds like it performs the action (make a backorder), but it just selects the items. Perhaps:
def backorder_items(order) | |
def backorderable_items(order) |
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.
Looks good 👍
@mkllnk in case you missed it, deployment to staging failed. |
What? Why?
We now want stock controlled items to trigger backorders as well.
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates