10432 vouchers bare minimum checkout

[rioug]

What? Why?

• Closes [Vouchers] Implement bare minimum checkout #10432-

Implement the minimal checkout functionality to be able to apply a voucher to an order.

What should we test?

You'll need to enable the vouchers feature in the backoffice, see #10523

General test

In the back office logged as an enterprise user :

• Add a voucher for your shop

On the website logged as a customer :

• Add one or more product in your cart from the shop above
• Proceed to checkout
• Enter details and shipping info, click on "Next - Payment method"
  --> You should see a "Apply voucher" section

• Enter the voucher code and click "Apply"
  --> You should see a "$10 Voucher" box and a "Remove code" link

• Click on "Remove Code"
  --> it should remove the voucher, and you should see the Input and Apply button as before
• Re apply the voucher code, choose a payment method and click on "Next - Order summary"
  --> you should see the voucher applied to the order in the order summary column, highlighted in blue

• Click on "Complete order"
  --> you should see the voucher applied to the order in the order summary

Tax included in price

In the backoffice logged as an admin:

• check tax configuration , Go to "Configuration" Tab -> Left hand menu "tax rates"
• check that "included in price" is set to true for your preferred tax

On the website logged as a customer :

• Follow the steps above to add voucher to an order
• Proceed to "Order Summary"
  --> you should see the voucher applied to the order in the order summary column, highlighted in blue, and a "(Includes tax)" line

• Click on "Complete order"
  --> you should see the voucher applied to the order in the order summary and a "(Includes tax)" line

Tax not included in price

In the backoffice logged as an admin:

• check tax configuration , Go to "Configuration" Tab -> Left hand menu "tax rates"
• check that "included in price" is set to false for your preferred tax

On the website logged as a customer :

• Follow the steps above to add voucher to an order
• Proceed to "Order Summary"
  --> you should see the voucher and the voucher tax portion applied to the order in the order summary column, highlighted in blue, and a "(Includes tax)" line

• Click on "Complete order"
  --> you should see the voucher and the voucher tax portion applied to the order in the order summary and a "(Includes tax)" line

Voucher covers more than the order total

On the website logged as a customer :

• proceed to checkout with an order total under $10
• proceed to "Payment method"
• Enter the voucher code and click "Apply"
  --> You should see a "$10 Voucher" box and a "Remove code" link, and a warning.

• Choose a payment method and click on "Next -Order summary"
  --> you should see the voucher applied to the order in the order summary column, highlighted in blue. The value should be equal to minus the order total (and not -$10 as before). And the order total should be 0.

Release notes

Changelog Category: User facing changes

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

Dependencies

Backoffice change will need to be merged first #10523 Merged already

Documentation updates

[rioug]

@mkllnk I committed my latest changes, it still a WIP on handling taxes when tax is not included in the order price : e19d7da

I went with using the same Voucher model as the adjustment originator and add "Tax" to the label so we know it's the tax part of the voucher. It might better to go with your suggested implementation (having a VoucherTax subclass) so we can easily search for "Tax voucher" adjustment. Anyway I just wanted to have basic solution working for now. It will need some refactoring later on.

TODO:

• Fix checkout process so we can create the correct voucher adjustments when tax not included in price :
  As described in commit message, it's not really working right now as when we apply the voucher to the order in SplitCheckoutController taxes are yet to be applied to order : https://github.com/rioug/openfoodnetwork/blob/e19d7da3ca248319b73dc958de034671283279ae/app/controllers/split_checkout_controller.rb#L31 so we can't know if taxes are included or not in the price, at least as far as I know. Maybe there is a better way to check that ? Looking at the settings ?
  My thinking was to call recalculate_taxes : https://github.com/rioug/openfoodnetwork/blob/e19d7da3ca248319b73dc958de034671283279ae/app/controllers/split_checkout_controller.rb#L287, before applying the voucher, but I am not sure of the implication of doing this.
• Fix #compute_amount : https://github.com/rioug/openfoodnetwork/blob/e19d7da3ca248319b73dc958de034671283279ae/app/models/voucher.rb#L77 to handle tax not included in the order price
  I am trying to do something like :

  def compute_amount(order)
    amount = calculator.compute(order)
    
    # Use the tax on top of order price to check the amount to use
    total = order.total + order.additional_tax_total
    return -total if amount.abs > total

    amount
  end

I am trying to set up test data to unit test this, looks like the existing order factories don't offer what I need.

• Add system specs to cover adding voucher
  • when tax is included in order price
  • when tax is not included in order price
• Add system specs to cover using a voucher that is more than the order price + tax when tax not included in order price

[rioug]

Ideally all the calculation would be encapsulated in a Calculator but I don't think creating the tax adjustment belongs in one of them. So I kept it in the Voucher class.
That means we need to call Voucher.adjust! and we can't just rely on "Spree::Ajustment.update_adjustment!" ( It is call by the order updater )

openfoodnetwork/app/models/spree/adjustment.rb

