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

fix: Calculate Tax properly #7843

Merged
merged 6 commits into from
Sep 22, 2021
Merged

Conversation

divyamtayal
Copy link
Member

@divyamtayal divyamtayal commented Sep 22, 2021

Fixes #7835

Short description of what this resolves:

If we buy paid tickets of more than one type, the tax on orders page shows 0.00 for all tickets. There is probably some calculation issue in tax system.

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@vercel
Copy link

vercel bot commented Sep 22, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/6k1eFMBvTp2sV8Dpmkqu4rXaEMgZ
✅ Preview: https://open-event-frontend-git-fork-daretobedifferent1-260f52-eventyay.vercel.app

@divyamtayal divyamtayal changed the title Calculate Tax properly fix: Calculate Tax properly Sep 22, 2021
@auto-label auto-label bot added the fix label Sep 22, 2021
@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #7843 (42a3824) into development (cd012e9) will increase coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 42a3824 differs from pull request most recent head 421017f. Consider uploading reports for the commit 421017f to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development    #7843      +/-   ##
===============================================
+ Coverage        18.56%   18.59%   +0.02%     
===============================================
  Files              613      614       +1     
  Lines             7201     7206       +5     
  Branches           149      149              
===============================================
+ Hits              1337     1340       +3     
- Misses            5837     5839       +2     
  Partials            27       27              
Impacted Files Coverage Δ
app/models/order.js 0.00% <0.00%> (ø)
app/components/tabbed-navigation.js 53.33% <0.00%> (+20.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd012e9...421017f. Read the comment docs.

if (taxType) {
return ((taxRate * this.amount) / (100 + taxRate)).toFixed(2);
} else {
return ((this.amount) / (1 + taxRate / 100) * (taxRate / 100)).toFixed(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share thoght process behind this.

Copy link
Member Author

@divyamtayal divyamtayal Sep 22, 2021

Choose a reason for hiding this comment

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

Let x be amount without taxIncluded
So we need to calculate tax which will can be calculated by totalAmount = x + x*taxRate/100;
where x * taxRate / 100 is tax added on top of it which is given by

totalAmount / ( 1 + taxRate / 100 ) * ( taxRate / 100 )  which gives
(taxRate * totalAmount) / (100 + taxRate)

here both if else statement means same, so I'll change that.

@maze-runnar pls have a look

@maze-runnar
Copy link
Contributor

ok, so if my ticket price is 100, and the tax rate is 50%. And I have selected tax is included in the price, it is showing 33.33 tax amount price.

@divyamtayal
Copy link
Member Author

divyamtayal commented Sep 22, 2021

ok, so if my ticket price is 100, and the tax rate is 50%. And I have selected tax is included in the price, it is showing 33.33 tax amount price.

@maze-runnar it is correct because 100 is ticket price inlcuding tax so actual ticket is of 66.66 + 33.33(tax) which gives 100
and if tax would be on top than total would be 100 + 50 = 150

@maze-runnar
Copy link
Contributor

ok, so if my ticket price is 100, and the tax rate is 50%. And I have selected tax is included in the price, it is showing 33.33 tax amount price.

@maze-runnar it is correct because 100 is ticket price inlcuding tax so actual ticket is of 66.66 + 33.33(tax) which gives 100

yes, it is 🔥. good work.

@maze-runnar maze-runnar merged commit e09a2bd into fossasia:development Sep 22, 2021
@divyamtayal divyamtayal deleted the tax#7835 branch September 22, 2021 15:45
@mariobehling
Copy link
Member

This did not solve the issue. Tax is still not showing up e.g. compare here https://test.eventyay.com/e/554ef820

Screenshot from 2021-09-23 05-37-24

@divyamtayal
Copy link
Member Author

divyamtayal commented Sep 23, 2021

@mariobehling This porblem is not showing on localsystem.
I think there is some problem in new changes been merges not properly deplying or maybe not deployed to
https://eventyay.com/

bcz the changes made in PR #3820 is also not showing (below ScreenShot).
Screenshot from 2021-09-23 11-12-11

I think the problem is in deployment and same goes with this PR.
@maze-runnar pls have a look at this problem

@mariobehling
Copy link
Member

@daretobedifferent18 We are not deploying on live server. The server with the latest changes of the dev branch is https://test.eventyay.com.

@divyamtayal
Copy link
Member Author

@mariobehling ok let me see what is the problem

@divyamtayal divyamtayal restored the tax#7835 branch September 23, 2021 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tax is not being calculated properly while ordering tickets
3 participants