From 6d6164c8f661a0e8fa923b06dc9a950a1ba1021b Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Wed, 26 Jun 2024 16:30:47 +0200 Subject: [PATCH 1/4] Keep stock selection when error on saving - added 2 not to be persisted attributes aimed at dealing with the UI - added them to the permitted list - updated view to switch mode about on_hand/on_demand that is: from an already persisted variant or not - Not persisted deals with on_*_desired not to be persisted fields - Persisted mode deals with regular on_* fields - the corresponding spec for both on_hand/on_demand --- app/models/spree/variant.rb | 14 +++++++ app/services/permitted_attributes/variant.rb | 3 +- app/services/sets/product_set.rb | 11 +++++- .../admin/products_v3/_variant_row.html.haml | 11 +++--- spec/system/admin/products_v3/update_spec.rb | 39 +++++++++++++++++++ 5 files changed, 70 insertions(+), 8 deletions(-) diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index acf0e784087..f79340a4f40 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -11,6 +11,11 @@ class Variant < ApplicationRecord self.belongs_to_required_by_default = false + # 2 not to be persisted attributes to store preferences. + # Values to be set via the UI that can be passed by back to UI + # in a not yet persisted variant. Setters are below. + attr_reader :on_hand_desired, :on_demand_desired + acts_as_paranoid searchable_attributes :sku, :display_as, :display_name, :primary_taxon_id, :supplier_id @@ -244,6 +249,15 @@ def variant_unit_with_scale=(variant_unit_with_scale) ) end + # aiming to deal with UI that deals with 0/"0"/1/"1" + def on_demand_desired=(val) + @on_demand_desired = ActiveModel::Type::Boolean.new.cast(val) + end + + def on_hand_desired=(val) + @on_hand_desired = ActiveModel::Type::Integer.new.cast(val) + end + private def check_currency diff --git a/app/services/permitted_attributes/variant.rb b/app/services/permitted_attributes/variant.rb index 9d20975cb0e..e333eded251 100644 --- a/app/services/permitted_attributes/variant.rb +++ b/app/services/permitted_attributes/variant.rb @@ -4,7 +4,8 @@ module PermittedAttributes class Variant def self.attributes [ - :id, :sku, :on_hand, :on_demand, :shipping_category_id, :price, :unit_value, + :id, :sku, :on_hand, :on_hand_desired, :on_demand, :on_demand_desired, + :shipping_category_id, :price, :unit_value, :unit_description, :variant_unit, :variant_unit_name, :variant_unit_scale, :display_name, :display_as, :tax_category_id, :weight, :height, :width, :depth, :taxon_ids, :primary_taxon_id, :supplier_id diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index 156f73f875e..2a692a08a05 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -127,11 +127,11 @@ def create_variant(product, variant_attributes) on_demand = variant_attributes.delete(:on_demand) variant = product.variants.create(variant_attributes) + return variant if variant.errors.present? begin - variant.on_demand = on_demand if on_demand.present? - variant.on_hand = on_hand.to_i if on_hand.present? + create_stock_for_variant(variant, on_demand, on_hand) rescue StandardError => e notify_bugsnag(e, product, variant, variant_attributes) raise e @@ -153,5 +153,12 @@ def notify_bugsnag(error, product, variant, variant_attributes) report.add_metadata(:product_set, :variant_error, variant.errors.first) if !variant.valid? end end + + def create_stock_for_variant(variant, on_demand, on_hand) + variant.on_demand = on_demand if on_demand.present? + variant.on_demand = variant.on_demand_desired if variant.on_demand_desired.present? + variant.on_hand = on_hand.to_i if on_hand.present? + variant.on_hand = variant.on_hand_desired.to_i if variant.on_hand_desired.present? + end end end diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 0df083d76fa..8624121eca8 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -1,3 +1,4 @@ +- method_on_demand, method_on_hand = variant.new_record? ? [:on_demand_desired, :on_hand_desired ]: [:on_demand, :on_hand] %td.col-image -# empty %td.col-name.field.naked_inputs @@ -37,14 +38,14 @@ = error_message_on variant, :price %td.col-on_hand.field.popout{'data-controller': "popout"} %button.popout__button{'data-popout-target': "button", 'aria-label': t('admin.products_page.columns.on_hand')} - = variant.on_demand ? t(:on_demand) : variant.on_hand + = variant.send(method_on_demand) ? t(:on_demand) : variant.send(method_on_hand) %div.popout__container{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" } .field - = f.number_field :on_hand, min: 0, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: f.object.on_demand - = error_message_on variant, :on_hand + = f.number_field method_on_hand, min: 0, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: f.object.send(method_on_demand) + = error_message_on variant, method_on_hand .field.checkbox - = f.label :on_demand do - = f.check_box :on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked' + = f.label method_on_demand do + = f.check_box method_on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked' = t(:on_demand) %td.col-producer.field.naked_inputs = render(SearchableDropdownComponent.new(form: f, diff --git a/spec/system/admin/products_v3/update_spec.rb b/spec/system/admin/products_v3/update_spec.rb index 6dddf79c1f1..d7fd2b0c025 100644 --- a/spec/system/admin/products_v3/update_spec.rb +++ b/spec/system/admin/products_v3/update_spec.rb @@ -654,6 +654,45 @@ end end + context 'When trying to save an invalid variant with Stock value ' do + let(:new_variant_row) { find_field("Name", placeholder: "Apples", with: "").ancestor("tr") } + + before do + visit admin_products_url + click_on "New variant" + end + + it 'displays the correct value afterwards for On Hand' do + within new_variant_row do + fill_in "Name", with: "Large box" + click_on "On Hand" + fill_in "On Hand", with: "19" + end + + click_button "Save changes" + + expect(page).to have_content "Please review the errors and try again" + within row_containing_name("Large box") do + expect(page).to have_content "19" + end + end + + it 'displays the correct value afterwards for On demand' do + within new_variant_row do + fill_in "Name", with: "Large box" + click_on "On Hand" + check "On demand" + end + + click_button "Save changes" + + expect(page).to have_content "Please review the errors and try again" + within row_containing_name("Large box") do + expect(page).to have_content "On demand" + end + end + end + context "pagination" do let!(:product_a) { create(:simple_product, name: "zucchini") } # appears on p2 From 1d3906d431c8e289e0553ddedda352137bd08e44 Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Thu, 27 Jun 2024 17:15:00 +0200 Subject: [PATCH 2/4] Fixes linter error --- .rubocop_todo.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 8f01f0a7269..42a63b3b7db 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -160,6 +160,7 @@ Metrics/ClassLength: - 'app/services/cart_service.rb' - 'app/services/order_cycles/form_service.rb' - 'app/services/orders/sync_service.rb' + - 'app/services/sets/product_set.rb' - 'engines/order_management/app/services/order_management/order/updater.rb' - 'lib/open_food_network/enterprise_fee_calculator.rb' - 'lib/open_food_network/order_cycle_form_applicator.rb' From 0aecb6873a0cce2582a348cc78b3b2a0689bd2da Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Fri, 28 Jun 2024 16:37:59 +0200 Subject: [PATCH 3/4] Requested changes - 2 new methods for reading either current/desired on hand/on demand depending on variant state. Goal is to get rid of send method in View - referring in on_hand/on_demand is in fact irrelevant. In the piece of code, only desired on_hand/on_demand can be called as we are only in new variant (non persisted) mode - View does not use send method anymore, replaced by current_or_desired - refactor of the spec -> 2 examples in one to get more speed. --- app/models/concerns/variant_stock.rb | 12 ++++++++++++ app/services/sets/product_set.rb | 11 +++++++++-- app/views/admin/products_v3/_variant_row.html.haml | 4 ++-- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/variant_stock.rb b/app/models/concerns/variant_stock.rb index 7489aa5893e..7e40bd2afaf 100644 --- a/app/models/concerns/variant_stock.rb +++ b/app/models/concerns/variant_stock.rb @@ -67,6 +67,18 @@ def on_demand=(new_value) end end + def on_demand_desired_or_current + return on_demand_desired if new_record? + + on_demand + end + + def on_hand_desired_or_current + return on_hand_desired if new_record? + + on_hand + end + # Moving Spree::Stock::Quantifier.can_supply? to the variant enables us # to override this behaviour for variant overrides # We can have this responsibility here in the variant because there is diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index 2a692a08a05..75034ebf764 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -131,7 +131,11 @@ def create_variant(product, variant_attributes) return variant if variant.errors.present? begin - create_stock_for_variant(variant, on_demand, on_hand) + if on_hand || on_demand + create_stock_for_variant(variant, on_demand, on_hand) + else + create_stock_for_variant_from_desired(variant) + end rescue StandardError => e notify_bugsnag(e, product, variant, variant_attributes) raise e @@ -156,8 +160,11 @@ def notify_bugsnag(error, product, variant, variant_attributes) def create_stock_for_variant(variant, on_demand, on_hand) variant.on_demand = on_demand if on_demand.present? - variant.on_demand = variant.on_demand_desired if variant.on_demand_desired.present? variant.on_hand = on_hand.to_i if on_hand.present? + end + + def create_stock_for_variant_from_desired(variant) + variant.on_demand = variant.on_demand_desired if variant.on_demand_desired.present? variant.on_hand = variant.on_hand_desired.to_i if variant.on_hand_desired.present? end end diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml index 8624121eca8..f32474e50e1 100644 --- a/app/views/admin/products_v3/_variant_row.html.haml +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -38,10 +38,10 @@ = error_message_on variant, :price %td.col-on_hand.field.popout{'data-controller': "popout"} %button.popout__button{'data-popout-target': "button", 'aria-label': t('admin.products_page.columns.on_hand')} - = variant.send(method_on_demand) ? t(:on_demand) : variant.send(method_on_hand) + = variant.on_demand_desired_or_current ? t(:on_demand) : variant.on_hand_desired_or_current %div.popout__container{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" } .field - = f.number_field method_on_hand, min: 0, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: f.object.send(method_on_demand) + = f.number_field method_on_hand, min: 0, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: f.object.on_demand_desired_or_current = error_message_on variant, method_on_hand .field.checkbox = f.label method_on_demand do From 2bc5ed49e60cb786dcd94f155ceab276e955e159 Mon Sep 17 00:00:00 2001 From: cyrillefr Date: Wed, 12 Feb 2025 15:58:17 +0100 Subject: [PATCH 4/4] Changes in spec Due to the splitting of products_v3_spec in 4 parts. --- spec/system/admin/products_v3/create_spec.rb | 4 ++-- spec/system/admin/products_v3/update_spec.rb | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/spec/system/admin/products_v3/create_spec.rb b/spec/system/admin/products_v3/create_spec.rb index d2af1c8899a..e36ba0c9fae 100644 --- a/spec/system/admin/products_v3/create_spec.rb +++ b/spec/system/admin/products_v3/create_spec.rb @@ -90,9 +90,9 @@ select taxon.name, from: 'Category' if stock == "on_hand" - find('input[id$="_on_hand"]').fill_in with: "66" + find('input[id$="_on_hand_desired"]').fill_in with: "66" elsif stock == "on_demand" - find('input[id$="_on_demand"]').check + find('input[id$="_on_demand_desired"]').check end end diff --git a/spec/system/admin/products_v3/update_spec.rb b/spec/system/admin/products_v3/update_spec.rb index d7fd2b0c025..d82fa774003 100644 --- a/spec/system/admin/products_v3/update_spec.rb +++ b/spec/system/admin/products_v3/update_spec.rb @@ -667,6 +667,9 @@ fill_in "Name", with: "Large box" click_on "On Hand" fill_in "On Hand", with: "19" + tomselect_select("Weight (kg)", from: "Unit scale") + click_on "Unit" + fill_in "Unit value", with: "1" end click_button "Save changes" @@ -682,6 +685,9 @@ fill_in "Name", with: "Large box" click_on "On Hand" check "On demand" + tomselect_select("Weight (kg)", from: "Unit scale") + click_on "Unit" + fill_in "Unit value", with: "1" end click_button "Save changes"