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] https://github.com/solidusio/solidus/issues/4004 [2] https://github.com/solidusio/solidus/pull/2913/

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

Checklist:

• [x] I have followed Pull Request guidelines
• [x] I have added a detailed description into each commit message
• [x] I have updated Guides and README accordingly to this change (if needed)
• [x] I have added tests to cover this change (if needed)
• [x] 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 https://github.com/solidusio/solidus/issues/2652 and would automatically resolve https://github.com/solidusio/solidus/issues/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 :slightly_smiling_face: I'm happy to help here if it's needed.

[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.

[codecov[bot]]

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%)	:arrow_up:
backend/lib/spree/backend/callbacks.rb	93.33% <0.00%> (+46.66%)	:arrow_up:
backend/lib/spree/backend_configuration.rb	100.00% <0.00%> (+48.57%)	:arrow_up:
...controllers/spree/admin/option_types_controller.rb	60.00% <0.00%> (+60.00%)	:arrow_up:
...d/app/controllers/spree/admin/taxons_controller.rb	66.66% <0.00%> (+66.66%)	:arrow_up:
.../app/helpers/spree/admin/stock_movements_helper.rb	70.58% <0.00%> (+70.58%)	:arrow_up:
...s/spree/admin/promotion_code_batches_controller.rb	75.00% <0.00%> (+75.00%)	:arrow_up:
...ollers/spree/admin/promotion_actions_controller.rb	81.25% <0.00%> (+81.25%)	:arrow_up:
...d/app/controllers/spree/admin/stores_controller.rb	83.33% <0.00%> (+83.33%)	:arrow_up:
... and 54 more

:mega: 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%? 😅

[RyanofWoods]

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