Payment methods display on boolean #1540

gevann · 2016-10-19T16:22:29Z

This PR changes the display_on column of Spree::PaymentMethods to two boolean columns, available_to_users and available_to_admin. This allows clearer selection than the string-based display_on column, enabling usage of scopes on the model, and improving readability and intention of the affected code.
Closes #1532

This PR will be reworked into two-value logic after the merge of PR #1541, which fixes invalid specs for payment methods.

jhawthorn · 2016-10-19T19:24:02Z

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

+          <%= label_tag nil, Spree::PaymentMethod.human_attribute_name(:available_to_users) %>
+          <ul>
+            <li>
+              <%= radio_button :payment_method, :available_to_users, true %>


A checkbox might be simpler than a radio button

agreed - will change

jhawthorn · 2016-10-19T19:31:11Z

core/app/models/spree/order.rb

+          store.payment_methods
+        else
+          Spree::PaymentMethod
+        end


This is not ideal because the order is holding knowledge about what payments are available to what stores. This logic would be better in either the Payment or Store class.

I'd recommend adding a Payment.available_to_store(store) scope.

sounds good, I'll add that scope

jhawthorn · 2016-10-19T20:14:32Z

core/app/models/spree/payment_method.rb

-          (store.nil? || store.payment_methods.empty? || store.payment_methods.include?(p))
+      Spree::Deprecation.warn "Spree::PaymentMethod.available is deprecated."\
+        "Please use .active, .available_to_users, and .available_to_admin scopes instead."\
+        "For payment methods associated with a specific store, use your_store.payment_methods."


This isn't a precise replacement ,your_store.payment_methods won't return payments if none are associated (it should return all payments in this case). Payment.available_to_store(store) which I suggest above would solve this.

jhawthorn · 2016-10-19T20:16:28Z

...igrate/20161014221052_add_available_to_columns_and_remove_display_on_from_payment_methods.rb

@@ -0,0 +1,28 @@
+class AddAvailableToColumnsAndRemoveDisplayOnFromPaymentMethods < ActiveRecord::Migration[5.0]
+  def up
+    add_column(:spree_payment_methods, :available_to_users, :boolean, default: nil)


What meaning does the default, nil, have?

I would prefer that the default to both be true (matching the old default behaviour) and that the column was null: false to avoid any tri-value logic.

I agree. Originally it was meant to be default: true and avoid tri-value logic, however the edge case of nil valued display_ons make tri-value logic necessary.

jhawthorn · 2016-10-19T20:19:59Z

...igrate/20161014221052_add_available_to_columns_and_remove_display_on_from_payment_methods.rb

+             "WHERE (display_on='front_end' OR display_on='' OR display_on='both')")
+    execute("UPDATE spree_payment_methods "\
+             "SET available_to_admin='true' "\
+             "WHERE (display_on='back_end' OR display_on='' OR display_on='both')")


👍 Thank you for using plain SQL in the migration

jhawthorn · 2016-10-19T20:27:07Z

core/app/models/spree/payment_method.rb

+        when 'back_end'
+          active.available_to_admin
+        else
+          active.available_to_users.available_to_admin + active.nil_available_to_users.nil_available_to_admin


I don't follow this line. I don't think it replicates the existing functionality.

In the existing core/spec/models/spree/payment_method_spec.rb file's describe .available specs, there are four payment methods created, with display_on values of 'front_end', 'back_end', 'both', and nil.
The existing functionality filters with (p.display_on == display_on.to_s || p.display_on.blank?). This behaviour of .blank? makes the live above (and the three value logic) necessary; the existing specs assert that there are two methods available for both. This should mean that there would be three each available for frontend and backend, as there are two for both and one each for those particular locations, however the specs show that this is not the case.

jhawthorn · 2016-10-19T20:58:37Z

...igrate/20161014221052_add_available_to_columns_and_remove_display_on_from_payment_methods.rb

+    add_column(:spree_payment_methods, :available_to_admin, :boolean, default: nil)
+    execute("UPDATE spree_payment_methods "\
+             "SET available_to_users='true' "\
+             "WHERE (display_on='front_end' OR display_on='' OR display_on='both')")


'both' should not be included here.

cbrunsdon · 2016-11-02T21:53:48Z

...igrate/20161014221052_add_available_to_columns_and_remove_display_on_from_payment_methods.rb

+             "WHERE NOT (display_on='front_end' OR display_on='' OR display_on IS NULL)")
+    execute("UPDATE spree_payment_methods "\
+             "SET available_to_admin='false' "\
+             "WHERE NOT (display_on='back_end' OR display_on='' OR display_on IS NULL)")


I'm not 100% sure thats safe, I'd prefer something like:

"SET available_to_admin=#{ActiveRecord::Base.connection.quote(true)}"

peterberkenbosch · 2016-11-08T00:03:42Z

@gevann this needs a rebase with master.

peterberkenbosch · 2016-11-08T00:04:28Z

LGTM! 👍

PaymentMethod.display_on column is split in to two boolean columns available_to_users, and available_to_admin. Each value is set based on the value in the display_on column. Finally, the display_on column is removed.

Adjust Spree::Payment method creation in specs to use new boolean columns (available_to_users, available_to_admin) rather than string column 'display_on' Update specs and factories to use new columns

This allows any existing code which used display_on to continue working. It's likely that specs will be using this.

