From f7a488a1f3959f1da9d0190db502bb388ef2d76d Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Wed, 22 Nov 2017 15:37:57 +0100 Subject: [PATCH 1/4] Add admin_form_preference_names When rendering preference form fields we currently just render every preference type defined on the preferable class. Some classes use complex types (`Array` and `Hash`) to store developer facing preferences. This leads to bugs where this preference field partial might not be available. This collection helps to render only the preference form fields we have partials for. It provides ability for the extending class to change this behavior by overwriting the class method `allowed_admin_form_preference_types`. --- core/lib/spree/preferences/preferable.rb | 104 ++++++++++++------ .../preferences/preferable_class_methods.rb | 22 ++++ .../spree/preferences/preferable_spec.rb | 44 ++++++++ 3 files changed, 136 insertions(+), 34 deletions(-) diff --git a/core/lib/spree/preferences/preferable.rb b/core/lib/spree/preferences/preferable.rb index fd28f979eb1..36008856a54 100644 --- a/core/lib/spree/preferences/preferable.rb +++ b/core/lib/spree/preferences/preferable.rb @@ -1,43 +1,55 @@ -# Preferable allows defining preference accessor methods. -# -# A class including Preferable must implement #preferences which should return -# an object responding to .fetch(key), []=(key, val), and .delete(key). -# -# The generated writer method performs typecasting before assignment into the -# preferences object. -# -# Examples: -# -# # Spree::Base includes Preferable and defines preferences as a serialized -# # column. -# class Settings < Spree::Base -# preference :color, :string, default: 'red' -# preference :temperature, :integer, default: 21 -# end -# -# s = Settings.new -# s.preferred_color # => 'red' -# s.preferred_temperature # => 21 -# -# s.preferred_color = 'blue' -# s.preferred_color # => 'blue' -# -# # Typecasting is performed on assignment -# s.preferred_temperature = '24' -# s.preferred_color # => 24 -# -# # Modifications have been made to the .preferences hash -# s.preferences #=> {color: 'blue', temperature: 24} -# -# # Save the changes. All handled by activerecord -# s.save! - require 'spree/preferences/preferable_class_methods' require 'active_support/concern' require 'active_support/core_ext/hash/keys' module Spree module Preferences + # Preferable allows defining preference accessor methods. + # + # A class including Preferable must implement #preferences which should return + # an object responding to .fetch(key), []=(key, val), and .delete(key). + # + # The generated writer method performs typecasting before assignment into the + # preferences object. + # + # Examples: + # + # # Spree::Base includes Preferable and defines preferences as a serialized + # # column. + # class Settings < Spree::Base + # preference :color, :string, default: 'red' + # preference :temperature, :integer, default: 21 + # end + # + # s = Settings.new + # s.preferred_color # => 'red' + # s.preferred_temperature # => 21 + # + # s.preferred_color = 'blue' + # s.preferred_color # => 'blue' + # + # # Typecasting is performed on assignment + # s.preferred_temperature = '24' + # s.preferred_color # => 24 + # + # # Modifications have been made to the .preferences hash + # s.preferences #=> {color: 'blue', temperature: 24} + # + # # Save the changes. All handled by activerecord + # s.save! + # + # Each preference gets rendered as a form field in Solidus backend. + # + # As not all supported preference types are representable as a form field, only + # some of them get rendered per default. Arrays and Hashes for instance are + # supported preference field types, but do not represent well as a form field. + # + # Overwrite +allowed_admin_form_preference_types+ in your class if you want to + # provide more fields. If you do so, you also need to provide a preference field + # partial that lives in: + # + # +app/views/spree/admin/shared/preference_fields/+ + # module Preferable extend ActiveSupport::Concern @@ -101,6 +113,30 @@ def default_preferences ] end + # Preference names representable as form fields in Solidus backend + # + # Not all preferences are representable as a form field. + # + # Arrays and Hashes for instance are supported preference field types, + # but do not represent well as a form field. + # + # As these kind of preferences are mostly developer facing + # and not admin facing we should not render them. + # + # Overwrite +allowed_admin_form_preference_types+ in your class that + # includes +Spree::Preferable+ if you want to provide more fields. + # If you do so, you also need to provide a preference field partial + # that lives in: + # + # +app/views/spree/admin/shared/preference_fields/+ + # + # @return [Array] + def admin_form_preference_names + defined_preferences.keep_if do |type| + preference_type(type).in? self.class.allowed_admin_form_preference_types + end + end + private def convert_preference_value(value, type) diff --git a/core/lib/spree/preferences/preferable_class_methods.rb b/core/lib/spree/preferences/preferable_class_methods.rb index 2abcf14d1c1..823e0f6e703 100644 --- a/core/lib/spree/preferences/preferable_class_methods.rb +++ b/core/lib/spree/preferences/preferable_class_methods.rb @@ -1,5 +1,14 @@ module Spree::Preferences module PreferableClassMethods + DEFAULT_ADMIN_FORM_PREFERENCE_TYPES = %i( + boolean + decimal + integer + password + string + text + ) + def defined_preferences [] end @@ -60,5 +69,18 @@ def preference_default_getter_method(name) def preference_type_getter_method(name) "preferred_#{name}_type".to_sym end + + # List of preference types allowed as form fields in the Solidus admin + # + # Overwrite this method in your class that includes +Spree::Preferable+ + # if you want to provide more fields. If you do so, you also need to provide + # a preference field partial that lives in: + # + # +app/views/spree/admin/shared/preference_fields/+ + # + # @return [Array] + def allowed_admin_form_preference_types + DEFAULT_ADMIN_FORM_PREFERENCE_TYPES + end end end diff --git a/core/spec/models/spree/preferences/preferable_spec.rb b/core/spec/models/spree/preferences/preferable_spec.rb index ebec0c347ba..783e7d61eff 100644 --- a/core/spec/models/spree/preferences/preferable_spec.rb +++ b/core/spec/models/spree/preferences/preferable_spec.rb @@ -105,6 +105,50 @@ class B < A }) end + describe '#admin_form_preference_names' do + subject do + ComplexPreferableClass.new.admin_form_preference_names + end + + before do + class ComplexPreferableClass + include Spree::Preferences::Preferable + preference :name, :string + preference :password, :password + preference :mapping, :hash + preference :recipients, :array + end + end + + it "returns an array of preference names excluding preferences not presentable as form field" do + is_expected.to contain_exactly(:name, :password) + end + + context 'with overwritten allowed_admin_form_preference_types class method' do + subject do + ComplexOverwrittenPreferableClass.new.admin_form_preference_names + end + + before do + class ComplexOverwrittenPreferableClass + include Spree::Preferences::Preferable + preference :name, :string + preference :password, :password + preference :mapping, :hash + preference :recipients, :array + + def self.allowed_admin_form_preference_types + %i(string password hash array) + end + end + end + + it 'returns these types instead' do + is_expected.to contain_exactly(:name, :password, :mapping, :recipients) + end + end + end + context "converts integer preferences to integer values" do before do A.preference :is_integer, :integer From be51a3004a90fb7b8cf5aed5b75274f94f5c31c3 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Wed, 22 Nov 2017 16:26:28 +0100 Subject: [PATCH 2/4] Only show fields on payment method form that we have preference form fields for We do not ship preference form fields for Array and Hash preference types. Some extensions, like solidus_paypal_braintree, do use them to store very specific settings. As arrays and hashes won't represent well in a form field, we should not render them. The settings stored in these preferences are considered developer facing only. Admins should not change these kind of values. --- .../admin/payment_methods/_form.html.erb | 7 ++++--- .../configuration/payment_methods_spec.rb | 21 +++++++++++++++++-- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/backend/app/views/spree/admin/payment_methods/_form.html.erb b/backend/app/views/spree/admin/payment_methods/_form.html.erb index 9d9eb3c46e1..b6b4a32248a 100644 --- a/backend/app/views/spree/admin/payment_methods/_form.html.erb +++ b/backend/app/views/spree/admin/payment_methods/_form.html.erb @@ -19,9 +19,10 @@ <% if @object.preference_source.present? %> <%= t('spree.preference_source_using', name: @object.preference_source) %> <% elsif !@object.new_record? %> - <% @object.preferences.keys.each do |key| %> - <%= render "spree/admin/shared/preference_fields/#{@object.preference_type(key)}", - form: f, attribute: "preferred_#{key}", label: t(key, scope: 'spree') %> + <% @object.admin_form_preference_names.each do |name| %> + <%= render "spree/admin/shared/preference_fields/#{@object.preference_type(name)}", + form: f, attribute: "preferred_#{name}", + label: t(name, scope: 'spree', default: name.to_s.humanize) %> <% end %> <% end %> diff --git a/backend/spec/features/admin/configuration/payment_methods_spec.rb b/backend/spec/features/admin/configuration/payment_methods_spec.rb index 1d93b9aca49..f56003e93f5 100644 --- a/backend/spec/features/admin/configuration/payment_methods_spec.rb +++ b/backend/spec/features/admin/configuration/payment_methods_spec.rb @@ -45,8 +45,9 @@ end context "admin editing a payment method" do - before(:each) do - create(:check_payment_method) + let!(:payment_method) { create(:check_payment_method) } + + before do click_link "Payments" expect(page).to have_link 'Payment Methods' within("table#listing_payment_methods") do @@ -66,6 +67,22 @@ click_button "Update" expect(page).to have_content("Name can't be blank") end + + context 'with payment method having hash and array as preference' do + class ComplexPayments < Spree::PaymentMethod + preference :name, :string + preference :mapping, :hash + preference :recipients, :array + end + + let!(:payment_method) { ComplexPayments.create!(name: 'Complex Payments') } + + it "does not display preference fields that are hash or array" do + expect(page).to have_field("Name") + expect(page).to_not have_field("Mapping") + expect(page).to_not have_field("Recipients") + end + end end context "changing type and payment_source", js: true do From a04d0d26719a6d5ed9d7957a65291c383f015a48 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Wed, 22 Nov 2017 17:17:49 +0100 Subject: [PATCH 3/4] Only show fields for promotion action calculators that we have preference form fields for We do not ship preference form fields for Array and Hash preference types. Some promotion actions might use them to store very specific settings. As arrays and hashes won't represent well in a form field, we should not render them. --- .../calculators/_default_fields.html.erb | 9 ++-- .../admin/promotion_adjustments_spec.rb | 43 +++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/backend/app/views/spree/admin/promotions/calculators/_default_fields.html.erb b/backend/app/views/spree/admin/promotions/calculators/_default_fields.html.erb index 78273ae01cf..0bdcfa9286e 100644 --- a/backend/app/views/spree/admin/promotions/calculators/_default_fields.html.erb +++ b/backend/app/views/spree/admin/promotions/calculators/_default_fields.html.erb @@ -1,5 +1,6 @@ -<% calculator.preferences.keys.map do |key| %> - <%= render "spree/admin/shared/preference_fields/#{calculator.preference_type(key)}", - name: "#{prefix}[calculator_attributes][preferred_#{key}]", - value: calculator.get_preference(key), label: t(key.to_s, scope: 'spree') %> +<% calculator.admin_form_preference_names.map do |name| %> + <%= render "spree/admin/shared/preference_fields/#{calculator.preference_type(name)}", + name: "#{prefix}[calculator_attributes][preferred_#{name}]", + value: calculator.get_preference(name), + label: t(name.to_s, scope: 'spree', default: name.to_s.humanize) %> <% end %> diff --git a/backend/spec/features/admin/promotion_adjustments_spec.rb b/backend/spec/features/admin/promotion_adjustments_spec.rb index e44949eccb9..793053e4e5e 100644 --- a/backend/spec/features/admin/promotion_adjustments_spec.rb +++ b/backend/spec/features/admin/promotion_adjustments_spec.rb @@ -242,5 +242,48 @@ expect(first_action.calculator.class).to eq(Spree::Calculator::FlatRate) expect(first_action.calculator.preferred_amount).to eq(5) end + + context 'creating a promotion with promotion action that has a calculator with complex preferences' do + before do + class ComplexCalculator < Spree::Calculator + preference :amount, :decimal + preference :currency, :string + preference :mapping, :hash + preference :list, :array + + def self.description + "Complex Calculator" + end + end + @calculators = Rails.application.config.spree.calculators.promotion_actions_create_item_adjustments + Rails.application.config.spree.calculators.promotion_actions_create_item_adjustments = [ComplexCalculator] + end + + after do + Rails.application.config.spree.calculators.promotion_actions_create_item_adjustments = @calculators + end + + it "does not show array and hash form fields" do + fill_in "Name", with: "SAVE SAVE SAVE" + choose "Apply to all orders" + click_button "Create" + expect(page).to have_title("SAVE SAVE SAVE - Promotions") + + select "Create per-line-item adjustment", from: "Add action of type" + within('#action_fields') do + click_button "Add" + select "Complex Calculator", from: "Base Calculator" + end + within('#actions_container') { click_button "Update" } + expect(page).to have_text 'successfully updated' + + within('#action_fields') do + expect(page).to have_field('Amount') + expect(page).to have_field('Currency') + expect(page).to_not have_field('Mapping') + expect(page).to_not have_field('List') + end + end + end end end From 79df1c3717f848da004e8169b8241fda1ab60ff9 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Wed, 22 Nov 2017 18:05:09 +0100 Subject: [PATCH 4/4] Only show fields for calculators that we have preference form fields for We do not ship preference form fields for Array and Hash preference types. Some shipping or tax rate calculators might use them to store very specific settings. As arrays and hashes won't represent well in a form field, we should not render them. --- .../admin/shared/_calculator_fields.html.erb | 7 ++-- .../configuration/shipping_methods_spec.rb | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/backend/app/views/spree/admin/shared/_calculator_fields.html.erb b/backend/app/views/spree/admin/shared/_calculator_fields.html.erb index 1bfc3a2ea64..3d078f24f30 100644 --- a/backend/app/views/spree/admin/shared/_calculator_fields.html.erb +++ b/backend/app/views/spree/admin/shared/_calculator_fields.html.erb @@ -11,9 +11,10 @@ <% calculator = f.object.calculator.class == calculator_class ? f.object.calculator : calculator_class.new %>
<%= f.fields_for :calculator, calculator do |calculator_form| %> - <% calculator.preferences.keys.each do |key| %> - <%= render "spree/admin/shared/preference_fields/#{calculator.preference_type(key)}", - form: calculator_form, attribute: "preferred_#{key}", label: t(key, scope: 'spree') %> + <% calculator.admin_form_preference_names.each do |name| %> + <%= render "spree/admin/shared/preference_fields/#{calculator.preference_type(name)}", + form: calculator_form, attribute: "preferred_#{name}", + label: t(name, scope: 'spree', default: name.to_s.humanize) %> <% end %> <% end %>
diff --git a/backend/spec/features/admin/configuration/shipping_methods_spec.rb b/backend/spec/features/admin/configuration/shipping_methods_spec.rb index e63700a4174..63446a6964d 100644 --- a/backend/spec/features/admin/configuration/shipping_methods_spec.rb +++ b/backend/spec/features/admin/configuration/shipping_methods_spec.rb @@ -35,6 +35,46 @@ click_on "Create" expect(current_path).to eql(spree.edit_admin_shipping_method_path(Spree::ShippingMethod.last)) end + + context 'with shipping method having a calculator with array or hash preference type' do + before do + class ComplexShipments < Spree::ShippingCalculator + preference :amount, :decimal + preference :currency, :string + preference :mapping, :hash + preference :list, :array + + def self.description + "Complex Shipments" + end + end + @calculators = Rails.application.config.spree.calculators.shipping_methods + Rails.application.config.spree.calculators.shipping_methods = [ComplexShipments] + end + + after do + Rails.application.config.spree.calculators.shipping_methods = @calculators + end + + it "does not show array and hash form fields" do + click_link "New Shipping Method" + + fill_in "shipping_method_name", with: "bullock cart" + + within("#shipping_method_categories_field") do + check first("input[type='checkbox']")["name"] + end + + click_on "Create" + select 'Complex Shipments', from: 'Base Calculator' + click_on "Update" + + expect(page).to have_field('Amount') + expect(page).to have_field('Currency') + expect(page).to_not have_field('Mapping') + expect(page).to_not have_field('List') + end + end end # Regression test for https://github.com/spree/spree/issues/1331