From 5639b21c7773ec2beb9f7328c0d0529aa28b5da5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 4 May 2020 15:03:50 +0200 Subject: [PATCH 1/8] Add tests for current soft-deleted variant behavior in CartService --- spec/services/cart_service_spec.rb | 57 ++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 10 deletions(-) diff --git a/spec/services/cart_service_spec.rb b/spec/services/cart_service_spec.rb index 0baa3abe26b..b5d2f69c946 100644 --- a/spec/services/cart_service_spec.rb +++ b/spec/services/cart_service_spec.rb @@ -15,14 +15,18 @@ context "end-to-end" do let(:order) { create(:order, distributor: distributor, order_cycle: order_cycle) } let(:distributor) { create(:distributor_enterprise) } - let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor], variants: [v]) } + let(:order_cycle) { create(:simple_order_cycle, distributors: [distributor], + variants: [variant]) } let(:cart_service) { CartService.new(order) } - let(:v) { create(:variant) } + let(:variant) { create(:variant) } - describe "populate" do + describe "#populate" do it "adds a variant" do - cart_service.populate({ variants: { v.id.to_s => { quantity: '1', max_quantity: '2' } } }, true) - li = order.find_line_item_by_variant(v) + cart_service.populate( + { variants: { variant.id.to_s => { quantity: '1', max_quantity: '2' } } }, + true + ) + li = order.find_line_item_by_variant(variant) expect(li).to be expect(li.quantity).to eq(1) expect(li.max_quantity).to eq(2) @@ -30,10 +34,13 @@ end it "updates a variant's quantity, max quantity and final_weight_volume" do - order.add_variant v, 1, 2 + order.add_variant variant, 1, 2 - cart_service.populate({ variants: { v.id.to_s => { quantity: '2', max_quantity: '3' } } }, true) - li = order.find_line_item_by_variant(v) + cart_service.populate( + { variants: { variant.id.to_s => { quantity: '2', max_quantity: '3' } } }, + true + ) + li = order.find_line_item_by_variant(variant) expect(li).to be expect(li.quantity).to eq(2) expect(li.max_quantity).to eq(3) @@ -41,13 +48,43 @@ end it "removes a variant" do - order.add_variant v, 1, 2 + order.add_variant variant, 1, 2 cart_service.populate({ variants: {} }, true) order.line_items(:reload) - li = order.find_line_item_by_variant(v) + li = order.find_line_item_by_variant(variant) expect(li).not_to be end + + context "when a variant has been soft-deleted" do + let(:relevant_line_item) { order.reload.find_line_item_by_variant(variant) } + + describe "when the soft-deleted variant is not in the cart yet" do + xit "doesn't fail, and does not add the deleted variant to the cart" do + variant.delete + + cart_service.populate({ variants: { variant.id.to_s => { quantity: '2' } } }, true) + + expect(relevant_line_item).to be_nil + expect(cart_service.errors.count).to be 0 + end + end + + describe "when the soft-deleted variant already has a line_item in the cart" do + let!(:existing_line_item) { + create(:line_item, variant: variant, quantity: 2, order: order) + } + + xit "doesn't fail, and removes the line_item from the cart" do + variant.delete + + cart_service.populate({ variants: { variant.id.to_s => { quantity: '2' } } }, true) + + expect(relevant_line_item).to be_nil + expect(cart_service.errors.count).to be 0 + end + end + end end end From d3de1ce47e0634ada94938caaf818db9392dea13 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 4 May 2020 19:15:23 +0200 Subject: [PATCH 2/8] Add spec for current soft deletion behaviour in VariantOverride#indexed --- spec/models/variant_override_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/models/variant_override_spec.rb b/spec/models/variant_override_spec.rb index 4c5e5e71afc..22b93a06d40 100644 --- a/spec/models/variant_override_spec.rb +++ b/spec/models/variant_override_spec.rb @@ -32,6 +32,11 @@ expect(VariantOverride.indexed(hub1)).to eq( variant => vo1 ) expect(VariantOverride.indexed(hub2)).to eq( variant => vo2 ) end + + it "does not include overrides for soft-deleted variants" do + variant.delete + expect(VariantOverride.indexed(hub1)).to eq( nil => vo1 ) + end end end From eb51b87beaf7d51126adecf9434df8f500def442 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 4 May 2020 14:21:58 +0200 Subject: [PATCH 3/8] Add spec for current soft deletion behaviour in OrderCycle#variants_distributed_by scope --- spec/models/order_cycle_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 42fc8759119..22cfeb2b70b 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -227,6 +227,14 @@ expect(oc.variants_distributed_by(d2)).not_to include p1_v_hidden, p1_v_deleted expect(oc.variants_distributed_by(d1)).to include p2_v end + + context "with soft-deleted variants" do + it "does not consider soft-deleted variants to be currently distributed in the oc" do + p2_v.delete + + expect(oc.variants_distributed_by(d1)).to_not include p2_v + end + end end context "when hub prefers product selection from inventory only" do From 0e429da377fafdad945e00b719aa326087e20365 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 4 May 2020 19:39:20 +0200 Subject: [PATCH 4/8] Return zero available stock (total_on_hand) for variants that are soft-deleted --- app/models/spree/stock/quantifier.rb | 2 ++ spec/models/spree/stock/quantifier_spec.rb | 24 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 spec/models/spree/stock/quantifier_spec.rb diff --git a/app/models/spree/stock/quantifier.rb b/app/models/spree/stock/quantifier.rb index 8c437dc5800..037f5295fd7 100644 --- a/app/models/spree/stock/quantifier.rb +++ b/app/models/spree/stock/quantifier.rb @@ -11,6 +11,8 @@ def initialize(variant) end def total_on_hand + return 0 if @variant.deleted? + stock_items.sum(&:count_on_hand) end diff --git a/spec/models/spree/stock/quantifier_spec.rb b/spec/models/spree/stock/quantifier_spec.rb new file mode 100644 index 00000000000..2a06f743441 --- /dev/null +++ b/spec/models/spree/stock/quantifier_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +module Spree + module Stock + describe Quantifier do + let(:quantifier) { Spree::Stock::Quantifier.new(variant) } + let(:variant) { create(:variant, on_hand: 99) } + + describe "#total_on_hand" do + context "with a soft-deleted variant" do + before do + variant.delete + end + + it "returns zero stock for the variant" do + expect(quantifier.total_on_hand).to eq 0 + end + end + end + end + end +end From a3458aa562b244dc4d41865ff459522196f5b505 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 4 May 2020 19:57:53 +0200 Subject: [PATCH 5/8] Ensure VariantStockLevels can process soft-deleted variants --- app/services/variants_stock_levels.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/variants_stock_levels.rb b/app/services/variants_stock_levels.rb index 0d778b9a0dc..a72abc8ffb5 100644 --- a/app/services/variants_stock_levels.rb +++ b/app/services/variants_stock_levels.rb @@ -8,7 +8,7 @@ def call(order, requested_variant_ids) variant_stock_levels = variant_stock_levels(order.line_items.includes(variant: :stock_items)) order_variant_ids = variant_stock_levels.keys - missing_variants = Spree::Variant.includes(:stock_items). + missing_variants = Spree::Variant.with_deleted.includes(:stock_items). where(id: (requested_variant_ids - order_variant_ids)) missing_variants.each do |missing_variant| From 26ba76cff984bd7b899cec2b789aa2dd48696853 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 4 May 2020 19:59:04 +0200 Subject: [PATCH 6/8] Fix soft-deletion in CartService and update spec --- app/services/cart_service.rb | 16 ++++++++++++---- spec/services/cart_service_spec.rb | 10 +++++----- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb index 5818f8f91fb..9df85cfe459 100644 --- a/app/services/cart_service.rb +++ b/app/services/cart_service.rb @@ -29,11 +29,15 @@ def attempt_cart_add_variants(variants_data) variants_data.each do |variant_data| loaded_variant = loaded_variants[variant_data[:variant_id].to_i] + + if loaded_variant.deleted? + remove_deleted_variant(loaded_variant) + next + end + next unless varies_from_cart(variant_data, loaded_variant) - attempt_cart_add( - loaded_variant, variant_data[:quantity], variant_data[:max_quantity] - ) + attempt_cart_add(loaded_variant, variant_data[:quantity], variant_data[:max_quantity]) end end @@ -41,12 +45,16 @@ def indexed_variants(variants_data) @indexed_variants ||= begin variant_ids_in_data = variants_data.map{ |v| v[:variant_id] } - Spree::Variant.where(id: variant_ids_in_data). + Spree::Variant.with_deleted.where(id: variant_ids_in_data). includes(:default_price, :stock_items, :product). index_by(&:id) end end + def remove_deleted_variant(variant) + line_item_for_variant(variant).andand.destroy + end + def attempt_cart_add(variant, quantity, max_quantity = nil) quantity = quantity.to_i max_quantity = max_quantity.to_i if max_quantity diff --git a/spec/services/cart_service_spec.rb b/spec/services/cart_service_spec.rb index b5d2f69c946..397014e08a9 100644 --- a/spec/services/cart_service_spec.rb +++ b/spec/services/cart_service_spec.rb @@ -60,7 +60,7 @@ let(:relevant_line_item) { order.reload.find_line_item_by_variant(variant) } describe "when the soft-deleted variant is not in the cart yet" do - xit "doesn't fail, and does not add the deleted variant to the cart" do + it "does not add the deleted variant to the cart" do variant.delete cart_service.populate({ variants: { variant.id.to_s => { quantity: '2' } } }, true) @@ -70,17 +70,17 @@ end end - describe "when the soft-deleted variant already has a line_item in the cart" do + describe "when the soft-deleted variant is already in the cart" do let!(:existing_line_item) { create(:line_item, variant: variant, quantity: 2, order: order) } - xit "doesn't fail, and removes the line_item from the cart" do + it "removes the line_item from the cart" do variant.delete - cart_service.populate({ variants: { variant.id.to_s => { quantity: '2' } } }, true) + cart_service.populate({ variants: { variant.id.to_s => { quantity: '3' } } }, true) - expect(relevant_line_item).to be_nil + expect(Spree::LineItem.where(id: relevant_line_item).first).to be_nil expect(cart_service.errors.count).to be 0 end end From 0a28abbf2d66f5fdcc71f71ec72b3a917d0e0e99 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 4 May 2020 19:59:48 +0200 Subject: [PATCH 7/8] Add additional feature specs for soft-deleted variants in cart --- .../consumer/shopping/shopping_spec.rb | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/spec/features/consumer/shopping/shopping_spec.rb b/spec/features/consumer/shopping/shopping_spec.rb index 432f750f173..429f4ba3d33 100644 --- a/spec/features/consumer/shopping/shopping_spec.rb +++ b/spec/features/consumer/shopping/shopping_spec.rb @@ -429,6 +429,34 @@ end end end + + context "when a variant is soft-deleted" do + describe "adding the soft-deleted variant to the cart" do + it "handles it as if the variant has gone out of stock" do + variant.delete + + fill_in "variants[#{variant.id}]", with: '1' + + expect_out_of_stock_behavior + end + end + + context "when the soft-deleted variant has an associated override" do + describe "adding the soft-deleted variant to the cart" do + let!(:variant_override) { + create(:variant_override, variant: variant, hub: distributor, count_on_hand: 100) + } + + it "handles it as if the variant has gone out of stock" do + variant.delete + + fill_in "variants[#{variant.id}]", with: '1' + + expect_out_of_stock_behavior + end + end + end + end end context "when no order cycles are available" do @@ -543,4 +571,24 @@ def wait_for_debounce # waiting period before submitting the data... sleep 0.6 end + + def expect_out_of_stock_behavior + wait_for_debounce + wait_until { !cart_dirty } + + # Shows an "out of stock" modal, with helpful user feedback + within(".out-of-stock-modal") do + expect(page).to have_content I18n.t('js.out_of_stock.out_of_stock_text') + end + + # Removes the item from the client-side cart and marks the variant as unavailable + expect(page).to have_field "variants[#{variant.id}]", with: '0', disabled: true + expect(page).to have_selector "#variant-#{variant.id}.out-of-stock" + expect(page).to have_selector "#variants_#{variant.id}[ofn-on-hand='0']" + expect(page).to have_selector "#variants_#{variant.id}[disabled='disabled']" + + # We need to wait again for the cart to finish updating in Angular or the test can fail + # as the session cannot be reset properly (after the test) while it's still loading + wait_until { !cart_dirty } + end end From 6afda87baf9fd9b63e303e3f787d026648c08609 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 5 May 2020 12:19:05 +0200 Subject: [PATCH 8/8] Add explanatory comment on soft-deleted variant stock logic --- app/models/spree/stock/quantifier.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/spree/stock/quantifier.rb b/app/models/spree/stock/quantifier.rb index 037f5295fd7..21f1750ca09 100644 --- a/app/models/spree/stock/quantifier.rb +++ b/app/models/spree/stock/quantifier.rb @@ -11,6 +11,8 @@ def initialize(variant) end def total_on_hand + # Associated stock_items no longer exist if the variant has been soft-deleted. A variant + # may still be in an active cart after it's deleted, so this will mark it as out of stock. return 0 if @variant.deleted? stock_items.sum(&:count_on_hand)