From ee30d3d1390b71b26ad7962e3c4c7d13fc9af9fd Mon Sep 17 00:00:00 2001 From: James Wu Date: Sun, 22 Jan 2023 23:58:38 +0900 Subject: [PATCH 01/32] Fix PaymentMethodFactory creating extra Calculator --- lib/tasks/sample_data/payment_method_factory.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/tasks/sample_data/payment_method_factory.rb b/lib/tasks/sample_data/payment_method_factory.rb index 4febf09fa4..ba545b2d11 100644 --- a/lib/tasks/sample_data/payment_method_factory.rb +++ b/lib/tasks/sample_data/payment_method_factory.rb @@ -53,8 +53,8 @@ module SampleData environment: Rails.env, distributor_ids: [enterprise.id] ) - calculator.calculable = payment_method - calculator.save! + payment_method.calculator = calculator + payment_method.save! end end end From b9b2fa7c947c69068b0afd58db13074dee759a7b Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 15:41:46 +0900 Subject: [PATCH 02/32] Fix error converting String to BigDecimal --- app/models/spree/preference.rb | 3 ++- app/models/spree/preferences/preferable.rb | 2 +- spec/models/spree/preference_spec.rb | 9 +++++++++ spec/models/spree/preferences/preferable_spec.rb | 7 +++++++ 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/models/spree/preference.rb b/app/models/spree/preference.rb index dfef2a146d..ea2c63688c 100644 --- a/app/models/spree/preference.rb +++ b/app/models/spree/preference.rb @@ -22,7 +22,8 @@ module Spree when :password self[:value].to_s when :decimal - BigDecimal(self[:value].to_s).round(2, BigDecimal::ROUND_HALF_UP) + BigDecimal(self[:value].to_s, exception: false)&.round(2, BigDecimal::ROUND_HALF_UP) || + self[:value] when :integer self[:value].to_i when :boolean diff --git a/app/models/spree/preferences/preferable.rb b/app/models/spree/preferences/preferable.rb index 2c0f61b2e2..af775ab37d 100644 --- a/app/models/spree/preferences/preferable.rb +++ b/app/models/spree/preferences/preferable.rb @@ -117,7 +117,7 @@ module Spree value.to_s when :decimal value = 0 if value.blank? - BigDecimal(value.to_s).round(2, BigDecimal::ROUND_HALF_UP) + BigDecimal(value.to_s, exception: false)&.round(2, BigDecimal::ROUND_HALF_UP) || value when :integer value.to_i when :boolean diff --git a/spec/models/spree/preference_spec.rb b/spec/models/spree/preference_spec.rb index 780434f768..2eacaddfe7 100644 --- a/spec/models/spree/preference_spec.rb +++ b/spec/models/spree/preference_spec.rb @@ -58,6 +58,15 @@ describe Spree::Preference do expect(pref.value_type).to eq value_type.to_s end + it "incovertible :decimal" do + value_type = :decimal + value = "invalid" + key = "decimal_key" + pref = round_trip_preference(key, value, value_type) + expect(pref.value).to eq value + expect(pref.value_type).to eq value_type.to_s + end + it ":string" do value_type = :string value = "This is a string" diff --git a/spec/models/spree/preferences/preferable_spec.rb b/spec/models/spree/preferences/preferable_spec.rb index 8b7edba9f0..e95e069de0 100644 --- a/spec/models/spree/preferences/preferable_spec.rb +++ b/spec/models/spree/preferences/preferable_spec.rb @@ -165,6 +165,13 @@ describe Spree::Preferences::Preferable do @a.set_preference(:if_decimal, '') expect(@a.preferences[:if_decimal]).to eq 0.0 end + + context "when the value cannot be converted to BigDecimal" do + it "returns the original value" do + @a.set_preference(:if_decimal, "invalid") + expect(@a.preferences[:if_decimal]).to eq "invalid" + end + end end context "converts boolean preferences to boolean values" do From 7b51f45b4d6b59712d849d00b90ec37f94f8546d Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 15:45:40 +0900 Subject: [PATCH 03/32] Fix spelling in test description --- spec/models/spree/preference_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/spree/preference_spec.rb b/spec/models/spree/preference_spec.rb index 2eacaddfe7..0eb32f9837 100644 --- a/spec/models/spree/preference_spec.rb +++ b/spec/models/spree/preference_spec.rb @@ -11,7 +11,7 @@ describe Spree::Preference do expect(@preference).to be_valid end - describe "type coversion for values" do + describe "type conversion for values" do def round_trip_preference(key, value, value_type) p = Spree::Preference.new p.value = value From 3dfaf882a3f5508c45e5c0c5163606a2c22cd23c Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 15:58:52 +0900 Subject: [PATCH 04/32] Add translation for Active Record error message --- config/locales/en.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/locales/en.yml b/config/locales/en.yml index 60e29711e7..4bd159557a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -80,6 +80,8 @@ en: variant_override: count_on_hand: "On Hand" errors: + messages: + calculator_preferred_value_error: "Invalid input. Please use only numbers. For example: 10, 5.5, -20" models: spree/user: attributes: From 096e388fdf659bd703b11dcabe9c0ccc33c131e3 Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 16:13:33 +0900 Subject: [PATCH 05/32] Add numericality validation for FlatPercentItemTotal --- app/models/calculator/flat_percent_item_total.rb | 3 +++ spec/models/calculator/flat_percent_item_total_spec.rb | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/app/models/calculator/flat_percent_item_total.rb b/app/models/calculator/flat_percent_item_total.rb index d4c2067444..a457749674 100644 --- a/app/models/calculator/flat_percent_item_total.rb +++ b/app/models/calculator/flat_percent_item_total.rb @@ -10,6 +10,9 @@ module Calculator localize_number :preferred_flat_percent + validates :preferred_flat_percent, + numericality: { message: :calculator_preferred_value_error } + def self.description Spree.t(:flat_percent) end diff --git a/spec/models/calculator/flat_percent_item_total_spec.rb b/spec/models/calculator/flat_percent_item_total_spec.rb index 7d34211259..8095fbc9f2 100644 --- a/spec/models/calculator/flat_percent_item_total_spec.rb +++ b/spec/models/calculator/flat_percent_item_total_spec.rb @@ -8,6 +8,11 @@ describe Calculator::FlatPercentItemTotal do before { allow(calculator).to receive_messages preferred_flat_percent: 10 } + it do + should validate_numericality_of(:preferred_flat_percent). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + it "computes amount correctly for a single line item" do expect(calculator.compute(line_item)).to eq(1.0) end From 23beea8a1373d44c00e967b494658b12b696dc95 Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 16:15:14 +0900 Subject: [PATCH 06/32] Add numericality validation for FlatPercentPerItem --- app/models/calculator/flat_percent_per_item.rb | 3 +++ spec/models/calculator/flat_percent_per_item_spec.rb | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/app/models/calculator/flat_percent_per_item.rb b/app/models/calculator/flat_percent_per_item.rb index f9e197c594..dfcc71ed99 100644 --- a/app/models/calculator/flat_percent_per_item.rb +++ b/app/models/calculator/flat_percent_per_item.rb @@ -14,6 +14,9 @@ class Calculator::FlatPercentPerItem < Spree::Calculator localize_number :preferred_flat_percent + validates :preferred_flat_percent, + numericality: { message: :calculator_preferred_value_error } + def self.description I18n.t(:flat_percent_per_item) end diff --git a/spec/models/calculator/flat_percent_per_item_spec.rb b/spec/models/calculator/flat_percent_per_item_spec.rb index 6f16a9fe6a..24c5a68f4b 100644 --- a/spec/models/calculator/flat_percent_per_item_spec.rb +++ b/spec/models/calculator/flat_percent_per_item_spec.rb @@ -5,6 +5,11 @@ require 'spec_helper' describe Calculator::FlatPercentPerItem do let(:calculator) { Calculator::FlatPercentPerItem.new preferred_flat_percent: 20 } + it do + should validate_numericality_of(:preferred_flat_percent). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + it "calculates for a simple line item" do line_item = Spree::LineItem.new price: 50, quantity: 2 expect(calculator.compute(line_item)).to eq 20 From 073f46e59014573e5154312f95b38bff61f00bf9 Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 16:16:47 +0900 Subject: [PATCH 07/32] Add numericality validation for FlatRate --- app/models/calculator/flat_rate.rb | 3 +++ spec/models/calculator/flat_rate_spec.rb | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/app/models/calculator/flat_rate.rb b/app/models/calculator/flat_rate.rb index a8ab771b98..2fc5931611 100644 --- a/app/models/calculator/flat_rate.rb +++ b/app/models/calculator/flat_rate.rb @@ -10,6 +10,9 @@ module Calculator localize_number :preferred_amount + validates :preferred_amount, + numericality: { message: :calculator_preferred_value_error } + def self.description I18n.t(:flat_rate_per_order) end diff --git a/spec/models/calculator/flat_rate_spec.rb b/spec/models/calculator/flat_rate_spec.rb index cb0cce62d2..7f09635a39 100644 --- a/spec/models/calculator/flat_rate_spec.rb +++ b/spec/models/calculator/flat_rate_spec.rb @@ -7,6 +7,11 @@ describe Calculator::FlatRate do before { allow(calculator).to receive_messages preferred_amount: 10 } + it do + should validate_numericality_of(:preferred_amount). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + context "extends LocalizedNumber" do it_behaves_like "a model using the LocalizedNumber module", [:preferred_amount] end From 2cb25e636618d5266c7c0e0e46da000ebf96a0ef Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 16:19:17 +0900 Subject: [PATCH 08/32] Add numericality validation for FlexiRate --- app/models/calculator/flexi_rate.rb | 4 ++++ spec/models/calculator/flexi_rate_spec.rb | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/app/models/calculator/flexi_rate.rb b/app/models/calculator/flexi_rate.rb index bb6bb1ff9b..e8bb81458c 100644 --- a/app/models/calculator/flexi_rate.rb +++ b/app/models/calculator/flexi_rate.rb @@ -13,6 +13,10 @@ module Calculator localize_number :preferred_first_item, :preferred_additional_item + validates :preferred_first_item, + :preferred_additional_item, + numericality: { message: :calculator_preferred_value_error } + def self.description I18n.t(:flexible_rate) end diff --git a/spec/models/calculator/flexi_rate_spec.rb b/spec/models/calculator/flexi_rate_spec.rb index 81bbc95ee6..481df1a93e 100644 --- a/spec/models/calculator/flexi_rate_spec.rb +++ b/spec/models/calculator/flexi_rate_spec.rb @@ -12,6 +12,16 @@ describe Calculator::FlexiRate do ) end + it do + should validate_numericality_of(:preferred_first_item). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + + it do + should validate_numericality_of(:preferred_additional_item). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + context 'when nb of items ordered is above preferred max' do let(:quantity) { 4.0 } From c68987c0f2ce1cc8c302975879b3f6eb73004d35 Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 16:23:04 +0900 Subject: [PATCH 09/32] Add numericality validation for PerItem --- app/models/calculator/per_item.rb | 3 +++ spec/models/calculator/per_item_spec.rb | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/app/models/calculator/per_item.rb b/app/models/calculator/per_item.rb index f8cce8e7a7..c7329add95 100644 --- a/app/models/calculator/per_item.rb +++ b/app/models/calculator/per_item.rb @@ -10,6 +10,9 @@ module Calculator localize_number :preferred_amount + validates :preferred_amount, + numericality: { message: :calculator_preferred_value_error } + def self.description I18n.t(:flat_rate_per_item) end diff --git a/spec/models/calculator/per_item_spec.rb b/spec/models/calculator/per_item_spec.rb index cde6cae8f5..56bc0c96a2 100644 --- a/spec/models/calculator/per_item_spec.rb +++ b/spec/models/calculator/per_item_spec.rb @@ -7,6 +7,11 @@ describe Calculator::PerItem do let(:shipping_calculable) { double(:calculable) } let(:line_item) { build_stubbed(:line_item, quantity: 5) } + it do + should validate_numericality_of(:preferred_amount). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + it "correctly calculates on a single line item object" do allow(calculator).to receive_messages(calculable: shipping_calculable) expect(calculator.compute(line_item).to_f).to eq(50) # 5 x 10 From 6b1426d0c6453a144cc307bc52cf46fce88988b4 Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 16:25:44 +0900 Subject: [PATCH 10/32] Add numericality validation for PriceSack --- app/models/calculator/price_sack.rb | 5 +++++ spec/models/calculator/price_sack_spec.rb | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/app/models/calculator/price_sack.rb b/app/models/calculator/price_sack.rb index 910b663b1d..d337e77b2f 100644 --- a/app/models/calculator/price_sack.rb +++ b/app/models/calculator/price_sack.rb @@ -14,6 +14,11 @@ module Calculator :preferred_normal_amount, :preferred_discount_amount + validates :preferred_minimal_amount, + :preferred_normal_amount, + :preferred_discount_amount, + numericality: { message: :calculator_preferred_value_error } + def self.description I18n.t(:price_sack) end diff --git a/spec/models/calculator/price_sack_spec.rb b/spec/models/calculator/price_sack_spec.rb index 0034b20963..c9b5272c33 100644 --- a/spec/models/calculator/price_sack_spec.rb +++ b/spec/models/calculator/price_sack_spec.rb @@ -12,6 +12,21 @@ describe Calculator::PriceSack do end let(:line_item) { build_stubbed(:line_item, price: price, quantity: 2) } + it do + should validate_numericality_of(:preferred_minimal_amount). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + + it do + should validate_numericality_of(:preferred_normal_amount). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + + it do + should validate_numericality_of(:preferred_discount_amount). + with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") + end + context 'when the order amount is below preferred minimal' do let(:price) { 2 } From 1f3e66316bcab34f6b61c699c514ee7a93d9000e Mon Sep 17 00:00:00 2001 From: James Wu Date: Tue, 24 Jan 2023 16:54:43 +0900 Subject: [PATCH 11/32] Modify Calculator validation error messages --- .../concerns/nested_calculator_validation.rb | 51 +++++++++++++++++++ app/models/spree/payment_method.rb | 1 + .../concerns/calculator_validation_spec.rb | 48 +++++++++++++++++ spec/models/spree/payment_method_spec.rb | 3 ++ 4 files changed, 103 insertions(+) create mode 100644 app/models/concerns/nested_calculator_validation.rb create mode 100644 spec/models/concerns/calculator_validation_spec.rb diff --git a/app/models/concerns/nested_calculator_validation.rb b/app/models/concerns/nested_calculator_validation.rb new file mode 100644 index 0000000000..e7473e7413 --- /dev/null +++ b/app/models/concerns/nested_calculator_validation.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module NestedCalculatorValidation + extend ActiveSupport::Concern + + included do + validates_associated :calculator + after_validation :set_custom_error_messages, if: :calculator_errors? + end + + def calculator_errors? + calculator.errors.any? + end + + def set_custom_error_messages + add_custom_error_messages + delete_generic_error_message + delete_preferred_value_errors + end + + def add_custom_error_messages + calculator.errors.messages&.each do |attribute, msgs| + msgs.each do |msg| + errors.add(:base, "#{localize_calculator_attributes[attribute]}: #{msg}") + end + end + end + + def delete_generic_error_message + errors.delete(:calculator) if errors[:calculator] && errors[:calculator][0] == "is invalid" + end + + def delete_preferred_value_errors + calculator.preferences.each do |k, _v| + errors.delete("calculator.preferred_#{k}".to_sym ) + end + end + + def localize_calculator_attributes + { + preferred_amount: I18n.t('spree.amount'), + preferred_flat_percent: I18n.t('spree.flat_percent'), + preferred_first_item: I18n.t('spree.first_item'), + preferred_additional_item: I18n.t('spree.additional_item'), + preferred_max_items: I18n.t('spree.max_items'), + preferred_normal_amount: I18n.t('spree.normal_amount'), + preferred_discount_amount: I18n.t('spree.discount_amount'), + preferred_minimal_amount: I18n.t('spree.minimal_amount'), + } + end +end diff --git a/app/models/spree/payment_method.rb b/app/models/spree/payment_method.rb index f90a526f7c..ac46c864c8 100644 --- a/app/models/spree/payment_method.rb +++ b/app/models/spree/payment_method.rb @@ -6,6 +6,7 @@ module Spree class PaymentMethod < ApplicationRecord include CalculatedAdjustments include PaymentMethodDistributors + include NestedCalculatorValidation acts_as_taggable acts_as_paranoid diff --git a/spec/models/concerns/calculator_validation_spec.rb b/spec/models/concerns/calculator_validation_spec.rb new file mode 100644 index 0000000000..ada8e67ce3 --- /dev/null +++ b/spec/models/concerns/calculator_validation_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +shared_examples "a parent model that has a Calculator" do |parent_name| + context "when the associated Calculator is valid" do + let(:parent) do + build(parent_name, calculator: Calculator::FlatRate.new(preferred_amount: 10)) + end + + it "is valid" do + expect(parent).to be_valid + end + end + + context "when the associated Calculator is invalid" do + let(:parent) do + build(parent_name, calculator: Calculator::FlatRate.new(preferred_amount: "invalid")) + end + + before do + parent.valid? + end + + it "is invalid" do + expect(parent).not_to be_valid + end + + it "adds custom error messages to base" do + expect(parent.errors[:base]).to include(/^Amount: Invalid input/) + end + + it "has the correct number of errors messages" do + error_messages = parent.errors.full_messages + expect(error_messages.count).to eq 1 + end + + it "does not include the generic Calculator error message" do + error_messages = parent.errors.full_messages + expect(error_messages).not_to include(/^Calculator is invalid$/) + end + + it "does not include error message that begins with 'Calculator preferred'" do + error_messages = parent.errors.full_messages + expect(error_messages).not_to include(/^Calculator preferred/) + end + end +end diff --git a/spec/models/spree/payment_method_spec.rb b/spec/models/spree/payment_method_spec.rb index 32dba2f627..41d7d92641 100644 --- a/spec/models/spree/payment_method_spec.rb +++ b/spec/models/spree/payment_method_spec.rb @@ -1,11 +1,14 @@ # frozen_string_literal: true require 'spec_helper' +require './spec/models/concerns/calculator_validation_spec' class Spree::Gateway::Test < Spree::Gateway end describe Spree::PaymentMethod do + it_behaves_like "a parent model that has a Calculator", :payment_method + describe "#available" do let(:enterprise) { create(:enterprise) } From 48753df5f014d495ef8e634b7a7b17fb8a61e377 Mon Sep 17 00:00:00 2001 From: James Wu Date: Wed, 25 Jan 2023 21:35:55 +0900 Subject: [PATCH 12/32] Improve input validation and error notification --- .../spree/admin/payment_methods_controller.rb | 26 --------- spec/system/admin/payment_method_spec.rb | 53 +++++++++++++++++++ 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb index e467e8356c..6d43556718 100644 --- a/app/controllers/spree/admin/payment_methods_controller.rb +++ b/app/controllers/spree/admin/payment_methods_controller.rb @@ -7,7 +7,6 @@ module Spree before_action :load_data before_action :validate_payment_method_provider, only: [:create] before_action :load_hubs, only: [:new, :edit, :update] - before_action :validate_calculator_preferred_value, only: [:update] respond_to :html @@ -185,31 +184,6 @@ module Spree params_for_update end end - - def validate_calculator_preferred_value - return if calculator_preferred_values.all? do |value| - preferred_value_from_params = gateway_params.dig(:calculator_attributes, value) - preferred_value_from_params.nil? || Float(preferred_value_from_params, - exception: false) - end - - flash[:error] = I18n.t(:calculator_preferred_value_error) - redirect_to spree.edit_admin_payment_method_path(@payment_method) - end - - def calculator_preferred_values - [ - :preferred_amount, - :preferred_flat_percent, - :preferred_flat_percent, - :preferred_first_item, - :preferred_additional_item, - :preferred_max_items, - :preferred_normal_amount, - :preferred_discount_amount, - :preferred_minimal_amount - ] - end end end end diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index c6937a0327..6673273835 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -345,4 +345,57 @@ describe ' end end end + + context "when updating a payment method with invalid data" do + let(:payment_method) { create(:payment_method, :flat_rate, amount: 10) } + + before do + login_as_admin_and_visit spree.edit_admin_payment_method_path payment_method + fill_in "payment_method_name", with: "" + fill_in "payment_method_description", with: "Edited description" + uncheck "payment_method_distributor_ids_#{@distributors[0].id}" + fill_in "Amount", with: 'invalid' + click_button 'Update' + end + + it "displays the number of errors" do + expect(page).to have_content("3 errors") + end + + it "displays error messages" do + expect(page).to have_content("Name can't be blank") + expect(page).to have_content("At least one hub must be selected") + expect(page).to have_content("Amount: Invalid input. Please use only numbers.") + end + + it "highlights invalid fields" do + within '.calculator-settings .field .field_with_errors' do + expect(page).to have_field "Amount" + end + + within '.field_with_errors:has(#payment_method_name)' do + expect(page).to have_field "payment_method_name" + end + + within '#hubs' do + expect(page).to have_selector(".red") + end + end + + it "keeps information that was just edited, even after multiple unsuccessful updates" do + expect(page).to have_field "Amount", with: "invalid" + expect(page).to have_field "payment_method_name", with: '' + expect(page).to have_field "payment_method_description", with: "Edited description" + expect(page).to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" + + fill_in "payment_method_name", with: "Test name" + fill_in "Amount", with: 'invalid string' + click_button 'Update' + + expect(page).to have_field "payment_method_name", with: 'Test name' + expect(page).to have_field "Amount", with: "invalid string" + expect(page).to have_field "payment_method_description", with: "Edited description" + expect(page).to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" + end + end end From 41ce4fbc16fa99983b1319a93c9aa6a99f368ddf Mon Sep 17 00:00:00 2001 From: James Wu Date: Thu, 26 Jan 2023 15:01:27 +0900 Subject: [PATCH 13/32] Fix cache issue with invalid form data --- .../spree/admin/payment_methods_controller.rb | 7 +++++++ spec/system/admin/payment_method_spec.rb | 17 +++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb index 6d43556718..a6114266de 100644 --- a/app/controllers/spree/admin/payment_methods_controller.rb +++ b/app/controllers/spree/admin/payment_methods_controller.rb @@ -43,6 +43,7 @@ module Spree redirect_to spree.edit_admin_payment_method_path(@payment_method) else respond_with(@payment_method) + clear_preference_cache end end @@ -184,6 +185,12 @@ module Spree params_for_update end end + + def clear_preference_cache + @payment_method.calculator.preferences.each_key do |key| + Rails.cache.delete(@payment_method.calculator.preference_cache_key(key)) + end + end end end end diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index 6673273835..c4343619bb 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -349,7 +349,10 @@ describe ' context "when updating a payment method with invalid data" do let(:payment_method) { create(:payment_method, :flat_rate, amount: 10) } + # Persist preferences to test that when preferred values are not found in the cache, + # they are fetched from the database and displayed correctly before do + Spree::Preferences::Store.instance.persistence = true login_as_admin_and_visit spree.edit_admin_payment_method_path payment_method fill_in "payment_method_name", with: "" fill_in "payment_method_description", with: "Edited description" @@ -358,6 +361,10 @@ describe ' click_button 'Update' end + after do + Spree::Preferences::Store.instance.persistence = false + end + it "displays the number of errors" do expect(page).to have_content("3 errors") end @@ -397,5 +404,15 @@ describe ' expect(page).to have_field "payment_method_description", with: "Edited description" expect(page).to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" end + + it 'displays data fetched from the database after navigating away from the page' do + click_link 'Back To Payment Methods List' + click_link href: /#{spree.edit_admin_payment_method_path(payment_method)}/ + + expect(page).to have_field 'Amount', with: '10.0' + expect(page).not_to have_field 'payment_method_name', with: '' + expect(page).not_to have_field 'payment_method_description', with: 'Edited description' + expect(page).not_to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" + end end end From ae5f2cc19d950d10dd11c733879afdb792e00bb2 Mon Sep 17 00:00:00 2001 From: James Wu Date: Thu, 26 Jan 2023 16:07:26 +0900 Subject: [PATCH 14/32] Fix error saving data on new Calculator --- .../spree/admin/payment_methods_controller.rb | 9 +++ spec/system/admin/payment_method_spec.rb | 68 +++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb index a6114266de..9fc0207d77 100644 --- a/app/controllers/spree/admin/payment_methods_controller.rb +++ b/app/controllers/spree/admin/payment_methods_controller.rb @@ -182,6 +182,11 @@ module Spree end end + # Ensure the calculator to be updated is the correct type + if params_for_update["calculator_type"] && params_for_update["calculator_attributes"] + add_type_to_calculator_attributes(params_for_update) + end + params_for_update end end @@ -191,6 +196,10 @@ module Spree Rails.cache.delete(@payment_method.calculator.preference_cache_key(key)) end end + + def add_type_to_calculator_attributes(hash) + hash["calculator_attributes"]["type"] = hash["calculator_type"] + end end end end diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index c4343619bb..e079252694 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -415,4 +415,72 @@ describe ' expect(page).not_to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" end end + + context 'when updating payment method with invalid data and changing calculator type' do + let(:payment_method) { create(:payment_method, :flat_rate, amount: 10) } + + # Persist preferences to test that when preferred values are not found in the cache, + # they are fetched from the database and displayed correctly + before do + Spree::Preferences::Store.instance.persistence = true + login_as_admin_and_visit spree.edit_admin_payment_method_path payment_method + fill_in 'payment_method_name', with: '' + fill_in 'payment_method_description', with: 'Edited description' + uncheck "payment_method_distributor_ids_#{@distributors[0].id}" + select2_select 'Flat Percent', from: 'calc_type' + click_button 'Update' + end + + after do + Spree::Preferences::Store.instance.persistence = false + end + + it 'displays the number of errors' do + expect(page).to have_content('2 errors') + end + + it 'displays error messages' do + expect(page).to have_content("Name can't be blank") + expect(page).to have_content('At least one hub must be selected') + end + + it 'displays the new calculator fields' do + expect(page).to have_field 'Flat Percent' + end + + it 'highlights invalid fields' do + within '.field_with_errors:has(#payment_method_name)' do + expect(page).to have_field 'payment_method_name' + end + + within '#hubs' do + expect(page).to have_selector('.red') + end + end + + it 'keeps information that was just edited, even after multiple unsuccessful updates' do + expect(page).to have_field 'payment_method_name', with: '' + expect(page).to have_field 'payment_method_description', with: 'Edited description' + expect(page).to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" + + fill_in 'payment_method_name', with: 'Test name' + fill_in 'Flat Percent', with: 'invalid string' + click_button 'Update' + + expect(page).to have_field 'payment_method_name', with: 'Test name' + expect(page).to have_field 'Flat Percent', with: 'invalid string' + expect(page).to have_field 'payment_method_description', with: 'Edited description' + expect(page).to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" + end + + it 'displays data fetched from the database after navigating away from the page' do + click_link 'Back To Payment Methods List' + click_link href: /#{spree.edit_admin_payment_method_path(payment_method)}/ + + expect(page).to have_field 'Amount', with: '10.0' + expect(page).not_to have_field 'payment_method_name', with: '' + expect(page).not_to have_field 'payment_method_description', with: 'Edited description' + expect(page).not_to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" + end + end end From 00cc8da2fce84e36e9076416aac03ae0a698fe68 Mon Sep 17 00:00:00 2001 From: James Wu Date: Thu, 26 Jan 2023 21:51:55 +0900 Subject: [PATCH 15/32] Fix rubocop class length error --- app/models/spree/payment_method.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models/spree/payment_method.rb b/app/models/spree/payment_method.rb index ac46c864c8..964065b605 100644 --- a/app/models/spree/payment_method.rb +++ b/app/models/spree/payment_method.rb @@ -41,9 +41,8 @@ module Spree where(id: non_unique_matches.map(&:id)) } - scope :for_distributor, lambda { |distributor| - joins(:distributors). - where('enterprises.id = ?', distributor) + scope :for_distributor, ->(distributor) { + joins(:distributors).where('enterprises.id = ?', distributor) } scope :for_subscriptions, -> { where(type: Subscription::ALLOWED_PAYMENT_METHOD_TYPES) } From 824d4adf216f5ed7721213f6a380580b965d829b Mon Sep 17 00:00:00 2001 From: James Wu Date: Thu, 2 Mar 2023 18:04:53 +0900 Subject: [PATCH 16/32] Rename spec file --- ..._validation_spec.rb => nested_calculator_validation_spec.rb} | 0 spec/models/spree/payment_method_spec.rb | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename spec/models/concerns/{calculator_validation_spec.rb => nested_calculator_validation_spec.rb} (100%) diff --git a/spec/models/concerns/calculator_validation_spec.rb b/spec/models/concerns/nested_calculator_validation_spec.rb similarity index 100% rename from spec/models/concerns/calculator_validation_spec.rb rename to spec/models/concerns/nested_calculator_validation_spec.rb diff --git a/spec/models/spree/payment_method_spec.rb b/spec/models/spree/payment_method_spec.rb index 41d7d92641..9dd4ee1f4d 100644 --- a/spec/models/spree/payment_method_spec.rb +++ b/spec/models/spree/payment_method_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -require './spec/models/concerns/calculator_validation_spec' +require './spec/models/concerns/nested_calculator_validation_spec' class Spree::Gateway::Test < Spree::Gateway end From 7af26e65795aa34898eec899275946c20a4ee358 Mon Sep 17 00:00:00 2001 From: James Wu Date: Fri, 3 Mar 2023 00:29:22 +0900 Subject: [PATCH 17/32] Add errors for invalid input with number localization enabled --- lib/spree/localized_number.rb | 2 +- spec/lib/spree/localized_number_spec.rb | 6 ++++++ .../nested_calculator_validation_spec.rb | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/spree/localized_number.rb b/lib/spree/localized_number.rb index 77270d49ee..773c81ce45 100644 --- a/lib/spree/localized_number.rb +++ b/lib/spree/localized_number.rb @@ -44,7 +44,7 @@ module Spree def self.valid_localizable_number?(number) return true unless number.is_a?(String) || number.respond_to?(:to_d) - return false if number.to_s =~ /[.,]\d{2}[.,]/ + return false if number.to_s =~ /[.,]\d{2}[.,]/ || number.to_s =~ /[^0-9,.]+/ true end diff --git a/spec/lib/spree/localized_number_spec.rb b/spec/lib/spree/localized_number_spec.rb index b13f8ddf6e..bafe0c822d 100644 --- a/spec/lib/spree/localized_number_spec.rb +++ b/spec/lib/spree/localized_number_spec.rb @@ -59,5 +59,11 @@ describe Spree::LocalizedNumber do expect(described_class.valid_localizable_number?(1599.99)).to eql true end end + + context "with letters" do + it "returns false" do + expect(described_class.valid_localizable_number?('invalid')).to eql false + end + end end end diff --git a/spec/models/concerns/nested_calculator_validation_spec.rb b/spec/models/concerns/nested_calculator_validation_spec.rb index ada8e67ce3..7900e26f6d 100644 --- a/spec/models/concerns/nested_calculator_validation_spec.rb +++ b/spec/models/concerns/nested_calculator_validation_spec.rb @@ -45,4 +45,21 @@ shared_examples "a parent model that has a Calculator" do |parent_name| expect(error_messages).not_to include(/^Calculator preferred/) end end + + context "when number localization is enabled and the associated Calculator is invalid" do + let(:localized_parent) do + build(parent_name, calculator: Calculator::FlatRate.new(preferred_amount: "invalid")) + end + + before do + allow(Spree::Config).to receive(:enable_localized_number?).and_return true + localized_parent.valid? + end + + it "adds custom error messages to base" do + expect(localized_parent.errors[:base]).to include( + /#{I18n.t('spree.localized_number.invalid_format')}/ + ) + end + end end From f5e2edb02881ec7568a94767494c2f59f4c6c81a Mon Sep 17 00:00:00 2001 From: James Wu Date: Fri, 3 Mar 2023 00:53:29 +0900 Subject: [PATCH 18/32] fixup! Modify Calculator validation error messages [wip] DC: the error message has now become "Amount Invalid input". --- .../concerns/nested_calculator_validation.rb | 2 +- .../nested_calculator_validation_spec.rb | 22 +++++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/app/models/concerns/nested_calculator_validation.rb b/app/models/concerns/nested_calculator_validation.rb index e7473e7413..b6cf2abc1b 100644 --- a/app/models/concerns/nested_calculator_validation.rb +++ b/app/models/concerns/nested_calculator_validation.rb @@ -21,7 +21,7 @@ module NestedCalculatorValidation def add_custom_error_messages calculator.errors.messages&.each do |attribute, msgs| msgs.each do |msg| - errors.add(:base, "#{localize_calculator_attributes[attribute]}: #{msg}") + errors.add(:base, "#{localize_calculator_attributes[attribute]} #{msg}") end end end diff --git a/spec/models/concerns/nested_calculator_validation_spec.rb b/spec/models/concerns/nested_calculator_validation_spec.rb index 7900e26f6d..d6e9551a5b 100644 --- a/spec/models/concerns/nested_calculator_validation_spec.rb +++ b/spec/models/concerns/nested_calculator_validation_spec.rb @@ -4,44 +4,44 @@ require 'spec_helper' shared_examples "a parent model that has a Calculator" do |parent_name| context "when the associated Calculator is valid" do - let(:parent) do + let(:valid_parent) do build(parent_name, calculator: Calculator::FlatRate.new(preferred_amount: 10)) end it "is valid" do - expect(parent).to be_valid + expect(valid_parent).to be_valid end end context "when the associated Calculator is invalid" do - let(:parent) do + let(:invalid_parent) do build(parent_name, calculator: Calculator::FlatRate.new(preferred_amount: "invalid")) end before do - parent.valid? + invalid_parent.valid? end it "is invalid" do - expect(parent).not_to be_valid + expect(invalid_parent).not_to be_valid end it "adds custom error messages to base" do - expect(parent.errors[:base]).to include(/^Amount: Invalid input/) + expect(invalid_parent.errors[:base]).to include(/^Amount: Invalid input/) end it "has the correct number of errors messages" do - error_messages = parent.errors.full_messages + error_messages = invalid_parent.errors.full_messages expect(error_messages.count).to eq 1 end it "does not include the generic Calculator error message" do - error_messages = parent.errors.full_messages + error_messages = invalid_parent.errors.full_messages expect(error_messages).not_to include(/^Calculator is invalid$/) end it "does not include error message that begins with 'Calculator preferred'" do - error_messages = parent.errors.full_messages + error_messages = invalid_parent.errors.full_messages expect(error_messages).not_to include(/^Calculator preferred/) end end @@ -57,9 +57,7 @@ shared_examples "a parent model that has a Calculator" do |parent_name| end it "adds custom error messages to base" do - expect(localized_parent.errors[:base]).to include( - /#{I18n.t('spree.localized_number.invalid_format')}/ - ) + expect(localized_parent.errors[:base]).to include(/Amount has an invalid format/) end end end From d40f3414eace10ec91f5e632ab6213250d8459fc Mon Sep 17 00:00:00 2001 From: James Wu Date: Fri, 3 Mar 2023 00:57:16 +0900 Subject: [PATCH 19/32] fixup! Add translation for Active Record error message --- 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 4bd159557a..1d00c22551 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -81,7 +81,7 @@ en: count_on_hand: "On Hand" errors: messages: - calculator_preferred_value_error: "Invalid input. Please use only numbers. For example: 10, 5.5, -20" + calculator_preferred_value_error: "has an invalid input. Please use only numbers. For example: 10, 5.5, -20" models: spree/user: attributes: From 29ddc68d2058b4bed21b53a141de872787825a5f Mon Sep 17 00:00:00 2001 From: James Wu Date: Thu, 16 Mar 2023 16:02:37 +0900 Subject: [PATCH 20/32] Fix duplicate validation error message --- .../calculator/flat_percent_item_total.rb | 3 ++- .../calculator/flat_percent_per_item.rb | 3 ++- app/models/calculator/flat_rate.rb | 3 ++- app/models/calculator/flexi_rate.rb | 3 ++- app/models/calculator/per_item.rb | 3 ++- app/models/calculator/price_sack.rb | 3 ++- lib/spree/localized_number.rb | 2 +- .../flat_percent_item_total_spec.rb | 2 ++ .../calculator/flat_percent_per_item_spec.rb | 2 ++ spec/models/calculator/flat_rate_spec.rb | 1 + spec/models/calculator/flexi_rate_spec.rb | 2 ++ spec/models/calculator/per_item_spec.rb | 1 + spec/models/calculator/price_sack_spec.rb | 3 +++ spec/support/localized_number_helper.rb | 21 +++++++++++++++++++ 14 files changed, 45 insertions(+), 7 deletions(-) diff --git a/app/models/calculator/flat_percent_item_total.rb b/app/models/calculator/flat_percent_item_total.rb index a457749674..f43cae017d 100644 --- a/app/models/calculator/flat_percent_item_total.rb +++ b/app/models/calculator/flat_percent_item_total.rb @@ -11,7 +11,8 @@ module Calculator localize_number :preferred_flat_percent validates :preferred_flat_percent, - numericality: { message: :calculator_preferred_value_error } + numericality: { message: :calculator_preferred_value_error }, + unless: -> { Spree::Config.enable_localized_number? } def self.description Spree.t(:flat_percent) diff --git a/app/models/calculator/flat_percent_per_item.rb b/app/models/calculator/flat_percent_per_item.rb index dfcc71ed99..c6a8693fd0 100644 --- a/app/models/calculator/flat_percent_per_item.rb +++ b/app/models/calculator/flat_percent_per_item.rb @@ -15,7 +15,8 @@ class Calculator::FlatPercentPerItem < Spree::Calculator localize_number :preferred_flat_percent validates :preferred_flat_percent, - numericality: { message: :calculator_preferred_value_error } + numericality: { message: :calculator_preferred_value_error }, + unless: -> { Spree::Config.enable_localized_number? } def self.description I18n.t(:flat_percent_per_item) diff --git a/app/models/calculator/flat_rate.rb b/app/models/calculator/flat_rate.rb index 2fc5931611..0cdb2b6fa4 100644 --- a/app/models/calculator/flat_rate.rb +++ b/app/models/calculator/flat_rate.rb @@ -11,7 +11,8 @@ module Calculator localize_number :preferred_amount validates :preferred_amount, - numericality: { message: :calculator_preferred_value_error } + numericality: { message: :calculator_preferred_value_error }, + unless: -> { Spree::Config.enable_localized_number? } def self.description I18n.t(:flat_rate_per_order) diff --git a/app/models/calculator/flexi_rate.rb b/app/models/calculator/flexi_rate.rb index e8bb81458c..8a400088b0 100644 --- a/app/models/calculator/flexi_rate.rb +++ b/app/models/calculator/flexi_rate.rb @@ -15,7 +15,8 @@ module Calculator validates :preferred_first_item, :preferred_additional_item, - numericality: { message: :calculator_preferred_value_error } + numericality: { message: :calculator_preferred_value_error }, + unless: -> { Spree::Config.enable_localized_number? } def self.description I18n.t(:flexible_rate) diff --git a/app/models/calculator/per_item.rb b/app/models/calculator/per_item.rb index c7329add95..8458f4d432 100644 --- a/app/models/calculator/per_item.rb +++ b/app/models/calculator/per_item.rb @@ -11,7 +11,8 @@ module Calculator localize_number :preferred_amount validates :preferred_amount, - numericality: { message: :calculator_preferred_value_error } + numericality: { message: :calculator_preferred_value_error }, + unless: -> { Spree::Config.enable_localized_number? } def self.description I18n.t(:flat_rate_per_item) diff --git a/app/models/calculator/price_sack.rb b/app/models/calculator/price_sack.rb index d337e77b2f..30b02be331 100644 --- a/app/models/calculator/price_sack.rb +++ b/app/models/calculator/price_sack.rb @@ -17,7 +17,8 @@ module Calculator validates :preferred_minimal_amount, :preferred_normal_amount, :preferred_discount_amount, - numericality: { message: :calculator_preferred_value_error } + numericality: { message: :calculator_preferred_value_error }, + unless: -> { Spree::Config.enable_localized_number? } def self.description I18n.t(:price_sack) diff --git a/lib/spree/localized_number.rb b/lib/spree/localized_number.rb index 773c81ce45..66d75e2b6b 100644 --- a/lib/spree/localized_number.rb +++ b/lib/spree/localized_number.rb @@ -21,7 +21,7 @@ module Spree elsif Spree::Config.enable_localized_number? @invalid_localized_number ||= [] @invalid_localized_number << attribute - number = nil + number = nil unless is_a?(Spree::Calculator) end if has_attribute?(attribute) # In this case it's a regular AR attribute with standard setters diff --git a/spec/models/calculator/flat_percent_item_total_spec.rb b/spec/models/calculator/flat_percent_item_total_spec.rb index 8095fbc9f2..afcd2ea23a 100644 --- a/spec/models/calculator/flat_percent_item_total_spec.rb +++ b/spec/models/calculator/flat_percent_item_total_spec.rb @@ -19,6 +19,8 @@ describe Calculator::FlatPercentItemTotal do context "extends LocalizedNumber" do it_behaves_like "a model using the LocalizedNumber module", [:preferred_flat_percent] + it_behaves_like "a Spree Calculator model using the LocalizedNumber module", + [:preferred_flat_percent] end it "computes amount correctly for a given OrderManagement::Stock::Package" do diff --git a/spec/models/calculator/flat_percent_per_item_spec.rb b/spec/models/calculator/flat_percent_per_item_spec.rb index 24c5a68f4b..8758cb7e95 100644 --- a/spec/models/calculator/flat_percent_per_item_spec.rb +++ b/spec/models/calculator/flat_percent_per_item_spec.rb @@ -22,5 +22,7 @@ describe Calculator::FlatPercentPerItem do context "extends LocalizedNumber" do it_behaves_like "a model using the LocalizedNumber module", [:preferred_flat_percent] + it_behaves_like "a Spree Calculator model using the LocalizedNumber module", + [:preferred_flat_percent] end end diff --git a/spec/models/calculator/flat_rate_spec.rb b/spec/models/calculator/flat_rate_spec.rb index 7f09635a39..de8b999d9e 100644 --- a/spec/models/calculator/flat_rate_spec.rb +++ b/spec/models/calculator/flat_rate_spec.rb @@ -14,5 +14,6 @@ describe Calculator::FlatRate do context "extends LocalizedNumber" do it_behaves_like "a model using the LocalizedNumber module", [:preferred_amount] + it_behaves_like "a Spree Calculator model using the LocalizedNumber module", [:preferred_amount] end end diff --git a/spec/models/calculator/flexi_rate_spec.rb b/spec/models/calculator/flexi_rate_spec.rb index 481df1a93e..f5a45f3b72 100644 --- a/spec/models/calculator/flexi_rate_spec.rb +++ b/spec/models/calculator/flexi_rate_spec.rb @@ -46,5 +46,7 @@ describe Calculator::FlexiRate do context "extends LocalizedNumber" do it_behaves_like "a model using the LocalizedNumber module", [:preferred_first_item, :preferred_additional_item] + it_behaves_like "a Spree Calculator model using the LocalizedNumber module", + [:preferred_first_item, :preferred_additional_item] end end diff --git a/spec/models/calculator/per_item_spec.rb b/spec/models/calculator/per_item_spec.rb index 56bc0c96a2..c372c91f82 100644 --- a/spec/models/calculator/per_item_spec.rb +++ b/spec/models/calculator/per_item_spec.rb @@ -19,5 +19,6 @@ describe Calculator::PerItem do context "extends LocalizedNumber" do it_behaves_like "a model using the LocalizedNumber module", [:preferred_amount] + it_behaves_like "a Spree Calculator model using the LocalizedNumber module", [:preferred_amount] end end diff --git a/spec/models/calculator/price_sack_spec.rb b/spec/models/calculator/price_sack_spec.rb index c9b5272c33..966ee25248 100644 --- a/spec/models/calculator/price_sack_spec.rb +++ b/spec/models/calculator/price_sack_spec.rb @@ -95,5 +95,8 @@ describe Calculator::PriceSack do it_behaves_like "a model using the LocalizedNumber module", [:preferred_minimal_amount, :preferred_normal_amount, :preferred_discount_amount] + it_behaves_like "a Spree Calculator model using the LocalizedNumber module", + [:preferred_minimal_amount, :preferred_normal_amount, + :preferred_discount_amount] end end diff --git a/spec/support/localized_number_helper.rb b/spec/support/localized_number_helper.rb index 62ed435de0..75e8803915 100644 --- a/spec/support/localized_number_helper.rb +++ b/spec/support/localized_number_helper.rb @@ -21,3 +21,24 @@ shared_examples "a model using the LocalizedNumber module" do |attributes| end end end + +shared_examples "a Spree Calculator model using the LocalizedNumber module" do |attributes| + before do + allow(Spree::Config).to receive(:enable_localized_number?).and_return true + end + + attributes.each do |attribute| + setter = "#{attribute}=" + + it do + should_not validate_numericality_of(attribute). + with_message(I18n.t('activerecord.errors.messages.calculator_preferred_value_error')) + end + + it "does not modify the attribute value when it is invalid" do + subject.send(setter, 'invalid') + subject.valid? + expect(subject.send(attribute)).to eq 'invalid' + end + end +end From 2a7cb50fa981723e17812f4af8aa337a8060c940 Mon Sep 17 00:00:00 2001 From: James Wu Date: Sat, 18 Mar 2023 23:02:23 +0900 Subject: [PATCH 21/32] Keep and display submitted data on new payment method page --- .../spree/admin/payment_methods_controller.rb | 4 ++ .../payment_methods/_providers.html.haml | 2 +- spec/system/admin/payment_method_spec.rb | 38 +++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/payment_methods_controller.rb b/app/controllers/spree/admin/payment_methods_controller.rb index 9fc0207d77..077cddbca8 100644 --- a/app/controllers/spree/admin/payment_methods_controller.rb +++ b/app/controllers/spree/admin/payment_methods_controller.rb @@ -16,6 +16,10 @@ module Spree @payment_method = payment_method_class.constantize.new(base_params) @object = @payment_method + if !base_params["calculator_type"] && gateway_params["calculator_type"] + @payment_method.calculator_type = gateway_params["calculator_type"] + end + load_hubs if @payment_method.save diff --git a/app/views/spree/admin/payment_methods/_providers.html.haml b/app/views/spree/admin/payment_methods/_providers.html.haml index aa1fce7fe8..b25a7b09c5 100644 --- a/app/views/spree/admin/payment_methods/_providers.html.haml +++ b/app/views/spree/admin/payment_methods/_providers.html.haml @@ -3,6 +3,6 @@ .alpha.four.columns = label :payment_method, :type, t('.provider') .omega.twelve.columns - = collection_select(:payment_method, :type, @providers, :to_s, :clean_name, (!@object.persisted? ? { :selected => "Spree::PaymentMethod::Check"} : {}), { class: 'select2 fullwidth', 'provider-prefs-for' => "#{@object.id}"}) + = collection_select(:payment_method, :type, @providers, :to_s, :clean_name, {}, { class: 'select2 fullwidth', 'provider-prefs-for' => "#{@object.id}"}) %div{"ng-include" => "include_html" } diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index e079252694..dd35e75471 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -112,6 +112,44 @@ describe ' expect(page).to have_field "payment_method_distributor_ids_#{@distributors[2].id}", checked: false end + + it "retains and displays data that was just submitted" do + login_as_admin_and_visit spree.edit_admin_general_settings_path + click_link 'Payment Methods' + click_link 'New Payment Method' + + fill_in "payment_method_name", with: 'Cheque payment method' + fill_in "payment_method_description", with: "Payment description" + select2_select "PayPal Express", from: "payment_method_type" + select2_select "Flat Percent", from: 'calc_type' + + click_button 'Create' + + expect(page).to have_field "payment_method_name", with: "Cheque payment method" + expect(page).to have_field "payment_method_description", with: "Payment description" + expect(page).to have_select("payment_method_type", selected: "PayPal Express") + expect(page).to have_select("calc_type", selected: "Flat Percent") + + select2_select "Price Sack", from: 'calc_type' + + click_button 'Create' + + expect(page).to have_select("calc_type", selected: "Price Sack") + expect(page).to have_field "payment_method_name", with: "Cheque payment method" + expect(page).to have_field "payment_method_description", with: "Payment description" + expect(page).to have_select("payment_method_type", selected: "PayPal Express") + + check "payment_method_distributor_ids_#{@distributors[0].id}" + + click_button 'Create' + + expect(page).to have_field "payment_method_distributor_ids_#{@distributors[0].id}", + checked: true + expect(page).to have_select("calc_type", selected: "Price Sack") + expect(page).to have_field "payment_method_name", with: "Cheque payment method" + expect(page).to have_field "payment_method_description", with: "Payment description" + expect(page).to have_select("payment_method_type", selected: "PayPal Express") + end end it "updating a payment method" do From 364ccc2146a3ea9ff01431e1f49279cf74c52e58 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 20 Mar 2023 13:54:47 +1100 Subject: [PATCH 22/32] Add comment --- lib/spree/localized_number.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/spree/localized_number.rb b/lib/spree/localized_number.rb index 66d75e2b6b..a1ba18accd 100644 --- a/lib/spree/localized_number.rb +++ b/lib/spree/localized_number.rb @@ -44,6 +44,7 @@ module Spree def self.valid_localizable_number?(number) return true unless number.is_a?(String) || number.respond_to?(:to_d) + # Invalid if only two digits between dividers, or if any non-number characters return false if number.to_s =~ /[.,]\d{2}[.,]/ || number.to_s =~ /[^0-9,.]+/ true From ce1bd127602070755588cb137aa4eef5e15648c3 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 4 Apr 2023 16:25:10 +1000 Subject: [PATCH 23/32] Consolidate specs It takes time to set up this test case. Given that there's no interactions in the expectations, we should save time to generate it only once. --- spec/system/admin/payment_method_spec.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index dd35e75471..98f852edb6 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -403,17 +403,14 @@ describe ' Spree::Preferences::Store.instance.persistence = false end - it "displays the number of errors" do + it "displays error details" do expect(page).to have_content("3 errors") - end - it "displays error messages" do expect(page).to have_content("Name can't be blank") expect(page).to have_content("At least one hub must be selected") expect(page).to have_content("Amount: Invalid input. Please use only numbers.") - end - it "highlights invalid fields" do + # Highlighting invalid fields within '.calculator-settings .field .field_with_errors' do expect(page).to have_field "Amount" end From f6fac018ea4585b98161f99ba19d62d2a93711e4 Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 31 Mar 2023 16:59:24 +1100 Subject: [PATCH 24/32] [wip] Use a custom message format for associated errors This is much simpler. Multiple messages get combined onto one line though, which is not perfect. And.. it still seems to duplicate the errors. Weird, it's fine on my frontend though. --- .../concerns/nested_calculator_validation.rb | 45 ++----------------- app/models/spree/payment_method.rb | 2 +- config/locales/en.yml | 11 +++++ .../nested_calculator_validation_spec.rb | 6 ++- 4 files changed, 19 insertions(+), 45 deletions(-) diff --git a/app/models/concerns/nested_calculator_validation.rb b/app/models/concerns/nested_calculator_validation.rb index b6cf2abc1b..cddd3c506a 100644 --- a/app/models/concerns/nested_calculator_validation.rb +++ b/app/models/concerns/nested_calculator_validation.rb @@ -4,48 +4,9 @@ module NestedCalculatorValidation extend ActiveSupport::Concern included do - validates_associated :calculator - after_validation :set_custom_error_messages, if: :calculator_errors? - end - - def calculator_errors? - calculator.errors.any? - end - - def set_custom_error_messages - add_custom_error_messages - delete_generic_error_message - delete_preferred_value_errors - end - - def add_custom_error_messages - calculator.errors.messages&.each do |attribute, msgs| - msgs.each do |msg| - errors.add(:base, "#{localize_calculator_attributes[attribute]} #{msg}") - end - end - end - - def delete_generic_error_message - errors.delete(:calculator) if errors[:calculator] && errors[:calculator][0] == "is invalid" - end - - def delete_preferred_value_errors - calculator.preferences.each do |k, _v| - errors.delete("calculator.preferred_#{k}".to_sym ) - end - end - - def localize_calculator_attributes - { - preferred_amount: I18n.t('spree.amount'), - preferred_flat_percent: I18n.t('spree.flat_percent'), - preferred_first_item: I18n.t('spree.first_item'), - preferred_additional_item: I18n.t('spree.additional_item'), - preferred_max_items: I18n.t('spree.max_items'), - preferred_normal_amount: I18n.t('spree.normal_amount'), - preferred_discount_amount: I18n.t('spree.discount_amount'), - preferred_minimal_amount: I18n.t('spree.minimal_amount'), + validates_associated :calculator, message: ->(class_obj, obj) { + # Include all error messages from object + obj[:value].errors.full_messages.join("; ") } end end diff --git a/app/models/spree/payment_method.rb b/app/models/spree/payment_method.rb index 964065b605..90d227b840 100644 --- a/app/models/spree/payment_method.rb +++ b/app/models/spree/payment_method.rb @@ -6,7 +6,6 @@ module Spree class PaymentMethod < ApplicationRecord include CalculatedAdjustments include PaymentMethodDistributors - include NestedCalculatorValidation acts_as_taggable acts_as_paranoid @@ -18,6 +17,7 @@ module Spree validates :name, presence: true validate :distributor_validation + include NestedCalculatorValidation after_initialize :init diff --git a/config/locales/en.yml b/config/locales/en.yml index 1d00c22551..d08c02fce5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -79,6 +79,17 @@ en: orders_close_at: Close date variant_override: count_on_hand: "On Hand" + spree/calculator: + preferred_flat_percent: "Flat Percent" + preferred_amount: "Amount" + preferred_first_item: "First Item" + preferred_additional_item: "Additional Item Cost" + preferred_max_items: "Max Items" + preferred_minimal_amount: "Minimal Amount" + preferred_normal_amount: "Normal Amount" + preferred_discount_amount: "Discount Amount" + preferred_unit_from_list: "Unit From List" + preferred_per_unit: "Per Unit" errors: messages: calculator_preferred_value_error: "has an invalid input. Please use only numbers. For example: 10, 5.5, -20" diff --git a/spec/models/concerns/nested_calculator_validation_spec.rb b/spec/models/concerns/nested_calculator_validation_spec.rb index d6e9551a5b..843d965da0 100644 --- a/spec/models/concerns/nested_calculator_validation_spec.rb +++ b/spec/models/concerns/nested_calculator_validation_spec.rb @@ -27,7 +27,8 @@ shared_examples "a parent model that has a Calculator" do |parent_name| end it "adds custom error messages to base" do - expect(invalid_parent.errors[:base]).to include(/^Amount: Invalid input/) + error_messages = invalid_parent.errors.full_messages + expect(error_messages).to include(/^Calculator Amount has an invalid input./) end it "has the correct number of errors messages" do @@ -57,7 +58,8 @@ shared_examples "a parent model that has a Calculator" do |parent_name| end it "adds custom error messages to base" do - expect(localized_parent.errors[:base]).to include(/Amount has an invalid format/) + error_messages = localized_parent.errors.full_messages + expect(error_messages).to include(/^Calculator Amount has an invalid format./) end end end From e43a018dc79f3d3f3bb9465d712390516ce372ae Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 4 Apr 2023 14:41:16 +1000 Subject: [PATCH 25/32] [wip] Copy each calculator error But we still have the duplicate problem. Wait a minute, it copies them in the same format that I am copying them. So.. I don't need to copy them at all! Now I see that we just needed the right format in the translation file. --- .../concerns/nested_calculator_validation.rb | 17 ++++++++++--- config/locales/en.yml | 22 ++++++++-------- .../nested_calculator_validation_spec.rb | 25 +++++++++++++++++-- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/app/models/concerns/nested_calculator_validation.rb b/app/models/concerns/nested_calculator_validation.rb index cddd3c506a..e85c9b265b 100644 --- a/app/models/concerns/nested_calculator_validation.rb +++ b/app/models/concerns/nested_calculator_validation.rb @@ -4,9 +4,18 @@ module NestedCalculatorValidation extend ActiveSupport::Concern included do - validates_associated :calculator, message: ->(class_obj, obj) { - # Include all error messages from object - obj[:value].errors.full_messages.join("; ") - } + validate :associated_calculator + end + + def associated_calculator + # Calculator errors have already been added, don't know why. + errors.each do |error| + errors.delete(error.attribute) if error.attribute.match? /^calculator./ + end + # Copy errors from associated calculator to the base object, prepending "calculator." to the attribute name. + # wait a minute, that's what the messages were before! we just needed to get the translate keys right! + calculator.tap(&:valid?).errors.each do |error| + errors.import error, attribute: [:calculator, error.attribute].join('.') + end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index d08c02fce5..af0d4718fd 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -79,17 +79,17 @@ en: orders_close_at: Close date variant_override: count_on_hand: "On Hand" - spree/calculator: - preferred_flat_percent: "Flat Percent" - preferred_amount: "Amount" - preferred_first_item: "First Item" - preferred_additional_item: "Additional Item Cost" - preferred_max_items: "Max Items" - preferred_minimal_amount: "Minimal Amount" - preferred_normal_amount: "Normal Amount" - preferred_discount_amount: "Discount Amount" - preferred_unit_from_list: "Unit From List" - preferred_per_unit: "Per Unit" + spree/payment_method/calculator: + preferred_flat_percent: "Calculator Flat Percent" + preferred_amount: "Calculator Amount" + preferred_first_item: "Calculator First Item" + preferred_additional_item: "Calculator Additional Item Cost" + preferred_max_items: "Calculator Max Items" + preferred_minimal_amount: "Calculator Minimal Amount" + preferred_normal_amount: "Calculator Normal Amount" + preferred_discount_amount: "Calculator Discount Amount" + preferred_unit_from_list: "Calculator Unit From List" + preferred_per_unit: "Calculator Per Unit" errors: messages: calculator_preferred_value_error: "has an invalid input. Please use only numbers. For example: 10, 5.5, -20" diff --git a/spec/models/concerns/nested_calculator_validation_spec.rb b/spec/models/concerns/nested_calculator_validation_spec.rb index 843d965da0..0cc48673f9 100644 --- a/spec/models/concerns/nested_calculator_validation_spec.rb +++ b/spec/models/concerns/nested_calculator_validation_spec.rb @@ -15,7 +15,7 @@ shared_examples "a parent model that has a Calculator" do |parent_name| context "when the associated Calculator is invalid" do let(:invalid_parent) do - build(parent_name, calculator: Calculator::FlatRate.new(preferred_amount: "invalid")) + build(parent_name, calculator: Calculator::FlexiRate.new(preferred_first_item: "invalid")) end before do @@ -28,7 +28,7 @@ shared_examples "a parent model that has a Calculator" do |parent_name| it "adds custom error messages to base" do error_messages = invalid_parent.errors.full_messages - expect(error_messages).to include(/^Calculator Amount has an invalid input./) + expect(error_messages).to include(/^Calculator First Item has an invalid input./) end it "has the correct number of errors messages" do @@ -45,6 +45,27 @@ shared_examples "a parent model that has a Calculator" do |parent_name| error_messages = invalid_parent.errors.full_messages expect(error_messages).not_to include(/^Calculator preferred/) end + + context "with multiple errors" do + let(:invalid_parent) do + build(parent_name, + calculator: Calculator::FlexiRate.new( + preferred_first_item: "invalid", + preferred_additional_item: "invalid", + )) + end + + it "adds custom error messages to base" do + error_messages = invalid_parent.errors.full_messages + expect(error_messages[0]).to match(/^Calculator Additional Item Cost has an invalid input./) + expect(error_messages[1]).to match(/^Calculator First Item has an invalid input./) + end + + it "has the correct number of errors messages" do + error_messages = invalid_parent.errors.full_messages + expect(error_messages.count).to eq 2 + end + end end context "when number localization is enabled and the associated Calculator is invalid" do From 353acfd6ae9ba80e107debdaf43e42d53716177c Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 4 Apr 2023 15:34:39 +1000 Subject: [PATCH 26/32] Validate associated calculator with built in error handling Except that pesky generic message was appearing. I still don't know how to control that so just deleted it. --- .../concerns/nested_calculator_validation.rb | 16 +++++--------- config/locales/en.yml | 22 +++++++++---------- .../nested_calculator_validation_spec.rb | 8 +++---- spec/system/admin/payment_method_spec.rb | 2 +- 4 files changed, 21 insertions(+), 27 deletions(-) diff --git a/app/models/concerns/nested_calculator_validation.rb b/app/models/concerns/nested_calculator_validation.rb index e85c9b265b..418996b82e 100644 --- a/app/models/concerns/nested_calculator_validation.rb +++ b/app/models/concerns/nested_calculator_validation.rb @@ -4,18 +4,12 @@ module NestedCalculatorValidation extend ActiveSupport::Concern included do - validate :associated_calculator + validates_associated :calculator + after_validation :remove_calculator_error end - def associated_calculator - # Calculator errors have already been added, don't know why. - errors.each do |error| - errors.delete(error.attribute) if error.attribute.match? /^calculator./ - end - # Copy errors from associated calculator to the base object, prepending "calculator." to the attribute name. - # wait a minute, that's what the messages were before! we just needed to get the translate keys right! - calculator.tap(&:valid?).errors.each do |error| - errors.import error, attribute: [:calculator, error.attribute].join('.') - end + def remove_calculator_error + # Remove generic calculator message, in favour of messages provided by validates_associated + errors.delete(:calculator) if errors.key?(:calculator) end end diff --git a/config/locales/en.yml b/config/locales/en.yml index af0d4718fd..4497370d2e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -80,19 +80,19 @@ en: variant_override: count_on_hand: "On Hand" spree/payment_method/calculator: - preferred_flat_percent: "Calculator Flat Percent" - preferred_amount: "Calculator Amount" - preferred_first_item: "Calculator First Item" - preferred_additional_item: "Calculator Additional Item Cost" - preferred_max_items: "Calculator Max Items" - preferred_minimal_amount: "Calculator Minimal Amount" - preferred_normal_amount: "Calculator Normal Amount" - preferred_discount_amount: "Calculator Discount Amount" - preferred_unit_from_list: "Calculator Unit From List" - preferred_per_unit: "Calculator Per Unit" + preferred_flat_percent: "Calculator Flat Percent:" + preferred_amount: "Calculator Amount:" + preferred_first_item: "Calculator First Item:" + preferred_additional_item: "Calculator Additional Item Cost:" + preferred_max_items: "Calculator Max Items:" + preferred_minimal_amount: "Calculator Minimal Amount:" + preferred_normal_amount: "Calculator Normal Amount:" + preferred_discount_amount: "Calculator Discount Amount:" + preferred_unit_from_list: "Calculator Unit From List:" + preferred_per_unit: "Calculator Per Unit:" errors: messages: - calculator_preferred_value_error: "has an invalid input. Please use only numbers. For example: 10, 5.5, -20" + calculator_preferred_value_error: "Invalid input. Please use only numbers. For example: 10, 5.5, -20" models: spree/user: attributes: diff --git a/spec/models/concerns/nested_calculator_validation_spec.rb b/spec/models/concerns/nested_calculator_validation_spec.rb index 0cc48673f9..0488abf0d5 100644 --- a/spec/models/concerns/nested_calculator_validation_spec.rb +++ b/spec/models/concerns/nested_calculator_validation_spec.rb @@ -28,7 +28,7 @@ shared_examples "a parent model that has a Calculator" do |parent_name| it "adds custom error messages to base" do error_messages = invalid_parent.errors.full_messages - expect(error_messages).to include(/^Calculator First Item has an invalid input./) + expect(error_messages).to include(/^Calculator First Item: Invalid input./) end it "has the correct number of errors messages" do @@ -57,8 +57,8 @@ shared_examples "a parent model that has a Calculator" do |parent_name| it "adds custom error messages to base" do error_messages = invalid_parent.errors.full_messages - expect(error_messages[0]).to match(/^Calculator Additional Item Cost has an invalid input./) - expect(error_messages[1]).to match(/^Calculator First Item has an invalid input./) + expect(error_messages[0]).to match(/^Calculator First Item: Invalid input./) + expect(error_messages[1]).to match(/^Calculator Additional Item Cost: Invalid input./) end it "has the correct number of errors messages" do @@ -80,7 +80,7 @@ shared_examples "a parent model that has a Calculator" do |parent_name| it "adds custom error messages to base" do error_messages = localized_parent.errors.full_messages - expect(error_messages).to include(/^Calculator Amount has an invalid format./) + expect(error_messages).to include(/^Calculator Amount: has an invalid format./) end end end diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index 98f852edb6..8dc1bac5e0 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -408,7 +408,7 @@ describe ' expect(page).to have_content("Name can't be blank") expect(page).to have_content("At least one hub must be selected") - expect(page).to have_content("Amount: Invalid input. Please use only numbers.") + expect(page).to have_content("Calculator Amount: Invalid input. Please use only numbers.") # Highlighting invalid fields within '.calculator-settings .field .field_with_errors' do From f0110d20a21075db5ead0cfb42ce915c82ab29d9 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 5 Apr 2023 15:35:14 +1000 Subject: [PATCH 27/32] Move config to base helper This config is relevant for all specs, including system specs. --- spec/base_spec_helper.rb | 3 +++ spec/spec_helper.rb | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/base_spec_helper.rb b/spec/base_spec_helper.rb index cc4dc35e65..19206bd0b5 100644 --- a/spec/base_spec_helper.rb +++ b/spec/base_spec_helper.rb @@ -142,6 +142,9 @@ RSpec.configure do |config| config.infer_spec_type_from_file_location! + # You can use `rspec -n` to run only failed specs. + config.example_status_persistence_file_path = "tmp/rspec-status.txt" + # Helpers config.include FactoryBot::Syntax::Methods config.include JsonSpec::Helpers diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index da264f58e3..5422778d4a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -12,7 +12,4 @@ RSpec.configure do |config| # Fix encoding issue in Rails 5.0; allows passing empty arrays or hashes as params. config.before(:each, type: :controller) { @request.env["CONTENT_TYPE"] = 'application/json' } - - # You can use `rspec -n` to run only failed specs. - config.example_status_persistence_file_path = "tmp/rspec-status.txt" end From 0f3e2ae5722f18883d2b3f12868fa678d233f952 Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 5 Apr 2023 15:16:12 +1000 Subject: [PATCH 28/32] Use browser validation for calculator values They're all numbers, so we can reliably validate in the browser, removing the need for additional validation logic. They will still be validated server-side, and in the unlikely even that there is an error, the generic 'Calculator is invalid' message will appear, with the relevant fields highlighted red. I think that's fine. --- app/helpers/spree/admin/base_helper.rb | 21 +++-- .../concerns/nested_calculator_validation.rb | 15 ---- app/models/spree/payment_method.rb | 2 +- .../nested_calculator_validation_spec.rb | 86 ------------------- spec/models/spree/payment_method_spec.rb | 3 - spec/system/admin/payment_method_spec.rb | 11 +-- 6 files changed, 18 insertions(+), 120 deletions(-) delete mode 100644 app/models/concerns/nested_calculator_validation.rb delete mode 100644 spec/models/concerns/nested_calculator_validation_spec.rb diff --git a/app/helpers/spree/admin/base_helper.rb b/app/helpers/spree/admin/base_helper.rb index 3983cf3fda..07334812f8 100644 --- a/app/helpers/spree/admin/base_helper.rb +++ b/app/helpers/spree/admin/base_helper.rb @@ -29,8 +29,8 @@ module Spree def preference_field_tag(name, value, options) case options[:type] - when :integer - text_field_tag(name, value, preference_field_options(options)) + when :integer, :decimal + number_field_tag(name, value, preference_field_options(options)) when :boolean hidden_field_tag(name, 0) + check_box_tag(name, 1, value, preference_field_options(options)) @@ -49,8 +49,8 @@ module Spree def preference_field_for(form, field, options, object) case options[:type] - when :integer - form.text_field(field, preference_field_options(options)) + when :integer, :decimal + form.number_field(field, preference_field_options(options)) when :boolean form.check_box(field, preference_field_options(options)) when :string @@ -82,8 +82,17 @@ module Spree def preference_field_options(options) field_options = case options[:type] when :integer - { size: 10, - class: 'input_integer' } + { + size: 10, + class: 'input_integer', + step: 1, + } + when :decimal + { + size: 10, + class: 'input_integer', + step: :any, # Allow any number of decimal places + } when :boolean {} when :string diff --git a/app/models/concerns/nested_calculator_validation.rb b/app/models/concerns/nested_calculator_validation.rb deleted file mode 100644 index 418996b82e..0000000000 --- a/app/models/concerns/nested_calculator_validation.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module NestedCalculatorValidation - extend ActiveSupport::Concern - - included do - validates_associated :calculator - after_validation :remove_calculator_error - end - - def remove_calculator_error - # Remove generic calculator message, in favour of messages provided by validates_associated - errors.delete(:calculator) if errors.key?(:calculator) - end -end diff --git a/app/models/spree/payment_method.rb b/app/models/spree/payment_method.rb index 90d227b840..bbb9d4053d 100644 --- a/app/models/spree/payment_method.rb +++ b/app/models/spree/payment_method.rb @@ -17,7 +17,7 @@ module Spree validates :name, presence: true validate :distributor_validation - include NestedCalculatorValidation + validates_associated :calculator after_initialize :init diff --git a/spec/models/concerns/nested_calculator_validation_spec.rb b/spec/models/concerns/nested_calculator_validation_spec.rb deleted file mode 100644 index 0488abf0d5..0000000000 --- a/spec/models/concerns/nested_calculator_validation_spec.rb +++ /dev/null @@ -1,86 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -shared_examples "a parent model that has a Calculator" do |parent_name| - context "when the associated Calculator is valid" do - let(:valid_parent) do - build(parent_name, calculator: Calculator::FlatRate.new(preferred_amount: 10)) - end - - it "is valid" do - expect(valid_parent).to be_valid - end - end - - context "when the associated Calculator is invalid" do - let(:invalid_parent) do - build(parent_name, calculator: Calculator::FlexiRate.new(preferred_first_item: "invalid")) - end - - before do - invalid_parent.valid? - end - - it "is invalid" do - expect(invalid_parent).not_to be_valid - end - - it "adds custom error messages to base" do - error_messages = invalid_parent.errors.full_messages - expect(error_messages).to include(/^Calculator First Item: Invalid input./) - end - - it "has the correct number of errors messages" do - error_messages = invalid_parent.errors.full_messages - expect(error_messages.count).to eq 1 - end - - it "does not include the generic Calculator error message" do - error_messages = invalid_parent.errors.full_messages - expect(error_messages).not_to include(/^Calculator is invalid$/) - end - - it "does not include error message that begins with 'Calculator preferred'" do - error_messages = invalid_parent.errors.full_messages - expect(error_messages).not_to include(/^Calculator preferred/) - end - - context "with multiple errors" do - let(:invalid_parent) do - build(parent_name, - calculator: Calculator::FlexiRate.new( - preferred_first_item: "invalid", - preferred_additional_item: "invalid", - )) - end - - it "adds custom error messages to base" do - error_messages = invalid_parent.errors.full_messages - expect(error_messages[0]).to match(/^Calculator First Item: Invalid input./) - expect(error_messages[1]).to match(/^Calculator Additional Item Cost: Invalid input./) - end - - it "has the correct number of errors messages" do - error_messages = invalid_parent.errors.full_messages - expect(error_messages.count).to eq 2 - end - end - end - - context "when number localization is enabled and the associated Calculator is invalid" do - let(:localized_parent) do - build(parent_name, calculator: Calculator::FlatRate.new(preferred_amount: "invalid")) - end - - before do - allow(Spree::Config).to receive(:enable_localized_number?).and_return true - localized_parent.valid? - end - - it "adds custom error messages to base" do - error_messages = localized_parent.errors.full_messages - expect(error_messages).to include(/^Calculator Amount: has an invalid format./) - end - end -end diff --git a/spec/models/spree/payment_method_spec.rb b/spec/models/spree/payment_method_spec.rb index 9dd4ee1f4d..32dba2f627 100644 --- a/spec/models/spree/payment_method_spec.rb +++ b/spec/models/spree/payment_method_spec.rb @@ -1,14 +1,11 @@ # frozen_string_literal: true require 'spec_helper' -require './spec/models/concerns/nested_calculator_validation_spec' class Spree::Gateway::Test < Spree::Gateway end describe Spree::PaymentMethod do - it_behaves_like "a parent model that has a Calculator", :payment_method - describe "#available" do let(:enterprise) { create(:enterprise) } diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index 8dc1bac5e0..b8044e3012 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -395,6 +395,7 @@ describe ' fill_in "payment_method_name", with: "" fill_in "payment_method_description", with: "Edited description" uncheck "payment_method_distributor_ids_#{@distributors[0].id}" + # Although text is not permitted by input[type=number], let's see what happens fill_in "Amount", with: 'invalid' click_button 'Update' end @@ -404,17 +405,12 @@ describe ' end it "displays error details" do - expect(page).to have_content("3 errors") + expect(page).to have_content("2 errors") expect(page).to have_content("Name can't be blank") expect(page).to have_content("At least one hub must be selected") - expect(page).to have_content("Calculator Amount: Invalid input. Please use only numbers.") # Highlighting invalid fields - within '.calculator-settings .field .field_with_errors' do - expect(page).to have_field "Amount" - end - within '.field_with_errors:has(#payment_method_name)' do expect(page).to have_field "payment_method_name" end @@ -425,7 +421,6 @@ describe ' end it "keeps information that was just edited, even after multiple unsuccessful updates" do - expect(page).to have_field "Amount", with: "invalid" expect(page).to have_field "payment_method_name", with: '' expect(page).to have_field "payment_method_description", with: "Edited description" expect(page).to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" @@ -435,7 +430,6 @@ describe ' click_button 'Update' expect(page).to have_field "payment_method_name", with: 'Test name' - expect(page).to have_field "Amount", with: "invalid string" expect(page).to have_field "payment_method_description", with: "Edited description" expect(page).to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" end @@ -503,7 +497,6 @@ describe ' click_button 'Update' expect(page).to have_field 'payment_method_name', with: 'Test name' - expect(page).to have_field 'Flat Percent', with: 'invalid string' expect(page).to have_field 'payment_method_description', with: 'Edited description' expect(page).to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" end From 65708913496fde1b7e6265a28b4236cd1dd0f0fc Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 5 Apr 2023 17:00:10 +1000 Subject: [PATCH 29/32] Fix linter issues Although I don't think the size was an issue here, small hashes can easily fit on one line. --- app/helpers/spree/admin/base_helper.rb | 47 +++++++++--------------- spec/system/admin/payment_method_spec.rb | 4 +- 2 files changed, 20 insertions(+), 31 deletions(-) diff --git a/app/helpers/spree/admin/base_helper.rb b/app/helpers/spree/admin/base_helper.rb index 07334812f8..d527b04d1d 100644 --- a/app/helpers/spree/admin/base_helper.rb +++ b/app/helpers/spree/admin/base_helper.rb @@ -80,35 +80,24 @@ module Spree end def preference_field_options(options) - field_options = case options[:type] - when :integer - { - size: 10, - class: 'input_integer', - step: 1, - } - when :decimal - { - size: 10, - class: 'input_integer', - step: :any, # Allow any number of decimal places - } - when :boolean - {} - when :string - { size: 10, - class: 'input_string fullwidth' } - when :password - { size: 10, - class: 'password_string fullwidth' } - when :text - { rows: 15, - cols: 85, - class: 'fullwidth' } - else - { size: 10, - class: 'input_string fullwidth' } - end + field_options = + case options[:type] + when :integer + { size: 10, class: 'input_integer', step: 1 } + when :decimal + # Allow any number of decimal places + { size: 10, class: 'input_integer', step: :any } + when :boolean + {} + when :string + { size: 10, class: 'input_string fullwidth' } + when :password + { size: 10, class: 'password_string fullwidth' } + when :text + { rows: 15, cols: 85, class: 'fullwidth' } + else + { size: 10, class: 'input_string fullwidth' } + end field_options.merge!( readonly: options[:readonly], diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index b8044e3012..412c28ee8a 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -441,7 +441,7 @@ describe ' expect(page).to have_field 'Amount', with: '10.0' expect(page).not_to have_field 'payment_method_name', with: '' expect(page).not_to have_field 'payment_method_description', with: 'Edited description' - expect(page).not_to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" + expect(page).to have_checked_field "payment_method_distributor_ids_#{@distributors[0].id}" end end @@ -508,7 +508,7 @@ describe ' expect(page).to have_field 'Amount', with: '10.0' expect(page).not_to have_field 'payment_method_name', with: '' expect(page).not_to have_field 'payment_method_description', with: 'Edited description' - expect(page).not_to have_unchecked_field "payment_method_distributor_ids_#{@distributors[0].id}" + expect(page).to have_checked_field "payment_method_distributor_ids_#{@distributors[0].id}" end end end From 43e51cb5923883adb1bd91dbdd69224060f3ec2a Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 18 Apr 2023 10:01:19 +1000 Subject: [PATCH 30/32] Simplify login spec helpers, avoid long lines Follow on from 5c6d9a092e0812ec3d746b254096010268ec431e --- spec/system/admin/payment_method_spec.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/system/admin/payment_method_spec.rb b/spec/system/admin/payment_method_spec.rb index 412c28ee8a..e6de2bca7e 100644 --- a/spec/system/admin/payment_method_spec.rb +++ b/spec/system/admin/payment_method_spec.rb @@ -114,7 +114,8 @@ describe ' end it "retains and displays data that was just submitted" do - login_as_admin_and_visit spree.edit_admin_general_settings_path + login_as_admin + visit spree.edit_admin_general_settings_path click_link 'Payment Methods' click_link 'New Payment Method' @@ -391,7 +392,8 @@ describe ' # they are fetched from the database and displayed correctly before do Spree::Preferences::Store.instance.persistence = true - login_as_admin_and_visit spree.edit_admin_payment_method_path payment_method + login_as_admin + visit spree.edit_admin_payment_method_path payment_method fill_in "payment_method_name", with: "" fill_in "payment_method_description", with: "Edited description" uncheck "payment_method_distributor_ids_#{@distributors[0].id}" @@ -452,7 +454,8 @@ describe ' # they are fetched from the database and displayed correctly before do Spree::Preferences::Store.instance.persistence = true - login_as_admin_and_visit spree.edit_admin_payment_method_path payment_method + login_as_admin + visit spree.edit_admin_payment_method_path payment_method fill_in 'payment_method_name', with: '' fill_in 'payment_method_description', with: 'Edited description' uncheck "payment_method_distributor_ids_#{@distributors[0].id}" From a46eef291c0594bab9c10fb8fdfd72beaec3ce25 Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 28 Apr 2023 11:59:47 +1000 Subject: [PATCH 31/32] Remove localized number logic from calculators The browser is now responsible for dealing with the decimal separator, for all numeric preferences (as input[type=number]; see Spree::Admin::BaseHelper::preference_field_tag). Calculators are the only place that numeric preferences are used. It will enforce that only one comma or dot (depending on user's locale) is entered, thus avoiding any ambiguity, and mis-interpretation (eg 100,001 could be interpreted as more than 100 thousand or 100 with a decimal place). --- app/models/calculator/flat_percent_item_total.rb | 9 +-------- app/models/calculator/flat_percent_per_item.rb | 9 +-------- app/models/calculator/flat_rate.rb | 9 +-------- app/models/calculator/flexi_rate.rb | 10 +--------- app/models/calculator/per_item.rb | 9 +-------- app/models/calculator/price_sack.rb | 11 +---------- .../models/calculator/flat_percent_item_total_spec.rb | 6 ------ spec/models/calculator/flat_percent_per_item_spec.rb | 6 ------ spec/models/calculator/flat_rate_spec.rb | 5 ----- spec/models/calculator/flexi_rate_spec.rb | 7 ------- spec/models/calculator/per_item_spec.rb | 5 ----- spec/models/calculator/price_sack_spec.rb | 9 --------- 12 files changed, 6 insertions(+), 89 deletions(-) diff --git a/app/models/calculator/flat_percent_item_total.rb b/app/models/calculator/flat_percent_item_total.rb index f43cae017d..83a5674833 100644 --- a/app/models/calculator/flat_percent_item_total.rb +++ b/app/models/calculator/flat_percent_item_total.rb @@ -1,18 +1,11 @@ # frozen_string_literal: false -require 'spree/localized_number' - module Calculator class FlatPercentItemTotal < Spree::Calculator - extend Spree::LocalizedNumber - preference :flat_percent, :decimal, default: 0 - localize_number :preferred_flat_percent - validates :preferred_flat_percent, - numericality: { message: :calculator_preferred_value_error }, - unless: -> { Spree::Config.enable_localized_number? } + numericality: { message: :calculator_preferred_value_error } def self.description Spree.t(:flat_percent) diff --git a/app/models/calculator/flat_percent_per_item.rb b/app/models/calculator/flat_percent_per_item.rb index c6a8693fd0..b1b6b2303b 100644 --- a/app/models/calculator/flat_percent_per_item.rb +++ b/app/models/calculator/flat_percent_per_item.rb @@ -1,22 +1,15 @@ # frozen_string_literal: true -require 'spree/localized_number' - class Calculator::FlatPercentPerItem < Spree::Calculator # Spree's FlatPercentItemTotal calculator sums all amounts, and then calculates a percentage # on them. # In the cart, we display line item individual amounts rounded, so to have consistent # calculations we do the same internally. Here, we round adjustments at the individual # item level first, then multiply by the item quantity. - extend Spree::LocalizedNumber - preference :flat_percent, :decimal, default: 0 - localize_number :preferred_flat_percent - validates :preferred_flat_percent, - numericality: { message: :calculator_preferred_value_error }, - unless: -> { Spree::Config.enable_localized_number? } + numericality: { message: :calculator_preferred_value_error } def self.description I18n.t(:flat_percent_per_item) diff --git a/app/models/calculator/flat_rate.rb b/app/models/calculator/flat_rate.rb index 0cdb2b6fa4..afcdc80b8d 100644 --- a/app/models/calculator/flat_rate.rb +++ b/app/models/calculator/flat_rate.rb @@ -1,18 +1,11 @@ # frozen_string_literal: false -require 'spree/localized_number' - module Calculator class FlatRate < Spree::Calculator - extend Spree::LocalizedNumber - preference :amount, :decimal, default: 0 - localize_number :preferred_amount - validates :preferred_amount, - numericality: { message: :calculator_preferred_value_error }, - unless: -> { Spree::Config.enable_localized_number? } + numericality: { message: :calculator_preferred_value_error } def self.description I18n.t(:flat_rate_per_order) diff --git a/app/models/calculator/flexi_rate.rb b/app/models/calculator/flexi_rate.rb index 8a400088b0..40556ef581 100644 --- a/app/models/calculator/flexi_rate.rb +++ b/app/models/calculator/flexi_rate.rb @@ -1,22 +1,14 @@ # frozen_string_literal: false -require 'spree/localized_number' - module Calculator class FlexiRate < Spree::Calculator - extend Spree::LocalizedNumber - preference :first_item, :decimal, default: 0.0 preference :additional_item, :decimal, default: 0.0 preference :max_items, :integer, default: 0 - localize_number :preferred_first_item, - :preferred_additional_item - validates :preferred_first_item, :preferred_additional_item, - numericality: { message: :calculator_preferred_value_error }, - unless: -> { Spree::Config.enable_localized_number? } + numericality: { message: :calculator_preferred_value_error } def self.description I18n.t(:flexible_rate) diff --git a/app/models/calculator/per_item.rb b/app/models/calculator/per_item.rb index 8458f4d432..c0525fd384 100644 --- a/app/models/calculator/per_item.rb +++ b/app/models/calculator/per_item.rb @@ -1,18 +1,11 @@ # frozen_string_literal: false -require 'spree/localized_number' - module Calculator class PerItem < Spree::Calculator - extend Spree::LocalizedNumber - preference :amount, :decimal, default: 0 - localize_number :preferred_amount - validates :preferred_amount, - numericality: { message: :calculator_preferred_value_error }, - unless: -> { Spree::Config.enable_localized_number? } + numericality: { message: :calculator_preferred_value_error } def self.description I18n.t(:flat_rate_per_item) diff --git a/app/models/calculator/price_sack.rb b/app/models/calculator/price_sack.rb index 30b02be331..86ada80dde 100644 --- a/app/models/calculator/price_sack.rb +++ b/app/models/calculator/price_sack.rb @@ -1,24 +1,15 @@ # frozen_string_literal: false -require 'spree/localized_number' - module Calculator class PriceSack < Spree::Calculator - extend Spree::LocalizedNumber - preference :minimal_amount, :decimal, default: 0 preference :normal_amount, :decimal, default: 0 preference :discount_amount, :decimal, default: 0 - localize_number :preferred_minimal_amount, - :preferred_normal_amount, - :preferred_discount_amount - validates :preferred_minimal_amount, :preferred_normal_amount, :preferred_discount_amount, - numericality: { message: :calculator_preferred_value_error }, - unless: -> { Spree::Config.enable_localized_number? } + numericality: { message: :calculator_preferred_value_error } def self.description I18n.t(:price_sack) diff --git a/spec/models/calculator/flat_percent_item_total_spec.rb b/spec/models/calculator/flat_percent_item_total_spec.rb index afcd2ea23a..c9ad3865c9 100644 --- a/spec/models/calculator/flat_percent_item_total_spec.rb +++ b/spec/models/calculator/flat_percent_item_total_spec.rb @@ -17,12 +17,6 @@ describe Calculator::FlatPercentItemTotal do expect(calculator.compute(line_item)).to eq(1.0) end - context "extends LocalizedNumber" do - it_behaves_like "a model using the LocalizedNumber module", [:preferred_flat_percent] - it_behaves_like "a Spree Calculator model using the LocalizedNumber module", - [:preferred_flat_percent] - end - it "computes amount correctly for a given OrderManagement::Stock::Package" do order = double(:order, line_items: [line_item] ) package = double(:package, order: order) diff --git a/spec/models/calculator/flat_percent_per_item_spec.rb b/spec/models/calculator/flat_percent_per_item_spec.rb index 8758cb7e95..b95e08d0ab 100644 --- a/spec/models/calculator/flat_percent_per_item_spec.rb +++ b/spec/models/calculator/flat_percent_per_item_spec.rb @@ -19,10 +19,4 @@ describe Calculator::FlatPercentPerItem do line_item = Spree::LineItem.new price: 0.86, quantity: 8 expect(calculator.compute(line_item)).to eq 1.36 end - - context "extends LocalizedNumber" do - it_behaves_like "a model using the LocalizedNumber module", [:preferred_flat_percent] - it_behaves_like "a Spree Calculator model using the LocalizedNumber module", - [:preferred_flat_percent] - end end diff --git a/spec/models/calculator/flat_rate_spec.rb b/spec/models/calculator/flat_rate_spec.rb index de8b999d9e..20ca13ec69 100644 --- a/spec/models/calculator/flat_rate_spec.rb +++ b/spec/models/calculator/flat_rate_spec.rb @@ -11,9 +11,4 @@ describe Calculator::FlatRate do should validate_numericality_of(:preferred_amount). with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") end - - context "extends LocalizedNumber" do - it_behaves_like "a model using the LocalizedNumber module", [:preferred_amount] - it_behaves_like "a Spree Calculator model using the LocalizedNumber module", [:preferred_amount] - end end diff --git a/spec/models/calculator/flexi_rate_spec.rb b/spec/models/calculator/flexi_rate_spec.rb index f5a45f3b72..5224f5a408 100644 --- a/spec/models/calculator/flexi_rate_spec.rb +++ b/spec/models/calculator/flexi_rate_spec.rb @@ -42,11 +42,4 @@ describe Calculator::FlexiRate do Calculator::FlexiRate.new(preferred_first_item: 1, preferred_additional_item: 1, preferred_max_items: 1) end - - context "extends LocalizedNumber" do - it_behaves_like "a model using the LocalizedNumber module", - [:preferred_first_item, :preferred_additional_item] - it_behaves_like "a Spree Calculator model using the LocalizedNumber module", - [:preferred_first_item, :preferred_additional_item] - end end diff --git a/spec/models/calculator/per_item_spec.rb b/spec/models/calculator/per_item_spec.rb index c372c91f82..4b0f72ecd8 100644 --- a/spec/models/calculator/per_item_spec.rb +++ b/spec/models/calculator/per_item_spec.rb @@ -16,9 +16,4 @@ describe Calculator::PerItem do allow(calculator).to receive_messages(calculable: shipping_calculable) expect(calculator.compute(line_item).to_f).to eq(50) # 5 x 10 end - - context "extends LocalizedNumber" do - it_behaves_like "a model using the LocalizedNumber module", [:preferred_amount] - it_behaves_like "a Spree Calculator model using the LocalizedNumber module", [:preferred_amount] - end end diff --git a/spec/models/calculator/price_sack_spec.rb b/spec/models/calculator/price_sack_spec.rb index 966ee25248..0754848035 100644 --- a/spec/models/calculator/price_sack_spec.rb +++ b/spec/models/calculator/price_sack_spec.rb @@ -90,13 +90,4 @@ describe Calculator::PriceSack do end end end - - context "extends LocalizedNumber" do - it_behaves_like "a model using the LocalizedNumber module", - [:preferred_minimal_amount, :preferred_normal_amount, - :preferred_discount_amount] - it_behaves_like "a Spree Calculator model using the LocalizedNumber module", - [:preferred_minimal_amount, :preferred_normal_amount, - :preferred_discount_amount] - end end From 918425fd93f3d945d71d2e41975d2bceb16d9ed5 Mon Sep 17 00:00:00 2001 From: David Cook Date: Fri, 28 Apr 2023 11:49:32 +1000 Subject: [PATCH 32/32] Remove customised error message We're now handling these values on the frontend, so can keep this simple. fixes up: Add numericality validation for * Add translation for Active Record error message --- .../calculator/flat_percent_item_total.rb | 2 +- app/models/calculator/flat_percent_per_item.rb | 2 +- app/models/calculator/flat_rate.rb | 2 +- app/models/calculator/flexi_rate.rb | 2 +- app/models/calculator/per_item.rb | 2 +- app/models/calculator/price_sack.rb | 2 +- config/locales/en.yml | 2 -- .../calculator/flat_percent_item_total_spec.rb | 5 +---- .../calculator/flat_percent_per_item_spec.rb | 5 +---- spec/models/calculator/flat_rate_spec.rb | 5 +---- spec/models/calculator/flexi_rate_spec.rb | 11 ++--------- spec/models/calculator/per_item_spec.rb | 5 +---- spec/models/calculator/price_sack_spec.rb | 17 +++-------------- 13 files changed, 15 insertions(+), 47 deletions(-) diff --git a/app/models/calculator/flat_percent_item_total.rb b/app/models/calculator/flat_percent_item_total.rb index 83a5674833..f0d7a330e5 100644 --- a/app/models/calculator/flat_percent_item_total.rb +++ b/app/models/calculator/flat_percent_item_total.rb @@ -5,7 +5,7 @@ module Calculator preference :flat_percent, :decimal, default: 0 validates :preferred_flat_percent, - numericality: { message: :calculator_preferred_value_error } + numericality: true def self.description Spree.t(:flat_percent) diff --git a/app/models/calculator/flat_percent_per_item.rb b/app/models/calculator/flat_percent_per_item.rb index b1b6b2303b..e078340d7b 100644 --- a/app/models/calculator/flat_percent_per_item.rb +++ b/app/models/calculator/flat_percent_per_item.rb @@ -9,7 +9,7 @@ class Calculator::FlatPercentPerItem < Spree::Calculator preference :flat_percent, :decimal, default: 0 validates :preferred_flat_percent, - numericality: { message: :calculator_preferred_value_error } + numericality: true def self.description I18n.t(:flat_percent_per_item) diff --git a/app/models/calculator/flat_rate.rb b/app/models/calculator/flat_rate.rb index afcdc80b8d..a130b88c17 100644 --- a/app/models/calculator/flat_rate.rb +++ b/app/models/calculator/flat_rate.rb @@ -5,7 +5,7 @@ module Calculator preference :amount, :decimal, default: 0 validates :preferred_amount, - numericality: { message: :calculator_preferred_value_error } + numericality: true def self.description I18n.t(:flat_rate_per_order) diff --git a/app/models/calculator/flexi_rate.rb b/app/models/calculator/flexi_rate.rb index 40556ef581..1959aec67e 100644 --- a/app/models/calculator/flexi_rate.rb +++ b/app/models/calculator/flexi_rate.rb @@ -8,7 +8,7 @@ module Calculator validates :preferred_first_item, :preferred_additional_item, - numericality: { message: :calculator_preferred_value_error } + numericality: true def self.description I18n.t(:flexible_rate) diff --git a/app/models/calculator/per_item.rb b/app/models/calculator/per_item.rb index c0525fd384..0df18ae3d4 100644 --- a/app/models/calculator/per_item.rb +++ b/app/models/calculator/per_item.rb @@ -5,7 +5,7 @@ module Calculator preference :amount, :decimal, default: 0 validates :preferred_amount, - numericality: { message: :calculator_preferred_value_error } + numericality: true def self.description I18n.t(:flat_rate_per_item) diff --git a/app/models/calculator/price_sack.rb b/app/models/calculator/price_sack.rb index 86ada80dde..217928581f 100644 --- a/app/models/calculator/price_sack.rb +++ b/app/models/calculator/price_sack.rb @@ -9,7 +9,7 @@ module Calculator validates :preferred_minimal_amount, :preferred_normal_amount, :preferred_discount_amount, - numericality: { message: :calculator_preferred_value_error } + numericality: true def self.description I18n.t(:price_sack) diff --git a/config/locales/en.yml b/config/locales/en.yml index 4497370d2e..0aecf7f347 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -91,8 +91,6 @@ en: preferred_unit_from_list: "Calculator Unit From List:" preferred_per_unit: "Calculator Per Unit:" errors: - messages: - calculator_preferred_value_error: "Invalid input. Please use only numbers. For example: 10, 5.5, -20" models: spree/user: attributes: diff --git a/spec/models/calculator/flat_percent_item_total_spec.rb b/spec/models/calculator/flat_percent_item_total_spec.rb index c9ad3865c9..a0c57c6266 100644 --- a/spec/models/calculator/flat_percent_item_total_spec.rb +++ b/spec/models/calculator/flat_percent_item_total_spec.rb @@ -8,10 +8,7 @@ describe Calculator::FlatPercentItemTotal do before { allow(calculator).to receive_messages preferred_flat_percent: 10 } - it do - should validate_numericality_of(:preferred_flat_percent). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end + it { is_expected.to validate_numericality_of(:preferred_flat_percent) } it "computes amount correctly for a single line item" do expect(calculator.compute(line_item)).to eq(1.0) diff --git a/spec/models/calculator/flat_percent_per_item_spec.rb b/spec/models/calculator/flat_percent_per_item_spec.rb index b95e08d0ab..ac873a0ba7 100644 --- a/spec/models/calculator/flat_percent_per_item_spec.rb +++ b/spec/models/calculator/flat_percent_per_item_spec.rb @@ -5,10 +5,7 @@ require 'spec_helper' describe Calculator::FlatPercentPerItem do let(:calculator) { Calculator::FlatPercentPerItem.new preferred_flat_percent: 20 } - it do - should validate_numericality_of(:preferred_flat_percent). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end + it { is_expected.to validate_numericality_of(:preferred_flat_percent) } it "calculates for a simple line item" do line_item = Spree::LineItem.new price: 50, quantity: 2 diff --git a/spec/models/calculator/flat_rate_spec.rb b/spec/models/calculator/flat_rate_spec.rb index 20ca13ec69..92eaa864d0 100644 --- a/spec/models/calculator/flat_rate_spec.rb +++ b/spec/models/calculator/flat_rate_spec.rb @@ -7,8 +7,5 @@ describe Calculator::FlatRate do before { allow(calculator).to receive_messages preferred_amount: 10 } - it do - should validate_numericality_of(:preferred_amount). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end + it { is_expected.to validate_numericality_of(:preferred_amount) } end diff --git a/spec/models/calculator/flexi_rate_spec.rb b/spec/models/calculator/flexi_rate_spec.rb index 5224f5a408..e5a777a412 100644 --- a/spec/models/calculator/flexi_rate_spec.rb +++ b/spec/models/calculator/flexi_rate_spec.rb @@ -12,15 +12,8 @@ describe Calculator::FlexiRate do ) end - it do - should validate_numericality_of(:preferred_first_item). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end - - it do - should validate_numericality_of(:preferred_additional_item). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end + it { is_expected.to validate_numericality_of(:preferred_first_item) } + it { is_expected.to validate_numericality_of(:preferred_additional_item) } context 'when nb of items ordered is above preferred max' do let(:quantity) { 4.0 } diff --git a/spec/models/calculator/per_item_spec.rb b/spec/models/calculator/per_item_spec.rb index 4b0f72ecd8..914ca33dfa 100644 --- a/spec/models/calculator/per_item_spec.rb +++ b/spec/models/calculator/per_item_spec.rb @@ -7,10 +7,7 @@ describe Calculator::PerItem do let(:shipping_calculable) { double(:calculable) } let(:line_item) { build_stubbed(:line_item, quantity: 5) } - it do - should validate_numericality_of(:preferred_amount). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end + it { is_expected.to validate_numericality_of(:preferred_amount) } it "correctly calculates on a single line item object" do allow(calculator).to receive_messages(calculable: shipping_calculable) diff --git a/spec/models/calculator/price_sack_spec.rb b/spec/models/calculator/price_sack_spec.rb index 0754848035..771b8428ec 100644 --- a/spec/models/calculator/price_sack_spec.rb +++ b/spec/models/calculator/price_sack_spec.rb @@ -12,20 +12,9 @@ describe Calculator::PriceSack do end let(:line_item) { build_stubbed(:line_item, price: price, quantity: 2) } - it do - should validate_numericality_of(:preferred_minimal_amount). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end - - it do - should validate_numericality_of(:preferred_normal_amount). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end - - it do - should validate_numericality_of(:preferred_discount_amount). - with_message("Invalid input. Please use only numbers. For example: 10, 5.5, -20") - end + it { is_expected.to validate_numericality_of(:preferred_minimal_amount) } + it { is_expected.to validate_numericality_of(:preferred_normal_amount) } + it { is_expected.to validate_numericality_of(:preferred_discount_amount) } context 'when the order amount is below preferred minimal' do let(:price) { 2 }