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

If an order has out of stock line items display them and let admins remove them #8438

Merged

Conversation

cillian
Copy link
Contributor

@cillian cillian commented Oct 29, 2021

What? Why?

For #8178. Before only customers could delete out of stock line items and out of stock line items would not be displayed when an order was viewed in the admin area. This changes things so the out of stock line items are displayed in the admin area and gives the admin the ability to delete them.

What should we test?

  1. First follow the Steps to reproduce in the issue Can't complete out-of-stock orders in "Address" state #8178.
  2. Then make sure the out of stock line item is visible on the edit order page in the admin area.
  3. Then make sure you can delete the out of stock line item.

Release notes

If an order has out of stock line items display them and let admins remove them.

Changelog Category: User facing changes

insufficient-stock-lines

@cillian cillian self-assigned this Oct 29, 2021
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Nice!

Can you resolve the merge conflict?

@cillian cillian force-pushed the show-out-of-stock-line-items branch from 098601b to d2aa7c6 Compare November 3, 2021 18:21
@cillian
Copy link
Contributor Author

cillian commented Nov 3, 2021

@mkllnk no problem. I think this is okay now, I rebased with the latest master but didn't get a conflict.

Copy link
Contributor

@Matt-Yorkley Matt-Yorkley left a comment

Choose a reason for hiding this comment

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

Alright... so as a bit of background to the underlying issue; the cart page has a certain bit of logic that uses line items with zero quantity as a kind of "placeholder" to allow the customer to see when an item in their cart has gone out of stock whilst shopping. You can't tell the customer which item is out of stock unless you keep a record in the database of that item. Makes sense. The problem with that is that the rest of the app code is not designed with that implementation in mind; the idea that a line item with zero quantity can even exist is basically a total anomaly from the point of view of the backoffice code, for example. That's where this inconsistency comes from, there's certain cases where these zero-quantity items can be created but the order edit page doesn't recognise them.

I think the approach you've gone for is good, it's definitely moving things in the right direction 👍👍 There's a little bit more complexity here that needs looking at though.

So... these zero-quantity items should only really exist in orders in an incomplete state, and they should only really be temporary. When a line item is in the cart but the order is not completed, the item hasn't actually been sold yet and the stock has not been reduced. An order can't be completed with a zero-quantity line item (because they can't be sold). Also; after an order is completed the items actually are sold, and the stock is reduced. At this point the sale has been done, and for example one of those line items in the order could very well be pointing to a product which has subsequently gone out of stock (imagine an order is placed for the last bag of carrots, after the order is completed there are no carrots left).

If you edit a completed order (with this PR) and some of the items are out of stock, it will show the "out of stock" feedback in big red letters, which is not actually correct in that context. So I think what we need to do is only apply that check (if @order.insufficient_stock_lines.any?) and the partial that shows the "out of stock" items when the order is not yet complete.

Does that make sense?

Whew, I had to burn a few neurons there putting all that into words 😅

@@ -2,6 +2,9 @@
- if @line_item.try(:errors).present?
= render :partial => 'spree/shared/error_messages', :locals => { :target => @line_item }

- if @order.insufficient_stock_lines.any?
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley Nov 16, 2021

Choose a reason for hiding this comment

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

TLDR version:

Suggested change
- if @order.insufficient_stock_lines.any?
- if [email protected]? && @order.insufficient_stock_lines.any?

I think that'll be sufficient ☝️

We might want a little bit more in terms of test coverage though..?

@cillian cillian force-pushed the show-out-of-stock-line-items branch 4 times, most recently from 86f9160 to ca00f4e Compare December 3, 2021 12:59
@cillian
Copy link
Contributor Author

cillian commented Dec 3, 2021

@Matt-Yorkley Thanks for the detailed explanation 😅, good to know. I have made that adjustment now, added view specs and rebased.

@Matt-Yorkley Matt-Yorkley force-pushed the show-out-of-stock-line-items branch from ca00f4e to e2b8108 Compare January 7, 2022 13:18
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley left a comment

Choose a reason for hiding this comment

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

Thanks ❤️

@Matt-Yorkley
Copy link
Contributor

Note: I rebased on master and tweaked some of the CSS bits, as we've recently changed how our CSS assets are bundled 👌

@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jan 24, 2022
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Jan 24, 2022

Hey @cillian ,
Thanks for this improvement!

Before this PR:

image

After this PR:
There is agreement with the order information displayed for the shopfront (customer) and backoffice (admin). Now the admin can:

  • delete any line items out of stock
  • add other items to the order -> this 'unblocks' the order and allows the customer to finish checking out or allows order completion in the backoffice.

Peek 2022-01-24 12-13

Awesome!! 🎉
Good to go.

@filipefurtad0 filipefurtad0 merged commit f17226f into openfoodfoundation:master Jan 24, 2022
@RachL RachL removed the pr-staged-uk staging.openfoodnetwork.org.uk label Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants