[vouchers] error moving between summary and cart pages

[rioug]

What? Why?

• Closes [Vouchers] Errors when/from iterating between /summary and /cart page #10857
• Should also fix [Vouchers] Checkout blocked after removing items from cart (order total < 0) #11233

Matt's work in #11003 fix some part of this issue but it doesn't cover the scenario when there is a tax included or excluded from the price.
Reviewers, new code starts here : 9fccea3

What should we test?

Assuming you have a shop with a voucher set up, follow the scenario below for voucher with no tax, with tax included in price and tax excluded from the price:

• Go through checkout and add a voucher at the payment step
• Proceed to summary and click on "edit order details"
• Go back to summary step by clicking checkout button
  -> Check the voucher is still displayed properly

Additional test :

As above proceed to the summary step, and then go to the /cart page, and then follow the scenario below, ,again for voucher with no tax, with tax included in price and tax excluded from the price:

• Update quantity
  -> check total and voucher are updated accordingly
• Click on the checkout button
• Go through checkout steps again
  -> Check the voucher is still there on the payment step
  -> Check the voucher is correct on the summary step

Payment fees and voucher

Payment fees are calculated based on the item total, and are recalculated when confirming the order. Vouchers are calculated based on the order total. The main thing to check here is to make sure

• when a voucher cover the order total or more, payment not is required, so the payment fee shouldn't apply
• when using a percentage based payment fee, the payment fee shouldn't change when confirming an order with a voucher

Release notes

Changelog Category: User facing changes

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

Dependencies

This PR #11003 #11135 need to be merged first

[dacook]

Looks like a good fix! I haven't reviewed the dependent PR though because it didn't seem ready.

It would be good to fix the typo, and I've left some comments to consider for bonus points :)

[mkllnk]

I was about to approve but changes were requested already. So I'll move this back to In Dev.

[mkllnk]

Great work!

[mkllnk]

This line is removed in another commit.

[rioug]

Thanks for the thorough testing @drummer83 !

Editing the cart after completing step 1 and 2 of checkout already, redirects to step 1 when clicking checkout. This has already been completed (as step 2 has been as well), so shouldn't we redirect to step 3 instead? Shipping method should be remembered.

I believe this is an existing behaviour, nothing to do with voucher. But I agree it's a little bit confusing, it should at least redirect to step 2 in case you need to change payment method

Tax calculation for tax excluded from price changed:

I don't think that was intentional, from memory the transaction fee was excluded from voucher calculation, but I haven't had time to double check that.

I fixed some of the issues, but I am still looking at anything related to payment fees.

[rioug]

@filipefurtad0 I am trying to create an order with a payment and payment fee. I am not quite sure how to create the payment fee, do you have any pointers ?
This is what I tried so far :

let(:bogus_payment) { create(:bogus_payment_method, distributors: [create(:enterprise)]) }

before do
  payment = create(:payment, amount: 1, payment_method: bogus_payment)
  order.payments << payment
  order.save!
  order.update_payment_fees!
end

I think I need to setup some sort of fees on the payment ?

[drummer83]

Ok, looks like you keep working within this PR so I will move back to In Dev.

[filipefurtad0]

Hey @rioug,

This is what I tried so far :

For a transaction fee to be applied, in addition to the payment method, we'd need to set up a calculator.

But maybe there's an easier way, by using the factory for orders with fees (:completed_order_with_fees).

In case you need more granularity, like setting the calculator type, or values, then the approach on the payment_spec.rb would be best, I guess.

Hope this helps.

[rioug]

@drummer83

Editing an order on /cart page doesn't update transaction fees. This happens only after proceeding to checkout again.

This is an existing bug so I won't be fixing it here. But as you noticed, it will be recalculated as we proceed to checkout again so it's a "minor" issue

Tax calculation for tax excluded from price changed. Was this intentional and decided somewhere? ❓

I looked at this again, and transaction fees are included in the tax calculation for tax excluded from price. Nothing should have changed here. So the "AFTER" is correct here.

I think I fixed all the other issue

[dacook]

Looks good, but pending a question about the payments.clear call.

[dacook]

Sorry but I have to ask, is this potentially clearing records of actual payments?
I'm guessing at this point the payments (and indeed order) are in an "incomplete" state, so it's ok to delete. But perhaps we should add a scope just in case? Eg:

    @order.payments.incomplete.clear

I looked at the Payment model and expected to see a before_destroy for this but didn't. Maybe it should be there..
@mkllnk do you have any insights?

[mkllnk]

That's a good question: do we ever have complete payments in this code path?

It looks like they would influence the calculation if we kept them. So that would need special consideration.

Looking into the state machine, we can restart the checkout any time until the order is finalised. At the time of finalising the order, the payment has been taken already. So I think, it can happen that we take a payment and then face a stock conflict and have to restart the checkout.

@rioug, do you agree with my theory? It would mean that you need to calculate totals for voucher amounts considering payments instead of clearing them out, I think.

[rioug]

My understanding is that payment are processed once we confirm the order, see below:

openfoodnetwork/app/controllers/split_checkout_controller.rb

Lines 115 to 126 in ad37c40

	def confirm_order
	return unless summary_step? && @order.confirmation?
	return unless validate_summary! && @order.errors.empty?
	@order.customer.touch :terms_and_conditions_accepted_at
	return true if redirect_to_payment_gateway
	@order.process_payments!
	@order.confirm!
	order_completion_reset @order
	end

