[Spree upgrade] Workaround Rails inheritance bug in Spree::Gateway #3370

mkllnk · 2019-01-23T06:58:04Z

What? Why?

Due to a bug in ActiveRecord we need to load the tagging code in Gateway which should have inherited it from its parent PaymentMethod. We have to call it before loading the PaymentMethod decorator because the tagging code won't load twice within the inheritance chain.

I tested what happens when you assign tags to a PayPalExpress gateway and they are correctly stored with :taggable_type => "Spree::PaymentMethod".

I also considered a tagging implementation that doesn't require decorating, but we would be working against the used gem which assumes to be called on the tagged ActiveRecord model. It may still be an option, but needs deeper study of the acts_as_taggable_on code.

What should we test?

If we test this: Create a PayPal payment method from the admin interface.

Please note: the affected spec/features/admin/payment_method_spec.rb:72 executes further, but does not pass. The following failure means that this pull request is successful:

     Failure/Error: expect(page).to have_field 'Password', with: ''
       expected to find visible field "Password" that is not disabled with value "" but there were no matches. Also found "", which matched the selector but not all filters.
     # ./spec/features/admin/payment_method_spec.rb:108:in `block (2 levels) in <top (required)>'

How is this related to the Spree upgrade?

This is part of the Spree upgrade.

luisramos0

Awesome work!!!
If the spec is not green yet, maybe we leave #3121 open to cover the task of fixing the spec?

mkllnk · 2019-01-23T23:52:25Z

I created a new issue #3379. The old one became a bit long and the agenda changed already.

mkllnk · 2019-01-23T23:56:05Z

Semaphore failed at another point in the spec:

     Failure/Error: expect(first('tags-input .tag-list ti-tag-item')).to have_content "member"
       expected to find text "member" in ""
     # ./spec/features/admin/payment_method_spec.rb:94:in `block (2 levels) in <top (required)>'

That has to do with tags, so I should double-check my PR.

mkllnk · 2019-01-24T02:33:34Z

Ah, I needed to include attr_accessible :tag_list as well. I'm still wondering if there is a better solution to implement tags without modifying Spree models...

luisramos0

ah, yes, there were two attributes.

luisramos0 · 2019-01-25T21:05:00Z

this PR fixes the same problem in 3 specs in spec/features/admin/subscriptions_spec.rb
👍

kristinalim

Nice investigation, @mkllnk!

I tested what happens when you assign tags to a PayPalExpress gateway and they are correctly stored with :taggable_type => "Spree::PaymentMethod".

Could you cover this with a test, so we will find out if this changes in future implementations or upgrade-related work?

sauloperez · 2019-02-06T20:12:31Z

spec/models/spree/gateway_tagging_spec.rb

+
+module Spree
+  describe PaymentMethod do
+    it_behaves_like "taggable"


why not moving this to the payment_method_spec.rb? That is where I'd go check that if I had to.

This spec is actually not about the PaymentMethod but about its descendants. Since I'm testing several classes for the same thing I do it in one place instead of scattering these tests in the spec files of every descendant. We are actually testing Ruby's inheritance here which doesn't belong to one specific class.

I added more comments to explain this so that the next developer doesn't have the same question. Thank you for prompting. 👍 :-)

sauloperez · 2019-02-06T20:12:59Z

spec/models/spree/gateway_tagging_spec.rb

+  describe Gateway::StripeConnect do
+    it_behaves_like "taggable"
+  end
+end


The same for the other models.

sauloperez · 2019-02-06T20:15:37Z

config/initializers/spree.rb

+Spree::Gateway.class_eval do
+  acts_as_taggable
+  attr_accessible :tag_list
+end


but if essentially this is a order-related bug doesn't a require 'spree/payment_method' in the appropriate place helps us here? apart from that, from rails/rails#3847 looks like some gem could be messing things up as well.

Yes, it is order-related. Yes, it could be a gem. It could even be Spree.

A require 'spree/payment_method' doesn't help, because the inheritance already happened when we access the Spree code. I tried that in many places, for hours, after Luis tried that in many places, for hours. You can have another go and maybe you can find a better solution. But we spent several dev days on this already and couldn't find any better solution.

Since this bug is hopefully fixed in the next version of Rails we shouldn't invest much more time.

One experiment I would support as a tech-debt task: Instead of adding tags to Spree::PaymentMethod, we could try to have a new class called TaggablePaymentMethod which basically decorates any other payment method. All code using tagging interface would use that class instead of using the payment method instance directly.