Lines 100 to 117 in c34ced2

	def update_adjustment!(calculable = nil, force: false)
	return amount if immutable? && !force
	if calculable.nil? && adjustable.nil?
	delete
	return 0.0
	end
	if originator.present?
	amount = originator.compute_amount(calculable || adjustable)
	update_columns(
	amount: amount,
	updated_at: Time.zone.now,
	)
	end
	amount
	end

[rioug]

I found a bug where the cart disappear after applying a voucher. I'll be looking at that but don't think it should delay the reviewing

[mkllnk]

Wow, great work. This is bigger than I imagined. Nice small commits though. 👍

I left some comments. Let me know what you think.

[mkllnk]

I think it's possible that an order has items including tax and items excluding tax and even items having both if they are subject to two tax categories. I think that we need to find all items for each type of tax calculation and then create adjustments for whichever kind is present. It means that we can't use the existing totals and have to add it up ourselves. Or am I overcomplicating it?

[rioug]

I am not sure about that, I always though that it was one or the other. Maybe something to clarify with Lynne/Kirsten.

[mkllnk]

Just to summarise here as well: It's possible to have mixed included and excluded tax on an order and even on single items but we are ignoring that for now and implement that later.

[dacook]

So, this implementation is incomplete and has a bug? Seems risky to leave it in, but maybe necessary for the interim.

[rioug]

I get a weird bug when adding a voucher to an order, where the page reload but the cart disappear. I figured out it's because the angularjs application isn't loading properly and the ng-cloak below means the raw template stays hidden

openfoodnetwork/app/views/shared/menu/_large_menu.html.haml

Lines 37 to 38 in 2f6b730

	%li.cart{"ng-cloak" => true}
	= render partial: "shared/menu/cart"

Code extract responsible for adding the voucher :
https://github.com/rioug/openfoodnetwork/blob/9ebd190d4700f41785b6142fae9c89e391a6ddf9/app/controllers/split_checkout_controller.rb#L31-L35

   if add_voucher
      return redirect_to checkout_step_path(:payment)
    elsif @order.errors.present?
      return render_error
    end

I am not quite sure why this happens but I did notice that when I add the voucher the request is processed as "CABLE_READY" , here we are redirecting to the payment step from the payment step. The car isn't displayed.

Processing by SplitCheckoutController#edit as CABLE_READY
     Parameters: {"step"=>"payment"}

If I reload the page, it is processed as "HTML" and the cart is displayed properly

 Processing by SplitCheckoutController#edit as HTML
     Parameters: {"step"=>"payment"}

The weird bit is: if I go to the "Payment step" from the "Details step" for instance , request is processed as "CABLE_READY" but the cart is displayed properly 😕 !

The remote:true below means that the form is submitted as an Ajax request, as far as I understand. Removing the remote: "true" fix the issue, as it ensure the page just load with no Ajax/cable ready interaction I think.

openfoodnetwork/app/views/split_checkout/_form.html.haml

Lines 5 to 6 in 2f6b730

	= form_with url: checkout_update_path(checkout_step), model: @order, method: :put,
	data: { remote: "true" } do |form|

I don't think removing remote: "true" is the correct solution here. I don't know much about mrujs (responsible for hijacking the form submit to transform it as an ajax request as far as I understand ) , cableready or turbo (not sure if it plays any role here ??).
I don't know what the request processed as "CABLE_READY" means, but my suspicion is somehow the page replace only the part of the HTML that changes. That said, looking at the request in the dev tools, I can see it returns a full html page, so maybe it's something else. Any ideas ? pointers ?

[rioug]

I get a weird bug when adding a voucher to an order, where the page reload but the cart disappear.

Fixed by this commit 8672cbe

[mkllnk]

Since we are using acts_as_paranoid, do we want to allow a real destruction? If we don't then we could remove the dependent option and I think then the destroy would fail if it's referenced.

[mkllnk]

Just to summarise here as well: It's possible to have mixed included and excluded tax on an order and even on single items but we are ignoring that for now and implement that later.

[mkllnk]

Great work! 💪

It's great how you solved the AngularJS problem with CableCar. This is ready for a second review. It's fairly big but I would imagine that rewriting history is a big task now as well. So maybe the next reviewer just has to be warned that there are a few detours in here. I'll let you decide if you want to update any commits before moving it into Code Review.

Thank you.

[mkllnk]

Oh, I just saw that it is in Code Review already. I would have left it there but then I saw a merge conflict.

[mkllnk]

Exciting! 👍

[Matt-Yorkley]

This formats: [:cable_ready] looks really weird to me and I think we don't actually need it, along with the multiple-format-rendering in the related controllers and the .cable_ready.haml extension on the views/split_checkout/_voucher_section (it can just be regular .html.haml).

I'll take a look at it...

[rioug]

