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

Include voucher tax adjustment in revenues by hub report #11914

Conversation

bmd08a1
Copy link
Contributor

@bmd08a1 bmd08a1 commented Dec 9, 2023

What? Why?

  • Include voucher tax adjustments on Revenues by hub report
  • Solution proposed:
    • Build tax data of grouped orders (by distributor id):
    • orders.total_included_tax: orders.sum(&:total)
    • orders.total_tax: orders.sum(&:total_tax) + orders.voucher_tax_adjustments
    • orders.total_excluded_tax: orders.total_included_tax - orders.total_tax

What should we test?

  • Set up an enterprise with vouchers and taxes.
  • Place three orders (without tax, with tax incl. in price and with added tax)
  • Run the report.
  • Check the numbers (compare to order details and adjustments in back office).

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

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

Dependencies

Documentation updates

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. Can you improve the spec setup?

spec/system/admin/reports/revenues_by_hub_spec.rb Outdated Show resolved Hide resolved
@mkllnk mkllnk requested a review from rioug December 10, 2023 23:05
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this ! Vouchers can be a bit tricky to understand let us know if you need some help.

spec/system/admin/reports/revenues_by_hub_spec.rb Outdated Show resolved Hide resolved
distributor: distributor3,
product_price: 110.0,
tax_rate_amount: 0.1,
included_in_price: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be included_in_price: false to get tax excluded from price. In this case it's not affecting the result as you are mocking VoucherAdjustmentsService

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Just one last thing, see my comment.
Also. we prefer to rebase the branch on master instead of merging.

Thanks !

order.update_shipping_fees!
order.update_order!

VoucherAdjustmentsService.new(order).update
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to add order.update_totals_and_states after VoucherAdjustmentsService.new(order).update otherwise the various order total won't take into account the voucher adjustment you just updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I updated the specs as you suggested

@rioug rioug requested a review from mkllnk December 21, 2023 22:46
@drummer83 drummer83 changed the title include voucher tax adjustment in revenues by hub report Include voucher tax adjustment in revenues by hub report Dec 22, 2023
@drummer83 drummer83 self-assigned this Dec 22, 2023
@drummer83 drummer83 added no-staging-UK A tag which does not trigger deployments, indicating a server is being used pr-staged-uk staging.openfoodnetwork.org.uk and removed no-staging-UK A tag which does not trigger deployments, indicating a server is being used labels Dec 22, 2023
@drummer83
Copy link
Contributor

Hi @bmd08a1,
Thank you for this important fix! 💪

I did some testing and here are the results.

Each order had a value of 10 $, tax (if applied) was 10 %, fees (if applied) were 10 %, voucher (always applied) was 10 %.

I placed three orders before staging your PR:

  • without tax,
  • with added tax,
  • with included tax.

After staging the PR I placed some orders again:

  • same three orders as before,
  • additional orders to check shipping fees and enterprise fees.

Results

No tax

Before:
image
After:
image

Tax excluded

Before:
image
After:
image

Tax included

Before:
image
After:
image

Total of all three orders

Before:
image
After:
image

With 1 $ shipping fee (no tax)

After - No tax:
image
After - Tax excl.:
image
After - Tax incl.:
image

Additional enterprise fee of 10 % (tax applies)

After - Tax excl.:
image

Summary

Excellent work! 💪 I couldn't find any issues with this. There are more complex scenarios for sure, but I tried to cover it systematically.

This one is ready for merging! 🎉 🚀
Thanks again!!!

@drummer83 drummer83 merged commit 398be49 into openfoodfoundation:master Dec 22, 2023
54 checks passed
@drummer83 drummer83 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Vouchers] [Reports] Include vouchers in report: Revenues by Hub
4 participants