From a0e665f774ac7e0342476388e1708bb81ed51970 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 22 Mar 2024 02:24:06 +0000 Subject: [PATCH] Wrap commonly-repeated calls to Spree::Config to reduce unnecessary cache reads These config values are relatively static but in some cases they can be called many times in the same request (like rendering a report or a large list of line_items in BOM). These values will now only get fetched from Redis/Postgres once at most per request/job. --- app/helpers/admin/injection_helper.rb | 2 +- app/models/current_config.rb | 19 +++++++++++++++++++ app/models/spree/adjustment.rb | 2 +- app/models/spree/order.rb | 4 ++-- app/models/spree/price.rb | 2 +- app/models/spree/return_authorization.rb | 2 +- app/models/spree/shipment.rb | 2 +- app/models/spree/variant.rb | 8 ++++---- .../admin/customer_with_balance_serializer.rb | 2 +- .../api/currency_config_serializer.rb | 10 +++++----- app/services/weights_and_measures.rb | 2 +- .../admin/general_settings/edit.html.haml | 2 +- .../admin/payments/paypal_refund.html.haml | 4 ++-- lib/reporting/reports/xero_invoices/base.rb | 2 +- lib/spree/core/controller_helpers/order.rb | 2 +- lib/spree/money.rb | 14 +++++++------- spec/base_spec_helper.rb | 1 + spec/lib/spree/money_spec.rb | 8 ++++++++ 18 files changed, 58 insertions(+), 30 deletions(-) create mode 100644 app/models/current_config.rb diff --git a/app/helpers/admin/injection_helper.rb b/app/helpers/admin/injection_helper.rb index 3585fb0817c..54ec2a238a0 100644 --- a/app/helpers/admin/injection_helper.rb +++ b/app/helpers/admin/injection_helper.rb @@ -162,7 +162,7 @@ def admin_inject_spree_api_key(spree_api_key) def admin_inject_available_units admin_inject_json "admin.products", "availableUnits", - Spree::Config.available_units + CurrentConfig.get(:available_units) end def admin_inject_json(ng_module, name, data) diff --git a/app/models/current_config.rb b/app/models/current_config.rb new file mode 100644 index 00000000000..ca79a5dfdf3 --- /dev/null +++ b/app/models/current_config.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +# Wraps repeatedly-called configs in a CurrentAttributes object so they only get fetched once +# per request at most, eg: CurrentConfig.get(:available_units) for Spree::Config[:available_units] + +class CurrentConfig < ActiveSupport::CurrentAttributes + attribute :display_currency, :hide_cents, :currency_decimal_mark, + :currency_thousands_separator, :currency_symbol_position, :available_units + + def get(config_key) + return public_send(config_key) unless public_send(config_key).nil? + + public_send("#{config_key}=", Spree::Config.public_send(config_key)) + end + + def currency + ENV.fetch("CURRENCY") + end +end diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 90d2cca514a..e3165f3ca9b 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -120,7 +120,7 @@ def update_adjustment!(calculable = nil, force: false) end def currency - adjustable ? adjustable.currency : Spree::Config[:currency] + adjustable ? adjustable.currency : CurrentConfig.get(:currency) end def display_amount diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 5e56610c07e..fbb41c5abb6 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -186,7 +186,7 @@ def pre_discount_total end def currency - self[:currency] || Spree::Config[:currency] + self[:currency] || CurrentConfig.get(:currency) end def display_item_total @@ -689,7 +689,7 @@ def use_billing? end def set_currency - self.currency = Spree::Config[:currency] if self[:currency].nil? + self.currency = CurrentConfig.get(:currency) if self[:currency].nil? end def using_guest_checkout? diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb index 710e4ef5c35..668f53b6b91 100644 --- a/app/models/spree/price.rb +++ b/app/models/spree/price.rb @@ -37,7 +37,7 @@ def price=(price) def check_price return unless currency.nil? - self.currency = Spree::Config[:currency] + self.currency = CurrentConfig.get(:currency) end # strips all non-price-like characters from the price, taking into account locale settings diff --git a/app/models/spree/return_authorization.rb b/app/models/spree/return_authorization.rb index ad291f2bf36..1b18b9ea4fa 100644 --- a/app/models/spree/return_authorization.rb +++ b/app/models/spree/return_authorization.rb @@ -29,7 +29,7 @@ class ReturnAuthorization < ApplicationRecord end def currency - order.nil? ? Spree::Config[:currency] : order.currency + order.nil? ? CurrentConfig.get(:currency) : order.currency end def display_amount diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index 0b060c727c5..8e59029abcd 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -163,7 +163,7 @@ def find_shipping_rate_for(shipping_method_id) end def currency - order ? order.currency : Spree::Config[:currency] + order ? order.currency : CurrentConfig.get(:currency) end def display_cost diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index d91498e3105..34d312baedb 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -42,7 +42,7 @@ class Variant < ApplicationRecord accepts_nested_attributes_for :images has_one :default_price, - -> { with_deleted.where(currency: Spree::Config[:currency]) }, + -> { with_deleted.where(currency: CurrentConfig.get(:currency)) }, class_name: 'Spree::Price', dependent: :destroy has_many :prices, @@ -161,7 +161,7 @@ def self.active(currency = nil) where("spree_variants.id in (?)", joins(:prices). where(deleted_at: nil). where('spree_prices.currency' => - currency || Spree::Config[:currency]). + currency || CurrentConfig.get(:currency)). where.not(spree_prices: { amount: nil }). select("spree_variants.id")) end @@ -210,7 +210,7 @@ def total_on_hand def check_currency return unless currency.nil? - self.currency = Spree::Config[:currency] + self.currency = CurrentConfig.get(:currency) end def save_default_price @@ -222,7 +222,7 @@ def find_or_build_default_price end def set_cost_currency - self.cost_currency = Spree::Config[:currency] if cost_currency.blank? + self.cost_currency = CurrentConfig.get(:currency) if cost_currency.blank? end def create_stock_items diff --git a/app/serializers/api/admin/customer_with_balance_serializer.rb b/app/serializers/api/admin/customer_with_balance_serializer.rb index 90f2726a104..6115d9efbef 100644 --- a/app/serializers/api/admin/customer_with_balance_serializer.rb +++ b/app/serializers/api/admin/customer_with_balance_serializer.rb @@ -12,7 +12,7 @@ class CustomerWithBalanceSerializer < CustomerSerializer delegate :balance_value, to: :object def balance - Spree::Money.new(balance_value, currency: Spree::Config[:currency]).to_s + Spree::Money.new(balance_value, currency: CurrentConfig.get(:currency)).to_s end def balance_status diff --git a/app/serializers/api/currency_config_serializer.rb b/app/serializers/api/currency_config_serializer.rb index a9b1bbd7f1d..485b2022b07 100644 --- a/app/serializers/api/currency_config_serializer.rb +++ b/app/serializers/api/currency_config_serializer.rb @@ -4,22 +4,22 @@ class Api::CurrencyConfigSerializer < ActiveModel::Serializer attributes :currency, :display_currency, :symbol, :symbol_position, :hide_cents def currency - Spree::Config[:currency] + CurrentConfig.get(:currency) end def display_currency - Spree::Config[:display_currency] + CurrentConfig.get(:display_currency) end def symbol - ::Money.new(1, Spree::Config[:currency]).symbol + ::Money.new(1, CurrentConfig.get(:currency)).symbol end def symbol_position - Spree::Config[:currency_symbol_position] + CurrentConfig.get(:currency_symbol_position) end def hide_cents - Spree::Config[:hide_cents] + CurrentConfig.get(:hide_cents) end end diff --git a/app/services/weights_and_measures.rb b/app/services/weights_and_measures.rb index 059075b7f86..1cb70b47522 100644 --- a/app/services/weights_and_measures.rb +++ b/app/services/weights_and_measures.rb @@ -40,7 +40,7 @@ def self.variant_unit_options end def self.available_units - Spree::Config.available_units.split(",") + CurrentConfig.get(:available_units).split(",") end def self.available_units_sorted diff --git a/app/views/spree/admin/general_settings/edit.html.haml b/app/views/spree/admin/general_settings/edit.html.haml index 5c938419e6f..f4c8c813a2a 100644 --- a/app/views/spree/admin/general_settings/edit.html.haml +++ b/app/views/spree/admin/general_settings/edit.html.haml @@ -79,7 +79,7 @@ %fieldset.available_units.no-border-bottom %legend{:align => "center"}= t('admin.available_units') .field - - available_units = Spree::Config[:available_units].split(",") + - available_units = CurrentConfig.get(:available_units).split(",") - all_units.each do |unit| - selected = available_units.include?(unit) = preference_field_tag("available_units[#{unit}]", selected, { type: :boolean, selected: selected }) diff --git a/app/views/spree/admin/payments/paypal_refund.html.haml b/app/views/spree/admin/payments/paypal_refund.html.haml index 98641d05a2a..7319be4dd30 100644 --- a/app/views/spree/admin/payments/paypal_refund.html.haml +++ b/app/views/spree/admin/payments/paypal_refund.html.haml @@ -18,8 +18,8 @@ %small %em= Spree.t(:original_amount, scope: 'paypal', amount: @payment.display_amount) %br/ - - symbol = ::Money.new(1, Spree::Config[:currency]).symbol - - if Spree::Config[:currency_symbol_position] == "before" + - symbol = ::Money.new(1, CurrentConfig.get(:currency)).symbol + - if CurrentConfig.get(:currency_symbol_position) == "before" = symbol = text_field_tag 'refund_amount', @payment.amount - else diff --git a/lib/reporting/reports/xero_invoices/base.rb b/lib/reporting/reports/xero_invoices/base.rb index d7a36c5c6ff..49d9fad4a65 100644 --- a/lib/reporting/reports/xero_invoices/base.rb +++ b/lib/reporting/reports/xero_invoices/base.rb @@ -189,7 +189,7 @@ def row(order, sku, description, quantity, amount, invoice_number, tax_type, opt '', '', '', - Spree::Config.currency, + CurrentConfig.get(:currency), '', order.paid? ? I18n.t(:y) : I18n.t(:n)] end diff --git a/lib/spree/core/controller_helpers/order.rb b/lib/spree/core/controller_helpers/order.rb index 3aea7c853d0..d2a1795e205 100644 --- a/lib/spree/core/controller_helpers/order.rb +++ b/lib/spree/core/controller_helpers/order.rb @@ -79,7 +79,7 @@ def set_current_order end def current_currency - Spree::Config[:currency] + CurrentConfig.get(:currency) end def ip_address diff --git a/lib/spree/money.rb b/lib/spree/money.rb index 4a11d334972..6c6b52a4f28 100644 --- a/lib/spree/money.rb +++ b/lib/spree/money.rb @@ -9,7 +9,7 @@ class Money delegate :cents, to: :money def initialize(amount, options = {}) - @money = ::Monetize.parse([amount, options[:currency] || Spree::Config[:currency]].join) + @money = ::Monetize.parse([amount, options[:currency] || CurrentConfig.get(:currency)].join) if options.key?(:symbol_position) options[:format] = position_to_format(options.delete(:symbol_position)) @@ -20,7 +20,7 @@ def initialize(amount, options = {}) # Return the currency symbol (on its own) for the current default currency def self.currency_symbol - ::Money.new(0, Spree::Config[:currency]).symbol + ::Money.new(0, CurrentConfig.get(:currency)).symbol end def to_s @@ -44,11 +44,11 @@ def ==(other) 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]) + with_currency: CurrentConfig.get(:display_currency), + no_cents: CurrentConfig.get(:hide_cents), + decimal_mark: CurrentConfig.get(:currency_decimal_mark), + thousands_separator: CurrentConfig.get(:currency_thousands_separator), + format: position_to_format(CurrentConfig.get(:currency_symbol_position)) } end diff --git a/spec/base_spec_helper.rb b/spec/base_spec_helper.rb index c875249ab75..f835bc883b6 100644 --- a/spec/base_spec_helper.rb +++ b/spec/base_spec_helper.rb @@ -194,6 +194,7 @@ spree_config.currency = currency spree_config.shipping_instructions = true end + CurrentConfig.clear_all end # Don't validate our invalid test data with expensive network requests. diff --git a/spec/lib/spree/money_spec.rb b/spec/lib/spree/money_spec.rb index 8cbc869bc11..b6f2fca8973 100644 --- a/spec/lib/spree/money_spec.rb +++ b/spec/lib/spree/money_spec.rb @@ -24,6 +24,11 @@ end context "with currency" do + before do + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("CURRENCY").and_return("USD") + end + it "passed in option" do money = Spree::Money.new(10, with_currency: true, html_wrap: false) expect(money.to_s).to eq("$10.00 USD") @@ -96,6 +101,9 @@ config.currency_symbol_position = :after config.display_currency = false end + + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with("CURRENCY").and_return("EUR") end # Regression test for Spree #2634