Remove split checkout toggle and legacy checkout

[abdellani]

What? Why?

• Closes Remove split_checkout toggle and legacy checkout #10636

Removes split_checkout feature toggle and updates various parts of the test suite that were not correctly using the new checkout and order flow logic.

Useful references:
#8810

What should we test?

99% of these changes are removing old checkout code split_checkout feature toggles and updating parts of the test suite that were not using the new checkout logic. Aside from that there's a couple of small changes here to actual application code in 32b8d72 and 4a8a146 which are effectively fixing bugs that are already in production but were not being picked up in the test suite...

I've pulled one of those changes out into a separate PR here: #10933 which could be tested separately (if that's easier). It affects marking the order as complete after processing stripe payments that require additional authorization (3DS).

The other change mostly affects Paypal redirects after a payment has failed for some reason at the checkout (like insufficient funds). It should now redirect to the checkout with the correct error message (currently I think it throws a 404/not found).

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

[abdellani]

rspec ./spec/services/process_payment_intent_spec.rb:110
rspec ./spec/services/process_payment_intent_spec.rb:130

are failing because the expected order state is 'complete' and now, it's 'confimation'.
I tried to make a payment using x-4242, and it's working without any issues (obviously because the confirm code doesn't use ProcessPaymentIntent).
PaymentGateways::StripeController#authorize, I need to check where in the UI, this endpoint is used.

rspec ./spec/controllers/payment_gateways/paypal_controller_spec.rb:91
rspec ./spec/controllers/payment_gateways/paypal_controller_spec.rb:102
are related to Paypal, I'll need to do some research to see how can I run Paypal in dev mode.

Multiple tests are failing due to this PR
rspec ./spec/system/consumer/shopping/cart_spec.rb:184
After , the checkout page doesn't show taxes if the split checkout is enabled.
There are other tests:
rspec ./spec/models/spree/adjustment_spec.rb:251
rspec ./spec/models/spree/adjustment_spec.rb:275
...
before continuing on the other tests, I prefer to get a feedback about create_tax_charge! because many tests depend on it.
@Matt-Yorkley can you please tell me what do you think?

[Matt-Yorkley]

Looks good so far, I think it's going in the right direction 👍

Yeah in this case I think the specs mostly need to be changed to reflect the what the code is doing and not the other way around. For example, taxes should not be applied before the order gets towards the end of the checkout process (payment state, I think) and there are some technical reasons for this (in some cases taxes need to be different depending on the user's address, but they haven't necessarily saved their address before the address state is passed on the /checkout/details step).

So those tests need updating a bit and I'd say that test at spec/system/consumer/shopping/cart_spec.rb:184 can just be deleted.

In those tax adjustment specs around spec/models/spree/adjustment_spec.rb:251 I think some of the test setup might need to be modified a bit so that the order being tested has correctly passed to the payment state with a valid shipping_method, payment_method and address, and maybe the current tests are not doing that and operating on an order in cart state?

