solidus

solidus copied to clipboard

This PR is still work in progress.

I'm currently building a shop with solidus and found some logic that seems unnecessary that originated from a 7 year old PR from before the fork: https://github.com/spree/spree/pull/5148

The current behaviour is this:

1. A user with an existing payment source transitions from delivery to payment state
2. Spree::Order#add_default_payment_from_wallet gets triggered by the state machine and creates a payment record.
3. User transitions to confirm state
4. Through how the checkout logic works with Spree::OrderUpdateAttributes, that default payment does not get reused and is always replaced with a new payment, resulting every time in an invalid payment record:

My expectations would be one of the following:

1. The default payment record has some kind of use (so far I haven't found one) and gets updated instead of invalidated (I'm also not sure how this affects fraud detection)
2. No default payment record gets created as it has no use

I started this PR by removing Spree::Order#add_default_payment_from_wallet to see what breaks. So far only the spec removed in this commit have been affected but it seems to me that these are directly tied to the creation of the default payment source and have no further use. The only spec I'm unsure about is the one about assigning a billing address if none exists yet, but I can't think of a scenario where this should be the case.

Another thing the removal of Spree::Order#add_default_payment_from_wallet would affect is that it makes Spree::Wallet::DefaultPaymentBuilder obsolete. So I guess there is a concern about breaking compatibility with existing shops that rely on that one. Although I'm not sure what a use case could be.

Checklist:

• [x] I have followed Pull Request guidelines
• [x] 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)

This is related to #2680.

@adammathys makes sense. If there are no other concerns, I'll gladly put something together.

Thanks @danielpuglisi (and @adammathys), that would be great!

Finally got the time to take a jab at this.

Added the deprecation preference and warnings based on @adammathys suggestion. The current default preference although creates a lot of noise when running the specs. What is the preferred way to handle this?

We appreciate your contribution, @danielpuglisi. After talking with @kennyadsl, we decided to put it on hold for the meantime. It looks like a legit change, although it's entirely possible it'd break some extensions. We feel we need to define a clear path for the evolution of the payment system before introducing more changes.

I understand. Let me know if I can be of any help in the future.

Hey @danielpuglisi, first of all thanks for the PR and sorry for taking so long to come back on this. We'd love to move this forward because this is annoying on all levels (Admin UX and for the developer).

We have a couple of things left to do before we can merge this:

1. Rebase the PR against master
2. Change the preference usage to use the new versioned preferences (here you can find a usage example).
3. run specs of a couple of payment gateway extensions against the local version of solidus that contains this change. I suggest to try with solidus_stripe and solidus_paypal_braintree, which are the most used.

Please let us know if you have time to take care of these last things otherwise, we will happily take over this work as soon as we have some availability.

Thanks again! 🙏

Hey @kennyadsl. I'm a bit flooded right now. Feel free to take over if you get the time and I haven't done anything.