What do you think? Should I create an issue for that?

It's fine. It doesn't make sense to spend any more time on this. I wouldn't add the tech debt task either for now.

Due to a bug in ActiveRecord we need to load the tagging code in Gateway which should have inherited it from its parent PaymentMethod. We have to call it before loading the PaymentMethod decorator because the tagging code won't load twice within the inheritance chain. #3121

mkllnk · 2019-02-13T05:28:32Z

@kristinalim, thank you for nudging me to increase the test coverage. I hesitated because it makes everything more complicated. It actually took me several hours to write a spec I'm content with. The difficult trade-off was between a flexible shared example that works with any payment method and using our factories. Let me know what you think about the new commit.

luisramos0

awesome @mkllnk, lets move on :-)

sauloperez · 2019-02-13T11:41:45Z

Waiting for @kristinalim 's answer to merge this.

kristinalim

I think this is great. 🙂

luisramos0 · 2019-02-13T12:28:09Z

🎉

mkllnk self-assigned this Jan 23, 2019

luisramos0 approved these changes Jan 23, 2019

View reviewed changes

mkllnk mentioned this pull request Jan 24, 2019

Use patch to hide Paypal gateway password #3380

Merged

luisramos0 approved these changes Jan 24, 2019

View reviewed changes

luisramos0 mentioned this pull request Jan 25, 2019

[Spree Upgrade] Remove allow backorders from subscriptions specs #3397

Merged

mkllnk requested a review from sauloperez February 5, 2019 04:18

kristinalim self-requested a review February 6, 2019 16:21

kristinalim suggested changes Feb 6, 2019

View reviewed changes

sauloperez suggested changes Feb 6, 2019

View reviewed changes

Test tagging polymorphism on a payment method

3b9fdfa

luisramos0 approved these changes Feb 13, 2019

View reviewed changes

sauloperez approved these changes Feb 13, 2019

View reviewed changes

kristinalim approved these changes Feb 13, 2019

View reviewed changes

luisramos0 merged commit 576c203 into openfoodfoundation:2-0-stable Feb 13, 2019

mkllnk deleted the 3121-spree-gateway-inheritance branch February 18, 2019 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spree upgrade] Workaround Rails inheritance bug in Spree::Gateway #3370

[Spree upgrade] Workaround Rails inheritance bug in Spree::Gateway #3370

mkllnk commented Jan 23, 2019

luisramos0 left a comment

mkllnk commented Jan 23, 2019

mkllnk commented Jan 23, 2019

mkllnk commented Jan 24, 2019

luisramos0 left a comment

luisramos0 commented Jan 25, 2019

kristinalim left a comment

sauloperez Feb 6, 2019

mkllnk Feb 13, 2019

mkllnk Feb 13, 2019

sauloperez Feb 6, 2019

sauloperez Feb 6, 2019

mkllnk Feb 13, 2019

sauloperez Feb 13, 2019

mkllnk commented Feb 13, 2019

luisramos0 left a comment

sauloperez commented Feb 13, 2019

kristinalim left a comment

luisramos0 commented Feb 13, 2019

[Spree upgrade] Workaround Rails inheritance bug in Spree::Gateway #3370

[Spree upgrade] Workaround Rails inheritance bug in Spree::Gateway #3370

Conversation

mkllnk commented Jan 23, 2019

What? Why?

What should we test?

How is this related to the Spree upgrade?

luisramos0 left a comment

Choose a reason for hiding this comment

mkllnk commented Jan 23, 2019

mkllnk commented Jan 23, 2019

mkllnk commented Jan 24, 2019

luisramos0 left a comment

Choose a reason for hiding this comment

luisramos0 commented Jan 25, 2019

kristinalim left a comment

Choose a reason for hiding this comment

sauloperez Feb 6, 2019

Choose a reason for hiding this comment

mkllnk Feb 13, 2019

Choose a reason for hiding this comment

mkllnk Feb 13, 2019

Choose a reason for hiding this comment

sauloperez Feb 6, 2019

Choose a reason for hiding this comment

sauloperez Feb 6, 2019

Choose a reason for hiding this comment

mkllnk Feb 13, 2019

Choose a reason for hiding this comment

sauloperez Feb 13, 2019

Choose a reason for hiding this comment

mkllnk commented Feb 13, 2019

luisramos0 left a comment

Choose a reason for hiding this comment

sauloperez commented Feb 13, 2019

kristinalim left a comment

Choose a reason for hiding this comment

luisramos0 commented Feb 13, 2019