diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index a164ac64fe..ba7bb23eef 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -245,7 +245,6 @@ Layout/EmptyLines: - 'spec/models/spree/product_spec.rb' - 'spec/models/spree/shipping_method_spec.rb' - 'spec/models/spree/variant_spec.rb' - - 'spec/models/variant_override_spec.rb' - 'spec/serializers/admin/exchange_serializer_spec.rb' - 'spec/serializers/admin/for_order_cycle/enterprise_serializer_spec.rb' - 'spec/serializers/admin/for_order_cycle/supplied_product_serializer_spec.rb' @@ -389,7 +388,6 @@ Layout/ExtraSpacing: - 'spec/models/spree/adjustment_spec.rb' - 'spec/models/spree/gateway/stripe_connect_spec.rb' - 'spec/models/spree/order_spec.rb' - - 'spec/models/variant_override_spec.rb' - 'spec/serializers/admin/for_order_cycle/enterprise_serializer_spec.rb' - 'spec/serializers/admin/for_order_cycle/supplied_product_serializer_spec.rb' - 'spec/spec_helper.rb' @@ -557,7 +555,6 @@ Layout/SpaceAfterColon: - 'spec/features/admin/variants_spec.rb' - 'spec/features/consumer/account_spec.rb' - 'spec/models/spree/ability_spec.rb' - - 'spec/models/variant_override_spec.rb' - 'spec/spec_helper.rb' # Offense count: 83 @@ -870,7 +867,6 @@ Layout/SpaceInsideHashLiteralBraces: - 'spec/models/spree/taxon_spec.rb' - 'spec/models/spree/variant_spec.rb' - 'spec/models/tag_rule/discount_order_spec.rb' - - 'spec/models/variant_override_spec.rb' - 'spec/performance/orders_controller_spec.rb' - 'spec/requests/checkout/failed_checkout_spec.rb' - 'spec/requests/checkout/stripe_connect_spec.rb' @@ -965,7 +961,6 @@ Lint/DuplicateMethods: Lint/IneffectiveAccessModifier: Exclude: - 'app/models/column_preference.rb' - - 'app/models/variant_override.rb' - 'lib/open_food_network/feature_toggle.rb' - 'lib/open_food_network/products_cache.rb' - 'lib/open_food_network/property_merge.rb' @@ -1130,7 +1125,6 @@ Lint/Void: - 'spec/models/spree/payment_spec.rb' - 'spec/models/spree/product_spec.rb' - 'spec/models/spree/variant_spec.rb' - - 'spec/models/variant_override_spec.rb' - 'spec/serializers/enterprise_serializer_spec.rb' - 'spec/support/request/web_helper.rb' @@ -1522,7 +1516,6 @@ Rails/TimeZone: - 'spec/lib/open_food_network/products_cache_refreshment_spec.rb' - 'spec/lib/open_food_network/products_cache_spec.rb' - 'spec/models/enterprise_relationship_spec.rb' - - 'spec/models/variant_override_spec.rb' # Offense count: 1 # Configuration parameters: Environments. @@ -1545,7 +1538,6 @@ Rails/Validation: - 'app/models/order_cycle.rb' - 'app/models/product_distribution.rb' - 'app/models/spree/product_decorator.rb' - - 'app/models/variant_override.rb' # Offense count: 18 # Cop supports --auto-correct. @@ -2283,7 +2275,6 @@ Style/RedundantSelf: - 'app/models/spree/taxon_decorator.rb' - 'app/models/spree/user_decorator.rb' - 'app/models/spree/variant_decorator.rb' - - 'app/models/variant_override.rb' - 'lib/open_food_network/locking.rb' - 'lib/open_food_network/rack_request_blocker.rb' - 'lib/open_food_network/reports/report.rb' diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index d54fbc4fcf..9b636efb8f 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -109,6 +109,20 @@ module VariantStock end end + # We can have this responsibility here in the variant because there is only one stock item per variant + # + # This enables us to override this behaviour for variant overrides + def move(quantity, originator = nil) + raise_error_if_no_stock_item_available + + # Creates a stock movement: it updates stock_item.count_on_hand and fills backorders + # + # This is the original Spree::StockLocation#move, + # except that we raise an error if the stock item is missing, + # because, unlike Spree, we should always have exactly one stock item per variant. + stock_item.stock_movements.create!(quantity: quantity, originator: originator) + end + private # Persists the single stock item associated to this variant. As defined in @@ -126,7 +140,10 @@ module VariantStock raise message if stock_items.empty? end - # Backwards compatible setting of stock levels in Spree 2.0. + # Overwrites stock_item.count_on_hand + # + # Calling stock_item.adjust_count_on_hand will bypass filling backorders and creating stock movements + # If that was required we could call self.move def overwrite_stock_levels(new_level) stock_item.adjust_count_on_hand(new_level - stock_item.count_on_hand) end diff --git a/app/models/spree/stock_location_decorator.rb b/app/models/spree/stock_location_decorator.rb new file mode 100644 index 0000000000..335c557674 --- /dev/null +++ b/app/models/spree/stock_location_decorator.rb @@ -0,0 +1,5 @@ +Spree::StockLocation.class_eval do + def move(variant, quantity, originator = nil) + variant.move(quantity, originator) + end +end diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index 89d2fdcd13..1e421c7adf 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -6,7 +6,8 @@ class VariantOverride < ActiveRecord::Base belongs_to :hub, class_name: 'Enterprise' belongs_to :variant, class_name: 'Spree::Variant' - validates_presence_of :hub_id, :variant_id + validates :hub_id, presence: true + validates :variant_id, presence: true # Default stock can be nil, indicating stock should not be reset or zero, meaning reset to zero. Need to ensure this can be set by the user. validates :default_stock, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true @@ -33,45 +34,20 @@ class VariantOverride < ActiveRecord::Base ] end - def self.price_for(hub, variant) - self.for(hub, variant).andand.price - end - - def self.count_on_hand_for(hub, variant) - self.for(hub, variant).andand.count_on_hand - end - - def self.stock_overridden?(hub, variant) - count_on_hand_for(hub, variant).present? - end - - def self.decrement_stock!(hub, variant, quantity) - vo = self.for(hub, variant) - - if vo.nil? - Bugsnag.notify RuntimeError.new "Attempting to decrement stock level for a variant without a VariantOverride." - else - vo.decrement_stock! quantity - end - end - def stock_overridden? count_on_hand.present? end - def decrement_stock!(quantity) - if stock_overridden? - decrement! :count_on_hand, quantity - else - Bugsnag.notify RuntimeError.new "Attempting to decrement stock level on a VariantOverride without a count_on_hand specified." + def move_stock!(quantity) + unless stock_overridden? + Bugsnag.notify RuntimeError.new "Attempting to move stock of a VariantOverride without a count_on_hand specified." + return end - end - def increment_stock!(quantity) - if stock_overridden? + if quantity > 0 increment! :count_on_hand, quantity - else - Bugsnag.notify RuntimeError.new "Attempting to decrement stock level on a VariantOverride without a count_on_hand specified." + elsif quantity < 0 + decrement! :count_on_hand, -quantity end end @@ -83,7 +59,7 @@ class VariantOverride < ActiveRecord::Base if resettable if default_stock? self.attributes = { count_on_hand: default_stock } - self.save + save else Bugsnag.notify RuntimeError.new "Attempting to reset stock level for a variant with no default stock level." end @@ -93,10 +69,6 @@ class VariantOverride < ActiveRecord::Base private - def self.for(hub, variant) - VariantOverride.where(variant_id: variant, hub_id: hub).first - end - def refresh_products_cache_from_save OpenFoodNetwork::ProductsCache.variant_override_changed self end diff --git a/lib/open_food_network/scope_variant_to_hub.rb b/lib/open_food_network/scope_variant_to_hub.rb index bc2afa63da..8b36161068 100644 --- a/lib/open_food_network/scope_variant_to_hub.rb +++ b/lib/open_food_network/scope_variant_to_hub.rb @@ -28,7 +28,8 @@ module OpenFoodNetwork on_demand || (count_on_hand > 0) end - def count_on_hand + # Uses variant_override.count_on_hand instead of Stock::Quantifier.stock_items.count_on_hand + def total_on_hand @variant_override.andand.count_on_hand || super end @@ -46,17 +47,13 @@ module OpenFoodNetwork end end - def decrement!(attribute, by = 1) - if attribute == :count_on_hand && @variant_override.andand.stock_overridden? - @variant_override.decrement_stock! by - else - super - end - end - - def increment!(attribute, by = 1) - if attribute == :count_on_hand && @variant_override.andand.stock_overridden? - @variant_override.increment_stock! by + # If it is an variant override with a count_on_hand value: + # - updates variant_override.count_on_hand + # - does not create stock_movement + # - does not update stock_item.count_on_hand + def move(quantity, originator = nil) + if @variant_override.andand.stock_overridden? + @variant_override.move_stock! quantity else super end diff --git a/spec/models/variant_override_spec.rb b/spec/models/variant_override_spec.rb index bb799f8603..1add54698e 100644 --- a/spec/models/variant_override_spec.rb +++ b/spec/models/variant_override_spec.rb @@ -9,7 +9,7 @@ describe VariantOverride do let(:hub2) { create(:distributor_enterprise) } let!(:vo1) { create(:variant_override, hub: hub1, variant: variant, import_date: Time.zone.now.yesterday) } let!(:vo2) { create(:variant_override, hub: hub2, variant: variant, import_date: Time.zone.now) } - let!(:vo3) { create(:variant_override, hub: hub1, variant: variant, permission_revoked_at: Time.now) } + let!(:vo3) { create(:variant_override, hub: hub1, variant: variant, permission_revoked_at: Time.zone.now) } it "ignores variant_overrides with revoked_permissions by default" do expect(VariantOverride.all).to_not include vo3 @@ -17,7 +17,7 @@ describe VariantOverride do end it "finds variant overrides for a set of hubs" do - VariantOverride.for_hubs([hub1, hub2]).should match_array [vo1, vo2] + expect(VariantOverride.for_hubs([hub1, hub2])).to match_array [vo1, vo2] end it "fetches import dates for hubs in descending order" do @@ -29,13 +29,12 @@ describe VariantOverride do describe "fetching variant overrides indexed by variant" do it "gets indexed variant overrides for one hub" do - VariantOverride.indexed(hub1).should == {variant => vo1} - VariantOverride.indexed(hub2).should == {variant => vo2} + expect(VariantOverride.indexed(hub1)).to eq( variant => vo1 ) + expect(VariantOverride.indexed(hub2)).to eq( variant => vo2 ) end end end - describe "callbacks" do let!(:vo) { create(:variant_override, hub: hub, variant: variant) } @@ -51,87 +50,71 @@ describe VariantOverride do end end + describe "with price" do + let(:variant_override) { create(:variant_override, variant: variant, hub: hub, price: 12.34) } - describe "looking up prices" do - it "returns the numeric price when present" do - VariantOverride.create!(variant: variant, hub: hub, price: 12.34) - VariantOverride.price_for(hub, variant).should == 12.34 - end - - it "returns nil otherwise" do - VariantOverride.price_for(hub, variant).should be_nil + it "returns the numeric price" do + expect(variant_override.price).to eq(12.34) end end - describe "looking up count on hand" do - it "returns the numeric stock level when present" do - VariantOverride.create!(variant: variant, hub: hub, count_on_hand: 12) - VariantOverride.count_on_hand_for(hub, variant).should == 12 - end + describe "with nil count on hand" do + let(:variant_override) { create(:variant_override, variant: variant, hub: hub, count_on_hand: nil) } - it "returns nil otherwise" do - VariantOverride.count_on_hand_for(hub, variant).should be_nil - end - end - - describe "checking if stock levels have been overriden" do - it "returns true when stock level has been overridden" do - create(:variant_override, variant: variant, hub: hub, count_on_hand: 12) - VariantOverride.stock_overridden?(hub, variant).should be true - end - - it "returns false when the override has no stock level" do - create(:variant_override, variant: variant, hub: hub, count_on_hand: nil) - VariantOverride.stock_overridden?(hub, variant).should be false - end - - it "returns false when there is no override for the hub/variant" do - VariantOverride.stock_overridden?(hub, variant).should be false - end - end - - describe "decrementing stock" do - it "decrements stock" do - vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12) - VariantOverride.decrement_stock! hub, variant, 2 - vo.reload.count_on_hand.should == 10 - end - - it "silently logs an error if the variant override does not exist" do - Bugsnag.should_receive(:notify) - VariantOverride.decrement_stock! hub, variant, 2 - end - end - - describe "incrementing stock" do - let!(:vo) { create(:variant_override, variant: variant, hub: hub, count_on_hand: 8) } - - context "when the vo overrides stock" do - it "increments stock" do - vo.increment_stock! 2 - vo.reload.count_on_hand.should == 10 + describe "stock_overridden?" do + it "returns false" do + expect(variant_override.stock_overridden?).to be false end end - context "when the vo doesn't override stock" do - before { vo.update_attributes(count_on_hand: nil) } - + describe "move_stock!" do it "silently logs an error" do - Bugsnag.should_receive(:notify) - vo.increment_stock! 2 + expect(Bugsnag).to receive(:notify) + variant_override.move_stock!(5) + end + end + end + + describe "with count on hand" do + let(:variant_override) { create(:variant_override, variant: variant, hub: hub, count_on_hand: 12) } + + it "returns the numeric count on hand" do + expect(variant_override.count_on_hand).to eq(12) + end + + describe "stock_overridden?" do + it "returns true" do + expect(variant_override.stock_overridden?).to be true + end + end + + describe "move_stock!" do + it "does nothing for quantity zero" do + variant_override.move_stock!(0) + expect(variant_override.reload.count_on_hand).to eq(12) + end + + it "increments count_on_hand when quantity is negative" do + variant_override.move_stock!(2) + expect(variant_override.reload.count_on_hand).to eq(14) + end + + it "decrements count_on_hand when quantity is negative" do + variant_override.move_stock!(-2) + expect(variant_override.reload.count_on_hand).to eq(10) end end end describe "checking default stock value is present" do - it "returns true when a default stock level has been set" do + it "returns true when a default stock level has been set" do vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12, default_stock: 20) - vo.default_stock?.should be true + expect(vo.default_stock?).to be true end it "returns false when the override has no default stock level" do - vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12, default_stock:nil) - vo.default_stock?.should be false + vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12, default_stock: nil) + expect(vo.default_stock?).to be false end end @@ -139,18 +122,18 @@ describe VariantOverride do it "resets the on hand level to the value in the default_stock field" do vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12, default_stock: 20, resettable: true) vo.reset_stock! - vo.reload.count_on_hand.should == 20 + expect(vo.reload.count_on_hand).to eq(20) end it "silently logs an error if the variant override doesn't have a default stock level" do - vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12, default_stock:nil, resettable: true) - Bugsnag.should_receive(:notify) + vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12, default_stock: nil, resettable: true) + expect(Bugsnag).to receive(:notify) vo.reset_stock! - vo.reload.count_on_hand.should == 12 + expect(vo.reload.count_on_hand).to eq(12) end it "doesn't reset the level if the behaviour is disabled" do vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12, default_stock: 10, resettable: false) vo.reset_stock! - vo.reload.count_on_hand.should == 12 + expect(vo.reload.count_on_hand).to eq(12) end end