For reference, I had a little draft PR here that was aimed at some of the cleanup related to this and the question of when taxes should (or shouldn't) be applied: https://github.com/openfoodfoundation/openfoodnetwork/pull/10776/files

[abdellani]

Pending tests

• spec/services/process_payment_intent_spec.rb:110
• spec/services/process_payment_intent_spec.rb:130

Those are failing because there is a new state confirmation after payment.
In those tests, we are expecting the order to move to complete.

• spec/system/consumer/shopping/checkout_stripe_spec .rb

I removed this because it's using the legacy checkout.

• spec/requests/checkout/stripe_sca_spec.rb:131
• spec/requests/checkout/stripe_sca_spec.rb:153
• spec/requests/checkout/stripe_sca_spec.rb:219
• spec/requests/checkout/stripe_sca_spec.rb:241
• spec/requests/checkout/stripe_sca_spec.rb:258
• spec/requests/checkout/stripe_sca_spec.rb:273
• spec/requests/checkout/stripe_sca_spec.rb:305
• spec/requests/checkout/stripe_sca_spec.rb:327
• spec/requests/checkout/stripe_sca_spec.rb:348

not sure if those need to be updated or removed

• spec/requests/checkout/paypal_spec.rb:51
  I'll need help with Paypal

• spec/requests/checkout/failed_checkout_spec.rb

• spec/system/consumer/multilingual_spec.rb:65

• spec/controllers/payment_gateways/paypal_controller_spec.rb:91

• spec/controllers/payment_gateways/paypal_controller_spec.rb:102

not sure about those

---

update:
All the pending tests are resolved.
Thank you @Matt-Yorkley 👍

[Matt-Yorkley]

Great ❤️

I think we should get this merged ASAP 👍

[dacook]

Great work, this turned out to be a big task!

It's clear there were still a lot of tests based on legacy checkout, thanks for working through these.
I have a few questions, and a suggestion for tidying up state_machine_spec.rb.

But I'm concerned about removing the concurrency spec, and disabling a number of others. Supporting the checkout process is pretty important and ideally we'd like to protect against bugs that we previous had specs for. Perhaps a practical compromise is to postpone these, but I think that needs to be agreed by product-circle or scheduled as a task.

PS I'm wondering what you think, and if we should discuss in the delivery circle meeting.

[dacook]

Just wondering why? Is this another bug that wasn't picked up in the test suite?

[abdellani]

if we keep the original code before_transition, create_tax_charge! will do nothing because of the guard clause.

    def create_tax_charge!
      return if state.in?(["cart", "address", "delivery"])
...
    end

It was introduced on this PR for optimization reasons.

[Matt-Yorkley]

Taxing the items in the order needs to happen after the order's addresses have been set but should be done before the user lands on the payments step page. It wasn't quite working correctly before as the order's state hadn't been updated at the point it was being checked in the before_transition hook. The after_transition hook is the correct place for it 👍

[dacook]

Thanks. It would be cool if we could add a spec to show that, but the main thing is we're improving 👍

[dacook]

Great teaming up on this massive much-needed cleanup!

Sorry for holding this up, I didn't see it clearly initially. I agree we need to move it forward.

Assuming you're happy with my little code cleanup, I'll mark ready for testing.

[Matt-Yorkley]

Rebased to resolve conflicts 👌

[filipefurtad0]

Hey @abdellani @Matt-Yorkley ,

Ok, so I've tested checking out, as a customer and using:

1. Paypal -> I could not really create insufficient funds cards or a similar setup to generate a failed payment... The closest I've got would be sending a webhook to test this - using this functionality. But I could not really get it to work. Regular payments/checkout work fine, as before, so I think we're good here.

2. Stripe -> performed no additional tests this was tested already in the context of #10933

As an admin:

3. Subscriptions using Stripe -> tested before/here as well

4. BO orders -> performed some additional tests here, created some orders manually and added Stripe SCA payments to it: 🟢

Uff. All good. Fun fact: if a customer has a card set up (the x-3155) which has expired (visible under /account#/cards) and the admin uses this card to place the BO order the expired date will overrule what one enters in the the form in the BO... This was a bit unexpected to me, but I guess this makes sense in the real world, at least in a way.

Without going much more into this black hole, if a new card is set up then yes, payments work, with this card type:

5. Tried to access the previous checkout UI - this was possible at some point, which would probably break things now. I was not able to (not sure how I did that in the past)

Ok, so - I've decided to merge despite insufficient testing on Paypal. shall we merge?, as this is blocking plenty of other stuff. If you happen to have any suggestions on how to test this (other than risky prod), then yeah, let's chat.

Merging!

[RachL]

@abdellani Hello :) I still see the toggle when I visit the page admin/feature-toggle/features is this because of some kind of cache?
ping @jibees I think we had this in the past with other feature toggle but I don't remember what you've done to remove them.

[jibees]

No cache issue I suppose since split_checkout feature is still in the code. I'll let answer @abdellani then ;)

[abdellani]

I remember that I removed it locally. Maybe I accidentally overwritten the commit that updates the list when I fetched the changes from the repo.

Anyway, enabling or disabling that feature will not have any impact on the current behavior.
I'll remove it on another PR.