and once the order is confirmed then the state machine will finalise the order. So If I understand correctly we shouldn't have any completed payments when navigating between checkout steps.
Also if the checkout is restarted, the the user will be going through the payment step again and will be able to (re) choose the payment method.

I added this to fix a specific scenario where the user goes through the checkout till the confirmation step, then goes back to the payment step and add a voucher covering the order amount. In this case we need to remove any existing payment/payment fee as now there is no payment required.

Would we have an already completed payment if we face a stock conflict and have to restart the checkout ? You would think that we would check the stock situation before taking a payment.
From what I can see, we would be redirected to cart if there is a stock issue when navigation checkout steps:

openfoodnetwork/app/controllers/concerns/checkout_callbacks.rb

Line 34 in ad37c40

redirect_to_cart_path && return unless valid_order_line_items?

openfoodnetwork/app/controllers/concerns/checkout_callbacks.rb

Lines 66 to 70 in ad37c40

	def valid_order_line_items?
	@order.insufficient_stock_lines.empty? &&
	OrderCycleDistributedVariants.new(@order.order_cycle, @order.distributor).
	distributes_order_variants?(@order)
	end

I couldn't see any other checks so maybe I am missing something here.

Anyway, I'll apply @dacook suggestion to be on the safer side.

[mkllnk]

Would we have an already completed payment if we face a stock conflict and have to restart the checkout ? You would think that we would check the stock situation before taking a payment.
From what I can see, we would be redirected to cart if there is a stock issue when navigation checkout steps

Of course we check the stock before we take a payment but there's always room for race conditions. We often process several checkouts in the same second and it can happen that two people try to checkout the same last item at the same time. It's really hard to predict what exactly happens because it depends on the timing and where the error is raised.

Actually, the CurrentOrderLocker should prevent this. So maybe it's not a problem any more. I'm just scared that this race condition will come back to haunt us... 😨

[dacook]

👍 Good one

[drummer83]

Hi @rioug,
We're getting closer. 👍
Thanks for continuing your work on this!

Conclusion

To unblock the percentage PR and to keep things simple I'd vote to merge this one and fix the open issues in separate PRs. I will create issues for those. Please go ahead and merge if you agree or let me know if you don't.

Testing notes AFTER staging

Test case	Tested for 'No tax', 'Tax incl. in price', 'Tax excl. from price'
Moving between summary and VIEWING cart	✔️
Moving between summary and INCREASING cart	✔️ 1st checkout: if order total < voucher amount then 'no payment required' is shown and it's listed on step 3 (img 5); ❌ After editing and increasing the cart (still order total < voucher amount) the 'no payment required' is NOT displayed, a payment method needs to be selected and then on step 3 the payment method and transaction fee is displayed even though the voucher covers the order total (img 6); ➡️ #11358
Moving between summary and REDUCING cart	✔️ 1st checkout: if order total < voucher amount then 'no payment required' is shown and it's listed on step 3 (img 5); ❌ After editing and reducing the cart (still order total < voucher amount) the 'no payment required' is NOT displayed, a payment method needs to be selected and then on step 3 the payment method and transaction fee is displayed even though the voucher covers the order total (img 6); ➡️ same issue as above
Voucher removed from order when moving between summary and cart?	No. ✔️
Order total > voucher amount	✔️
Order total < 0 due to editing order (#11233)	✔️
Payment fee doesn't change when applying the voucher	✔️
Payment fee is not applied when order total <= voucher amount	❌ Payment fee is still applied when editing the cart after being on step 3 of checkout already (img 6). ➡️ same issue as above

Image 5:

Image 6:

General issues AFTER staging

• Found the difference between these two results. It's not related to BEFORE and AFTER staging, it depends on how you finish the checkout steps. ➡️ [Vouchers] Excl. tax is calculated incorrectly when moving back in checkout process #11359
  • Finishing all three steps in a row and adding the voucher in step 2:
    • CORRECT: Tax on voucher is calculated including the transaction fee.
      
  • Leaving step 2 without adding the voucher and then going back from step 3 to step 2 to add the voucher yields this:
    • NOT CORRECT: Tax on voucher is calculated excluding the transaction fee.
      
• Going back to Step 2 after being on step 3 already shows 'no payment required', but then an error is displayed because no payment method is selected. ➡️ fixed ✔️
• Editing an order on /cart page doesn't update transaction fees and removes the shipping fees from the table. ➡️ [Cart] Shipping fees and transaction fees are not updated when editing the cart #11360
  https://github.com/openfoodfoundation/openfoodnetwork/assets/1790176/811c0ac3-9fe3-4e4e-a39a-37384f441038
• 'No payment required' remains visible after removing a voucher code. Error message when clicking Next. ➡️ fixed ✔️
• Adding the voucher on step 2 after step 3 had been visited already ignores the voucher. ➡️ fixed ✔️
• Editing the cart after completing step 1 and 2 of checkout already, redirects to step 1 when clicking checkout. This has already been completed (as step 2 has been as well), so shouldn't we redirect to step 3 instead? Shipping method should be remembered. ➡️ wishlist #454
• Negative order total (credit owed) after changing to a shipping method without fee. ➡️ [Vouchers] Negative order total (credit owed) after changing to a shipping method without fee #11361
  https://github.com/openfoodfoundation/openfoodnetwork/assets/1790176/c5fc9ebc-f7f7-40ac-b2bc-5d20bd73c7ea