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

Autoremove/update item from the cart if stock changed during checkout #13121

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions app/assets/javascripts/templates/out_of_stock.html.haml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// please update the modal html in app/view/checkout/edit.html.haml if updating this template
%a.close-reveal-modal{"ng-click" => "$close()"}
%i.ofn-i_009-close

Expand Down
5 changes: 5 additions & 0 deletions app/controllers/checkout_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def edit
if params[:step].blank?
redirect_to_step_based_on_order
else
handle_insufficient_stock if details_step?
update_order_state
check_step
end
Expand All @@ -36,6 +37,9 @@ def edit
end

def update
return render cable_ready: cable_car.redirect_to(url: checkout_step_path(:details)) \
unless sufficient_stock?

if confirm_order || update_order
return if performed?

Expand Down Expand Up @@ -152,6 +156,7 @@ def order_params
# state. We need to do this when moving back to a previous checkout step, the update action takes
# care of moving the order state forward.
def update_order_state
# debugger
Copy link
Member

Choose a reason for hiding this comment

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

?

return @order.back_to_payment if @order.confirmation? && payment_step?

return unless @order.after_delivery_state? && details_step?
Expand Down
1 change: 0 additions & 1 deletion app/controllers/concerns/checkout_callbacks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ module CheckoutCallbacks

before_action :ensure_order_not_completed
before_action :ensure_checkout_allowed
before_action :handle_insufficient_stock
before_action :check_authorization
end

Expand Down
17 changes: 10 additions & 7 deletions app/controllers/concerns/order_stock_check.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,20 @@ module OrderStockCheck
include CablecarResponses
extend ActiveSupport::Concern

delegate :sufficient_stock?, to: :check_stock_service

def valid_order_line_items?
@order.insufficient_stock_lines.empty? &&
OrderCycles::DistributedVariantsService.new(@order.order_cycle, @order.distributor).
distributes_order_variants?(@order)
OrderCycles::DistributedVariantsService.new(@order.order_cycle, @order.distributor).
distributes_order_variants?(@order)
end

def handle_insufficient_stock
@any_out_of_stock = false

return if sufficient_stock?

flash[:error] = Spree.t(:inventory_error_flash_for_insufficient_quantity)
redirect_to main_app.cart_path
@any_out_of_stock = true
@updated_variants = check_stock_service.update_line_items
end

def check_order_cycle_expiry
Expand All @@ -36,7 +39,7 @@ def check_order_cycle_expiry

private

def sufficient_stock?
@sufficient_stock ||= @order.insufficient_stock_lines.blank?
def check_stock_service
@check_stock_service ||= Orders::CheckStockService.new(order: @order)
end
end
4 changes: 4 additions & 0 deletions app/controllers/payment_gateways/paypal_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ class PaypalController < ::BaseController
after_action :reset_order_when_complete, only: :confirm

def express
return redirect_to order_failed_route if @any_out_of_stock == true

pp_request = provider.build_set_express_checkout(
express_checkout_request_details(@order)
)
Expand Down Expand Up @@ -41,6 +43,8 @@ def express
end

def confirm
return redirect_to order_failed_route if @any_out_of_stock == true

# At this point the user has come back from the Paypal form, and we get one
# last chance to interact with the payment process before the money moves...

Expand Down
5 changes: 4 additions & 1 deletion app/controllers/payment_gateways/stripe_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@ class StripeController < BaseController
before_action :load_checkout_order, only: :confirm
before_action :validate_payment_intent, only: :confirm
before_action :check_order_cycle_expiry, only: :confirm
before_action :validate_stock, only: :confirm

def confirm
validate_stock

return redirect_to order_failed_route if @any_out_of_stock == true

process_payment_completion!
end

Expand Down
27 changes: 27 additions & 0 deletions app/services/orders/check_stock_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# frozen_string_literal: true

module Orders
class CheckStockService
attr_reader :order

def initialize(order:)
@order = order
end

def sufficient_stock?
@sufficient_stock ||= order.insufficient_stock_lines.blank?
Copy link
Member

Choose a reason for hiding this comment

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

This kind of caching doesn't work with boolean values. If the result is false then it gets re-computed on every call. I think that you need to check for nil.

Copy link
Member

Choose a reason for hiding this comment

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

