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

[Split Checkout] Recalculate tax on summary step #9631

Merged
merged 5 commits into from
Sep 8, 2022

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Sep 5, 2022

What? Why?

What should we test?

Release notes

Changelog Category: User facing changes

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

Dependencies

Documentation updates

And reduce code by referring to objects instead of just ids.
Somehow the helper methods were testing two different orders even though
each spec is using only one order, the one which is currently being
checked out by the user.

So I brought the code closer into the example to easier see what is
related and tested.

I also changed the assertions for the pending spec but can't really
verify them yet until I have completely working code.
It reduced the execution time by 5% on my machine (2-3 seconds).
@mkllnk mkllnk self-assigned this Sep 5, 2022
@mkllnk mkllnk force-pushed the 9153-tax-recalculation branch from e50b1ad to 1d04510 Compare September 5, 2022 05:56
@mkllnk mkllnk marked this pull request as ready for review September 5, 2022 06:16
Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

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

Nothing to add! 🙏

@drummer83 drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Sep 7, 2022
@drummer83 drummer83 self-assigned this Sep 7, 2022
@drummer83
Copy link
Contributor

Hi @mkllnk,
This one was tricky to understand in the beginning, but I think I got it right!

Before this PR

My understanding is that before this PR:

  • taxes are not applied when checking out with split checkout
  • taxes are updated correctly when editing the order as a customer
  • taxes are updated correctly when clicking update and recalculate fees as a shop manager
  • taxes are applied correctly when checking out with legacy checkout

I was able to confirm this before staging this PR.

Nunavut (no taxes), legacy checkout:
image
Taxes are not applied --> OK

Ontario (taxes), legacy checkout:
image
Taxes are applied --> OK

Nunavut (no taxes), split checkout:
image
Taxes are not applied --> OK

Ontario (taxes), split checkout:
image
Taxes are not applied --> not OK

After this PR

Split checkout: Taxes applied to products and shipping (shipping address inside tax zone and shipping method with tax)
image

Legacy checkout: Taxes applied to products and shipping (shipping address inside tax zone and shipping method with tax)
image

Split checkout: Taxes applied to products, but not to shipping (shipping address inside tax zone but shipping method without tax):
image

Legacy checkout: Taxes applied to products, but not to shipping (shipping address inside tax zone but shipping method without tax):
image

Split checkout: No taxes applied (shipping address outside tax zone):
image

Legacy checkout: No taxes applied (shipping address outside tax zone):
image

Results

  • The numbers on legacy and split checkout are identical now and they are correct.
  • I have also made sure that taxes are calculated according to shipping address only and not according to billing address. 👍

It's looking good. Moving to Ready to Go! 🚀

@drummer83 drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Sep 7, 2022
@drummer83
Copy link
Contributor

Hold on! Moving back to Test Ready...

@drummer83
Copy link
Contributor

Ok, I was wondering about this, but we have that in master as well...
image.png
Moving to Ready to Go again!

@mkllnk mkllnk merged commit f201b9f into openfoodfoundation:master Sep 8, 2022
@mkllnk mkllnk deleted the 9153-tax-recalculation branch September 8, 2022 01:39
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.

[Split checkout] Taxes are not re-calculated if customer changes state
4 participants