Vouchers part 2

[Matt-Yorkley]

What? Why?

Closes #10857, #10865, #10869, #10855 and #10787

Brings in a few changes to the way vouchers are applied at the checkout.

• Splits the voucher form fields in the checkout payment step out into a separate <form> so it can submit to a separate controller as mentioned here 10432 vouchers bare minimum checkout #10587 (comment)
• Extracts all the voucher-applying logic out of the checkout controller and into a create action in the voucher controller (alongside the existing delete action which is already there)
• Improves visual feedback in the UI when a voucher isn't found
• Introduces the concept of a "zero priced order" explicitly into the code, which is needed in various places. This is an order which is valid and has line items but also has an order total of zero. This can be either because a shop has products with zero price (see Can't proceed to checkout if order total is equal to 0 (blocked at details step) #10787) or now (more commonly) because applying a voucher can bring the order's total to zero. There are a number of places the codebase needs to know about this "zero priced order" case and treat it as a valid scenario and a valid order, and previously it was not doing that.
• Adds some slightly new behaviour for handling these "zero priced order" cases during the checkout; namely that if the order total is zero, the payment method options are hidden and the user sees feedback that payment is not required. This is also shown in the order confirmation. A payment object with an amount of 0.0 gets added to the order in this case and the order can be completed successfully.

What should we test?

If an order is valid but doesn't require payment then... it doesn't require payment. Adding a voucher that brings the order total to zero hides the payment method options and lets the user know that no payment is required. The order can be completed successfully.

#10857, #10865, #10869, #10855 and #10787 should all be fixed/closed.

Release notes

Changelog Category: Technical changes

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

Follow up required

• There's probably various changes that need to be made around tax adjustments on vouchers
• Also update email template to match checkout summary (Vouchers part 2 #11135 (comment))

[drummer83]

This shouldn't be in column 'closed', should it?

[rioug]

When a voucher is applied to an order with tax excluded from the price, two voucher adjustment are created, one for the voucher and one for the tax component. I fixed the VoucherAdjustmentsController#destroy to take that into account.

[rioug]

Re added my review from the old PR.

[Matt-Yorkley]

When a voucher is applied to an order with tax excluded from the price, two voucher adjustment are created, one for the voucher and one for the tax component. I fixed the VoucherAdjustmentsController#destroy to take that into account.

I've dropped that commit for now. I think there's probably various changes that need to be made around tax adjustments on vouchers but the scope of this PR is already quite substantial.

[dacook]

Fantastic, looks like a big re-arrange that solves some problems and helps us move forward with vouchers.

I noticed a couple of additions without associated specs that I thought seemed important. If we don't add them now, can they be included in the next round?

Also I have several questions/comments soz

But overall I think this is good to proceed as-is (if the specs are included in the next round).

[dacook]

It's good to see the helper method result being cached instead of calling twice, but I would avoid that in the view. Also would avoid using assignment in a condition.
Still, this is better than it was before.

[rioug]

@Matt-Yorkley

I think there's probably various changes that need to be made around tax adjustments on vouchers

Is that being addressed anywhere ? are you looking at it ?

[dacook]

PS sorry one more question, do we need to update the email confirmation template too?

[rioug]

PS sorry one more question, do we need to update the email confirmation template too?

As the vouchers use adjustment, we get that for free, adjustments will show on the email confirmation.

[dacook]

Actually i was thinking, since we're updating the confirmation screen to show "No payment required", this might also be relevant for the confirmation email.

Upon looking, I think it is relevant. The confirm_email_for_customer (and several others) use app/views/spree/shared/_payment.html.haml which I guess would say something like "Not paid". As far as I understand, if we don't update this we'll have an inconsistency within the app.

So.. can we add this in a follow-up PR? (I don't want to stall this one any longer!)

[dacook]

I'm not sure about the usage of dup, so am wondering if we can avoid that?

Also I've applied Gaetan's suggestion.

I collated some follow up notes and added to the description of this PR too.

[dacook]

Hmm I've not seen that done before. It doesn't seem like best practise, but I can't think of a good alternative.

Why do we need to check validity? Is this important for all uses of zero_priced_order?Shouldn't the controller check the validity before proceeding anyway? Or is there a specific thing we can re-check here without using valid?

[dacook]

Indeed, why do we need this method as distinct from payment_required? I'll attempt to answer..
It seems that the zero_priced_order passes through the payment state, and records a zero payment. But subscriptions are a (weird) case where we want to treat it as a complete order without properly completing it.

[Matt-Yorkley]

There are multiple places where this method will be called from (not just one controller), and calling valid? flushes all errors on the current order object and re-checks them, which is not desirable in some situations (custom non-ActiveRecord errors added during checkout being a prime example).

I'll just drop the valid? check, maybe it's not necessary.

[dacook]

Specs pass... so I guess it's not 🤞

[dacook]

👍 LGTM!

[rioug]

Looks good !

[filipefurtad0]

Hey team,

There's quite a lot to unpack here, I think it's best to split this across different tests (evtl. testers). I'll start by:

1) Testing the new functionality of not having to add a payment method

i) For a simple order (no taxes or fees):

• adding a voucher in which the total of the voucher equals the order total
  • hides the payment method selector, on the /payment step

• allows completing an order (confirmation emails arrive) -> the payment method section is empty in this case

• the same goes for the payment section in the backoffice, when editing the order:
  

• no taxes are calculated here, as the order total is zero

ii) For an order with at least one enterprise fee with a per-order calculator:

