Rescue from `Spree::Order::InsufficientStock` on `frontend` checkout flow #3023

spaghetticode · 2019-01-04T14:09:38Z

When two users try to purchase the last item remaining from a non-backordeable
stock location at the same time then the last one will experience an unhandled
error Spree::Order::InsufficientStock.

This happens only if there is a second backorderable stock location for the
product.

When there is no backorderable stock location the controller before_action
ensure_sufficient_stock_lines is enough to catch the issue in advance.

The error is generated by this line in Spree::Order model:

before_transition to: :complete, do: :validate_line_item_availability

Generally, ensure_sufficient_stock_lines prevent customers to complete the
checkout process when there is not enough stock availability, but the case
above is not caught here.

So, by using rescue_from the customer is now redirected to the checkout
address page and shown an error message that suggests to repeat the
checkout process.

The order's shipments will be rebuilt using the backorderable stock location
in the delivery step, allowing them to (hopefully!) complete the purchase.

rescue_from is already used on the api and backend section in order to
manage InsufficientStock errors, so this was a natural choice also on the
frontend.

frontend/app/controllers/spree/checkout_controller.rb

kennyadsl

Thanks @spaghetticode !

kennyadsl

Checking twice, I left some comments. Let me know if they make sense, thanks!

kennyadsl · 2019-01-06T17:25:43Z

frontend/app/controllers/spree/checkout_controller.rb

@@ -161,6 +162,19 @@ def ensure_sufficient_stock_lines
      end
    end

+    def ensure_sufficient_shipment_stock_quantities
+      if @order.passed_checkout_step?('delivery')


This line is assuming the step after delivery is the last step, which is not the case for everyone since steps are configurable. Is there a way to refine this check in order to be more generic?

@kennyadsl It's not necessarily assuming that the following step is the last... but it must be executed after the order passed the delivery step, for some reasons.

I'll share some context regarding why I added this if clause, I think it may help us in finding a better solution for the issue you exposed.

The reason is that, without that if, this spec fails. The spec seems to be still relevant to me, so adding the if allowed me to add the functionality while still keeping the existing behavior.

Today I tried to dig deeper in the issue regarding the spec mentioned above, and this is what I found out. After moving my before_action to the end of the chain things improved a little, but not enough.

The reason for having that specific if is that the before_action CheckoutController::before_payment may manipulate the order, specifically these lines:

@differentiator = Spree::Stock::Differentiator.new(@order, packages) @differentiator.missing.each do |variant, quantity| @order.contents.remove(variant, quantity) end

Besides, the line @order.contents.remove(variant, quantity) has the side effect of changing the order state from payment to address 🤔. Have not checked why, but I guess some ActiveRecord callback is responsible for that.

Missing contents must be removed before running the new before_action, or behavior will change, and as said before, the removal happens only when the customer tries to see the checkout/payment page, which happens after delivery is completed. I think the state ordering delivery -> payment is quite robust, as generally order total should be calculated only after shipping charges and taxes are known, both depending on addresses. But YMMV, so robust may not be enough.

Another option would be to move the new code in the before_payment method, so it's executed when it starts to be relevant, but that would limit its execution only to that precise step, eventually limiting its usefulness, that's why I think this is not a viable solution.

The current solution has the strong disadvantage that @order.passed_checkout_step?('delivery') does not make clear why the step must be passed, and the strong connection with the existing before_payment method.

A different approach may be to leverage the existing code in before_payment and change the if to this, for example:

packages = @order.shipments.map(&:to_package) @differentiator = Spree::Stock::Differentiator.new(@order, packages) if @differentiator.missing.empty? # ...

This way, there's no more explicit dependence on order steps and their names. What do you think?

kennyadsl · 2019-01-06T17:28:49Z

frontend/app/controllers/spree/checkout_controller.rb

+          flash[:error] = t('spree.inventory_error_flash_for_insufficient_shipment_quantity', unavailable_items: unavailable_items)
+          @order.restart_checkout_flow
+          @order.next!
+          redirect_to spree.checkout_state_path(:delivery)


Is restarting the flow required? I think it will move the order to cart, and it's also not clear why next! is needed. What if we just push the user back to the delivery step?

