Fix AddPaymentSourcesToWallet changing default when reused

[RyanofWoods]

Fixes issue #4004 [1]

PR #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 be given and ignored as it
is already the default.

[1] #4004
[2] #2913

• Wallet#add creates or finds the WalletPaymentSource if exists.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[waiting-for-dev]

I might be a bit lost here. But isn't it effectively reverting #2913? Wouldn't #2652 still be an issue in this case?

I haven't studied it closely enough, but I guess that ideally, #add_to_wallet shouldn't try to set the default unless:

• Only one source exists
• The user checks a "set default" checkbox (so we'd need to push that data from the controller layer down to this method).

[RyanofWoods]

I might be a bit lost here. But isn't it effectively reverting #2913? Wouldn't #2652 still be an issue in this case?

I haven't studied it closely enough, but I guess that ideally, #add_to_wallet shouldn't try to set the default unless:

• Only one source exists
• The user checks a "set default" checkbox (so we'd need to push that data from the controller layer down to this method).

Thank you for raising this to my attention. Thinking about it more, I think you are correct. 👍 #add_to_wallet shouldn't set the default unless the user gives input to do so. If I change the PR to do this, then it can be to resolve #2652 and would automatically resolve #4004.

I think perhaps it would be more appropriate for Spree::Wallet to set default the wallet_payment_source if it is the only one. Then it can be done as callbacks for adding or removing a wallet_payment_source, what do you think?

[waiting-for-dev]

I think perhaps it would be more appropriate for Spree::Wallet to set default the wallet_payment_source if it is the only one. Then it can be done as callbacks for adding or removing a wallet_payment_source, what do you think?

Do you mean Active Record callbacks? I'd strongly recommend against using them. Coupling business logic with persistence is an endless source of problems. My feeling is that we're trying to move away from them in Solidus.

I'm not very familiar with that part of the system, so at first sight, I'd leave that piece of logic there if there's nowhere more appropriate. About making that method to know if a user has checked something, I have no idea how hard it'll be 🙂 I'm happy to help here if it's needed.

[adammathys]

I agree with @waiting-for-dev on almost all points here. Ideally we're only setting a default source if there isn't one or if the user prompts to have it changed. Also, I think the logic for setting the initial default fits fairly well within the AddPaymentSourcesToWallet class instead of trying to add it to callbacks. e.g.:

unless order.user.wallet.default_wallet_payment_source
  order.user.wallet.default_wallet_payment_source = new_default_source
end

That being said, I don't think this PR as is reverts #2913. The main purpose of that change was to make it easier to override the make_default behaviour so that users could decide to opt out of the default behaviour of having the most recently used credit card becoming the default. (I think the switch to have it be "most recently created" instead of "most recently used" was an unintended side-effect of that change.)

[waiting-for-dev]

It makes sense. Another point to consider, as @kennyadsl commented offline, is payments made offsite, where users can't select a default payment method. We definitely should give more thought and study a suitable architecture for a broad solution going beyond the scope of this PR.

[waiting-for-dev]

I think we can merge it. It's not the complete solution, but at least it's fixing the issue introduced in #2913.

@RyanofWoods, I left some suggestions that, when combined, get rid of the unneeded global state and make use of a local variable instead.

[kennyadsl]

@RyanofWoods hey, thanks a lot for this. I think it's close to something we can merge if you can address comments made by @waiting-for-dev and myself. 🙏

[codecov]

Codecov Report

Merging #4198 (88aed7e) into master (d7ef0f2) will increase coverage by 15.61%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #4198       +/-   ##
===========================================
+ Coverage   71.08%   86.69%   +15.61%     
===========================================
  Files         578      578               
  Lines       15611    14673      -938     
===========================================
+ Hits        11097    12721     +1624     
+ Misses       4514     1952     -2562     

Impacted Files	Coverage Δ
...dels/spree/wallet/add_payment_sources_to_wallet.rb	100.00% <100.00%> (ø)
backend/lib/spree/backend/action_callbacks.rb	100.00% <0.00%> (+40.00%)	⬆️
backend/lib/spree/backend/callbacks.rb	93.33% <0.00%> (+46.66%)	⬆️
backend/lib/spree/backend_configuration.rb	100.00% <0.00%> (+48.57%)	⬆️
...controllers/spree/admin/option_types_controller.rb	60.00% <0.00%> (+60.00%)	⬆️
...d/app/controllers/spree/admin/taxons_controller.rb	66.66% <0.00%> (+66.66%)	⬆️
.../app/helpers/spree/admin/stock_movements_helper.rb	70.58% <0.00%> (+70.58%)	⬆️
...s/spree/admin/promotion_code_batches_controller.rb	75.00% <0.00%> (+75.00%)	⬆️
...ollers/spree/admin/promotion_actions_controller.rb	81.25% <0.00%> (+81.25%)	⬆️
...d/app/controllers/spree/admin/stores_controller.rb	83.33% <0.00%> (+83.33%)	⬆️
... and 54 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[RyanofWoods]

I don't see how my change is going to decrease coverage by 15%? 😅

[waiting-for-dev]

Thanks, @RyanofWoods! I left a question about the test, but it looks good otherwise. I rerun the build, and the issue about the coverage is gone 🙂

[kennyadsl]

Found a typo in the test context. Also, there's a hanging discussion with @waiting-for-dev, is that solved?

[RyanofWoods]

Resolved both points, @waiting-for-dev and @kennyadsl, thanks!

[kennyadsl]

Thanks @RyanofWoods!