• adding a voucher in which the total of the voucher equals the order total
  • does not hide the payment method section. This feels like the correct behavior, as indeed, more expenses will be displayed on the final /summary step:

• completing checkout displays the order confimation per email, as usual

• and displays the payment as usual as well, on the backoffice:

It's relevant to note, how these orders are displayed in the Payments report - by Type - and that this does not make the report blow up 🎉

but that we have a missing translation for the none string:

Summary for test case 1)

All good!!
Missing translation on the none string, when payment method name is empty. Issue coming up.

Keeping in Test Ready for now, as we'll pick up other bits later.
Thanks - looking great so far 👏 👏 👏

Edit: So the way we go around this order total equaling zero is by creating a payment, which looks like this:

  id   | amount | order_id |         created_at         |         updated_at         | source_id | source_type | payment_method_id |   state   | response_code | avs_response | identifier | cvv_response_code | cvv_response_message |        captured_at         
-------+--------+----------+----------------------------+----------------------------+-----------+-------------+-------------------+-----------+---------------+--------------+------------+-------------------+----------------------+----------------------------
 36208 |   0.00 |   461582 | 2023-07-12 11:20:14.057009 | 2023-07-12 11:20:14.057009 |           |             |                   | completed |               |              | HQ3M4R69   |                   |                      | 2023-07-12 11:20:18.260116
(1 row)

A lot of missing info in there 👀 which is of course by design. Still, good to keep in mind in case this attributes are needed elsewhere in the app 👍

[filipefurtad0]

Ok, continuing:

• scenario for [Vouchers] Errors when/from iterating between /summary and /cart page #10857

Fixed: a snail is not triggered after visiting the cart page and resuming checkout. However, when resuming checkout, the user always seems to be redirected to the /details step (regardless of the previous step), making it start checkout over again -> issue .... (to be edited)

• scenario for [Vouchers] Redirection to HOME after updating the cart including a voucher #10865

Fixed: I could not reproduce the issue, it seems to have been related orders with total = 0.

• scenario for [Vouchers] No red highlighting of voucher code when code can't be found #10869

Fixed:

• scenario for [Vouchers] Order total in /cart page not updated #10855

Fixed: Visiting the /cart from the /payment step (and after adding a voucher) now updates the cart:

• scenario for Can't proceed to checkout if order total is equal to 0 (blocked at details step) #10787

Fixed: it's possible to checkout now for orders with total = 0 (without adding a voucher):

Order confirmation:

Summary
Whoa 5 / 5 🌟
Minor issue with the redirection to the /details page (after visiting the /cart). Edit -> this issue is not related to vouchers code nor the feature toggle, it occurs on master, already prior to merging 👍

Merging 💪