-
Notifications
You must be signed in to change notification settings - Fork 555
feat: Add tax for tickets #2206
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
Conversation
|
Thanks. Please also provide an example for countries where tax is included in the price. Check how this works in Europe on Eventbrite for example and also check the frontend, please. |
app/src/main/java/org/fossasia/openevent/general/OpenEventDatabase.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/fossasia/openevent/general/ticket/TicketsViewModel.kt
Outdated
Show resolved
Hide resolved
| val code = appliedDiscountCode | ||
| tickets.forEach { ticket -> | ||
| var price = ticket.price | ||
| totalTaxAmount += (ticket.price * taxRate / 100) * qty[index] |
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.
Why are we not using it.second instead of maintaining an index?
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.
it is nothing here. We are fetching the list of tickets and qty stored in an array.
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.
Why not use ticketAndQty?
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.
I think so. It's quite complex. Will try 👍
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.
It's not. it was created exactly so that index can be avoided
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.
No, it is a list of pair. So you iterate over it without the need of index
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.
Iterating is already used on a list of tickets which is getting from the database for the ticket price. So for this, two parallel iterators are required.
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.
Don't use tickets for iteration, use ticketAndQty, that's why it was created by @anhanh11001
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.
Well then ticketAndQty doesn't have the ticket price
E/TicketsViewModel: (47, 1, 0.0)
E/TicketsViewModel: (48, 2, 0.0)
So here we need to fetch selected tickets to get price. Only donation tickets have price at third place in ticketAndQty.
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.
Merging, but open an issue to either use or remove ticketAndQty
anhanh11001
left a comment
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.
Rest looks good to me
app/src/main/java/org/fossasia/openevent/general/ticket/TicketViewHolder.kt
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| price.let { prices += price * qty[index++] } | ||
| price.let { prices += price * qty[index] } |
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.
Unnecessary change?
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.
Not unnecessary, the index is updated at the end.


Fixes #2120