Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Feb 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions config/initializers/spree.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@

require 'spree/product_filters'

# 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.
# https://github.com/openfoodfoundation/openfoodnetwork/issues/3121
Spree::Gateway.class_eval do
acts_as_taggable
attr_accessible :tag_list
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


require "#{Rails.root}/app/models/spree/payment_method_decorator"
require "#{Rails.root}/app/models/spree/gateway_decorator"

Expand Down
56 changes: 56 additions & 0 deletions spec/models/spree/gateway_tagging_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
require "spec_helper"

# We extended Spree::PaymentMethod to be taggable. Unfortunately, an inheritance
# bug prevented the taggable code to be passed on to the descendants of
# PaymentMethod. We fixed that in config/initializers/spree.rb.
#
# This spec tests several descendants for their taggability. The tests are in
# a separate file, because they cover one aspect of several classes.
shared_examples "taggable" do |expected_taggable_type|
it "provides a tag list" do
expect(subject.tag_list).to eq []
end

it "stores tags for the root taggable type" do
subject.tag_list.add("one")
subject.save!

expect(subject.taggings.last.taggable_type).to eq expected_taggable_type
end
end

module Spree
describe "PaymentMethod and descendants" do

let(:shop) { create(:enterprise) }
let(:valid_subject) do
# Supply required parameters so that it can be saved to attach taggings.
described_class.new(
name: "Some payment method",
distributor_ids: [shop.id]
)
end
subject { valid_subject }

describe PaymentMethod do
it_behaves_like "taggable", "Spree::PaymentMethod"
end

describe Gateway do
it_behaves_like "taggable", "Spree::PaymentMethod"
end

describe Gateway::PayPalExpress do
it_behaves_like "taggable", "Spree::PaymentMethod"
end

describe Gateway::StripeConnect do
subject do
# StripeConnect needs an owner to be valid.
valid_subject.tap { |m| m.preferred_enterprise_id = shop.id }
end

it_behaves_like "taggable", "Spree::PaymentMethod"
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same for the other models.