I see that it was just copy and paste and broken before.

end

def update_line_items
return [] if sufficient_stock?

variants = []
order.insufficient_stock_lines.each do |line_item|
order.contents.update_item(line_item, { quantity: line_item.variant.on_hand })
variants.push line_item.variant
end

variants
end
end
end
23 changes: 23 additions & 0 deletions app/views/checkout/edit.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,29 @@
= render partial: "shopping_shared/order_cycles"

%div{ "data-controller": "guest-checkout", "data-guest-checkout-distributor-value": @order.distributor.id }
- if @any_out_of_stock
= render ModalComponent.new(id: "out-of-stock-items", modal_class: "medium" ,instant: true, close_button: false) do
- # please update app/assets/javascripts/templates/out_of_stock.html.haml if updating the modal html
%a.close-reveal-modal{"data-action": "click->modal#close" }
%i.ofn-i_009-close
%h3
= t("js.out_of_stock.reduced_stock_available")
%p
= t("js.out_of_stock.out_of_stock_text")
- @updated_variants.each do |variant|
- if variant.on_hand == 0
%p
%em
= "#{variant.name_to_display} - #{variant.unit_to_display}"
%span
= t("js.out_of_stock.now_out_of_stock")
- if variant.on_hand > 0
%p
%em
= "#{variant.name_to_display} - #{variant.unit_to_display}"
%span
= t("js.out_of_stock.only_n_remainging", num: variant.on_hand)
Copy link
Member

Choose a reason for hiding this comment

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

Typo? Oh no, the typo is in the translation files!

I would either add a comment # sic! here, rename it or maybe start new lazy translations so that we can delete the old ones when we get rid of AngularJS.


%div{ style: "display: #{spree_current_user ? 'block' : 'none'}", "data-guest-checkout-target": "checkout" }
= render partial: "checkout"

Expand Down
53 changes: 42 additions & 11 deletions spec/controllers/checkout_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,17 @@
expect(order.reload.state).to eq "payment"
end

context "with insufficient stock" do
it "redirects to details page" do
allow(order).to receive_message_chain(:insufficient_stock_lines,
:empty?).and_return false

put(:update, params:)

expect_cable_ready_redirect(response)
end
end

describe "saving default addresses" do
it "doesn't update default bill address on user" do
expect {
Expand Down Expand Up @@ -306,6 +317,17 @@
expect(response).to redirect_to checkout_step_path(:summary)
end
end

context "with insufficient stock" do
it "redirects to details page" do
allow(order).to receive_message_chain(:insufficient_stock_lines,
:empty?).and_return false

put(:update, params:)

expect_cable_ready_redirect(response)
end
end
end

context "with no payment source" do
Expand Down Expand Up @@ -468,6 +490,16 @@
end
end

context "with insufficient stock" do
it "redirects to details page" do
allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return false

put(:update, params:)

expect_cable_ready_redirect(response)
end
end

context "when accepting T&Cs is required" do
before do
allow(TermsOfService).to receive(:platform_terms_required?) { true }
Expand Down Expand Up @@ -582,17 +614,10 @@
)
end

shared_examples "handling stock issues" do |step|
shared_examples "handling not available items" do |step|
context "#{step} step" do
let(:step) { step.to_s }

it "redirects when some items are out of stock" do
allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return false

get :edit
expect(response).to redirect_to cart_path
end

it "redirects when some items are not available" do
allow(order).to receive_message_chain(:insufficient_stock_lines, :empty?).and_return true
expect(order_cycle_distributed_variants).to receive(
Expand All @@ -605,9 +630,9 @@
end
end

it_behaves_like "handling stock issues", "details"
it_behaves_like "handling stock issues", "payment"
it_behaves_like "handling stock issues", "summary"
it_behaves_like "handling not available items", "details"
it_behaves_like "handling not available items", "payment"
it_behaves_like "handling not available items", "summary"
end

def mock_voucher_adjustment_service
Expand All @@ -616,4 +641,10 @@ def mock_voucher_adjustment_service

voucher_adjustment_service
end

def expect_cable_ready_redirect(response)
expect(response.parsed_body).to eq(
[{ "url" => "/checkout/details", "operation" => "redirectTo" }].to_json
)
end
end
Loading
Loading