Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Vouchers] Fix order with voucher redirect to homepage after cart edit #10941

Conversation

rioug
Copy link
Collaborator

@rioug rioug commented Jun 5, 2023

What? Why?

After applying a voucher to an order and editing the order so that the total ends up being 0, when one goes through checkout again, the order was automatically confirmed when clicking on 'Next - Payment method'. This happened because the order was now considered a "free order" and payment was skipped. This is in place to allow unpaid subscription order to be completed.
So to avoid this issue, we make sure that payment is required for any order with a voucher applied.

What should we test?

  • Put items in your cart and proceed to checkout.
  • Enter your details and click 'Next - Payment method'.
  • Apply a voucher with an amount below the order total.
  • Select a payment method and click 'Next - Order summary'.
  • Click 'Edit' next to the order details.
  • Adjust the items in such way that the order total now is exactly the voucher amount (Total = 0.00 $).
  • Click 'Checkout'.
  • Click 'Next - Payment method'
    -> The payment step should load and the voucher should still be applied to the order (Note the voucher might be missing due to [Vouchers] Errors when/from iterating between /summary and /cart page #10857)

Release notes

Changelog Category: User facing changes

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

rioug added 2 commits June 5, 2023 14:31
There is no functional difference between `describe` and `context`,
the later being alias of the former, but it makes a difference on
the code readability.
In the case when an order total is 0 due to a voucher being applied, and
going through a checkout after a cart update, the order was confirmed
autmatically and the customer was redirected to the homepage.
In this scenario the order was considered a "Free order". To fix the issue
we require payment for any order with a voucher applied.
@rioug rioug marked this pull request as ready for review June 5, 2023 23:43
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit hacky but good to solve for now. I guess that we still have a problem with zero-price products and no vouchers in the cart.

@Matt-Yorkley
Copy link
Contributor

Matt-Yorkley commented Jun 8, 2023

Hey @rioug I ended up doing some work on Vouchers while you were away and I've already got quite a substantial branch that addresses this issue around the order having zero total and a few other things as well, like:

  • Improving some underlying things in the checkout generally
  • Refactoring the checkout form and vouchers form so they're separated into two forms that can be submitted separately
  • Extracting all the voucher-creating logic out of the checkout controller and into a create action in the VoucherAdjustmentsController (which the voucher form now submits to)
  • Implementing changes across the order and order states workflow to ensure the order can still be processed nicely if it's total drops to zero
  • Adding some improvements to the way payment method selection works when the order doesn't require payment
  • Handling cases where an existing voucher amount gets recalculated after it's changed

I haven't pushed it yet because it's effectively blocked waiting for PR #10913 to resolve issues around removing the split checkout feature toggle and updating the related checkout specs...

@mkllnk
Copy link
Member

mkllnk commented Jun 8, 2023

@Matt-Yorkley, that's really cool. It's just a shame that we are doubling up on work here. Let's coordinate a bit better. @rioug has been tasked with the Vouchers work for most part of this year now. Can you maybe outline the tasks that are still ahead (in budget) and then you two can agree on who's doing what?

In the meantime, while we wait for pull requests to be merged and rebased, maybe @rioug could pick up some other little tasks. Fixing some flaky specs is always worthwhile in the long term.

@mkllnk mkllnk added the blocked label Jun 8, 2023
@rioug
Copy link
Collaborator Author

rioug commented Jun 8, 2023

Thanks @Matt-Yorkley , I would have been good to know you were working on this.

  • Handling cases where an existing voucher amount gets recalculated after it's changed

I have been doing something similar while looking at #10857, I'll create a PR with what I have so far and we can look at it once we are unblocked by #10913 and you have pushed your work.

@Matt-Yorkley
Copy link
Contributor

It's just a shame that we are doubling up on work here.

Yeah, sorry. I got assigned to the Vouchers epic while @rioug was away and started looking at addressing the open issues, then I realised there was a lot of overlap with open checkout issues and vouchers issues, and I realised it was all kind of blocked until #10913 was resolved, so I had to pivot to helping Mohamed finish that off as the top priority and I ran out of time towards the end of the week.

I've been focussing on the behaviour around applying vouchers during checkout but the issues related to the admin vouchers UI (#10870 and #10871) are available to be worked on and shouldn't overlap.

@rioug
Copy link
Collaborator Author

rioug commented Jun 12, 2023

got assigned to the Vouchers epic while @rioug was away

I missed that sorry, I should have checked with you. I am assuming you'll look at this one as well #10855 ?

the issues related to the admin vouchers UI (#10870 and #10871) are available to be worked on and shouldn't overlap.

I'll leave these for now, I am not sure we have the budget for it.

@sigmundpetersen sigmundpetersen changed the title [voucher] fix order with voucher redirect to homepage after cart edit [Vouchers] Fix order with voucher redirect to homepage after cart edit Jun 13, 2023
@rioug rioug removed the blocked label Jun 26, 2023
@rioug
Copy link
Collaborator Author

rioug commented Jun 26, 2023

Will be superseded by #11003

@sigmundpetersen
Copy link
Contributor

Should we close here then?

@rioug
Copy link
Collaborator Author

rioug commented Jul 17, 2023

Yes we should, closing.

@rioug rioug closed this Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Vouchers] Redirection to HOME after updating the cart including a voucher
4 participants