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

tvdeyen · 2017-11-22T17:21:13Z

When rendering preference form fields we currently just render every
preference type defined on the preferable class.

Some classes - like solidus_paypal_braintree's Gateway - use complex types (Array and Hash) to store developer facing preferences.

Currently this leads to bugs because these preference field partials are not available.

As arrays and hashes won't represent well in a form field, we should not render them IMO.
The settings stored in these preferences are considered developer facing only.
Admins should not change these kind of values.

kennyadsl · 2017-11-22T17:28:27Z

Great, I was thinking at what should I do if I need to show just one preference hash or array field, for example because I know it's safe in that point and I want admin to be able to change only that. Is this a scenario we want to support?

kennyadsl · 2017-11-22T17:42:58Z

Great, I was thinking at what should I do if I need to show just one preference hash or array field, for example because I know it's safe in that point and I want admin to be able to change only that. Is this a scenario we want to support?

Ok, ignore it. I figured out I can just render another input in the view for that field outside the preferences partials code, so there's no need to support that in core.

tvdeyen · 2017-11-23T08:04:04Z

Updated this PR so that it is now possible to overwrite the class method allowed_admin_form_preference_types on the extending class in order to change the allowed fields. One have to provide their own preference field partials if they do so.

mamhoff

I would like some naming changes to more clearly differentiate between preference and type. Otherwise, as always, great work. 👍

mamhoff · 2017-11-23T08:25:21Z

core/spec/models/spree/preferences/preferable_spec.rb

+      subject { C.new.admin_form_preference_field_types }
+
+      before do
+        class C


C is very short, and maybe someone else creates an example class C, with different behaviour. In order to avoid conflicts and possibly test order issues because of the conflict, you might want to consider naming the class ClassWithPreferences or the like.

Uh-oh, there's also A, B, and D. Not concern of this PR, but 🗡

mamhoff · 2017-11-23T08:30:00Z

backend/app/views/spree/admin/payment_methods/_form.html.erb

-              <% @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_field_types.each do |type| %>


It seems to me this is a wrong change. Given a class with preferable :color, type: :string, previously this would render the form with attribute preferred_color, whereas now it seems like it renders preferred_string, no?

No, we're just not rendering a type here.

Ah, yes. You are right. Dang. Thanks for catching this!

mamhoff · 2017-11-23T08:34:19Z

core/lib/spree/preferences/preferable.rb

+      # +app/views/spree/admin/shared/preference_fields/+
+      #
+      # @return [Array]
+      def admin_form_preference_field_types


Previously, I misread code in a future commit because this method name includes types, prompting me to thing the result of this method is an array of type strings. Let's consider renaming it to admin_editable_preferences, so it communicates more clearly it returns an array of preferences?

tvdeyen · 2017-11-24T16:20:37Z

@mamhoff please review again

mamhoff

The variables inside the forms are now inconsistent with the plural of the method name :(

mamhoff · 2017-11-24T18:50:25Z

backend/app/views/spree/admin/promotions/calculators/_default_fields.html.erb

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


type should be name here.

mamhoff · 2017-11-24T18:50:43Z

backend/app/views/spree/admin/payment_methods/_form.html.erb

@@ -19,9 +19,9 @@
            <% if @object.preference_source.present? %>
              <%= t('spree.preference_source_using', name: @object.preference_source) %>
            <% elsif [email protected]_record? %>
-              <% @object.preferences.keys.each do |key| %>
+              <% @object.admin_form_preference_names.each do |key| %>


key should be name here.

mamhoff · 2017-11-24T18:50:59Z

backend/app/views/spree/admin/shared/_calculator_fields.html.erb

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


type should be name here.

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`.

…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.

…ence 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.

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.

tvdeyen · 2017-11-25T20:58:42Z

@mamhoff addressed

tvdeyen · 2017-11-29T07:55:41Z

@kennyadsl could you please re-review and give approval? Thanks

kennyadsl

Great, everything seems fully customizable if needed. Thanks!

After the merge of solidusio#2394 the admin promotion adjustment spec that got introduced by solidusio#2400 broke.

tvdeyen force-pushed the hash-preference-field branch from 227d922 to 9c3f4ac Compare November 22, 2017 17:25

tvdeyen force-pushed the hash-preference-field branch from 9c3f4ac to 45ab132 Compare November 23, 2017 08:00

solidusio deleted a comment from houndci-bot Nov 23, 2017

mamhoff suggested changes Nov 23, 2017

View reviewed changes

tvdeyen self-assigned this Nov 23, 2017

tvdeyen force-pushed the hash-preference-field branch 2 times, most recently from cd14dff to 909fcf4 Compare November 24, 2017 07:41

tvdeyen added the WIP label Nov 24, 2017

tvdeyen removed the WIP label Nov 24, 2017

tvdeyen removed their assignment Nov 24, 2017

mamhoff suggested changes Nov 24, 2017

View reviewed changes

tvdeyen force-pushed the hash-preference-field branch from 909fcf4 to 52e73cb Compare November 25, 2017 20:52

tvdeyen added 4 commits November 25, 2017 21:56

tvdeyen force-pushed the hash-preference-field branch from 52e73cb to 79df1c3 Compare November 25, 2017 20:57

mamhoff approved these changes Nov 26, 2017

View reviewed changes

tvdeyen added the Code review needed label Nov 29, 2017

kennyadsl approved these changes Nov 29, 2017

View reviewed changes

tvdeyen merged commit b3d1491 into solidusio:master Nov 30, 2017

tvdeyen deleted the hash-preference-field branch November 30, 2017 21:02

tvdeyen added a commit to tvdeyen/solidus that referenced this pull request Dec 1, 2017

Fix admin promotion adjustments spec

4fb9fe7

After the merge of solidusio#2394 the admin promotion adjustment spec that got introduced by solidusio#2400 broke.

tvdeyen mentioned this pull request Dec 1, 2017

Fix admin promotion adjustments spec #2423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tvdeyen commented Nov 22, 2017 •

edited

Loading

kennyadsl commented Nov 22, 2017

kennyadsl commented Nov 22, 2017

tvdeyen commented Nov 23, 2017 •

edited

Loading

mamhoff left a comment

mamhoff Nov 23, 2017

mamhoff Nov 23, 2017

mamhoff Nov 23, 2017

mamhoff Nov 23, 2017

tvdeyen Nov 23, 2017

mamhoff Nov 23, 2017 •

edited

Loading

tvdeyen commented Nov 24, 2017

mamhoff left a comment

mamhoff Nov 24, 2017

mamhoff Nov 24, 2017

mamhoff Nov 24, 2017

tvdeyen commented Nov 25, 2017

tvdeyen commented Nov 29, 2017

kennyadsl left a comment

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

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

Conversation

tvdeyen commented Nov 22, 2017 • edited Loading

kennyadsl commented Nov 22, 2017

kennyadsl commented Nov 22, 2017

tvdeyen commented Nov 23, 2017 • edited Loading

mamhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamhoff Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

tvdeyen commented Nov 24, 2017

mamhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen commented Nov 25, 2017

tvdeyen commented Nov 29, 2017

kennyadsl left a comment

Choose a reason for hiding this comment

tvdeyen commented Nov 22, 2017 •

edited

Loading

tvdeyen commented Nov 23, 2017 •

edited

Loading

mamhoff Nov 23, 2017 •

edited

Loading