diff --git a/.gitignore b/.gitignore index 4c0d217d06..370eba9a90 100644 --- a/.gitignore +++ b/.gitignore @@ -41,3 +41,5 @@ libpeerconnection.log node_modules vendor/bundle/ coverage +/reports/ +!/reports/README.md diff --git a/app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee b/app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee index 0fe051f496..6b7cea8243 100644 --- a/app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee +++ b/app/assets/javascripts/admin/variant_overrides/controllers/variant_overrides_controller.js.coffee @@ -13,6 +13,11 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", $scope.RequestMonitor = RequestMonitor $scope.selectView = Views.selectView $scope.currentView = -> Views.currentView + $scope.onDemandOptions = [ + { description: t('js.variant_overrides.on_demand.use_producer_settings'), value: null }, + { description: t('js.variant_overrides.on_demand.yes'), value: true }, + { description: t('js.variant_overrides.on_demand.no'), value: false } + ] $scope.views = Views.setViews inventory: { name: t('js.variant_overrides.inventory_products'), visible: true } @@ -105,3 +110,35 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", StatusMessage.display 'success', t('js.variant_overrides.stock_reset') .error (data, status) -> $timeout -> StatusMessage.display 'failure', $scope.updateError(data, status) + + # Variant override count_on_hand field placeholder logic: + # on_demand true -- Show "On Demand" + # on_demand false -- Show empty value to be set by the user + # on_demand nil -- Show producer on_hand value + $scope.countOnHandPlaceholder = (variant, hubId) -> + variantOverride = $scope.variantOverrides[hubId][variant.id] + + if variantOverride.on_demand + t('js.variants.on_demand.yes') + else if variantOverride.on_demand == false + '' + else + variant.on_hand + + # This method should only be used when the variant override on_demand is changed. + # + # Change the count_on_hand value to a suggested value. + $scope.updateCountOnHand = (variant, hubId) -> + variantOverride = $scope.variantOverrides[hubId][variant.id] + + suggested = $scope.countOnHandSuggestion(variant, hubId) + return if suggested == variantOverride.count_on_hand + variantOverride.count_on_hand = suggested + DirtyVariantOverrides.set hubId, variant.id, variantOverride.id, 'count_on_hand', suggested + + # Suggest producer count_on_hand if variant has limited stock and variant override forces limited + # stock. Otherwise, clear whatever value is set. + $scope.countOnHandSuggestion = (variant, hubId) -> + variantOverride = $scope.variantOverrides[hubId][variant.id] + return null unless !variant.on_demand && variantOverride.on_demand == false + variant.on_hand diff --git a/app/assets/stylesheets/admin/components/input.css.scss b/app/assets/stylesheets/admin/components/input.css.scss new file mode 100644 index 0000000000..5b4cf8ed05 --- /dev/null +++ b/app/assets/stylesheets/admin/components/input.css.scss @@ -0,0 +1,10 @@ +@import '../../darkswarm/branding'; + +.container { + input { + &[readonly] { + background-color: $disabled-light; + cursor: default; + } + } +} diff --git a/app/models/concerns/stock_settings_override_validation.rb b/app/models/concerns/stock_settings_override_validation.rb new file mode 100644 index 0000000000..b9e3624aec --- /dev/null +++ b/app/models/concerns/stock_settings_override_validation.rb @@ -0,0 +1,41 @@ +module StockSettingsOverrideValidation + extend ActiveSupport::Concern + + included do + before_validation :require_compatible_on_demand_and_count_on_hand + end + + def require_compatible_on_demand_and_count_on_hand + disallow_count_on_hand_if_using_producer_stock_settings + disallow_count_on_hand_if_on_demand + require_count_on_hand_if_limited_stock + end + + def disallow_count_on_hand_if_using_producer_stock_settings + return unless on_demand.nil? && count_on_hand.present? + + error_message = I18n.t("count_on_hand.using_producer_stock_settings_but_count_on_hand_set", + scope: i18n_scope_for_stock_settings_override_validation_error) + errors.add(:count_on_hand, error_message) + end + + def disallow_count_on_hand_if_on_demand + return unless on_demand? && count_on_hand.present? + + error_message = I18n.t("count_on_hand.on_demand_but_count_on_hand_set", + scope: i18n_scope_for_stock_settings_override_validation_error) + errors.add(:count_on_hand, error_message) + end + + def require_count_on_hand_if_limited_stock + return unless on_demand == false && count_on_hand.blank? + + error_message = I18n.t("count_on_hand.limited_stock_but_no_count_on_hand", + scope: i18n_scope_for_stock_settings_override_validation_error) + errors.add(:count_on_hand, error_message) + end + + def i18n_scope_for_stock_settings_override_validation_error + "activerecord.errors.models.#{self.class.name.underscore}" + end +end diff --git a/app/models/product_import/entry_validator.rb b/app/models/product_import/entry_validator.rb index b7f055e445..11c0aab7dd 100644 --- a/app/models/product_import/entry_validator.rb +++ b/app/models/product_import/entry_validator.rb @@ -68,6 +68,18 @@ module ProductImport private + def find_or_initialize_variant_override(entry, existing_variant) + existing_variant_override = VariantOverride.where( + variant_id: existing_variant.id, + hub_id: entry.enterprise_id + ).first + + existing_variant_override || VariantOverride.new( + variant_id: existing_variant.id, + hub_id: entry.enterprise_id + ) + end + def enterprise_validation(entry) return if name_presence_error entry return if enterprise_not_found_error entry @@ -310,21 +322,12 @@ module ProductImport end def create_inventory_item(entry, existing_variant) - existing_variant_override = VariantOverride.where( - variant_id: existing_variant.id, - hub_id: entry.enterprise_id - ).first + find_or_initialize_variant_override(entry, existing_variant).tap do |variant_override| + check_variant_override_stock_settings(entry, variant_override) - variant_override = existing_variant_override || VariantOverride.new( - variant_id: existing_variant.id, - hub_id: entry.enterprise_id - ) - - variant_override.assign_attributes(count_on_hand: entry.on_hand, import_date: @import_time) - check_on_hand_nil(entry, variant_override) - variant_override.assign_attributes(entry.attributes.slice('price', 'on_demand')) - - variant_override + variant_override.assign_attributes(import_date: @import_time) + variant_override.assign_attributes(entry.attributes.slice('price', 'on_demand')) + end end def mark_as_inventory_item(entry, variant_override) @@ -355,5 +358,11 @@ module ProductImport object.count_on_hand = 0 if object.respond_to?(:count_on_hand) entry.on_hand_nil = true end + + def check_variant_override_stock_settings(entry, object) + object.count_on_hand = entry.on_hand.presence + object.on_demand = object.count_on_hand.blank? if entry.on_demand.blank? + entry.on_hand_nil = object.count_on_hand.blank? + end end end diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index 89d2fdcd13..dd3b22a5e0 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -1,5 +1,6 @@ class VariantOverride < ActiveRecord::Base extend Spree::LocalizedNumber + include StockSettingsOverrideValidation acts_as_taggable @@ -82,7 +83,7 @@ class VariantOverride < ActiveRecord::Base def reset_stock! if resettable if default_stock? - self.attributes = { count_on_hand: default_stock } + self.attributes = { on_demand: false, count_on_hand: default_stock } self.save else Bugsnag.notify RuntimeError.new "Attempting to reset stock level for a variant with no default stock level." diff --git a/app/views/admin/variant_overrides/_products_variants.html.haml b/app/views/admin/variant_overrides/_products_variants.html.haml index 7c530a1cb2..27de58fdec 100644 --- a/app/views/admin/variant_overrides/_products_variants.html.haml +++ b/app/views/admin/variant_overrides/_products_variants.html.haml @@ -8,9 +8,9 @@ %td.price{ ng: { show: 'columns.price.visible' } } %input{name: 'variant-overrides-{{ variant.id }}-price', type: 'text', ng: {model: 'variantOverrides[hub_id][variant.id].price'}, placeholder: '{{ variant.price }}', 'ofn-track-variant-override' => 'price'} %td.on_hand{ ng: { show: 'columns.on_hand.visible' } } - %input{name: 'variant-overrides-{{ variant.id }}-count_on_hand', type: 'text', ng: {model: 'variantOverrides[hub_id][variant.id].count_on_hand'}, placeholder: '{{ variant.on_hand }}', 'ofn-track-variant-override' => 'count_on_hand'} + %input{name: 'variant-overrides-{{ variant.id }}-count_on_hand', type: 'text', ng: { model: 'variantOverrides[hub_id][variant.id].count_on_hand', readonly: 'variantOverrides[hub_id][variant.id].on_demand != false' }, placeholder: '{{ countOnHandPlaceholder(variant, hub_id) }}', 'ofn-track-variant-override' => 'count_on_hand'} %td.on_demand{ ng: { show: 'columns.on_demand.visible' } } - %input.field{ :type => 'checkbox', name: 'variant-overrides-{{ variant.id }}-on_demand', ng: { model: 'variantOverrides[hub_id][variant.id].on_demand' }, 'ofn-track-variant-override' => 'on_demand' } + %select{ name: 'variant-overrides-{{ variant.id }}-on_demand', ng: { model: 'variantOverrides[hub_id][variant.id].on_demand', change: 'updateCountOnHand(variant, hub_id)', options: 'option.value as option.description for option in onDemandOptions' }, 'ofn-track-variant-override' => 'on_demand' } %td.reset{ ng: { show: 'columns.reset.visible' } } %input{name: 'variant-overrides-{{ variant.id }}-resettable', type: 'checkbox', ng: {model: 'variantOverrides[hub_id][variant.id].resettable'}, placeholder: '{{ variant.resettable }}', 'ofn-track-variant-override' => 'resettable'} %td.reset{ ng: { show: 'columns.reset.visible' } } @@ -24,4 +24,4 @@ %button.icon-remove.hide.fullwidth{ :type => 'button', ng: { click: "setVisibility(hub_id,variant.id,false)" } } = t('admin.variant_overrides.index.hide') %td.import_date{ ng: { show: 'columns.import_date.visible' } } - %span {{variantOverrides[hub_id][variant.id].import_date | date:"MMMM dd, yyyy HH:mm"}} \ No newline at end of file + %span {{variantOverrides[hub_id][variant.id].import_date | date:"MMMM dd, yyyy HH:mm"}} diff --git a/config/application.rb b/config/application.rb index 14c0b82b86..f80c91aee1 100644 --- a/config/application.rb +++ b/config/application.rb @@ -88,6 +88,7 @@ module Openfoodnetwork # Custom directories with classes and modules you want to be autoloadable. config.autoload_paths += %W( + #{config.root}/app/models/concerns #{config.root}/app/presenters #{config.root}/app/jobs ) diff --git a/config/locales/en.yml b/config/locales/en.yml index 38bb9aa788..5775c90452 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -47,6 +47,11 @@ en: attributes: orders_close_at: after_orders_open_at: must be after open date + variant_override: + count_on_hand: + using_producer_stock_settings_but_count_on_hand_set: "must be blank because using producer stock settings" + on_demand_but_count_on_hand_set: "must be blank if on demand" + limited_stock_but_no_count_on_hand: "must be specified because forcing limited stock" activemodel: errors: models: @@ -2592,7 +2597,14 @@ See the %{link} to find out more about %{sitename}'s features and to start using in your cart have reduced. Here's what's changed: now_out_of_stock: is now out of stock. only_n_remainging: "now only has %{num} remaining." + variants: + on_demand: + "yes": "On demand" variant_overrides: + on_demand: + use_producer_settings: "Use producer stock settings" + "yes": "Yes" + "no": "No" inventory_products: "Inventory Products" hidden_products: "Hidden Products" new_products: "New Products" diff --git a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb new file mode 100644 index 0000000000..6627dca015 --- /dev/null +++ b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb @@ -0,0 +1,165 @@ +# This simplifies variant overrides to have only the following combinations: +# +# on_demand | count_on_hand +# -----------+--------------- +# true | nil +# false | set +# nil | nil +# +# Refer to the table {here}[https://github.com/openfoodfoundation/openfoodnetwork/issues/3067] for +# the effect of different variant and variant override stock configurations. +# +# Furthermore, this will allow all existing variant overrides to satisfy the newly added model +# validation rules. +class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration + class VariantOverride < ActiveRecord::Base + belongs_to :variant + belongs_to :hub, class_name: "Enterprise" + + scope :with_count_on_hand, -> { where("count_on_hand IS NOT NULL") } + scope :without_count_on_hand, -> { where(count_on_hand: nil) } + end + + class Variant < ActiveRecord::Base + self.table_name = "spree_variants" + + belongs_to :product + + def name + namer = OpenFoodNetwork::OptionValueNamer.new(self) + namer.name + end + end + + class Product < ActiveRecord::Base + self.table_name = "spree_products" + + belongs_to :supplier, class_name: "Enterprise" + end + + class Enterprise < ActiveRecord::Base; end + + def up + ensure_reports_path_exists + + CSV.open(csv_path, "w") do |csv| + csv << csv_header_row + + update_use_producer_stock_settings_with_count_on_hand(csv) + update_on_demand_with_count_on_hand(csv) + update_limited_stock_without_count_on_hand(csv) + end + + split_csv_by_distributor + end + + def down + CSV.foreach(csv_path, headers: true) do |row| + VariantOverride.where(id: row["variant_override_id"]) + .update_all(on_demand: row["previous_on_demand"], + count_on_hand: row["previous_count_on_hand"]) + end + end + + private + + def reports_path + Rails.root.join("reports", "SimplifyVariantOverrideStockSettings") + end + + def ensure_reports_path_exists + Dir.mkdir(reports_path) unless File.exist?(reports_path) + end + + def csv_path + reports_path.join("changed_variant_overrides.csv") + end + + def distributor_csv_path(name, id) + reports_path.join("changed_variant_overrides-#{name.parameterize('_')}-#{id}.csv") + end + + # When on_demand is nil but count_on_hand is set, force limited stock. + def update_use_producer_stock_settings_with_count_on_hand(csv) + variant_overrides = VariantOverride.where(on_demand: nil).with_count_on_hand + update_variant_overrides_and_log(csv, variant_overrides) do |variant_override| + variant_override.update_attributes!(on_demand: false) + end + end + + # Clear count_on_hand if forcing on demand. + def update_on_demand_with_count_on_hand(csv) + variant_overrides = VariantOverride.where(on_demand: true).with_count_on_hand + update_variant_overrides_and_log(csv, variant_overrides) do |variant_override| + variant_override.update_attributes!(count_on_hand: nil) + end + end + + # When on_demand is false but count on hand is not specified, set this to use producer stock + # settings. + def update_limited_stock_without_count_on_hand(csv) + variant_overrides = VariantOverride.where(on_demand: false).without_count_on_hand + update_variant_overrides_and_log(csv, variant_overrides) do |variant_override| + variant_override.update_attributes!(on_demand: nil) + end + end + + def update_variant_overrides_and_log(csv, variant_overrides) + variant_overrides.find_each do |variant_override| + csv << variant_override_log_row(variant_override) do + yield variant_override + end + end + end + + def csv_header_row + %w( + variant_override_id + distributor_name distributor_id + producer_name producer_id + product_name product_id + variant_description variant_id + previous_on_demand previous_count_on_hand + updated_on_demand updated_count_on_hand + ) + end + + def variant_override_log_row(variant_override) + variant = variant_override.variant + distributor = variant_override.hub + product = variant.andand.product + supplier = product.andand.supplier + + row = [ + variant_override.id, + distributor.andand.name, distributor.andand.id, + supplier.andand.name, supplier.andand.id, + product.andand.name, product.andand.id, + variant.andand.name, variant.andand.id, + variant_override.on_demand, variant_override.count_on_hand + ] + + yield variant_override + + row + [variant_override.on_demand, variant_override.count_on_hand] + end + + def split_csv_by_distributor + table = CSV.read(csv_path) + distributor_ids = table[1..-1].map { |row| row[2] }.uniq # Don't use the header row. + + distributor_ids.each do |distributor_id| + distributor_data_rows = filter_data_rows_for_distributor(table[1..-1], distributor_id) + distributor_name = distributor_data_rows.first[1] + + CSV.open(distributor_csv_path(distributor_name, distributor_id), "w") do |csv| + csv << table[0] # Header row + distributor_data_rows.each { |row| csv << row } + end + end + end + + def filter_data_rows_for_distributor(data_rows, distributor_id) + data_rows.select { |row| row[2] == distributor_id } + end +end diff --git a/db/schema.rb b/db/schema.rb index 7f3b49ca3c..8f067d9bb7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20181123012635) do +ActiveRecord::Schema.define(:version => 20181128054803) do create_table "account_invoices", :force => true do |t| t.integer "user_id", :null => false diff --git a/reports/README.md b/reports/README.md new file mode 100644 index 0000000000..84c8566c33 --- /dev/null +++ b/reports/README.md @@ -0,0 +1,4 @@ +## `reports/` Directory + +This directory may be used for reports generated for the OFN instance. For example, a database +migration may save in this directory a spreadsheet of changes it made during execution. diff --git a/spec/factories.rb b/spec/factories.rb index 8751094d45..eee3cb553f 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -189,9 +189,20 @@ FactoryBot.define do factory :variant_override, :class => VariantOverride do price 77.77 + on_demand false count_on_hand 11111 default_stock 2000 resettable false + + trait :on_demand do + on_demand true + count_on_hand nil + end + + trait :use_producer_stock_settings do + on_demand nil + count_on_hand nil + end end factory :inventory_item, :class => InventoryItem do diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index 60130ddc6d..f2b0bb916b 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -140,8 +140,8 @@ feature %q{ fill_in "variant-overrides-#{variant.id}-sku", with: 'NEWSKU' fill_in "variant-overrides-#{variant.id}-price", with: '777.77' + select_on_demand variant, :no fill_in "variant-overrides-#{variant.id}-count_on_hand", with: '123' - check "variant-overrides-#{variant.id}-on_demand" page.should have_content "Changes to one override remain unsaved." expect do @@ -154,14 +154,15 @@ feature %q{ vo.hub_id.should == hub.id vo.sku.should == "NEWSKU" vo.price.should == 777.77 + expect(vo.on_demand).to eq(false) vo.count_on_hand.should == 123 - vo.on_demand.should == true end describe "creating and then updating the new override" do it "updates the same override instead of creating a duplicate" do # When I create a new override fill_in "variant-overrides-#{variant.id}-price", with: '777.77' + select_on_demand variant, :no fill_in "variant-overrides-#{variant.id}-count_on_hand", with: '123' page.should have_content "Changes to one override remain unsaved." @@ -186,6 +187,7 @@ feature %q{ vo.variant_id.should == variant.id vo.hub_id.should == hub.id vo.price.should == 111.11 + expect(vo.on_demand).to eq(false) vo.count_on_hand.should == 111 end end @@ -218,7 +220,7 @@ feature %q{ end context "with overrides" do - let!(:vo) { create(:variant_override, variant: variant, hub: hub, price: 77.77, count_on_hand: 11111, default_stock: 1000, resettable: true, tag_list: ["tag1","tag2","tag3"]) } + let!(:vo) { create(:variant_override, :on_demand, variant: variant, hub: hub, price: 77.77, default_stock: 1000, resettable: true, tag_list: ["tag1","tag2","tag3"]) } let!(:vo_no_auth) { create(:variant_override, variant: variant, hub: hub2, price: 1, count_on_hand: 2) } let!(:product2) { create(:simple_product, supplier: producer, variant_unit: 'weight', variant_unit_scale: 1) } let!(:variant2) { create(:variant, product: product2, unit_value: 8, price: 1.00, on_hand: 12) } @@ -235,11 +237,15 @@ feature %q{ it "product values are affected by overrides" do page.should have_input "variant-overrides-#{variant.id}-price", with: '77.77', placeholder: '1.23' - page.should have_input "variant-overrides-#{variant.id}-count_on_hand", with: '11111', placeholder: '12' + expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "", placeholder: I18n.t("js.variants.on_demand.yes") + expect(page).to have_select "variant-overrides-#{variant.id}-on_demand", selected: I18n.t("js.variant_overrides.on_demand.yes") + + expect(page).to have_input "variant-overrides-#{variant2.id}-count_on_hand", with: "40", placeholder: "" end it "updates existing overrides" do fill_in "variant-overrides-#{variant.id}-price", with: '22.22' + select_on_demand variant, :no fill_in "variant-overrides-#{variant.id}-count_on_hand", with: '8888' page.should have_content "Changes to one override remain unsaved." @@ -252,13 +258,36 @@ feature %q{ vo.variant_id.should == variant.id vo.hub_id.should == hub.id vo.price.should == 22.22 + expect(vo.on_demand).to eq(false) vo.count_on_hand.should == 8888 end + it "updates on_demand settings" do + select_on_demand variant, :no + click_button I18n.t("save_changes") + expect(page).to have_content I18n.t("js.changes_saved") + + vo.reload + expect(vo.on_demand).to eq(false) + + select_on_demand variant, :yes + click_button I18n.t("save_changes") + expect(page).to have_content I18n.t("js.changes_saved") + + vo.reload + expect(vo.on_demand).to eq(true) + + select_on_demand variant, :use_producer_settings + click_button I18n.t("save_changes") + expect(page).to have_content I18n.t("js.changes_saved") + + vo.reload + expect(vo.on_demand).to be_nil + end + # Any new fields added to the VO model need to be added to this test it "deletes overrides when values are cleared" do first("div#columns-dropdown", :text => "COLUMNS").click - first("div#columns-dropdown div.menu div.menu_item", text: "On Demand").click first("div#columns-dropdown div.menu div.menu_item", text: "Enable Stock Reset?").click first("div#columns-dropdown div.menu div.menu_item", text: "Tags").click first("div#columns-dropdown", :text => "COLUMNS").click @@ -272,6 +301,7 @@ feature %q{ # Clearing values manually fill_in "variant-overrides-#{variant.id}-price", with: '' fill_in "variant-overrides-#{variant.id}-count_on_hand", with: '' + select_on_demand variant, :use_producer_settings fill_in "variant-overrides-#{variant.id}-default_stock", with: '' within "tr#v_#{variant.id}" do vo.tag_list.each do |tag| @@ -297,7 +327,7 @@ feature %q{ first("div#bulk-actions-dropdown div.menu div.menu_item", text: "Reset Stock Levels To Defaults").click page.should have_content 'Stocks reset to defaults.' vo.reload - page.should have_input "variant-overrides-#{variant.id}-count_on_hand", with: '1000', placeholder: '12' + expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "1000", placeholder: "" vo.count_on_hand.should == 1000 end @@ -305,7 +335,7 @@ feature %q{ first("div#bulk-actions-dropdown").click first("div#bulk-actions-dropdown div.menu div.menu_item", text: "Reset Stock Levels To Defaults").click vo_no_reset.reload - page.should have_input "variant-overrides-#{variant2.id}-count_on_hand", with: '40', placeholder: '12' + expect(page).to have_input "variant-overrides-#{variant2.id}-count_on_hand", with: "40", placeholder: "" vo_no_reset.count_on_hand.should == 40 end @@ -315,9 +345,52 @@ feature %q{ first("div#bulk-actions-dropdown div.menu div.menu_item", text: "Reset Stock Levels To Defaults").click page.should have_content "Save changes first" end + + describe "ensuring that on demand and count on hand settings are compatible" do + it "clears count on hand when not limited stock" do + # It clears count_on_hand when selecting true on_demand. + select_on_demand variant, :no + fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" + select_on_demand variant, :yes + expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "" + + # It clears count_on_hand when selecting nil on_demand. + select_on_demand variant, :no + fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" + select_on_demand variant, :use_producer_settings + expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "" + + # It saves the changes. + click_button I18n.t("save_changes") + expect(page).to have_content I18n.t("js.changes_saved") + vo.reload + expect(vo.count_on_hand).to be_nil + expect(vo.on_demand).to be_nil + end + + it "provides explanation when attempting to save variant override with incompatible stock settings" do + # Successfully change stock settings. + select_on_demand variant, :no + fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "1111" + click_button I18n.t("save_changes") + expect(page).to have_content I18n.t("js.changes_saved") + + # Make stock settings incompatible. + select_on_demand variant, :no + fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "" + + # It does not save the changes. + click_button I18n.t("save_changes") + expect(page).to have_content I18n.t("activerecord.errors.models.variant_override.count_on_hand.limited_stock_but_no_count_on_hand") + expect(page).to have_no_content I18n.t("js.changes_saved") + + vo.reload + expect(vo.count_on_hand).to eq(1111) + expect(vo.on_demand).to eq(false) + end + end end end - end describe "when manually placing an order" do @@ -382,4 +455,9 @@ feature %q{ end end end + + def select_on_demand(variant, value_sym) + option_label = I18n.t(value_sym, scope: "js.variant_overrides.on_demand") + select option_label, from: "variant-overrides-#{variant.id}-on_demand" + end end diff --git a/spec/features/consumer/shopping/variant_overrides_spec.rb b/spec/features/consumer/shopping/variant_overrides_spec.rb index ff101fd5a0..6fb52c3e1f 100644 --- a/spec/features/consumer/shopping/variant_overrides_spec.rb +++ b/spec/features/consumer/shopping/variant_overrides_spec.rb @@ -22,7 +22,7 @@ feature "shopping with variant overrides defined", js: true, retry: 3 do let(:v4) { create(:variant, product: p1, price: 44.44, unit_value: 4) } let(:v5) { create(:variant, product: p3, price: 55.55, unit_value: 5, on_demand: true) } let(:v6) { create(:variant, product: p3, price: 66.66, unit_value: 6, on_demand: true) } - let!(:vo1) { create(:variant_override, hub: hub, variant: v1, price: 55.55, count_on_hand: nil, default_stock: nil, resettable: false) } + let!(:vo1) { create(:variant_override, :use_producer_stock_settings, hub: hub, variant: v1, price: 55.55, default_stock: nil, resettable: false) } let!(:vo2) { create(:variant_override, hub: hub, variant: v2, count_on_hand: 0, default_stock: nil, resettable: false) } let!(:vo3) { create(:variant_override, hub: hub, variant: v3, count_on_hand: 0, default_stock: nil, resettable: false) } let!(:vo4) { create(:variant_override, hub: hub, variant: v4, count_on_hand: 3, default_stock: nil, resettable: false) } diff --git a/spec/javascripts/unit/admin/controllers/variant_overrides_controller_spec.js.coffee b/spec/javascripts/unit/admin/controllers/variant_overrides_controller_spec.js.coffee index 450041a417..3ff397eb18 100644 --- a/spec/javascripts/unit/admin/controllers/variant_overrides_controller_spec.js.coffee +++ b/spec/javascripts/unit/admin/controllers/variant_overrides_controller_spec.js.coffee @@ -87,3 +87,115 @@ describe "VariantOverridesCtrl", -> $httpBackend.flush() expect(VariantOverrides.updateData).toHaveBeenCalledWith variant_overrides_mock expect(StatusMessage.display).toHaveBeenCalledWith 'success', 'Stocks reset to defaults.' + + describe "suggesting count_on_hand when on_demand is changed", -> + variant = null + + beforeEach -> + scope.variantOverrides = {123: {}} + + describe "when variant is on demand", -> + beforeEach -> + # Ideally, count_on_hand is blank when the variant is on demand. However, this rule is not + # enforced. + variant = {id: 2, on_demand: true, count_on_hand: 20, on_hand: "On demand"} + + it "clears count_on_hand when variant override uses producer stock settings", -> + scope.variantOverrides[123][2] = {on_demand: null, count_on_hand: 1} + scope.updateCountOnHand(variant, 123) + + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() + + it "clears count_on_hand when variant override forces on demand", -> + scope.variantOverrides[123][2] = {on_demand: true, count_on_hand: 1} + scope.updateCountOnHand(variant, 123) + + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() + + it "clears count_on_hand when variant override forces limited stock", -> + scope.variantOverrides[123][2] = {on_demand: false, count_on_hand: 1} + scope.updateCountOnHand(variant, 123) + + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() + + describe "when variant has limited stock", -> + beforeEach -> + variant = {id: 2, on_demand: false, count_on_hand: 20, on_hand: 20} + + it "clears count_on_hand when variant override uses producer stock settings", -> + scope.variantOverrides[123][2] = {on_demand: null, count_on_hand: 1} + scope.updateCountOnHand(variant, 123) + + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() + + it "clears count_on_hand when variant override forces on demand", -> + scope.variantOverrides[123][2] = {on_demand: true, count_on_hand: 1} + scope.updateCountOnHand(variant, 123) + + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() + + it "sets to producer count_on_hand when variant override forces limited stock", -> + scope.variantOverrides[123][2] = {on_demand: false, count_on_hand: 1} + scope.updateCountOnHand(variant, 123) + + expect(scope.variantOverrides[123][2].count_on_hand).toBe(20) + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBe(20) + + describe "count on hand placeholder", -> + beforeEach -> + scope.variantOverrides = {123: {}} + + describe "when variant is on demand", -> + variant = null + + beforeEach -> + # Ideally, count_on_hand is blank when the variant is on demand. However, this rule is not + # enforced. + variant = {id: 2, on_demand: true, count_on_hand: 20, on_hand: t("on_demand")} + + it "is 'On demand' when variant override uses producer stock settings", -> + scope.variantOverrides[123][2] = {on_demand: null, count_on_hand: 1} + placeholder = scope.countOnHandPlaceholder(variant, 123) + expect(placeholder).toBe(t("on_demand")) + + it "is 'On demand' when variant override is on demand", -> + scope.variantOverrides[123][2] = {on_demand: true, count_on_hand: 1} + placeholder = scope.countOnHandPlaceholder(variant, 123) + expect(placeholder).toBe(t("js.variants.on_demand.yes")) + + it "is blank when variant override is limited stock", -> + scope.variantOverrides[123][2] = {on_demand: false, count_on_hand: 1} + placeholder = scope.countOnHandPlaceholder(variant, 123) + expect(placeholder).toBe('') + + describe "when variant is limited stock", -> + variant = null + + beforeEach -> + variant = {id: 2, on_demand: false, count_on_hand: 20, on_hand: 20} + + it "is variant count on hand when variant override uses producer stock settings", -> + scope.variantOverrides[123][2] = {on_demand: null, count_on_hand: 1} + placeholder = scope.countOnHandPlaceholder(variant, 123) + expect(placeholder).toBe(20) + + it "is 'On demand' when variant override is on demand", -> + scope.variantOverrides[123][2] = {on_demand: true, count_on_hand: 1} + placeholder = scope.countOnHandPlaceholder(variant, 123) + expect(placeholder).toBe(t("js.variants.on_demand.yes")) + + it "is blank when variant override is limited stock", -> + scope.variantOverrides[123][2] = {on_demand: false, count_on_hand: 1} + placeholder = scope.countOnHandPlaceholder(variant, 123) + expect(placeholder).toBe('') diff --git a/spec/lib/open_food_network/scope_variant_to_hub_spec.rb b/spec/lib/open_food_network/scope_variant_to_hub_spec.rb index c3b2f2f8ec..aa5ce4b317 100644 --- a/spec/lib/open_food_network/scope_variant_to_hub_spec.rb +++ b/spec/lib/open_food_network/scope_variant_to_hub_spec.rb @@ -5,7 +5,7 @@ module OpenFoodNetwork let(:hub) { create(:distributor_enterprise) } let(:v) { create(:variant, price: 11.11, count_on_hand: 1, on_demand: true, sku: "VARIANTSKU") } let(:vo) { create(:variant_override, hub: hub, variant: v, price: 22.22, count_on_hand: 2, on_demand: false, sku: "VOSKU") } - let(:vo_price_only) { create(:variant_override, hub: hub, variant: v, price: 22.22, count_on_hand: nil) } + let(:vo_price_only) { create(:variant_override, :use_producer_stock_settings, hub: hub, variant: v, price: 22.22) } let(:scoper) { ScopeVariantToHub.new(hub) } describe "overriding price" do diff --git a/spec/models/variant_override_spec.rb b/spec/models/variant_override_spec.rb index bb799f8603..f785bf352c 100644 --- a/spec/models/variant_override_spec.rb +++ b/spec/models/variant_override_spec.rb @@ -35,6 +35,81 @@ describe VariantOverride do end end + describe "validation" do + describe "ensuring that on_demand and count_on_hand are compatible" do + let(:variant_override) { build(:variant_override, hub: hub, variant: variant, + on_demand: on_demand, count_on_hand: count_on_hand) } + + context "when using producer stock settings" do + let(:on_demand) { nil } + + context "when count_on_hand is blank" do + let(:count_on_hand) { nil } + + it "is valid" do + expect(variant_override.save).to be_truthy + end + end + + context "when count_on_hand is set" do + let(:count_on_hand) { 1 } + + it "is invalid" do + expect(variant_override.save).to be_falsey + error_message = I18n.t("using_producer_stock_settings_but_count_on_hand_set", + scope: [i18n_scope_for_error, "count_on_hand"]) + expect(variant_override.errors[:count_on_hand]).to eq([error_message]) + end + end + end + + context "when on demand" do + let(:on_demand) { true } + + context "when count_on_hand is blank" do + let(:count_on_hand) { nil } + + it "is valid" do + expect(variant_override.save).to be_truthy + end + end + + context "when count_on_hand is set" do + let(:count_on_hand) { 1 } + + it "is invalid" do + expect(variant_override.save).to be_falsey + error_message = I18n.t("on_demand_but_count_on_hand_set", + scope: [i18n_scope_for_error, "count_on_hand"]) + expect(variant_override.errors[:count_on_hand]).to eq([error_message]) + end + end + end + + context "when limited stock" do + let(:on_demand) { false } + + context "when count_on_hand is blank" do + let(:count_on_hand) { nil } + + it "is invalid" do + expect(variant_override.save).to be_falsey + error_message = I18n.t("limited_stock_but_no_count_on_hand", + scope: [i18n_scope_for_error, "count_on_hand"]) + expect(variant_override.errors[:count_on_hand]).to eq([error_message]) + end + end + + context "when count_on_hand is set" do + let(:count_on_hand) { 1 } + + it "is valid" do + expect(variant_override.save).to be_truthy + end + end + end + end + end describe "callbacks" do let!(:vo) { create(:variant_override, hub: hub, variant: variant) } @@ -51,7 +126,6 @@ describe VariantOverride do end end - describe "looking up prices" do it "returns the numeric price when present" do VariantOverride.create!(variant: variant, hub: hub, price: 12.34) @@ -65,7 +139,7 @@ describe VariantOverride do 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.create!(variant: variant, hub: hub, count_on_hand: 12, on_demand: false) VariantOverride.count_on_hand_for(hub, variant).should == 12 end @@ -76,12 +150,12 @@ describe VariantOverride do 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) + create(:variant_override, variant: variant, hub: hub, on_demand: false, 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) + create(:variant_override, variant: variant, hub: hub, on_demand: nil, count_on_hand: nil) VariantOverride.stock_overridden?(hub, variant).should be false end @@ -136,17 +210,42 @@ describe VariantOverride do end describe "resetting stock levels" 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 + describe "forcing the on hand level to the value in the default_stock field" do + it "succeeds for variant override that forces limited stock" do + vo = create(:variant_override, variant: variant, hub: hub, count_on_hand: 12, default_stock: 20, resettable: true) + vo.reset_stock! + + vo.reload + expect(vo.on_demand).to eq(false) + expect(vo.count_on_hand).to eq(20) + end + + it "succeeds for variant override that forces unlimited stock" do + vo = create(:variant_override, :on_demand, variant: variant, hub: hub, default_stock: 20, resettable: true) + vo.reset_stock! + + vo.reload + expect(vo.on_demand).to eq(false) + expect(vo.count_on_hand).to eq(20) + end + + it "succeeds for variant override that uses producer stock settings" do + vo = create(:variant_override, :use_producer_stock_settings, variant: variant, hub: hub, default_stock: 20, resettable: true) + vo.reset_stock! + + vo.reload + expect(vo.on_demand).to eq(false) + expect(vo.count_on_hand).to eq(20) + end 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.reset_stock! vo.reload.count_on_hand.should == 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! @@ -157,4 +256,8 @@ describe VariantOverride do context "extends LocalizedNumber" do it_behaves_like "a model using the LocalizedNumber module", [:price] end + + def i18n_scope_for_error + "activerecord.errors.models.variant_override" + end end