From e355a0072465378f7ee07eac10f8cbf050ce5919 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Mar 2021 11:02:46 +0100 Subject: [PATCH 1/9] Remove redundant spec context This is already tested by the top-most before block and besides, we there's no OFN in Japan. We don't need to test all supported currencies but ensure that the various arguments work as intended. --- spec/lib/spree/money_spec.rb | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/spec/lib/spree/money_spec.rb b/spec/lib/spree/money_spec.rb index 12b1d975b8..6454be64fc 100644 --- a/spec/lib/spree/money_spec.rb +++ b/spec/lib/spree/money_spec.rb @@ -84,21 +84,6 @@ describe Spree::Money do end end - context "JPY" do - before do - configure_spree_preferences do |config| - config.currency = "JPY" - config.currency_symbol_position = :before - config.display_currency = false - end - end - - it "formats correctly" do - money = Spree::Money.new(1000, html: false) - expect(money.to_s).to eq("¥1,000") - end - end - context "EUR" do before do configure_spree_preferences do |config| From 485449e289d184b32cb16a60c59a1868b4d97b52 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Mar 2021 10:12:56 +0100 Subject: [PATCH 2/9] Fix Money gem deprecation warning with :format This removes millions of deprecation warnings like the following ``` [DEPRECATION] `symbol_position: :before` is deprecated - you can replace it with `format: %u %n` ``` from the build. It gets printed every time a `Spree::Money` is instantiated. This should result in a non-negligible speed up of the test suite. --- lib/spree/money.rb | 40 ++++++++++++++++++++++++++++-------- spec/lib/spree/money_spec.rb | 5 +++++ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/lib/spree/money.rb b/lib/spree/money.rb index 8ff14ce203..d0d3e581e9 100644 --- a/lib/spree/money.rb +++ b/lib/spree/money.rb @@ -10,15 +10,12 @@ module Spree def initialize(amount, options = {}) @money = ::Monetize.parse([amount, (options[:currency] || Spree::Config[:currency])].join) - @options = {} - @options[:with_currency] = Spree::Config[:display_currency] - @options[:symbol_position] = Spree::Config[:currency_symbol_position].to_sym - @options[:no_cents] = Spree::Config[:hide_cents] - @options[:decimal_mark] = Spree::Config[:currency_decimal_mark] - @options[:thousands_separator] = Spree::Config[:currency_thousands_separator] - @options.merge!(options) - # Must be a symbol because the Money gem doesn't do the conversion - @options[:symbol_position] = @options[:symbol_position].to_sym + + if options.key?(:symbol_position) + options[:format] = position_to_format(options.delete(:symbol_position)) + end + + @options = defaults.merge(options) end # Return the currency symbol (on its own) for the current default currency @@ -47,5 +44,30 @@ module Spree def ==(other) @money == other.money end + + private + + def defaults + { + with_currency: Spree::Config[:display_currency], + no_cents: Spree::Config[:hide_cents], + decimal_mark: Spree::Config[:currency_decimal_mark], + thousands_separator: Spree::Config[:currency_thousands_separator], + format: position_to_format(Spree::Config[:currency_symbol_position]) + } + end + + def position_to_format(position) + return if position.nil? + + case position.to_sym + when :before + '%u%n' + when :after + '%n %u' + else + raise 'Invalid symbol position' + end + end end end diff --git a/spec/lib/spree/money_spec.rb b/spec/lib/spree/money_spec.rb index 6454be64fc..9b7ad8e534 100644 --- a/spec/lib/spree/money_spec.rb +++ b/spec/lib/spree/money_spec.rb @@ -82,6 +82,11 @@ describe Spree::Money do money = Spree::Money.new(10, html: false) expect(money.to_s).to eq("10.00 $") end + + it 'raises with invalid position' do + expect { Spree::Money.new(10, symbol_position: 'invalid') } + .to raise_error('Invalid symbol position') + end end context "EUR" do From 96bcde61a33812e0fdfcd26a40536c8ae0e25272 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Mar 2021 10:25:16 +0100 Subject: [PATCH 3/9] Fix Money deprecation warning with :html_wrap This fixes the following deprecation warning ``` [DEPRECATION] `html` is deprecated - use `html_wrap` instead. Please note that `html_wrap` will wrap all parts of currency and if you use `with_currency` option, currency element class changes from `currency` to `money-currency`. ``` --- lib/spree/money.rb | 4 ++-- spec/lib/spree/money_spec.rb | 20 ++++++++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/spree/money.rb b/lib/spree/money.rb index d0d3e581e9..7ac0bd2722 100644 --- a/lib/spree/money.rb +++ b/lib/spree/money.rb @@ -27,9 +27,9 @@ module Spree @money.format(@options) end - def to_html(options = { html: true }) + def to_html(options = { html_wrap: true }) output = @money.format(@options.merge(options)) - if options[:html] + if options[:html_wrap] # 1) prevent blank, breaking spaces # 2) prevent escaping of HTML character entities output = output.sub(" ", " ").html_safe diff --git a/spec/lib/spree/money_spec.rb b/spec/lib/spree/money_spec.rb index 9b7ad8e534..e1ab7663af 100644 --- a/spec/lib/spree/money_spec.rb +++ b/spec/lib/spree/money_spec.rb @@ -25,13 +25,13 @@ describe Spree::Money do context "with currency" do it "passed in option" do - money = Spree::Money.new(10, with_currency: true, html: false) + money = Spree::Money.new(10, with_currency: true, html_wrap: false) expect(money.to_s).to eq("$10.00 USD") end it "config option" do Spree::Config[:display_currency] = true - money = Spree::Money.new(10, html: false) + money = Spree::Money.new(10, html_wrap: false) expect(money.to_s).to eq("$10.00 USD") end end @@ -53,14 +53,14 @@ describe Spree::Money do context "currency parameter" do context "when currency is specified in Canadian Dollars" do it "uses the currency param over the global configuration" do - money = Spree::Money.new(10, currency: 'CAD', with_currency: true, html: false) + money = Spree::Money.new(10, currency: 'CAD', with_currency: true, html_wrap: false) expect(money.to_s).to eq("$10.00 CAD") end end context "when currency is specified in Japanese Yen" do it "uses the currency param over the global configuration" do - money = Spree::Money.new(100, currency: 'JPY', html: false) + money = Spree::Money.new(100, currency: 'JPY', html_wrap: false) expect(money.to_s).to eq("¥100") end end @@ -68,18 +68,18 @@ describe Spree::Money do context "symbol positioning" do it "passed in option" do - money = Spree::Money.new(10, symbol_position: :after, html: false) + money = Spree::Money.new(10, symbol_position: :after, html_wrap: false) expect(money.to_s).to eq("10.00 $") end it "passed in option string" do - money = Spree::Money.new(10, symbol_position: "after", html: false) + money = Spree::Money.new(10, symbol_position: "after", html_wrap: false) expect(money.to_s).to eq("10.00 $") end it "config option" do Spree::Config[:currency_symbol_position] = :after - money = Spree::Money.new(10, html: false) + money = Spree::Money.new(10, html_wrap: false) expect(money.to_s).to eq("10.00 $") end @@ -118,10 +118,14 @@ describe Spree::Money do expect(money.to_s).to eq("1.000.00 €") end + # rubocop:disable Layout/LineLength it "formats as HTML if asked (nicely) to" do money = Spree::Money.new(10) # The HTMLified version of the euro sign - expect(money.to_html).to eq("10.00 €") + expect(money.to_html).to eq( + "10.00 €" + ) end + # rubocop:enable Layout/LineLength end end From ccfb6ae26ea3260f5f7bf18761352f96cb0caf7b Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Mar 2021 10:44:02 +0100 Subject: [PATCH 4/9] Fix Money deprecation warning with :rounding_mode This removes the deprecation warning: ``` [WARNING] The default rounding mode will change from `ROUND_HALF_EVEN` to `ROUND_HALF_UP` in the next major release. Set it explicitly using `Money.rounding_mode=` to avoid potential problems. ``` by specifying the default rounding mode at boot time so that it's only set once for the whole app. See https://github.com/RubyMoney/money#rounding for details. --- config/initializers/money.rb | 1 + 1 file changed, 1 insertion(+) create mode 100644 config/initializers/money.rb diff --git a/config/initializers/money.rb b/config/initializers/money.rb new file mode 100644 index 0000000000..4f9d8847f1 --- /dev/null +++ b/config/initializers/money.rb @@ -0,0 +1 @@ +Money.rounding_mode = BigDecimal::ROUND_HALF_EVEN From 41011ce28acdfb7d1c7d0d6ddb0d8fca009bd190 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Mar 2021 10:48:25 +0100 Subject: [PATCH 5/9] Fix Money deprecation warning w/ :default_currency This removes the deprecation warning: ``` [WARNING] The default currency will change from `USD` to `nil` in the next major release. Make sure to set it explicitly using `Money.default_currency=` to avoid potential issues ``` --- config/initializers/money.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/config/initializers/money.rb b/config/initializers/money.rb index 4f9d8847f1..fd63b2c88f 100644 --- a/config/initializers/money.rb +++ b/config/initializers/money.rb @@ -1 +1,2 @@ Money.rounding_mode = BigDecimal::ROUND_HALF_EVEN +Money.default_currency = Money::Currency.new('USD') From ca268d5c84bca9d15d2c9d9f33389eb1da1b801f Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Mar 2021 13:34:57 +0100 Subject: [PATCH 6/9] Remove unnecessary indirection in test --- .../admin/configuration/general_settings_spec.rb | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/spec/features/admin/configuration/general_settings_spec.rb b/spec/features/admin/configuration/general_settings_spec.rb index 6c5b70a4ba..e4ad401365 100644 --- a/spec/features/admin/configuration/general_settings_spec.rb +++ b/spec/features/admin/configuration/general_settings_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' describe "General Settings" do include AuthenticationHelper - before(:each) do + before do login_as_admin_and_visit spree.admin_dashboard_path click_link "Configuration" click_link "General Settings" @@ -24,16 +24,10 @@ describe "General Settings" do fill_in "site_name", with: "OFN Demo Site99" click_button "Update" - assert_successful_update_message(:general_settings) - - expect(find("#site_name").value).to eq("OFN Demo Site99") - end - - def assert_successful_update_message(resource) - flash = Spree.t(:successfully_updated, resource: Spree.t(resource)) within("[class='flash success']") do - expect(page).to have_content(flash) + expect(page).to have_content(Spree.t(:successfully_updated, resource: Spree.t(:general_settings))) end + expect(find("#site_name").value).to eq("OFN Demo Site99") end end end From 7792bc34c89ecc53290972300f2b3f2601b206b1 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Mar 2021 13:35:13 +0100 Subject: [PATCH 7/9] Test that currency symbol position can be changed --- .../admin/configuration/general_settings_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/features/admin/configuration/general_settings_spec.rb b/spec/features/admin/configuration/general_settings_spec.rb index e4ad401365..ecb6d61c91 100644 --- a/spec/features/admin/configuration/general_settings_spec.rb +++ b/spec/features/admin/configuration/general_settings_spec.rb @@ -30,4 +30,19 @@ describe "General Settings" do expect(find("#site_name").value).to eq("OFN Demo Site99") end end + + context 'editing currency symbol position' do + it 'updates its position' do + expect(page).to have_content('Currency Settings') + + within('.currency') do + find("[for='currency_symbol_position_after']").click + end + + click_button 'Update' + + expect(page).to have_content(Spree.t(:successfully_updated, resource: Spree.t(:general_settings))) + expect(page).to have_checked_field('10.00 $') + end + end end From 9c642e29578181fd55cfff64d79fb9d3f99cdd62 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 9 Mar 2021 09:00:37 +0100 Subject: [PATCH 8/9] Use the instance currency as Money's default --- config/initializers/money.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/money.rb b/config/initializers/money.rb index fd63b2c88f..1dea6e473b 100644 --- a/config/initializers/money.rb +++ b/config/initializers/money.rb @@ -1,2 +1,2 @@ Money.rounding_mode = BigDecimal::ROUND_HALF_EVEN -Money.default_currency = Money::Currency.new('USD') +Money.default_currency = Money::Currency.new(Spree::Config[:currency]) From 0eb14bc0a4012e8f16774e25c23ae6f5930e5171 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 10 Mar 2021 16:22:22 +0100 Subject: [PATCH 9/9] Do not modify the HTML returned by Money gem It was due to these lines that we were returning a broken HTML tag but also, there's no need to remove blanks. --- lib/spree/money.rb | 8 +------- spec/lib/spree/money_spec.rb | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/spree/money.rb b/lib/spree/money.rb index 7ac0bd2722..994aceabb7 100644 --- a/lib/spree/money.rb +++ b/lib/spree/money.rb @@ -28,13 +28,7 @@ module Spree end def to_html(options = { html_wrap: true }) - output = @money.format(@options.merge(options)) - if options[:html_wrap] - # 1) prevent blank, breaking spaces - # 2) prevent escaping of HTML character entities - output = output.sub(" ", " ").html_safe - end - output + @money.format(@options.merge(options)).html_safe end def format(options = {}) diff --git a/spec/lib/spree/money_spec.rb b/spec/lib/spree/money_spec.rb index e1ab7663af..676572eff6 100644 --- a/spec/lib/spree/money_spec.rb +++ b/spec/lib/spree/money_spec.rb @@ -123,7 +123,7 @@ describe Spree::Money do money = Spree::Money.new(10) # The HTMLified version of the euro sign expect(money.to_html).to eq( - "10.00 €" + "10.00 €" ) end # rubocop:enable Layout/LineLength