Yes, the goal is to revert the order status to delivery, and there is no apparent need to restart from the cart state, but my guess is that using #restart_checkout_flow and add one #next! call may be better, at least in the long run, as #ensure_updated_shipments might develop in a more complex method that adds more (required) behavior to the restarting process. I'd rather follow the path than create a new one with code such as @order.update state: :delivery or someting similar.

On the other side what makes me frown a bit is the @order.next! line, as it may have one meaning today (advance the order state to delivery) but that can change in the future.

Maybe the best thing would be to restart_checkout_flow and remove the next!.

I'm not sure about what's the best solution, so I'm committed to following the advice of people with a better understanding of the checkout process and its intricacies.

I found something that it's not clear on a second look

kennyadsl

Found a little issue but I think this works. Thanks @spaghetticode !

kennyadsl · 2019-01-09T21:54:26Z

frontend/app/controllers/spree/checkout_controller.rb

+      packages = @order.shipments.map(&:to_package)
+      if packages.empty?
+        flash[:error] = I18n.t('spree.insufficient_stock_for_order')
+        redirect_to cart_admin_order_url(@order)


This path should probably not contain _admin_. Also maybe we should use _path instead of _url, right?

good catch! 👍

kennyadsl · 2019-01-09T21:55:23Z

frontend/spec/features/checkout_confirm_insufficient_stock_spec.rb

+        it 'redirects to cart page and shows an unavailable product message' do
+          click_button "Place Order"
+          expect(page).to have_content "#{order_product.name} became unavailable"
+          expect(page).to have_current_path spree.cart_path


not sure how this could pass if the path in the controller was wrong, can you please double check?

Specs pass because they don't exercise the path that was wrong. I'll fix the path and add another test for that branch of the if.

spaghetticode · 2019-01-10T12:57:48Z

@kennyadsl I updated the code according to your comments, adding a couple of commits to facilitate your understanding of the changes.

When it's ok for you I'd like to rebase/squash and update the PR description/commit message in order to reflect the latest changes introduced. Just let me know when it's fine for you (or please add more comments if something is still not OK 😸 )

kennyadsl · 2019-01-10T13:05:57Z

@spaghetticode go ahead with the commits squash!

kennyadsl

Thanks!

rubenochiavone · 2019-01-12T21:10:05Z

frontend/app/controllers/spree/checkout_controller.rb

+      else
+        availability_validator = Spree::Stock::AvailabilityValidator.new
+        unless @order.line_items.all? { |item| availability_validator.validate(item) }
+          unavailable_items = @order.line_items.map(&:name).to_sentence


@order.line_items.all? { |item| availability_validator.validate(item) } means that at least one line item is not valid, right? So, I'm not sure that unavailable_items = @order.line_items.map(&:name).to_sentence is right because it lists all line items as unavailable. Perhaps

unavailable_items = @order.line_items.select { |line_item| !availability_validator.validate(line_item) }.map(&:name).to_sentence