if you have the partial as a regular .html.haml then this will break (as you can see in the failing test in your PR) :

  def render_voucher_section
    render(
      status: :ok,
      cable_ready: cable_car.replace(
        "#voucher-section",
        partial(
          "split_checkout/voucher_section",
          locals: { order: @order, voucher_adjustment: @order.voucher_adjustments.first }
        )
      )
    )
  end

That's why I am using formats: [:cable_ready] . I agree it looks weird but I couldn't find any other solution, do you have any suggestions ?

[Matt-Yorkley]

Ah, I missed those failing request specs on the VoucherAdjustmentsController. Okay, so this is partly why I was suggesting the changes in that other commit rioug@71e92ae, which is that the code path that runs when you actually use the app and the code path that is being encountered in those tests are actually two separate code paths with different outcomes, and it's really tricky to see which is which by just looking at the code in the specs.

Removing the explicit setting of request headers in that spec and removing the last test in it fixes the tests, and there's nothing to fix outside of the specs because it doesn't actually throw that error outside of the scenarios in the test suite.

[mkllnk]

I just rebased on master which includes the fix (previously last commit) already.

[mkllnk]

Let's test this again.

[filipefurtad0]

Hey @rioug @abdellani ,

Thanks for the fix and the review. Going through the tests:

1) General test

In the back office logged as an enterprise user :

Add a voucher for your shop 

On the website logged as a customer :

Add one or more product in your cart from the shop above
Proceed to checkout
Enter details and shipping info, click on "Next - Payment method"
--> You should see a "Apply voucher" section :heavy_check_mark:

Applying the voucher adds a negative amount to the order.

2) Taxes and invoices

It is possible to consider taxes on the invoice amounts:

Related test cases:

• inserting an invalid voucher name

• adding a voucher which is higher than the order amount

Ok, so I'd say that the bare minimum is implemented 🎉
I guess we can merge this one?

There are some issues thought - which will perhaps be addressed in the other related PR, but here in advance:

x) Edge cases

i) creating a voucher with a negative value At the moment, we can only create vouchers with an amount of 10 👍
ii) A cart has items of different tax categories; which tax category is applied to the voucher? I'm not clear if this is the intended result - an order with one line item only, generates this order confirmation:

iii) adding a voucher; visiting the /cart page; emptying the cart -> 🐌
https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/646250cd638dcc0008cadb24?event_id=646250cd00bd31d0dbc40000&i=sk&m=nw
iv) Transaction fees are not considered, when adding the voucher:

A user can see the warning that the voucher is insufficient to cover for the order total, however this happens on the /payment step. In this case, the voucher is displayed as the part of the order which it does not cover for
order total = 12
voucher = 10 (displayed as -2)

This is behind a feature toggle - so I think we can merge. Moving to ready to go!

[mkllnk]

Great testing, I will merge this. It's good as a first iteration but we should create issues for what you found.

adding a voucher; visiting the /cart page; emptying the cart -> snail

Definitely needs an issue.

Transaction fees are not considered, when adding the voucher

Transaction fees are applied after the voucher. Gaetan knew this issue but the adjustments should be updated during checkout and the final result should be correct. I'm confused by your examples. A voucher amount of -$2 doesn't make sense to me in any case. I find the tax amounts confusing as well. I would need to know all the items and their tax rates to verify that it's correct.

A cart has items of different tax categories; which tax category is applied to the voucher?

All of them. Well, the current implementation just looks at the totals. So if you have a $10 order with a total tax of $2 included (coming from different tax categories, doesn't matter) and you apply a $10 voucher then it should have -$2 included tax taken off. It's all covered. If you apply a $5 voucher then it should take only -$1 tax off.

It currently assumes that there's only included tax or only excluded tax, no mix.

[filipefurtad0]

Thanks @mkllnk ,

Yes, indeed - I'll have to investigate each of these test cases, and make issues out of them.

Gaetan knew this issue but the adjustments should be updated during checkout and the final result should be correct.

Ok, great - that was my main point really, to make sure we're all aware of this.

PS and a comment a bit outside the testing scope: with the current design, the customer may see an unnecessary "voucher value higher than order" message on step 2, as the order total may become higher than the voucher (due to the transaction fee), on step 3. I think this is misleading, and may suggest customers to place more items in the cart, to make best use of the voucher (i.e., not proceeding with checkout, going back to /shop).

Maybe we could consider displaying that message on step 3 instead?

[mkllnk]

PS and a comment a bit outside the testing scope: with the current design, the customer may see an unnecessary "voucher value higher than order" message on step 2, as the order total may become higher than the voucher (due to the transaction fee), on step 3. I think this is misleading, and may suggest customers to place more items in the cart, to make best use of the voucher (i.e., not proceeding with checkout, going back to /shop).

Maybe we could consider displaying that message on step 3 instead?

That's a good idea from the UX perspective. I'm not sure why Gaetan didn't do it, maybe it's difficult. Or is Step 3 only displayed on some instances?

[filipefurtad0]

Thanks @mkllnk , I'll raise this on Slack also as discussed in delivery circle.

Or is Step 3 only displayed on some instances?

Step 3 is always displayed