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

luukveenis · 2017-03-27T16:48:49Z

The issue

After upgrading to Solidus 2.1, I noticed that my inactive payment methods were being displayed on the payment page in checkout and the payments#new page in the admin. The issue was introduced in #1540 because the Spree::PaymentMethod.available scope used to filter payment methods by active, but the new available_to_users and available_to_admin scopes only look at their respective attributes on the payment method. The Spree::Order#available_payment_methods method wasn't updated to include the active scope accordingly. Similarly, the load_data method in the admin payment methods controller is missing the active scope.

While writing the failing spec for this, I noticed that the available_to_store scope was not behaving as expected. It returned self if the store did not have any payment methods, which presumably was expected to return the current scope, but actually returns the Spree::PaymentMethod class constant. This wasn't breaking specs because we chained other scopes that would return payment methods as expected.

The fix

I've added the active scope to #available_payment_methods and in #load_data to ensure we don't include inactive payment methods.

I also updated the available_to_store scope to raise an ArgumentError if the store provided is nil, and return all payment methods in the current scope if the store doesn't have any payment methods. I added specs that should catch this issue in the future.

houndci-bot · 2017-03-27T16:49:03Z

core/app/models/spree/payment_method.rb

-    scope :available_to_store, -> (store) { (store.present? && store.payment_methods.empty?) ? self : store.payment_methods }
+    scope :available_to_store, -> (store) do
+      raise ArgumentError, "You must provide a store" if store.nil?
+      (store.payment_methods.empty?) ? all : where(id: store.payment_method_ids)


Don't use parentheses around a method call.
Omit parentheses for ternary conditions.

houndci-bot · 2017-03-27T16:49:03Z

core/app/models/spree/payment_method.rb

@@ -17,7 +17,10 @@ class PaymentMethod < Spree::Base
    scope :active, -> { where(active: true) }
    scope :available_to_users, -> { where(available_to_users: true) }
    scope :available_to_admin, -> { where(available_to_admin: true) }
-    scope :available_to_store, -> (store) { (store.present? && store.payment_methods.empty?) ? self : store.payment_methods }
+    scope :available_to_store, -> (store) do


Do not use spaces between -> and opening brace in lambda literals

mtomov · 2017-03-27T17:10:44Z

core/app/models/spree/payment_method.rb

+    scope :available_to_store, ->(store) do
+      raise ArgumentError, "You must provide a store" if store.nil?
+      store.payment_methods.empty? ? all : where(id: store.payment_method_ids)
+    end


The whole method is not very clearly written. It's sort of all in-reverse. I think it should be just:

scope :available_to_store, -> (store) { where(store: store) }

That should also work if store is nil.

The raise ArgumentError is not much improvement over the Undefined method payment_methods on Nil:Class that one would usually get.

Thanks for the feedback!

Unfortunately where(store: store) doesn't work, because the payment method <-> store association is a has_many :through.

The reason for the conditional is to keep the existing, or at least expected, behaviour where if the store doesn't have any payment methods, we return all of them instead of an empty collection.

I agree the ArgumentError is not a drastic improvement. I discussed this with @jhawthorn on Friday and would be okay just letting it error the usual way too.

That won't quite work due to the nature of the relationship between stores and payment methods. It could be writen as:

scope :available_to_store, -> (store) { joins(:stores).merge(Spree::Store.where(id: store.id)) }

However, this would mean a divergence from how we typically handle store associations. Normally, if a store doesn't have anything associated to it we assume everything belongs to that store. I believe this decision was made a while back to help handle single store apps by ensuring everything continued to work without having to create the missing associations.

While I'm definitely not opposed to changing that behavior, it would require a larger amount of work and a conscious decision to have all single store apps require some additional configuration behind the scenes.

Absolutely you are both right, I did not see it through, and I am not that familiar with that part of the code. Thanks for the explanation!

jhawthorn

Thanks Luuk

jhawthorn · 2017-03-27T18:31:31Z

This was a 2.1 regression, and I think we should backport it for a 2.1.1 release.

jordan-brough

Nice find, thanks for the fixing this and writing this up!

One nitpick request for removing the nil check but very 👍 other than that.

jordan-brough · 2017-04-10T18:56:08Z

core/app/models/spree/payment_method.rb

@@ -17,7 +17,10 @@ class PaymentMethod < Spree::Base
    scope :active, -> { where(active: true) }
    scope :available_to_users, -> { where(available_to_users: true) }
    scope :available_to_admin, -> { where(available_to_admin: true) }
