Add make_default method on AddPaymentSourcesToWallet class #2913

vassalloandrea · 2018-10-12T15:52:59Z

Update:

After some consideration with @tvdeyen

With this PR we want to separate the add_to_wallet behavior from the make_default one to help if someone wants to change only the make_default logic without overriding the add_to_wallet.

How it works

Now the add_to_wallet method calls the make_default method after adding on the user wallet the payment sources.
To change the make_default behavior now we can create own class and override the make_default method.

E.g.: #2913 (comment)

tvdeyen

I don’t think we should do this. Storing data on behalf of the user is a decision the user needs to do not the store owner.

Instead we should add a checkbox to the confirm page that asks the user if we should store the payment method and don’t do it if not. Currently we always store the payment method, what should not be the case in times of GDPR and friends.

kennyadsl · 2018-10-12T19:18:43Z

@tvdeyen I think you are right but we are already storing the credit card in the database, this just changes which one is considered the default one.

I think it does not change which cards are being shown to the user on the next purchase but the last one used will be preselected I think, while at the moment the preselected one is the first added. Right @vassalloandrea ?

UPDATE: it's the other way around (now you can choose to select the last one as default or it will always use the first one) but they are both saved in the database anyway.

tvdeyen · 2018-10-12T23:10:28Z

I think you are right but we are already storing the credit card in the database

We store the payment source used on the payment, yes, but also we store it as default payment source in the users wallet. And this should not be the case IMO unless the user wants to. They should opt-in for this.

Also the additional configuration option in this PR seems to be very store specific and should not be in core IMO. What could be in core is a user account page that let the use decide which payment source that they decided to be stored is their default one. Having the store deciding this is wrong IMO.

kennyadsl · 2018-10-15T14:50:18Z

Also the additional configuration option in this PR seems to be very store specific and should not be in core IMO. What could be in core is a user account page that let the use decide which payment source that they decided to be stored is their default one. Having the store deciding this is wrong IMO.

👍

What about a class that each store can customize? Like:

Spree::Config::Wallet.default_payment_method_setter_class

which by default acts exactly as the one we have in core right now?

tvdeyen · 2018-10-15T16:01:38Z

What about a class that each store can customize?

Why not replace add_payment_sources_to_wallet_class with your custom code in your stores config?

Maybe it makes sense to extract the "make payment method the default one" into a method, that a store then can override?

Spree.config do |config|
  ...
  config.add_payment_sources_to_wallet_class = 'My::AddPaymentSourcesToWallet'
  ...
end

class My::AddPaymentSourcesToWallet < Spree::Wallet::AddPaymentSourcesToWallet
  def make_default
    order.user.wallet.default_wallet_payment_source = wallet_payment_sources.first
  end
end

kennyadsl · 2018-10-18T20:26:20Z

I think this is a great solution @tvdeyen, thanks!

core/spec/models/spree/wallet/add_payment_sources_to_wallet_spec.rb

jacobherrington

I want to wait on @tvdeyen's feedback, but I'm okay with this

tvdeyen

Nice. Thanks. Two suggestions.

core/spec/models/spree/wallet/add_payment_sources_to_wallet_spec.rb

core/app/models/spree/wallet/add_payment_sources_to_wallet.rb

vassalloandrea · 2019-06-14T14:56:05Z

Hey @tvdeyen, I made the changes 👍

tvdeyen

Thanks.

@kennyadsl I actually lost track of this one. Where are we with the feature to ask the user if they actually want to store the payment source in the wallet? This is a feature we should provide (especially in times of GDPR)

core/spec/models/spree/wallet/add_payment_sources_to_wallet_spec.rb

aldesantis · 2020-02-03T16:54:39Z

@vassalloandrea there's something off with the specs. Could you take a look?

Move the logic to make the last payment source as default outside the add_to_wallet method to take advantage if someone wants to change only the make default behavior.

aldesantis · 2020-02-07T10:57:14Z

Thanks @vassalloandrea!

Fixes issue solidusio#4004 [1] PR solidusio#2913 [2] changed how the new default WalletPaymentSource was set, from using the last one found/created * within the Order's PaymentSources to the user's last WalletPaymentSource. A subtle difference which assumes that the user's last WalletPaymentSource was just created by add_to_wallet. However, if the Order's PaymentSource was from the User's wallet default and add_to_wallet is called, a new WalletPaymentSource would not be created * and make_default would try to set the new default as the last WalletPaymentSource. This would change the default if the last WalletPaymentSource was not the default. Now, the WalletPaymentSources are scoped to the order's PaymentSources and the last one is given to Wallet#default_wallet_payment_source=. Meaning that in this scenario, the default would given and ignored as it is already the default. [1] solidusio#4004 [2] solidusio#2913 * Wallet#add creates or finds the WalletPaymentSource if exists.

PR solidusio#2913 [1] changed how the new default WalletPaymentSource was set. Before: - using the last one found/created by the Order's PaymentSources After: - using the user's last WalletPaymentSource. This is problematic in the scenario where the user uses their default WalletPaymentSource for an order, but their default source is not their most recent WalletPaymentSource. Fixes issue solidusio#4004 [2] Now, #make_default uses WalletPaymentSources found or created by the order, it means it will keep the default payment source if used. This is because wallet#add finds or creates. [1] solidusio#2913 [2] solidusio#4004

