Skip to content
This repository was archived by the owner on Apr 14, 2023. It is now read-only.

Allows the paypal_braintree source to be added to the wallet on checkout #123

Merged
merged 4 commits into from
Oct 20, 2017

Conversation

joeljackson
Copy link
Contributor

No description provided.

@joeljackson
Copy link
Contributor Author

Looking for some feedback here. This is intended to address #122.

Wanted to ensure a source is always reusable and that making it a subclass of Spree::PaymentSource wasn't going to ruin anyone's day before proceeding.

@swcraig
Copy link

swcraig commented Sep 2, 2017

Although it would make sense, I don't think that subclassing Spree::PaymentSource is the way to go here. Spree::PaymentSource was only introduced in Solidus v2.2, so subclassing it will break this extension with all versions of Solidus before that.

I believe the other two changes are what is needed to get this source added to the wallet.

@tvdeyen
Copy link
Member

tvdeyen commented Sep 2, 2017

Unfortunately it’s needed to subclass from PaymentSource because Solidus introduced a validation on the payment source class in https://github.com/solidusio/solidus/blob/292e7acea02803560f69a4720417f5b6d62a5808/core/app/models/spree/wallet_payment_source.rb#L13.

We either should remove this validation (as this is useless IMO) or need to change this code so it only subclasses on Solidus 2.2 and above.

@joeljackson
Copy link
Contributor Author

Seems like solidusio/solidus#2201 should allow us to not subclass the PaymentSource. Will make appropriate changes.

@joeljackson
Copy link
Contributor Author

Notably thinking about this it needs to be a subclass of PaymentSource on Solidus 2.3 or this wont

@joeljackson joeljackson closed this Sep 8, 2017
@joeljackson joeljackson reopened this Sep 8, 2017
Copy link

@jordan-brough jordan-brough left a comment

Choose a reason for hiding this comment

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

I think this is the right direction to go and I think we should roll back solidusio/solidus#2201.

Please see my comments for a way to change this PR such that it should be compatible with all versions of Solidus.

@@ -1,12 +1,13 @@
module SolidusPaypalBraintree
class Source < ApplicationRecord
class Source < Spree::PaymentSource
Copy link

@jordan-brough jordan-brough Sep 15, 2017

Choose a reason for hiding this comment

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

Can you change this to SolidusSupport.payment_source_parent_class please?
See solidusio/solidus_support#1

You'll also want to ensure you require at least version 0.1.2 of solidus_support in the gemspec for this gem.

PAYPAL = "PayPalAccount"
APPLE_PAY = "ApplePayCard"
CREDIT_CARD = "CreditCard"

belongs_to :user, class_name: Spree::UserClassHandle.new
belongs_to :payment_method, class_name: 'Spree::PaymentMethod'
has_many :payments, as: :source, class_name: "Spree::Payment"
has_many :wallet_payment_sources, class_name: 'Spree::WalletPaymentSource', as: :payment_source, inverse_of: :payment_source

Choose a reason for hiding this comment

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

You shouldn't need this if you inherit from SolidusSupport.payment_source_parent_class

@jordan-brough
Copy link

@joeljackson can you please also update the gemspec to require at least version 0.1.2 of solidus_support?

@joeljackson
Copy link
Contributor Author

@jordan-brough Done. Thanks 👍

@jordan-brough
Copy link

Thanks @joeljackson. Lgtm. Seeing if we can figure out what's wrong w/ the spec failures.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

👍 the spec errors are due to the unstable test suite we had. I fixed a lot of this in master. Merging as this change is good

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants