raggedstaff/openfoodnetwork
1
0
Fork 0
mirror of https://github.com/openfoodfoundation/openfoodnetwork synced 2026-04-05 07:19:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #10329 from thejwuscript/9861-improve-input-validation

Browse Source 
Improve input validation on new and edit Payment Method pages
This commit is contained in:
Konrad
2023-05-03 16:11:21 +02:00
committed by GitHub
parent 2e267091f6 918425fd93
commit b60a6fbd05
28 changed files with 305 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files

36
app/controllers/spree/admin/payment_methods_controller.rb
View File

@@ -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
@@ -17,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
@@ -44,6 +47,7 @@ module Spree
redirect_to spree.edit_admin_payment_method_path(@payment_method)
else
respond_with(@payment_method)
clear_preference_cache
end
end
@@ -182,33 +186,23 @@ 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
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)
def clear_preference_cache
@payment_method.calculator.preferences.each_key do |key|
Rails.cache.delete(@payment_method.calculator.preference_cache_key(key))
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
]
def add_type_to_calculator_attributes(hash)
hash["calculator_attributes"]["type"] = hash["calculator_type"]
end
end
end

46
app/helpers/spree/admin/base_helper.rb
View File

@@ -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
@@ -80,26 +80,24 @@ module Spree
end
def preference_field_options(options)
field_options = case options[:type]
when :integer
{ size: 10,
class: 'input_integer' }
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],

7
app/models/calculator/flat_percent_item_total.rb
View File

@@ -1,14 +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: true
def self.description
Spree.t(:flat_percent)

7
app/models/calculator/flat_percent_per_item.rb
View File

@@ -1,18 +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: true
def self.description
I18n.t(:flat_percent_per_item)

7
app/models/calculator/flat_rate.rb
View File

@@ -1,14 +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: true
def self.description
I18n.t(:flat_rate_per_order)

9
app/models/calculator/flexi_rate.rb
View File

@@ -1,17 +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: true
def self.description
I18n.t(:flexible_rate)

7
app/models/calculator/per_item.rb
View File

@@ -1,14 +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: true
def self.description
I18n.t(:flat_rate_per_item)

11
app/models/calculator/price_sack.rb
View File

@@ -1,18 +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: true
def self.description
I18n.t(:price_sack)

6
app/models/spree/payment_method.rb
View File

@@ -17,6 +17,7 @@ module Spree
validates :name, presence: true
validate :distributor_validation
validates_associated :calculator
after_initialize :init
@@ -40,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) }

3
app/models/spree/preference.rb
View File

@@ -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

2
app/models/spree/preferences/preferable.rb
View File

@@ -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

2
app/views/spree/admin/payment_methods/_providers.html.haml
View File

@@ -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" }

11
config/locales/en.yml
View File

@@ -79,6 +79,17 @@ en:
orders_close_at: Close date
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:"
errors:
models:
spree/user:

5
lib/spree/localized_number.rb
View File

@@ -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
@@ -44,7 +44,8 @@ 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}[.,]/
# 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
end

4
lib/tasks/sample_data/payment_method_factory.rb
View File

@@ -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

3
spec/base_spec_helper.rb
View File

@@ -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

6
spec/lib/spree/localized_number_spec.rb
View File

@@ -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

6
spec/models/calculator/flat_percent_item_total_spec.rb
View File

@@ -8,14 +8,12 @@ describe Calculator::FlatPercentItemTotal do
before { allow(calculator).to receive_messages preferred_flat_percent: 10 }
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)
end
context "extends LocalizedNumber" do
it_behaves_like "a 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)

6
spec/models/calculator/flat_percent_per_item_spec.rb
View File

@@ -5,6 +5,8 @@ require 'spec_helper'
describe Calculator::FlatPercentPerItem do
let(:calculator) { Calculator::FlatPercentPerItem.new preferred_flat_percent: 20 }
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
expect(calculator.compute(line_item)).to eq 20
@@ -14,8 +16,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]
end
end

4
spec/models/calculator/flat_rate_spec.rb
View File

@@ -7,7 +7,5 @@ describe Calculator::FlatRate do
before { allow(calculator).to receive_messages preferred_amount: 10 }
context "extends LocalizedNumber" do
it_behaves_like "a model using the LocalizedNumber module", [:preferred_amount]
end
it { is_expected.to validate_numericality_of(:preferred_amount) }
end

8
spec/models/calculator/flexi_rate_spec.rb
View File

@@ -12,6 +12,9 @@ describe Calculator::FlexiRate do
)
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 }
@@ -32,9 +35,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]
end
end

6
spec/models/calculator/per_item_spec.rb
View File

@@ -7,12 +7,10 @@ describe Calculator::PerItem do
let(:shipping_calculable) { double(:calculable) }
let(:line_item) { build_stubbed(:line_item, quantity: 5) }
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)
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]
end
end

10
spec/models/calculator/price_sack_spec.rb
View File

@@ -12,6 +12,10 @@ describe Calculator::PriceSack do
end
let(:line_item) { build_stubbed(:line_item, price: price, quantity: 2) }
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 }
@@ -75,10 +79,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]
end
end

11
spec/models/spree/preference_spec.rb
View File

@@ -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
@@ -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"

7
spec/models/spree/preferences/preferable_spec.rb
View File

@@ -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

3
spec/spec_helper.rb
View File

@@ -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

21
spec/support/localized_number_helper.rb
View File

@@ -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

169
spec/system/admin/payment_method_spec.rb
View File

@@ -112,6 +112,45 @@ 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
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
@@ -345,4 +384,134 @@ describe '
end
end
end
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
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}"
# Although text is not permitted by input[type=number], let's see what happens
fill_in "Amount", with: 'invalid'
click_button 'Update'
end
after do
Spree::Preferences::Store.instance.persistence = false
end
it "displays error details" do
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")
# Highlighting invalid fields
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 "Amount", with: 'invalid string'
click_button 'Update'
expect(page).to have_field "payment_method_name", with: 'Test name'
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).to have_checked_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
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 '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).to have_checked_field "payment_method_distributor_ids_#{@distributors[0].id}"
end
end
end