PR solidusio#2913 [1] changed how the new default WalletPaymentSource was set. Before: - using the last one found/created by the Order's PaymentSources After: - using the user's last WalletPaymentSource. This is problematic when an order uses a default WalletPaymentSource but it's not the most recent one. Fixes issue solidusio#4004 [2] Now, #make_default uses WalletPaymentSources found or created by the order, which means it will keep the default payment source if used. [1] solidusio#2913 [2] solidusio#4004

tvdeyen requested changes Oct 12, 2018

View reviewed changes

vassalloandrea force-pushed the last_payment_source_as_default_for_user_wallet branch from 774691f to 38b18f9 Compare December 12, 2018 16:00

vassalloandrea changed the title ~~Add configuration to settle the last payment source as default~~ Add make_default method on AddPaymentSourcesToWallet class Dec 12, 2018

vassalloandrea changed the title ~~Add make_default method on AddPaymentSourcesToWallet class~~ Add make_default method on AddPaymentSourcesToWallet class Dec 12, 2018

vassalloandrea mentioned this pull request Dec 12, 2018

Add make_default method on AddPaymentSourcesToWallet class nebulab/solidus#17

Merged

kennyadsl requested changes Dec 12, 2018

View reviewed changes

core/spec/models/spree/wallet/add_payment_sources_to_wallet_spec.rb Outdated Show resolved Hide resolved

vassalloandrea force-pushed the last_payment_source_as_default_for_user_wallet branch from 38b18f9 to 44e5e20 Compare December 13, 2018 15:48

kennyadsl approved these changes Jan 4, 2019

View reviewed changes

jacobherrington approved these changes Jan 5, 2019

View reviewed changes

tvdeyen reviewed Jan 7, 2019

View reviewed changes

jacobherrington added the Needs Work label Feb 1, 2019

vassalloandrea force-pushed the last_payment_source_as_default_for_user_wallet branch from 44e5e20 to 48dd2fe Compare June 14, 2019 14:06

tvdeyen approved these changes Jun 14, 2019

View reviewed changes

jacobherrington removed the Needs Work label Jul 20, 2019

aldesantis requested changes Jan 10, 2020

View reviewed changes

core/spec/models/spree/wallet/add_payment_sources_to_wallet_spec.rb Outdated Show resolved Hide resolved

vassalloandrea force-pushed the last_payment_source_as_default_for_user_wallet branch from 48dd2fe to 3f9a3ed Compare January 31, 2020 14:48

vassalloandrea requested a review from aldesantis January 31, 2020 15:48

Add make_default method on AddPaymentSourcesToWallet class

27fb811

Move the logic to make the last payment source as default outside the add_to_wallet method to take advantage if someone wants to change only the make default behavior.

vassalloandrea force-pushed the last_payment_source_as_default_for_user_wallet branch from 3f9a3ed to 27fb811 Compare February 7, 2020 10:08

aldesantis approved these changes Feb 7, 2020

View reviewed changes

aldesantis merged commit eadd9d8 into solidusio:master Feb 7, 2020

AlessioRocco deleted the last_payment_source_as_default_for_user_wallet branch February 13, 2020 16:21

adammathys mentioned this pull request Mar 22, 2021

AddPaymentSourcesToWallet "randomly" changing the default #4004

Closed

RyanofWoods mentioned this pull request Nov 1, 2021

Fix AddPaymentSourcesToWallet changing default when reused #4198

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add make_default method on AddPaymentSourcesToWallet class #2913

Add make_default method on AddPaymentSourcesToWallet class #2913

vassalloandrea commented Oct 12, 2018 •

edited

Loading

tvdeyen left a comment

kennyadsl commented Oct 12, 2018 •

edited

Loading

tvdeyen commented Oct 12, 2018

kennyadsl commented Oct 15, 2018

tvdeyen commented Oct 15, 2018

kennyadsl commented Oct 18, 2018

jacobherrington left a comment

tvdeyen left a comment

vassalloandrea commented Jun 14, 2019

tvdeyen left a comment

aldesantis commented Feb 3, 2020

aldesantis commented Feb 7, 2020

Add make_default method on AddPaymentSourcesToWallet class #2913

Add make_default method on AddPaymentSourcesToWallet class #2913

Conversation

vassalloandrea commented Oct 12, 2018 • edited Loading

Update:

After some consideration with @tvdeyen

How it works

tvdeyen left a comment

Choose a reason for hiding this comment

kennyadsl commented Oct 12, 2018 • edited Loading

tvdeyen commented Oct 12, 2018

kennyadsl commented Oct 15, 2018

tvdeyen commented Oct 15, 2018

kennyadsl commented Oct 18, 2018

jacobherrington left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

vassalloandrea commented Jun 14, 2019

tvdeyen left a comment

Choose a reason for hiding this comment

aldesantis commented Feb 3, 2020

aldesantis commented Feb 7, 2020

vassalloandrea commented Oct 12, 2018 •

edited

Loading

kennyadsl commented Oct 12, 2018 •

edited

Loading