Vouchers part 2

[Matt-Yorkley]

What? Why?

Depends on #11002.

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.

[rioug]

I am assuming you are waiting on #11002 , but in the interest of speed I reviewed this one as well.
I left a few comment, the main thing would to move the voucher_adjustment controller specs to request spec.

[rioug]

I am lacking context around exchange I think. If I understand correctly, this add the first line item in the order to the exchange (which is an Enterprise from the order cycle point of view ) , is that correct ? If so why is it needed ? and shouldn't the factory :order_with_line_items set that up for us ?

[rioug]

can we move this to https://github.com/openfoodfoundation/openfoodnetwork/blob/master/spec/requests/voucher_adjustments_spec.rb ? The rails team recommend using request specs nowadays.

[rioug]

I find this hard to read and it's easy to miss you are not doing a comparison here (my rails console also gives me a nice warning : warning: found '= literal' in conditional, should be ==) .
I'd prefer something like this for clarity:

Suggested change

	return unless (id = payment.payment_method_id)
	id = payment.payment_method_id
	return if id.nil?

[rioug]

I would argue it would be better to keep load_saved_credit_cards (with out setting the @selected_card probably) in the controller/concern. To me it feels wrong as the view is essentially doing a query, which should be the responsibility of the controller. That said, it's more an issue with rails view system allowing you to do that sort of things.

[Matt-Yorkley]

So these little changes around loading/assigning credit cards and loading/assigning the voucher adjustment object are related. The issue is that both the checkout controller and the vouchers controller need to potentially render some of these views as they relate to vouchers. The view doesn't work unless the cards are loaded, but if we do it at the controller level then essentially we need to write the exact same code in multiple controllers. So it turns out this logic isn't really specific to either controller, but specific to the view. Know what I mean? So what I wanted to do was avoid duplicating that logic in both controllers, essentially both of them would have to do something like this:

@saved_credit_cards = <load the cards>
@voucher_adjustment = <load the voucher>

So moving these bits around and actually moving them out of the controller level means the view has no external dependencies and it becomes a lot more portable in terms of being able to render it more easily from different contexts.

It does look a bit funky sitting in the view. I guess the more idiomatic way of doing this would be to extract it to a method in a view helper. Maybe that's preferable..? I think I avoided doing that for now just because currently it's only used one place, so the method would only have one caller. What do you think?

[rioug]

I can see what you are trying to do but it still doesn't sit well with me. I don't think the view helper is the solution either, you would just be moving the problem around, the view would still be making the query, it's just moved away from the view file.

The only thing I can think of would be to add the data loading to their own concern which would need to be included in the related controllers. But it does keep the view external dependency.

I guess loading the data in the view is an acceptable trade off, but I can't help wondering if it's a bit of a premature optimisation, are these views going be to be render in many different context ?

[rioug]

Same as my previous comment, I did it that way so the view "isn't the one doing the query".
Now that I think about it, the view would still be doing the query as I assume the query will happen when the view try to use @voucher_adjustment. 🤷

[rioug]

For context, we though that wasn't necessary as unless you have access to Vouchers in backoffice (which is behind a feature toggle.), a user wouldn't be able to add a voucher to an enterprise, so the voucher section wouldn't show on the payment step.

[rioug]

Hey @Matt-Yorkley I found a bug when removing a voucher from an order, the "voucher section" isn't re rendered properly and it becomes impossible to add a voucher again.
Step to reproduce, assuming you have an enterprise set up with one or more voucher :

• checkout to payment step
• add a voucher
• remove voucher
• try adding voucher again <- 💥 broken

https://github.com/Matt-Yorkley/openfoodnetwork/blob/a211605b9b9caa4d8696eeba1d9eec2393a8dd7e/app/controllers/voucher_adjustments_controller.rb#LL53C1-L58C6
At first glance it looks like we need to pass the @order to the split_checkout/payment partial but it doesn't fix the issue, can you have a look if you have time ?

[dacook]

It looks like there are unresolved comments from the first review, and a bug introduced, so I'll move to In Dev.
If this is ready to review can someone add a comment and mark it ready?

[Matt-Yorkley]

Hmmm Github just auto-closed this PR when the other was merged. Looks like I can't edit the target branch or reopen this RP, so I'll have to open a new one.

[Matt-Yorkley]

Moved to #11135

I found a bug when removing a voucher from an order, the "voucher section" isn't re rendered properly and it becomes impossible to add a voucher again.

I'm not able to replicate that bug on this branch, it seems like it's working fine?