-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Display additional tax total in order - fixes #11680 #12427
Display additional tax total in order - fixes #11680 #12427
Conversation
25382be
to
c833e78
Compare
Hi @abdulazizali77 👋 I see this is still a draft PR. |
22d17a3
to
1755726
Compare
@sigmundpetersen apologies for the delay, PTAL and review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help @abdulazizali77 🙏 !
The PR looks fine, but there are a couple things I would like you to change, see my comments.
The spec is fine, we could probably achieve the same with a per_item
enterprise fee but no need to change that.
I don't think we need to cover the tax included in the price here, but just to be sure I updated the test note so our testers can double check that.
1755726
to
8c8f4c7
Compare
Add new text key admin.order.edit.additional_tax_included_in_price Add spec file for additional tax display. Add new trait for enterprise fee and calculator factory
8c8f4c7
to
82f75e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, we need a better name for the translatable text.
NOTE for the future, once a PR has been reviewed it's best not to force push on the branch as we loose the commit history, and it can make it hard for other reviewer to follow what happened.
Hey @abdulazizali77 , I've verified that indeed taxes over enterprise fees are not appearing on the order edit page, for our both tax types (included / added):
After this PR, we can see that the tax over the enterprise fee is appearing correctly - however, this is only the case, when the tax is is inclided to the enterprise fee:
I'd say let's merge this PR. This is much more critical when the tax is added, as it leads to wrong totals. So, I'm not actually even sure we need this to work for inclued tax... In anycase and for now: merging! |
4f7d5ad
into
openfoodfoundation:master
What? Why?
The admin/orders#edit form doesnt display additional tax adjustments
UNCERTAIN
What should we test?
Tax included in price
Same scenario as above but with Tax Rate included in the price. Check it's working as before.
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.