The original implementation of `Spree::PaymentMethod.available` used `.select` which would turn the ActiveRecord::Relation into an array and meant we had to use `sort_by`. In solidusio#1540 `available` was deprecated in favour of `available_to_users` and `available_to_admin`, each of which returns an ActiveRecord::Relation object. That means we can use `order` to do the sorting in SQL, which also allows users to chain more scopes onto the return value if they wish.

Payment methods available on the backend used to be scope to active methods only. Unfortunately that functionality was lost in solidusio#1540 because the `available_to_admin` scope isn't limited to active payment methods.

This was the original behaviour in Solidus < 2.1, but the active scope got lost in solidusio#1540. Payment methods used to use the `available` filter which was scoped to active payment methods only. The `available_to_admin` scope isn't limited active payment methods, so we need to add it here.

I noticed that inactive payment methods were not being filtered out on the frontend. This used to happen in `Spree::PaymentMethod.available`, but in solidusio#1540 this filter was lost.

We use `Spree::Order#available_payment_methods` to determine which payment methods should be displayed on the payment page in checkout. The changes in solidusio#1540 accidentally dropped the scoping for active payment methods, causing inactive payment methods to show up on the frontend. This also required fixing the `available_to_store` scope, which behaved incorrectly. Calling `self` from inside the scope returns the `Spree::PaymentMethod` class itself and not the current scoping as the code implied. This scope will now raise an error if the store parameter is `nil`, return all payment methods in the current scope if the store has no payment methods, and return the payment methods for the store if it does have some.

Payment methods available on the backend used to be scope to active methods only. Unfortunately that functionality was lost in solidusio#1540 because the `available_to_admin` scope isn't limited to active payment methods.

This was the original behaviour in Solidus < 2.1, but the active scope got lost in solidusio#1540. Payment methods used to use the `available` filter which was scoped to active payment methods only. The `available_to_admin` scope isn't limited active payment methods, so we need to add it here.

gevann force-pushed the payment-methods-display-on-boolean branch from 38ce73d to 5ac4ba6 Compare October 19, 2016 17:11

jhawthorn reviewed Oct 19, 2016

View reviewed changes

jhawthorn suggested changes Oct 19, 2016

View reviewed changes

jhawthorn reviewed Oct 19, 2016

View reviewed changes

gevann force-pushed the payment-methods-display-on-boolean branch from 5ac4ba6 to aeb6857 Compare October 20, 2016 00:33

cbrunsdon suggested changes Nov 2, 2016

View reviewed changes

gevann force-pushed the payment-methods-display-on-boolean branch from aeb6857 to eb57967 Compare November 2, 2016 23:45

jhawthorn approved these changes Nov 7, 2016

View reviewed changes

jhawthorn added the Code review needed label Nov 7, 2016

jhawthorn mentioned this pull request Nov 7, 2016

RFC: Split Spree::PaymentMethod.display_on to two boolean columns #1532

Closed

gevann force-pushed the payment-methods-display-on-boolean branch from eb57967 to eb4b1d7 Compare November 8, 2016 00:42

Graeme Nathan added 7 commits November 7, 2016 16:58

Add migration for payment methods

7d96091

PaymentMethod.display_on column is split in to two boolean columns available_to_users, and available_to_admin. Each value is set based on the value in the display_on column. Finally, the display_on column is removed.

Add scopes for availability

ab18c6e

Update core order model #available_payment_methods

6077305

Update factories/specs with available_to_X

5b01004

Adjust Spree::Payment method creation in specs to use new boolean columns (available_to_users, available_to_admin) rather than string column 'display_on' Update specs and factories to use new columns

Use .available_to_admin in data loading

362d542

Update views to use new columns

48d231e

Update api controllers for new columns

fa94154

gevann force-pushed the payment-methods-display-on-boolean branch from eb4b1d7 to fa94154 Compare November 8, 2016 00:59

cbrunsdon approved these changes Nov 8, 2016

View reviewed changes

Add deprecated version of display_on

bd4e841

This allows any existing code which used display_on to continue working. It's likely that specs will be using this.

jhawthorn merged commit d56a442 into solidusio:master Nov 15, 2016

jhawthorn mentioned this pull request Nov 24, 2016

Replace ShippingMethod#display_on with ShippingMethod#available_to_users #1611

Merged

gevann mentioned this pull request Nov 30, 2016

Create migration #1626

Closed

luukveenis mentioned this pull request Mar 27, 2017

Don't display inactive payment methods on frontend or backend #1801

Merged

luukveenis mentioned this pull request Mar 27, 2017

Use order instead of sort_by when getting available payment methods #1802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Payment methods display on boolean #1540

Payment methods display on boolean #1540

gevann commented Oct 19, 2016 •

edited

Loading

jhawthorn Oct 19, 2016

gevann Oct 19, 2016

jhawthorn Oct 19, 2016

gevann Oct 19, 2016

jhawthorn Oct 19, 2016

jhawthorn Oct 19, 2016

gevann Oct 19, 2016

jhawthorn Oct 19, 2016

jhawthorn Oct 19, 2016

gevann Oct 19, 2016

jhawthorn Oct 19, 2016

cbrunsdon Nov 2, 2016

peterberkenbosch commented Nov 8, 2016

peterberkenbosch commented Nov 8, 2016

Payment methods display on boolean #1540

Payment methods display on boolean #1540

Conversation

gevann commented Oct 19, 2016 • edited Loading

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

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

peterberkenbosch commented Nov 8, 2016

peterberkenbosch commented Nov 8, 2016

gevann commented Oct 19, 2016 •

edited

Loading