From 4b3198a2b4dd4c72f89409f1ceda0c8e7163fb3d Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Wed, 21 Nov 2018 23:28:34 +0800 Subject: [PATCH 01/34] Allow reset of "on demand" for variant overrides Change the UI control for "on demand" in the Inventory page from a checkbox to a SELECT field with three options: nil, true, and false. This resolves the following issues: * There is no way to tell between nil and false - both are represented by an unticked checkbox. * There is no way to go back to nil. --- .../variant_overrides_controller.js.coffee | 4 +++ .../_products_variants.html.haml | 5 ++-- config/locales/en.yml | 5 ++++ spec/features/admin/variant_overrides_spec.rb | 30 +++++++++++++++++-- 4 files changed, 39 insertions(+), 5 deletions(-) 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..1bf77e160b 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,10 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", $scope.RequestMonitor = RequestMonitor $scope.selectView = Views.selectView $scope.currentView = -> Views.currentView + $scope.onDemandOptions = [ + { description: t('js.yes'), value: true }, + { description: t('js.no'), value: false } + ] $scope.views = Views.setViews inventory: { name: t('js.variant_overrides.inventory_products'), visible: true } diff --git a/app/views/admin/variant_overrides/_products_variants.html.haml b/app/views/admin/variant_overrides/_products_variants.html.haml index 7c530a1cb2..5a3f3b2b39 100644 --- a/app/views/admin/variant_overrides/_products_variants.html.haml +++ b/app/views/admin/variant_overrides/_products_variants.html.haml @@ -10,7 +10,8 @@ %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'} %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', options: 'option.value as option.description for option in onDemandOptions' }, 'ofn-track-variant-override' => 'on_demand' } + %option{ value: '' }= t(".on_demand.use_producer_settings") %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 +25,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/locales/en.yml b/config/locales/en.yml index de1222902e..25a5fcc179 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -623,6 +623,9 @@ en: new_powertip: These products are available to be added to your inventory. Click 'Add' to add a product to your inventory, or 'Hide' to hide it from view. You can always change your mind later! controls: back_to_my_inventory: Back to my inventory + products_variants: + on_demand: + use_producer_settings: "Use producer settings" orders: invoice_email_sent: 'Invoice email has been sent' order_email_resent: 'Order email has been resent' @@ -2445,6 +2448,8 @@ See the %{link} to find out more about %{sitename}'s features and to start using pending: Pending shipped: Shipped js: + "yes": "Yes" + "no": "No" saving: 'Saving...' changes_saved: 'Changes saved.' save_changes_first: Save changes first. diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index 60130ddc6d..1421cb496c 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -141,7 +141,7 @@ feature %q{ fill_in "variant-overrides-#{variant.id}-sku", with: 'NEWSKU' fill_in "variant-overrides-#{variant.id}-price", with: '777.77' fill_in "variant-overrides-#{variant.id}-count_on_hand", with: '123' - check "variant-overrides-#{variant.id}-on_demand" + select I18n.t("js.yes"), from: "variant-overrides-#{variant.id}-on_demand" page.should have_content "Changes to one override remain unsaved." expect do @@ -218,7 +218,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, variant: variant, hub: hub, price: 77.77, on_demand: true, count_on_hand: 11111, 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) } @@ -236,6 +236,7 @@ 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_select "variant-overrides-#{variant.id}-on_demand", selected: I18n.t("js.yes") end it "updates existing overrides" do @@ -255,10 +256,32 @@ feature %q{ vo.count_on_hand.should == 8888 end + it "updates on_demand settings" do + select I18n.t("js.no"), from: "variant-overrides-#{variant.id}-on_demand" + 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 I18n.t("js.yes"), from: "variant-overrides-#{variant.id}-on_demand" + 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 I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings"), from: "variant-overrides-#{variant.id}-on_demand" + 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 +295,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 I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings"), from: "variant-overrides-#{variant.id}-on_demand" fill_in "variant-overrides-#{variant.id}-default_stock", with: '' within "tr#v_#{variant.id}" do vo.tag_list.each do |tag| From 7ae555cd9ce0c5f11333a3e3962b45792de289b5 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Mon, 26 Nov 2018 13:41:32 +0800 Subject: [PATCH 02/34] Translate variant override on_demand yes and no --- .../controllers/variant_overrides_controller.js.coffee | 4 ++-- config/locales/en.yml | 3 +++ spec/features/admin/variant_overrides_spec.rb | 8 ++++---- 3 files changed, 9 insertions(+), 6 deletions(-) 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 1bf77e160b..4598ad817d 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 @@ -14,8 +14,8 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", $scope.selectView = Views.selectView $scope.currentView = -> Views.currentView $scope.onDemandOptions = [ - { description: t('js.yes'), value: true }, - { description: t('js.no'), value: false } + { description: t('js.variant_overrides.on_demand.yes'), value: true }, + { description: t('js.variant_overrides.on_demand.no'), value: false } ] $scope.views = Views.setViews diff --git a/config/locales/en.yml b/config/locales/en.yml index 25a5fcc179..9bb981c7f3 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2596,6 +2596,9 @@ See the %{link} to find out more about %{sitename}'s features and to start using now_out_of_stock: is now out of stock. only_n_remainging: "now only has %{num} remaining." variant_overrides: + on_demand: + "yes": "Yes" + "no": "No" inventory_products: "Inventory Products" hidden_products: "Hidden Products" new_products: "New Products" diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index 1421cb496c..07273dfc19 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -141,7 +141,7 @@ feature %q{ fill_in "variant-overrides-#{variant.id}-sku", with: 'NEWSKU' fill_in "variant-overrides-#{variant.id}-price", with: '777.77' fill_in "variant-overrides-#{variant.id}-count_on_hand", with: '123' - select I18n.t("js.yes"), from: "variant-overrides-#{variant.id}-on_demand" + select I18n.t("js.variant_overrides.on_demand.yes"), from: "variant-overrides-#{variant.id}-on_demand" page.should have_content "Changes to one override remain unsaved." expect do @@ -236,7 +236,7 @@ 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_select "variant-overrides-#{variant.id}-on_demand", selected: I18n.t("js.yes") + expect(page).to have_select "variant-overrides-#{variant.id}-on_demand", selected: I18n.t("js.variant_overrides.on_demand.yes") end it "updates existing overrides" do @@ -257,14 +257,14 @@ feature %q{ end it "updates on_demand settings" do - select I18n.t("js.no"), from: "variant-overrides-#{variant.id}-on_demand" + select I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" 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 I18n.t("js.yes"), from: "variant-overrides-#{variant.id}-on_demand" + select I18n.t("js.variant_overrides.on_demand.yes"), from: "variant-overrides-#{variant.id}-on_demand" click_button I18n.t("save_changes") expect(page).to have_content I18n.t("js.changes_saved") From a32bb0445fd33f35c41191c5bd720d59406b0e0a Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Wed, 28 Nov 2018 18:12:35 +0800 Subject: [PATCH 03/34] Change translation for variant override use_producer_settings --- config/locales/en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 9bb981c7f3..7738fed822 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -625,7 +625,7 @@ en: back_to_my_inventory: Back to my inventory products_variants: on_demand: - use_producer_settings: "Use producer settings" + use_producer_settings: "Use producer stock settings" orders: invoice_email_sent: 'Invoice email has been sent' order_email_resent: 'Order email has been resent' From cc003c99d54ed1b0508d4bc6a0c46322bf81a8bc Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Wed, 28 Nov 2018 19:31:39 +0800 Subject: [PATCH 04/34] Ensure in UI compatible VO count and on demand --- .../variant_overrides_controller.js.coffee | 12 ++++ .../_products_variants.html.haml | 4 +- spec/features/admin/variant_overrides_spec.rb | 36 ++++++++++- ...ariant_overrides_controller_spec.js.coffee | 63 +++++++++++++++++++ 4 files changed, 112 insertions(+), 3 deletions(-) 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 4598ad817d..c0c6144f49 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 @@ -109,3 +109,15 @@ 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) + + $scope.selectLimitedStockIfCountOnHandSet = (hubId, variantId) -> + variantOverride = $scope.variantOverrides[hubId][variantId] + if variantOverride.count_on_hand? && variantOverride.count_on_hand != '' && variantOverride.on_demand != false + variantOverride.on_demand = false + DirtyVariantOverrides.set hubId, variantId, variantOverride.id, 'on_demand', false + + $scope.clearCountOnHandUnlessLimitedStock = (hubId, variantId) -> + variantOverride = $scope.variantOverrides[hubId][variantId] + unless variantOverride.on_demand == false && variantOverride.count_on_hand? + variantOverride.count_on_hand = null + DirtyVariantOverrides.set hubId, variantId, variantOverride.id, 'count_on_hand', null diff --git a/app/views/admin/variant_overrides/_products_variants.html.haml b/app/views/admin/variant_overrides/_products_variants.html.haml index 5a3f3b2b39..38614ad2ba 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', change: 'selectLimitedStockIfCountOnHandSet(hub_id, variant.id)' }, placeholder: '{{ variant.on_hand }}', 'ofn-track-variant-override' => 'count_on_hand'} %td.on_demand{ ng: { show: 'columns.on_demand.visible' } } - %select{ name: 'variant-overrides-{{ variant.id }}-on_demand', ng: { model: 'variantOverrides[hub_id][variant.id].on_demand', options: 'option.value as option.description for option in onDemandOptions' }, 'ofn-track-variant-override' => 'on_demand' } + %select{ name: 'variant-overrides-{{ variant.id }}-on_demand', ng: { model: 'variantOverrides[hub_id][variant.id].on_demand', change: 'clearCountOnHandUnlessLimitedStock(hub_id, variant.id)', options: 'option.value as option.description for option in onDemandOptions' }, 'ofn-track-variant-override' => 'on_demand' } %option{ value: '' }= t(".on_demand.use_producer_settings") %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'} diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index 07273dfc19..9be249de4d 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -339,9 +339,43 @@ 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 "changes to limited stock when count on hand is set" do + # It sets on_demand to false when count_on_hand is filled. + fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" + expect(page).to have_select "variant-overrides-#{variant.id}-on_demand", selected: I18n.t("js.variant_overrides.on_demand.no") + + # 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 eq(200) + expect(vo.on_demand).to eq(false) + end + + it "clears count on hand when not limited stock" do + # It clears count_on_hand when selecting true on_demand. + fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" + select I18n.t("js.variant_overrides.on_demand.yes"), from: "variant-overrides-#{variant.id}-on_demand" + expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "" + + # It clears count_on_hand when selecting nil on_demand. + fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" + select I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings"), from: "variant-overrides-#{variant.id}-on_demand" + 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 + end end end - end describe "when manually placing an order" do 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..1ac20b4318 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,66 @@ describe "VariantOverridesCtrl", -> $httpBackend.flush() expect(VariantOverrides.updateData).toHaveBeenCalledWith variant_overrides_mock expect(StatusMessage.display).toHaveBeenCalledWith 'success', 'Stocks reset to defaults.' + + describe "ensuring that on demand and count on hand settings are compatible", -> + describe "changing to limited stock when count on hand is set", -> + beforeEach -> + scope.variantOverrides = {123: {2: {id: 5, on_demand: true}}} + + describe "when count on hand is set", -> + beforeEach -> + scope.variantOverrides[123][2].count_on_hand = 1 + + it "changes to limited stock and registers dirty attribute", -> + scope.selectLimitedStockIfCountOnHandSet(123, 2) + expect(scope.variantOverrides[123][2].on_demand).toBe(false) + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.on_demand).toBe(false) + + describe "when count on hand is null", -> + beforeEach -> + scope.variantOverrides[123][2].count_on_hand = null + + it "does not change to limited stock", -> + scope.selectLimitedStockIfCountOnHandSet(123, 2) + expect(scope.variantOverrides[123][2].on_demand).toBe(true) + + describe "when count on hand is blank string", -> + beforeEach -> + scope.variantOverrides[123][2].count_on_hand = '' + + it "does not change to limited stock", -> + scope.selectLimitedStockIfCountOnHandSet(123, 2) + expect(scope.variantOverrides[123][2].on_demand).toBe(true) + + describe "clearing count on hand when not limited stock", -> + beforeEach -> + scope.variantOverrides = {123: {2: {id: 5, count_on_hand: 1}}} + + describe "when on demand is false", -> + beforeEach -> + scope.variantOverrides[123][2].on_demand = false + + it "does not clear count on hand", -> + scope.clearCountOnHandUnlessLimitedStock(123, 2) + expect(scope.variantOverrides[123][2].count_on_hand).toEqual(1) + + describe "when on demand is true", -> + beforeEach -> + scope.variantOverrides[123][2].on_demand = true + + it "clears count on hand and registers dirty attribute", -> + scope.clearCountOnHandUnlessLimitedStock(123, 2) + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() + + describe "when on demand is null", -> + beforeEach -> + scope.variantOverrides[123][2].on_demand = null + + it "clears count on hand and registers dirty attribute", -> + scope.clearCountOnHandUnlessLimitedStock(123, 2) + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() From 09b9f968b7c4bc03ab2358ec8d6b4d7b15f46cdd Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 29 Nov 2018 04:08:08 +0800 Subject: [PATCH 05/34] Make VO count on hand readonly unless limited stock --- .../variant_overrides_controller.js.coffee | 6 ---- .../_products_variants.html.haml | 2 +- spec/features/admin/variant_overrides_spec.rb | 24 +++++---------- ...ariant_overrides_controller_spec.js.coffee | 30 ------------------- 4 files changed, 9 insertions(+), 53 deletions(-) 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 c0c6144f49..4294d88966 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 @@ -110,12 +110,6 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", .error (data, status) -> $timeout -> StatusMessage.display 'failure', $scope.updateError(data, status) - $scope.selectLimitedStockIfCountOnHandSet = (hubId, variantId) -> - variantOverride = $scope.variantOverrides[hubId][variantId] - if variantOverride.count_on_hand? && variantOverride.count_on_hand != '' && variantOverride.on_demand != false - variantOverride.on_demand = false - DirtyVariantOverrides.set hubId, variantId, variantOverride.id, 'on_demand', false - $scope.clearCountOnHandUnlessLimitedStock = (hubId, variantId) -> variantOverride = $scope.variantOverrides[hubId][variantId] unless variantOverride.on_demand == false && variantOverride.count_on_hand? diff --git a/app/views/admin/variant_overrides/_products_variants.html.haml b/app/views/admin/variant_overrides/_products_variants.html.haml index 38614ad2ba..d0e714e9fa 100644 --- a/app/views/admin/variant_overrides/_products_variants.html.haml +++ b/app/views/admin/variant_overrides/_products_variants.html.haml @@ -8,7 +8,7 @@ %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', change: 'selectLimitedStockIfCountOnHandSet(hub_id, variant.id)' }, 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: '{{ variant.on_hand }}', 'ofn-track-variant-override' => 'count_on_hand'} %td.on_demand{ ng: { show: 'columns.on_demand.visible' } } %select{ name: 'variant-overrides-{{ variant.id }}-on_demand', ng: { model: 'variantOverrides[hub_id][variant.id].on_demand', change: 'clearCountOnHandUnlessLimitedStock(hub_id, variant.id)', options: 'option.value as option.description for option in onDemandOptions' }, 'ofn-track-variant-override' => 'on_demand' } %option{ value: '' }= t(".on_demand.use_producer_settings") diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index 9be249de4d..e79e0293b9 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 I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" fill_in "variant-overrides-#{variant.id}-count_on_hand", with: '123' - select I18n.t("js.variant_overrides.on_demand.yes"), from: "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 I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" 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 @@ -241,6 +243,7 @@ feature %q{ it "updates existing overrides" do fill_in "variant-overrides-#{variant.id}-price", with: '22.22' + select I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" fill_in "variant-overrides-#{variant.id}-count_on_hand", with: '8888' page.should have_content "Changes to one override remain unsaved." @@ -253,6 +256,7 @@ 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 @@ -341,27 +345,15 @@ feature %q{ end describe "ensuring that on demand and count on hand settings are compatible" do - it "changes to limited stock when count on hand is set" do - # It sets on_demand to false when count_on_hand is filled. - fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" - expect(page).to have_select "variant-overrides-#{variant.id}-on_demand", selected: I18n.t("js.variant_overrides.on_demand.no") - - # 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 eq(200) - expect(vo.on_demand).to eq(false) - end - it "clears count on hand when not limited stock" do # It clears count_on_hand when selecting true on_demand. + select I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" select I18n.t("js.variant_overrides.on_demand.yes"), from: "variant-overrides-#{variant.id}-on_demand" expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "" # It clears count_on_hand when selecting nil on_demand. + select I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" select I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings"), from: "variant-overrides-#{variant.id}-on_demand" expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "" 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 1ac20b4318..203e3bc9ce 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 @@ -89,36 +89,6 @@ describe "VariantOverridesCtrl", -> expect(StatusMessage.display).toHaveBeenCalledWith 'success', 'Stocks reset to defaults.' describe "ensuring that on demand and count on hand settings are compatible", -> - describe "changing to limited stock when count on hand is set", -> - beforeEach -> - scope.variantOverrides = {123: {2: {id: 5, on_demand: true}}} - - describe "when count on hand is set", -> - beforeEach -> - scope.variantOverrides[123][2].count_on_hand = 1 - - it "changes to limited stock and registers dirty attribute", -> - scope.selectLimitedStockIfCountOnHandSet(123, 2) - expect(scope.variantOverrides[123][2].on_demand).toBe(false) - dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] - expect(dirtyVariantOverride.on_demand).toBe(false) - - describe "when count on hand is null", -> - beforeEach -> - scope.variantOverrides[123][2].count_on_hand = null - - it "does not change to limited stock", -> - scope.selectLimitedStockIfCountOnHandSet(123, 2) - expect(scope.variantOverrides[123][2].on_demand).toBe(true) - - describe "when count on hand is blank string", -> - beforeEach -> - scope.variantOverrides[123][2].count_on_hand = '' - - it "does not change to limited stock", -> - scope.selectLimitedStockIfCountOnHandSet(123, 2) - expect(scope.variantOverrides[123][2].on_demand).toBe(true) - describe "clearing count on hand when not limited stock", -> beforeEach -> scope.variantOverrides = {123: {2: {id: 5, count_on_hand: 1}}} From f5dc03e118d99d8dfeac1b2572b6ea64b70e1276 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 29 Nov 2018 04:36:06 +0800 Subject: [PATCH 06/34] Style disabled text fields in admin pages --- app/assets/stylesheets/admin/components/input.css.scss | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 app/assets/stylesheets/admin/components/input.css.scss 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..437e386161 --- /dev/null +++ b/app/assets/stylesheets/admin/components/input.css.scss @@ -0,0 +1,8 @@ +.container { + input { + &[readonly] { + background-color: $disabled-light; + cursor: default; + } + } +} From 4f2d96d763e70a1d886346776533eb73ce9b38c2 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 29 Nov 2018 15:18:01 +0800 Subject: [PATCH 07/34] Use correct placeholders for VO count_on_hand --- .../variant_overrides_controller.js.coffee | 10 ++++ .../_products_variants.html.haml | 2 +- config/locales/en.yml | 3 ++ spec/features/admin/variant_overrides_spec.rb | 4 +- ...ariant_overrides_controller_spec.js.coffee | 48 +++++++++++++++++++ 5 files changed, 64 insertions(+), 3 deletions(-) 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 4294d88966..4788e367e5 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 @@ -110,6 +110,16 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", .error (data, status) -> $timeout -> StatusMessage.display 'failure', $scope.updateError(data, status) + $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 + $scope.clearCountOnHandUnlessLimitedStock = (hubId, variantId) -> variantOverride = $scope.variantOverrides[hubId][variantId] unless variantOverride.on_demand == false && variantOverride.count_on_hand? diff --git a/app/views/admin/variant_overrides/_products_variants.html.haml b/app/views/admin/variant_overrides/_products_variants.html.haml index d0e714e9fa..2c915fa0ae 100644 --- a/app/views/admin/variant_overrides/_products_variants.html.haml +++ b/app/views/admin/variant_overrides/_products_variants.html.haml @@ -8,7 +8,7 @@ %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', readonly: 'variantOverrides[hub_id][variant.id].on_demand != false' }, 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' } } %select{ name: 'variant-overrides-{{ variant.id }}-on_demand', ng: { model: 'variantOverrides[hub_id][variant.id].on_demand', change: 'clearCountOnHandUnlessLimitedStock(hub_id, variant.id)', options: 'option.value as option.description for option in onDemandOptions' }, 'ofn-track-variant-override' => 'on_demand' } %option{ value: '' }= t(".on_demand.use_producer_settings") diff --git a/config/locales/en.yml b/config/locales/en.yml index 7738fed822..879f431452 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2595,6 +2595,9 @@ 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: "yes": "Yes" diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index e79e0293b9..120f5093cb 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -237,7 +237,7 @@ 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' + page.should have_input "variant-overrides-#{variant.id}-count_on_hand", with: '11111', 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") end @@ -325,7 +325,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' + page.should have_input "variant-overrides-#{variant.id}-count_on_hand", with: '1000', placeholder: I18n.t("js.variants.on_demand.yes") vo.count_on_hand.should == 1000 end 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 203e3bc9ce..e7e17648e8 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 @@ -120,3 +120,51 @@ describe "VariantOverridesCtrl", -> expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] expect(dirtyVariantOverride.count_on_hand).toBeNull() + + 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('') From cff4eb0005e4e8a8707e7fb8dc74ed292a995ce7 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 29 Nov 2018 16:33:34 +0800 Subject: [PATCH 08/34] Improve and simplify VO count on hand suggestions Set variant override count_on_hand in UI to variant count_on_hand value if on_demand is changed and both specify limited stock. --- .../variant_overrides_controller.js.coffee | 22 +++-- .../_products_variants.html.haml | 2 +- ...ariant_overrides_controller_spec.js.coffee | 81 +++++++++++++------ 3 files changed, 74 insertions(+), 31 deletions(-) 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 4788e367e5..7779a9d602 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 @@ -120,8 +120,20 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", else variant.on_hand - $scope.clearCountOnHandUnlessLimitedStock = (hubId, variantId) -> - variantOverride = $scope.variantOverrides[hubId][variantId] - unless variantOverride.on_demand == false && variantOverride.count_on_hand? - variantOverride.count_on_hand = null - DirtyVariantOverrides.set hubId, variantId, variantOverride.id, 'count_on_hand', null + # 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) + unless 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/views/admin/variant_overrides/_products_variants.html.haml b/app/views/admin/variant_overrides/_products_variants.html.haml index 2c915fa0ae..780590530f 100644 --- a/app/views/admin/variant_overrides/_products_variants.html.haml +++ b/app/views/admin/variant_overrides/_products_variants.html.haml @@ -10,7 +10,7 @@ %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', 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' } } - %select{ name: 'variant-overrides-{{ variant.id }}-on_demand', ng: { model: 'variantOverrides[hub_id][variant.id].on_demand', change: 'clearCountOnHandUnlessLimitedStock(hub_id, variant.id)', options: 'option.value as option.description for option in onDemandOptions' }, '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' } %option{ value: '' }= t(".on_demand.use_producer_settings") %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'} 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 e7e17648e8..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 @@ -88,38 +88,69 @@ describe "VariantOverridesCtrl", -> expect(VariantOverrides.updateData).toHaveBeenCalledWith variant_overrides_mock expect(StatusMessage.display).toHaveBeenCalledWith 'success', 'Stocks reset to defaults.' - describe "ensuring that on demand and count on hand settings are compatible", -> - describe "clearing count on hand when not limited stock", -> + describe "suggesting count_on_hand when on_demand is changed", -> + variant = null + + beforeEach -> + scope.variantOverrides = {123: {}} + + describe "when variant is on demand", -> beforeEach -> - scope.variantOverrides = {123: {2: {id: 5, count_on_hand: 1}}} + # 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"} - describe "when on demand is false", -> - beforeEach -> - scope.variantOverrides[123][2].on_demand = false + 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) - it "does not clear count on hand", -> - scope.clearCountOnHandUnlessLimitedStock(123, 2) - expect(scope.variantOverrides[123][2].count_on_hand).toEqual(1) + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() - describe "when on demand is true", -> - beforeEach -> - scope.variantOverrides[123][2].on_demand = true + 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) - it "clears count on hand and registers dirty attribute", -> - scope.clearCountOnHandUnlessLimitedStock(123, 2) - expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() - dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] - expect(dirtyVariantOverride.count_on_hand).toBeNull() + expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() + dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] + expect(dirtyVariantOverride.count_on_hand).toBeNull() - describe "when on demand is null", -> - beforeEach -> - scope.variantOverrides[123][2].on_demand = null + 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) - it "clears count on hand and registers dirty attribute", -> - scope.clearCountOnHandUnlessLimitedStock(123, 2) - expect(scope.variantOverrides[123][2].count_on_hand).toBeNull() - dirtyVariantOverride = DirtyVariantOverrides.dirtyVariantOverrides[123][2] - expect(dirtyVariantOverride.count_on_hand).toBeNull() + 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 -> From 34c34b94d1f2d6826adc71f92a716311bab3832d Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 30 Nov 2018 00:08:49 +0800 Subject: [PATCH 09/34] Describe method countOnHandPlaceholder for VOs --- .../controllers/variant_overrides_controller.js.coffee | 4 ++++ 1 file changed, 4 insertions(+) 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 7779a9d602..2727472b5a 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 @@ -110,6 +110,10 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", .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] From 47d51ca525923b82dc8e1f229a76c7f3d3de1f50 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 30 Nov 2018 00:13:52 +0800 Subject: [PATCH 10/34] Prefer guard clause in updateCountOnHand for VOs --- .../controllers/variant_overrides_controller.js.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 2727472b5a..933566a191 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 @@ -131,9 +131,9 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", variantOverride = $scope.variantOverrides[hubId][variant.id] suggested = $scope.countOnHandSuggestion(variant, hubId) - unless suggested == variantOverride.count_on_hand - variantOverride.count_on_hand = suggested - DirtyVariantOverrides.set hubId, variant.id, variantOverride.id, 'count_on_hand', suggested + 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. From d8908bdf4b30d37dfeadb7f73b47eb2ecd316d14 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 30 Nov 2018 00:15:46 +0800 Subject: [PATCH 11/34] Remove unused translation keys js.yes and js.no --- config/locales/en.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 879f431452..69b55c9125 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2448,8 +2448,6 @@ See the %{link} to find out more about %{sitename}'s features and to start using pending: Pending shipped: Shipped js: - "yes": "Yes" - "no": "No" saving: 'Saving...' changes_saved: 'Changes saved.' save_changes_first: Save changes first. From 802e3bb447599f4c746ec7ab1b0759943d7916c8 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 30 Nov 2018 17:40:00 +0800 Subject: [PATCH 12/34] Refactor choosing VO on_demand in feature specs We are still using the Rails view I18n scope for :use_producer_settings. Angular requires this replacement of the default blank option, which we use for nil on_demand, to be in the template itself. If we eventually move this template to a JS template, we can move it to the same I18n scope as the :yes and :no options. --- spec/features/admin/variant_overrides_spec.rb | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index 120f5093cb..4b89f87bfd 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -140,7 +140,7 @@ feature %q{ fill_in "variant-overrides-#{variant.id}-sku", with: 'NEWSKU' fill_in "variant-overrides-#{variant.id}-price", with: '777.77' - select I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" + 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." @@ -162,7 +162,7 @@ feature %q{ 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 I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" + 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." @@ -243,7 +243,7 @@ feature %q{ it "updates existing overrides" do fill_in "variant-overrides-#{variant.id}-price", with: '22.22' - select I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" + 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." @@ -261,21 +261,21 @@ feature %q{ end it "updates on_demand settings" do - select I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" + 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 I18n.t("js.variant_overrides.on_demand.yes"), from: "variant-overrides-#{variant.id}-on_demand" + 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 I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings"), from: "variant-overrides-#{variant.id}-on_demand" + select_on_demand variant, :use_producer_settings click_button I18n.t("save_changes") expect(page).to have_content I18n.t("js.changes_saved") @@ -299,7 +299,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 I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings"), from: "variant-overrides-#{variant.id}-on_demand" + 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| @@ -347,15 +347,15 @@ feature %q{ 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 I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" + select_on_demand variant, :no fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" - select I18n.t("js.variant_overrides.on_demand.yes"), from: "variant-overrides-#{variant.id}-on_demand" + 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 I18n.t("js.variant_overrides.on_demand.no"), from: "variant-overrides-#{variant.id}-on_demand" + select_on_demand variant, :no fill_in "variant-overrides-#{variant.id}-count_on_hand", with: "200" - select I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings"), from: "variant-overrides-#{variant.id}-on_demand" + select_on_demand variant, :use_producer_settings expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "" # It saves the changes. @@ -432,4 +432,16 @@ feature %q{ end end end + + def select_on_demand(variant, value_sym) + option_label = case value_sym + when :no + I18n.t("js.variant_overrides.on_demand.no") + when :yes + I18n.t("js.variant_overrides.on_demand.yes") + when :use_producer_settings + I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings") + end + select option_label, from: "variant-overrides-#{variant.id}-on_demand" + end end From 842a8e365478e41440d643b17975ca16f5e12825 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Wed, 5 Dec 2018 16:39:29 +0800 Subject: [PATCH 13/34] Move null option for VO use_producer_settings to JS --- .../controllers/variant_overrides_controller.js.coffee | 1 + .../admin/variant_overrides/_products_variants.html.haml | 1 - config/locales/en.yml | 4 +--- spec/features/admin/variant_overrides_spec.rb | 9 +-------- 4 files changed, 3 insertions(+), 12 deletions(-) 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 933566a191..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 @@ -14,6 +14,7 @@ angular.module("admin.variantOverrides").controller "AdminVariantOverridesCtrl", $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 } ] diff --git a/app/views/admin/variant_overrides/_products_variants.html.haml b/app/views/admin/variant_overrides/_products_variants.html.haml index 780590530f..27de58fdec 100644 --- a/app/views/admin/variant_overrides/_products_variants.html.haml +++ b/app/views/admin/variant_overrides/_products_variants.html.haml @@ -11,7 +11,6 @@ %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' } } %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' } - %option{ value: '' }= t(".on_demand.use_producer_settings") %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' } } diff --git a/config/locales/en.yml b/config/locales/en.yml index 69b55c9125..1b04166115 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -623,9 +623,6 @@ en: new_powertip: These products are available to be added to your inventory. Click 'Add' to add a product to your inventory, or 'Hide' to hide it from view. You can always change your mind later! controls: back_to_my_inventory: Back to my inventory - products_variants: - on_demand: - use_producer_settings: "Use producer stock settings" orders: invoice_email_sent: 'Invoice email has been sent' order_email_resent: 'Order email has been resent' @@ -2598,6 +2595,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using "yes": "On demand" variant_overrides: on_demand: + use_producer_settings: "Use producer stock settings" "yes": "Yes" "no": "No" inventory_products: "Inventory Products" diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index 4b89f87bfd..0e2476fdba 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -434,14 +434,7 @@ feature %q{ end def select_on_demand(variant, value_sym) - option_label = case value_sym - when :no - I18n.t("js.variant_overrides.on_demand.no") - when :yes - I18n.t("js.variant_overrides.on_demand.yes") - when :use_producer_settings - I18n.t("admin.variant_overrides.products_variants.on_demand.use_producer_settings") - end + option_label = I18n.t(value_sym, scope: "js.variant_overrides.on_demand") select option_label, from: "variant-overrides-#{variant.id}-on_demand" end end From 71db9f5289d33083827537defeeea793406e2942 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 6 Dec 2018 14:03:20 +0800 Subject: [PATCH 14/34] Import CSS variables in admin/components/input --- app/assets/stylesheets/admin/components/input.css.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/stylesheets/admin/components/input.css.scss b/app/assets/stylesheets/admin/components/input.css.scss index 437e386161..5b4cf8ed05 100644 --- a/app/assets/stylesheets/admin/components/input.css.scss +++ b/app/assets/stylesheets/admin/components/input.css.scss @@ -1,3 +1,5 @@ +@import '../../darkswarm/branding'; + .container { input { &[readonly] { From 83b6973bc2b5ea6f3a222b6705f176699753085d Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 29 Nov 2018 17:10:35 +0800 Subject: [PATCH 15/34] Require compatible VO on_demand and count_on_hand --- app/models/variant_override.rb | 36 ++++++++++++ config/locales/en.yml | 5 ++ spec/models/variant_override_spec.rb | 86 ++++++++++++++++++++++++++-- 3 files changed, 123 insertions(+), 4 deletions(-) diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index 89d2fdcd13..d28bb2771f 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -10,6 +10,8 @@ class VariantOverride < ActiveRecord::Base # 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 + before_validation :require_compatible_on_demand_and_count_on_hand + after_save :refresh_products_cache_from_save after_destroy :refresh_products_cache_from_destroy @@ -104,4 +106,38 @@ class VariantOverride < ActiveRecord::Base def refresh_products_cache_from_destroy OpenFoodNetwork::ProductsCache.variant_override_destroyed self 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("using_producer_stock_settings_but_count_on_hand_set", + scope: [i18n_scope_for_error, "count_on_hand"]) + 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("on_demand_but_count_on_hand_set", + scope: [i18n_scope_for_error, "count_on_hand"]) + 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("limited_stock_but_no_count_on_hand", + scope: [i18n_scope_for_error, "count_on_hand"]) + errors.add(:count_on_hand, error_message) + end + + def i18n_scope_for_error + "activerecord.errors.models.variant_override" + end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 1b04166115..4962171a82 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: diff --git a/spec/models/variant_override_spec.rb b/spec/models/variant_override_spec.rb index bb799f8603..258971e110 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 @@ -157,4 +231,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 From 4c0c0bbfd09b259a42c02a2e18a98fbfe8fcdfa6 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 29 Nov 2018 17:56:32 +0800 Subject: [PATCH 16/34] Simplify stock configuration in existing variant overrides --- ...nt_override_on_demand_and_count_on_hand.rb | 29 +++++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20181128054803_simplify_variant_override_on_demand_and_count_on_hand.rb diff --git a/db/migrate/20181128054803_simplify_variant_override_on_demand_and_count_on_hand.rb b/db/migrate/20181128054803_simplify_variant_override_on_demand_and_count_on_hand.rb new file mode 100644 index 0000000000..eeb5eb5e37 --- /dev/null +++ b/db/migrate/20181128054803_simplify_variant_override_on_demand_and_count_on_hand.rb @@ -0,0 +1,29 @@ +# 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 SimplifyVariantOverrideOnDemandAndCountOnHand < ActiveRecord::Migration + def up + # When on_demand is nil but count_on_hand is set, force limited stock. + VariantOverride.where(on_demand: nil).where("count_on_hand IS NOT NULL") + .update_all(on_demand: false) + + # Clear count_on_hand if forcing on demand. + VariantOverride.where(on_demand: true).update_all(count_on_hand: nil) + + # When on_demand is false but count on hand is not specified, set this to use producer stock + # settings. + VariantOverride.where(on_demand: false, count_on_hand: nil).update_all(on_demand: nil) + end + + def down; 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 From 210a11783cecd3104b9cc1c2591405761ba36f56 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 7 Dec 2018 15:34:03 +0800 Subject: [PATCH 17/34] Test that saving invalid variant override provides error --- spec/features/admin/variant_overrides_spec.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index 0e2476fdba..c218d6099b 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -365,6 +365,27 @@ feature %q{ 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 From ad35ea1102ffa613e9adb2c820324e4d5910e685 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Sun, 9 Dec 2018 21:46:31 +0800 Subject: [PATCH 18/34] Add factory stock traits for variant overrides --- spec/factories.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 From 34313c94ca7a31ab000b50908593815c42637158 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Sun, 9 Dec 2018 21:18:43 +0800 Subject: [PATCH 19/34] Make reset stock settings for VO force limited stock --- app/models/variant_override.rb | 2 +- spec/features/admin/variant_overrides_spec.rb | 4 +-- spec/models/variant_override_spec.rb | 33 ++++++++++++++++--- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/app/models/variant_override.rb b/app/models/variant_override.rb index d28bb2771f..5527f29af7 100644 --- a/app/models/variant_override.rb +++ b/app/models/variant_override.rb @@ -84,7 +84,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/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index c218d6099b..c215abe933 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -325,7 +325,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: I18n.t("js.variants.on_demand.yes") + expect(page).to have_input "variant-overrides-#{variant.id}-count_on_hand", with: "1000", placeholder: "" vo.count_on_hand.should == 1000 end @@ -333,7 +333,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 diff --git a/spec/models/variant_override_spec.rb b/spec/models/variant_override_spec.rb index 258971e110..f785bf352c 100644 --- a/spec/models/variant_override_spec.rb +++ b/spec/models/variant_override_spec.rb @@ -210,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! From d67b3faf015f3452114182bdce5625ebdb42d747 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 7 Dec 2018 15:44:32 +0800 Subject: [PATCH 20/34] Address affected specs from new VO validations --- spec/features/admin/variant_overrides_spec.rb | 6 ++++-- spec/features/consumer/shopping/variant_overrides_spec.rb | 2 +- spec/lib/open_food_network/scope_variant_to_hub_spec.rb | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/spec/features/admin/variant_overrides_spec.rb b/spec/features/admin/variant_overrides_spec.rb index c215abe933..f2b0bb916b 100644 --- a/spec/features/admin/variant_overrides_spec.rb +++ b/spec/features/admin/variant_overrides_spec.rb @@ -220,7 +220,7 @@ feature %q{ end context "with overrides" do - let!(:vo) { create(:variant_override, variant: variant, hub: hub, price: 77.77, on_demand: true, 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) } @@ -237,8 +237,10 @@ 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: I18n.t("js.variants.on_demand.yes") + 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 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/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 From 1bfdb708c1ea4bf13c5422730f5c6078a315661f Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Sun, 9 Dec 2018 23:22:46 +0800 Subject: [PATCH 21/34] Add app/models/concerns to autoload path --- config/application.rb | 1 + 1 file changed, 1 insertion(+) 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 ) From c873a7b8e3e322fc4f1892ed3d8b274ae020febc Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Sun, 9 Dec 2018 23:23:29 +0800 Subject: [PATCH 22/34] Move stock settings override validation to concern --- .../stock_settings_override_validation.rb | 41 +++++++++++++++++++ app/models/variant_override.rb | 37 +---------------- 2 files changed, 42 insertions(+), 36 deletions(-) create mode 100644 app/models/concerns/stock_settings_override_validation.rb 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/variant_override.rb b/app/models/variant_override.rb index 5527f29af7..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 @@ -10,8 +11,6 @@ class VariantOverride < ActiveRecord::Base # 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 - before_validation :require_compatible_on_demand_and_count_on_hand - after_save :refresh_products_cache_from_save after_destroy :refresh_products_cache_from_destroy @@ -106,38 +105,4 @@ class VariantOverride < ActiveRecord::Base def refresh_products_cache_from_destroy OpenFoodNetwork::ProductsCache.variant_override_destroyed self 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("using_producer_stock_settings_but_count_on_hand_set", - scope: [i18n_scope_for_error, "count_on_hand"]) - 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("on_demand_but_count_on_hand_set", - scope: [i18n_scope_for_error, "count_on_hand"]) - 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("limited_stock_but_no_count_on_hand", - scope: [i18n_scope_for_error, "count_on_hand"]) - errors.add(:count_on_hand, error_message) - end - - def i18n_scope_for_error - "activerecord.errors.models.variant_override" - end end From 0de829017ab55f8c595ff899add6333e82038dcc Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Mon, 10 Dec 2018 00:16:49 +0800 Subject: [PATCH 23/34] Refactor finding or initializing VO for product import --- app/models/product_import/entry_validator.rb | 23 +++++++++++--------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/app/models/product_import/entry_validator.rb b/app/models/product_import/entry_validator.rb index b7f055e445..ab6486a627 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,16 +322,7 @@ 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 - - variant_override = existing_variant_override || VariantOverride.new( - variant_id: existing_variant.id, - hub_id: entry.enterprise_id - ) - + variant_override = find_or_initialize_variant_override(entry, existing_variant) 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')) From cec50e81f5714e9a06c90bfb3939cb7caf8efc3d Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Sun, 9 Dec 2018 21:20:18 +0800 Subject: [PATCH 24/34] Make product import set correct VO stock settings --- app/models/product_import/entry_validator.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/models/product_import/entry_validator.rb b/app/models/product_import/entry_validator.rb index ab6486a627..1511160c65 100644 --- a/app/models/product_import/entry_validator.rb +++ b/app/models/product_import/entry_validator.rb @@ -323,8 +323,10 @@ module ProductImport def create_inventory_item(entry, existing_variant) variant_override = find_or_initialize_variant_override(entry, existing_variant) - variant_override.assign_attributes(count_on_hand: entry.on_hand, import_date: @import_time) - check_on_hand_nil(entry, variant_override) + + check_variant_override_stock_settings(entry, variant_override) + + variant_override.assign_attributes(import_date: @import_time) variant_override.assign_attributes(entry.attributes.slice('price', 'on_demand')) variant_override @@ -358,5 +360,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 From df51d137dc87a819b3646e501798079ac387046e Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Mon, 10 Dec 2018 00:21:13 +0800 Subject: [PATCH 25/34] Update some code style for product import --- app/models/product_import/entry_validator.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/app/models/product_import/entry_validator.rb b/app/models/product_import/entry_validator.rb index 1511160c65..11c0aab7dd 100644 --- a/app/models/product_import/entry_validator.rb +++ b/app/models/product_import/entry_validator.rb @@ -322,14 +322,12 @@ module ProductImport end def create_inventory_item(entry, existing_variant) - variant_override = find_or_initialize_variant_override(entry, existing_variant) + find_or_initialize_variant_override(entry, existing_variant).tap do |variant_override| + check_variant_override_stock_settings(entry, variant_override) - check_variant_override_stock_settings(entry, variant_override) - - variant_override.assign_attributes(import_date: @import_time) - 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) From 9e3332ba2aea02133cb9619b3b367d57c238ce38 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 13 Dec 2018 23:25:48 +0800 Subject: [PATCH 26/34] Rename DB migration file --- ... 20181128054803_simplify_variant_override_stock_settings.rb} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename db/migrate/{20181128054803_simplify_variant_override_on_demand_and_count_on_hand.rb => 20181128054803_simplify_variant_override_stock_settings.rb} (93%) diff --git a/db/migrate/20181128054803_simplify_variant_override_on_demand_and_count_on_hand.rb b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb similarity index 93% rename from db/migrate/20181128054803_simplify_variant_override_on_demand_and_count_on_hand.rb rename to db/migrate/20181128054803_simplify_variant_override_stock_settings.rb index eeb5eb5e37..ed42708d0d 100644 --- a/db/migrate/20181128054803_simplify_variant_override_on_demand_and_count_on_hand.rb +++ b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb @@ -11,7 +11,7 @@ # # Furthermore, this will allow all existing variant overrides to satisfy the newly added model # validation rules. -class SimplifyVariantOverrideOnDemandAndCountOnHand < ActiveRecord::Migration +class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration def up # When on_demand is nil but count_on_hand is set, force limited stock. VariantOverride.where(on_demand: nil).where("count_on_hand IS NOT NULL") From aa92dd877109cafcdbe6c89ecd3c0d5610fbc5cc Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Wed, 12 Dec 2018 01:27:52 +0800 Subject: [PATCH 27/34] Declare VariantOverride in migration for safe use --- ...simplify_variant_override_stock_settings.rb | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb index ed42708d0d..7bb370d9f4 100644 --- a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb +++ b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb @@ -12,17 +12,27 @@ # Furthermore, this will allow all existing variant overrides to satisfy the newly added model # validation rules. class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration + class VariantOverride < ActiveRecord::Base + scope :with_count_on_hand, -> { where("count_on_hand IS NOT NULL") } + scope :without_count_on_hand, -> { where(count_on_hand: nil) } + end + def up # When on_demand is nil but count_on_hand is set, force limited stock. - VariantOverride.where(on_demand: nil).where("count_on_hand IS NOT NULL") - .update_all(on_demand: false) + VariantOverride.where(on_demand: nil).with_count_on_hand.find_each do |variant_override| + variant_override.update_attributes!(on_demand: false) + end # Clear count_on_hand if forcing on demand. - VariantOverride.where(on_demand: true).update_all(count_on_hand: nil) + VariantOverride.where(on_demand: true).with_count_on_hand.find_each do |variant_override| + variant_override.update_attributes!(count_on_hand: nil) + end # When on_demand is false but count on hand is not specified, set this to use producer stock # settings. - VariantOverride.where(on_demand: false, count_on_hand: nil).update_all(on_demand: nil) + VariantOverride.where(on_demand: false).without_count_on_hand.find_each do |variant_override| + variant_override.update_attributes!(on_demand: nil) + end end def down; end From 842a11b564bbd6b14a2d4ddb158c6bd9dd95bf41 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 13 Dec 2018 19:15:31 +0800 Subject: [PATCH 28/34] Refactor DB migration script --- ...implify_variant_override_stock_settings.rb | 45 ++++++++++++------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb index 7bb370d9f4..be0718e163 100644 --- a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb +++ b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb @@ -18,22 +18,37 @@ class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration end def up - # When on_demand is nil but count_on_hand is set, force limited stock. - VariantOverride.where(on_demand: nil).with_count_on_hand.find_each do |variant_override| - variant_override.update_attributes!(on_demand: false) - end - - # Clear count_on_hand if forcing on demand. - VariantOverride.where(on_demand: true).with_count_on_hand.find_each do |variant_override| - variant_override.update_attributes!(count_on_hand: nil) - end - - # When on_demand is false but count on hand is not specified, set this to use producer stock - # settings. - VariantOverride.where(on_demand: false).without_count_on_hand.find_each do |variant_override| - variant_override.update_attributes!(on_demand: nil) - end + update_use_producer_stock_settings_with_count_on_hand + update_on_demand_with_count_on_hand + update_limited_stock_without_count_on_hand end def down; end + + private + + # When on_demand is nil but count_on_hand is set, force limited stock. + def update_use_producer_stock_settings_with_count_on_hand + variant_overrides = VariantOverride.where(on_demand: nil).with_count_on_hand + variant_overrides.find_each 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 + variant_overrides = VariantOverride.where(on_demand: true).with_count_on_hand + variant_overrides.find_each 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 + variant_overrides = VariantOverride.where(on_demand: false).without_count_on_hand + variant_overrides.find_each do |variant_override| + variant_override.update_attributes!(on_demand: nil) + end + end end From 5a98acd101948ea922aa10bee42dc802ff59f340 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 13 Dec 2018 23:36:16 +0800 Subject: [PATCH 29/34] Log changes from DB migration to CSV file --- ...implify_variant_override_stock_settings.rb | 88 +++++++++++++++++-- 1 file changed, 79 insertions(+), 9 deletions(-) diff --git a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb index be0718e163..965106cfa9 100644 --- a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb +++ b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb @@ -13,42 +13,112 @@ # 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 - update_use_producer_stock_settings_with_count_on_hand - update_on_demand_with_count_on_hand - update_limited_stock_without_count_on_hand + 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 end def down; end private + def csv_path + Rails.root.join("reports", "SimplifyVariantOverrideStockSettings-changed_variant_overrides.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 + def update_use_producer_stock_settings_with_count_on_hand(csv) variant_overrides = VariantOverride.where(on_demand: nil).with_count_on_hand - variant_overrides.find_each do |variant_override| + 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 + def update_on_demand_with_count_on_hand(csv) variant_overrides = VariantOverride.where(on_demand: true).with_count_on_hand - variant_overrides.find_each do |variant_override| + 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 + def update_limited_stock_without_count_on_hand(csv) variant_overrides = VariantOverride.where(on_demand: false).without_count_on_hand - variant_overrides.find_each do |variant_override| + 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 end From 645812789216aa22dc288dc7f34b5b01e1605a5a Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Thu, 13 Dec 2018 23:59:51 +0800 Subject: [PATCH 30/34] Add undo logic for DB migration script --- ...1128054803_simplify_variant_override_stock_settings.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb index 965106cfa9..0fac5c1ab6 100644 --- a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb +++ b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb @@ -49,7 +49,13 @@ class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration end end - def down; 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 From d859939c7989106aafe831abb41aa30704cb7057 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 14 Dec 2018 10:24:26 +0800 Subject: [PATCH 31/34] Add README.md to reports/ directory --- reports/README.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 reports/README.md 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. From 91e5a523f0abb9d3cfaa82b552d0b234f820436e Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 14 Dec 2018 10:08:29 +0800 Subject: [PATCH 32/34] Add /reports/ to .gitignore but ignore README Using the first slash to make sure that other reports/ directories are not ignored. --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) 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 From a6664d0ac675a5479e75169fdc625e0961540ec2 Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 14 Dec 2018 11:41:56 +0800 Subject: [PATCH 33/34] Move report generated by migration to subdirectory This is in preparation of an upcoming change to split the main CSV file by distributor. --- ...54803_simplify_variant_override_stock_settings.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb index 0fac5c1ab6..2db448389c 100644 --- a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb +++ b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb @@ -40,6 +40,8 @@ class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration class Enterprise < ActiveRecord::Base; end def up + ensure_reports_path_exists + CSV.open(csv_path, "w") do |csv| csv << csv_header_row @@ -59,8 +61,16 @@ class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration 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 - Rails.root.join("reports", "SimplifyVariantOverrideStockSettings-changed_variant_overrides.csv") + reports_path.join("changed_variant_overrides.csv") end # When on_demand is nil but count_on_hand is set, force limited stock. From 77a656276209c68ab23db00646a5c3d0106a214c Mon Sep 17 00:00:00 2001 From: Kristina Lim Date: Fri, 14 Dec 2018 13:00:02 +0800 Subject: [PATCH 34/34] Make DB migration generate summary by distributor --- ...implify_variant_override_stock_settings.rb | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb index 2db448389c..6627dca015 100644 --- a/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb +++ b/db/migrate/20181128054803_simplify_variant_override_stock_settings.rb @@ -49,6 +49,8 @@ class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration update_on_demand_with_count_on_hand(csv) update_limited_stock_without_count_on_hand(csv) end + + split_csv_by_distributor end def down @@ -73,6 +75,10 @@ class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration 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 @@ -137,4 +143,23 @@ class SimplifyVariantOverrideStockSettings < ActiveRecord::Migration 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