Merge pull request solidusio#2394 from tvdeyen/hash-preference-field

[RFC] Do not render complex preference types as form fields

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 %>
           </div>

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

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 %>
       <div class="js-calculator-preferences" data-calculator-type="<%= calculator_class %>">
         <%= 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 %>
       </div>

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

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

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

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)

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

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