would do (haven't tested). WDYT?

You're right, something went wrong with my cut & paste refactoring. It should be fine now, thank you for spotting the bug 🐛

houndci-bot · 2019-01-12T21:51:37Z

frontend/app/controllers/spree/checkout_controller.rb

+        redirect_to cart_path
+      else
+        availability_validator = Spree::Stock::AvailabilityValidator.new
+        unavailable_items = @order.line_items.select { |line_item| !availability_validator.validate(line_item) }


Style/InverseMethods: Use reject instead of inverting select.

When two users try to purchase the last item remaining from a non-backordeable stock location at the same time then the last one will experience an unhandled error `Spree::Order::InsufficientStock`. This happens only if there is a second backorderable stock location for the product. When there is no backorderable stock location the controller `before_action` `ensure_sufficient_stock_lines` is enough to catch the issue in advance. The error is generated by this line in Spree::Order model: `before_transition to: :complete, do: :validate_line_item_availability` Generally, `ensure_sufficient_stock_lines` prevent customers to complete the checkout process when there is not enough stock availability, but the case above is not caught here. So, by using `rescue_from` the customer is now redirected to the checkout `address` page and shown an error message that suggests to repeat the checkout process. The order's shipments will be rebuilt using the backorderable stock location in the delivery step, allowing them to (hopefully!) complete the purchase. `rescue_from` is already used on the `api` and `backend` section in order to manage `InsufficientStock` errors, so this was a natural choice also on the `frontend`.

ericsaupe

Works for me. I think it's a little awkward for the user but it's better than throwing an exception to the user.

houndci-bot reviewed Jan 4, 2019

View reviewed changes

frontend/app/controllers/spree/checkout_controller.rb Outdated Show resolved Hide resolved

spaghetticode force-pushed the issue-3020-availability-validations branch 2 times, most recently from eda8963 to ba0dff5 Compare January 4, 2019 14:13

kennyadsl previously approved these changes Jan 4, 2019

View reviewed changes

kennyadsl reviewed Jan 6, 2019

View reviewed changes

kennyadsl requested changes Jan 9, 2019

View reviewed changes

spaghetticode force-pushed the issue-3020-availability-validations branch from 99b56ea to e24a08a Compare January 10, 2019 11:05

spaghetticode force-pushed the issue-3020-availability-validations branch from e24a08a to 73fffa6 Compare January 10, 2019 16:21

spaghetticode changed the title ~~Improve stock validations on frontend checkout flow~~ Rescue from Spree::Order::InsufficientStock on frontend checkout flow Jan 10, 2019

kennyadsl approved these changes Jan 11, 2019

View reviewed changes

kennyadsl added type:bug Error, flaw or fault changelog:solidus_frontend Changes to the solidus_frontend gem Needs Core Team Decision labels Jan 12, 2019

rubenochiavone mentioned this pull request Jan 12, 2019

Fix Spree::Order::InsufficientStock error raised when confirming an order #3035

Closed

3 tasks

rubenochiavone reviewed Jan 12, 2019

View reviewed changes

spaghetticode force-pushed the issue-3020-availability-validations branch from 73fffa6 to 32cbdc6 Compare January 12, 2019 21:51

houndci-bot reviewed Jan 12, 2019

View reviewed changes

spaghetticode force-pushed the issue-3020-availability-validations branch from 32cbdc6 to fb9a631 Compare January 12, 2019 21:52

cbrunsdon approved these changes Jan 16, 2019

View reviewed changes

ericsaupe approved these changes Jan 16, 2019

View reviewed changes

kennyadsl merged commit b825223 into solidusio:master Jan 18, 2019

kennyadsl deleted the issue-3020-availability-validations branch January 18, 2019 14:44

kennyadsl removed the Needs Core Team Decision label Jan 18, 2019

kennyadsl mentioned this pull request Jan 18, 2019

Spree::Order::InsufficientStock error raised when confirming an order #3020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rescue from `Spree::Order::InsufficientStock` on `frontend` checkout flow #3023

Rescue from `Spree::Order::InsufficientStock` on `frontend` checkout flow #3023

spaghetticode commented Jan 4, 2019 •

edited by kennyadsl

kennyadsl left a comment

kennyadsl left a comment

kennyadsl Jan 6, 2019

spaghetticode Jan 7, 2019

kennyadsl Jan 6, 2019

spaghetticode Jan 6, 2019

kennyadsl left a comment

kennyadsl Jan 9, 2019

spaghetticode Jan 9, 2019

kennyadsl Jan 9, 2019

spaghetticode Jan 9, 2019

spaghetticode commented Jan 10, 2019 •

edited

kennyadsl commented Jan 10, 2019

kennyadsl left a comment

rubenochiavone Jan 12, 2019

spaghetticode Jan 12, 2019

houndci-bot Jan 12, 2019

ericsaupe left a comment

Rescue from Spree::Order::InsufficientStock on frontend checkout flow #3023

Rescue from Spree::Order::InsufficientStock on frontend checkout flow #3023

Conversation

spaghetticode commented Jan 4, 2019 • edited by kennyadsl

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl 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

kennyadsl 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

spaghetticode commented Jan 10, 2019 • edited

kennyadsl commented Jan 10, 2019

kennyadsl 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

ericsaupe left a comment

Choose a reason for hiding this comment

Rescue from `Spree::Order::InsufficientStock` on `frontend` checkout flow #3023

Rescue from `Spree::Order::InsufficientStock` on `frontend` checkout flow #3023

spaghetticode commented Jan 4, 2019 •

edited by kennyadsl

spaghetticode commented Jan 10, 2019 •

edited