-    scope :available_to_store, -> (store) { (store.present? && store.payment_methods.empty?) ? self : store.payment_methods }
+    scope :available_to_store, ->(store) do
+      raise ArgumentError, "You must provide a store" if store.nil?


I agree the ArgumentError is not a drastic improvement. I discussed this with @jhawthorn on Friday and would be okay just letting it error the usual way too.

^ I agree with removing the nil check. The prior code didn't actually protect against nils and as @mtomov mentioned it doesn't really add much and we don't do it in most places

jordan-brough

Changing my "Changes Requested" to "Approved" because the nil check isn't a blocker IMO. Personally I'd prefer to see it removed though.

mamhoff

Thanks @luukveenis for doing this. I'd like to have pending calls on the commits that introduce failing specs, and their respective removals in the commits that fix them. Otherwise, great! :)

mamhoff · 2017-04-10T19:16:39Z

core/spec/models/spree/order_spec.rb

@@ -538,6 +538,16 @@ def merge!(other_order, user = nil)
      expect(order.available_payment_methods).to include(payment_method)
    end

+    it "does not include inactive payment methods" do


This spec surely wants to have a pending call to make sure this commit doesn't break the suite, no?

mamhoff · 2017-04-10T19:18:34Z

backend/spec/controllers/spree/admin/payments_controller_spec.rb

+              @payment_method = create(:check_payment_method, available_to_admin: true)
+            end
+
+            it "loads the payment method" do


Can we have pending calls for both failing specs in this commit?

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.

I've rebased to add :pending to the failing specs (and removed when fixed)

houndci-bot reviewed Mar 27, 2017

View reviewed changes

luukveenis force-pushed the hide-inactive-payment-methods branch from 5eee81d to f400308 Compare March 27, 2017 16:53

mtomov reviewed Mar 27, 2017

View reviewed changes

jhawthorn approved these changes Mar 27, 2017

View reviewed changes

jhawthorn added the type:bug Error, flaw or fault label Mar 27, 2017

jhawthorn added the Code review needed label Mar 27, 2017

jhawthorn added this to the 2.2.0 milestone Apr 5, 2017

luukveenis changed the title ~~Don't include inactive payment methods in Spree::Order#available_payment_methods~~ Don't display inactive payment methods on frontend or backend Apr 7, 2017

luukveenis mentioned this pull request Apr 7, 2017

Only load active payment methods in the backend #1834

Closed

jordan-brough suggested changes Apr 10, 2017

View reviewed changes

jordan-brough approved these changes Apr 10, 2017

View reviewed changes

mamhoff previously requested changes Apr 10, 2017

View reviewed changes

Luuk Veenis added 4 commits April 10, 2017 12:37

Add failing spec for inactive payment methods

0296ebd

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.

Add failing spec for admin payment methods

fad9cbf

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.

jhawthorn force-pushed the hide-inactive-payment-methods branch from 77fc23f to 34a50ad Compare April 10, 2017 19:54

jhawthorn merged commit d6a715a into solidusio:master Apr 10, 2017

dependabot bot mentioned this pull request Aug 20, 2019

Bump nokogiri from 1.8.5 to 1.10.4 in /guides #3306

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

luukveenis commented Mar 27, 2017 •

edited

Loading

houndci-bot Mar 27, 2017

houndci-bot Mar 27, 2017

mtomov Mar 27, 2017 •

edited

Loading

luukveenis Mar 27, 2017

adammathys Mar 27, 2017

mtomov Mar 27, 2017

jhawthorn left a comment

jhawthorn commented Mar 27, 2017

jordan-brough left a comment

jordan-brough Apr 10, 2017

jordan-brough left a comment •

edited

Loading

mamhoff left a comment

mamhoff Apr 10, 2017

mamhoff Apr 10, 2017

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

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

Conversation

luukveenis commented Mar 27, 2017 • edited Loading

The issue

The fix

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtomov Mar 27, 2017 • 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

jhawthorn left a comment

Choose a reason for hiding this comment

jhawthorn commented Mar 27, 2017

jordan-brough left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordan-brough left a comment • edited Loading

Choose a reason for hiding this comment

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

luukveenis commented Mar 27, 2017 •

edited

Loading

mtomov Mar 27, 2017 •

edited

Loading

jordan-brough left a comment •

edited

Loading