Skip to content

Conversation

@shreyanshdwivedi
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi commented Aug 15, 2019

Fixes #6323

Changes proposed in this pull request:

  • Adds an API to fetch total order amount by posting tickets, their amount, event and discount code
    Payload
           {
                "tickets": [
                	{"id": "32", "quantity": 2, "price": 100},
                	{"id": "31", "quantity": 3}
                ],
                "discount-code": "16"
            }

Response

{
    "tax_included": false,
    "tickets": [
        {
            "discount": {},
            "id": 32,
            "name": "donation",
            "price": 100,
            "quantity": 1,
            "sub_total": 110.5,
            "tax": {
                "amount": 10.5,
                "percent": 10.5
            },
            "ticket_fee": 0
        },
        {
            "discount": {
                "amount": 15,
                "code": "s1",
                "percent": 17.65
            },
            "id": 31,
            "name": "paid",
            "price": 85,
            "quantity": 1,
            "sub_total": 153.85,
            "tax": {
                "amount": 7.35,
                "percent": 10.5
            },
            "ticket_fee": 76.5
        }
    ],
    "total_amount": 264.35,
    "total_discount": 15,
    "total_tax": 17.85
}

Checklist

Ensure it works:

  • with event including tax
  • excluding tax,
  • with discount code,
  • without discount code,
  • with donation ticket price
  • without donation ticket price

@auto-label auto-label bot added the feature label Aug 15, 2019
@shreyanshdwivedi shreyanshdwivedi force-pushed the orderAmount branch 2 times, most recently from 228b048 to 8eb03ed Compare August 15, 2019 10:43
@shreyanshdwivedi
Copy link
Member Author

@uds5501 @kushthedude @mrsaicharan1 can you guys please test and review this PR?

@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #6366 into development will decrease coverage by 0.27%.
The diff coverage is 10%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6366      +/-   ##
===============================================
- Coverage        64.74%   64.47%   -0.28%     
===============================================
  Files              288      288              
  Lines            14966    15039      +73     
===============================================
+ Hits              9690     9696       +6     
- Misses            5276     5343      +67
Impacted Files Coverage Δ
app/api/orders.py 27.15% <0%> (ø) ⬆️
app/api/helpers/ticketing.py 16.8% <10%> (-0.74%) ⬇️
app/api/helpers/order.py 23.96% <3.63%> (-16.95%) ⬇️
app/api/auth.py 24.3% <35.71%> (+0.26%) ⬆️

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 003bf08...ef3a70e. Read the comment docs.

@iamareebjamal
Copy link
Member

Great job. Event key should support event ID as well

@iamareebjamal
Copy link
Member

Secondly, create a function which takes tickets, event and discount_code as arguments and returns the computed dictionary so that it can be tested, and of course there will be several tests for this:

  • with event including tax
  • excluding tax,
  • with discount code,
  • without discount code,
  • with donation ticket

so that we have confidence it is working and fidelity for future

@shreyanshdwivedi
Copy link
Member Author

Sure. Will add functions and tests by tonight

@iamareebjamal
Copy link
Member

Also, use marshmallow for validation

@iamareebjamal
Copy link
Member

iamareebjamal commented Aug 15, 2019

Actually, event ID should not be needed. It can be extracted from ticket. Just verify every ticket has same event ID

@shreyanshdwivedi
Copy link
Member Author

Ok. Will make mentioned changes

@iamareebjamal
Copy link
Member

Also, you can't just return the amount in JSON. You have to return a full object with price breakdown of individual tickets with tax and what not

@shreyanshdwivedi
Copy link
Member Author

Also, you can't just return the amount in JSON. You have to return a full object with price breakdown of individual tickets with tax and what not

Yeah I was thinking about the same. Will return amount for each ticket, tax included in price or on the price, discount and overall total

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Aug 16, 2019

Currently, the response is -

{
    "grand_total": 270,
    "tickets": [
        {
            "discount_amount": 0,
            "discount_percent": 0,
            "exclusive_tax": 0,
            "id": 32,
            "inclusive_tax": 0,
            "price": 100,
            "quantity": 2,
            "sub_total": 200
        },
        {
            "discount_amount": 15,
            "discount_percent": 17.65,
            "exclusive_tax": 0,
            "id": 31,
            "inclusive_tax": 0,
            "price": 85,
            "quantity": 1,
            "sub_total": 70
        }
    ],
    "total_discount": 15,
    "total_tax": 0
}

If anyone wants anything else to be returned alongwith this info, please comment. Till then I'll test it out for different cases and add test to the order helper

@iamareebjamal
Copy link
Member

OK, how price of 158 with inclusive tax of 9.87 and discount of 16.59 amount to sub_total of 282.82?

grand_total -> total_price
Add discount_percentage and discount_amount

Will add more recommendations on tax after seeing more examples

@shreyanshdwivedi
Copy link
Member Author

For tax thing -> (158 - 16.59)*2
sub total includes multiplication with quantity too

About discount_percentage, so u need to calculate both % and amount for all cases of discount whether it is amount or percent type or not? Makes sense. Will help others to display on client directly.

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal I've tested all the cases mentioned by you in PR description. Should I add test for these cases too? If yes, then what should I test, sub-total, by replicating calculation in the helper?

@iamareebjamal
Copy link
Member

I think adding API documentation will serve both purposes, document it and also test the implementation

However, I will add more fields, so hold on to that

@anhanh11001 @liveHarshit, Please review the response and if it needs anything else

@liveHarshit
Copy link
Member

@anhanh11001 @liveHarshit, Please review the response and if it needs anything else

How should I test it as it is not merged in development? I think @anhanh11001 has local setup for the server?

@shreyanshdwivedi
Copy link
Member Author

Sub_total = (price + tax - discount)*quantity

Basically a user can get details of one ticket by adding tax to price (if tax is exclusive) and then subtract discount to get an effective price for a ticket

@shreyanshdwivedi
Copy link
Member Author

Also we cannot have exclusive and inclusive tax at the same time for an event. I've made changes to local and will push it once we are have other's feedback too

@shreyanshdwivedi
Copy link
Member Author

@uds5501 @mrsaicharan1 @CosmicCoder96 can you guys check this once? Please refer to the PR description for lastest request and response data format

@fossasia fossasia deleted a comment Aug 18, 2019
@iamareebjamal
Copy link
Member

Please remove the data section in request. It's unneeded

@iamareebjamal
Copy link
Member

And discount should be null if not applied

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Aug 18, 2019

@iamareebjamal removed data from request and also now if discount code or tax is not applied the discount/tax key have null data

@shreyanshdwivedi
Copy link
Member Author

@uds5501 @mrsaicharan1 @iamareebjamal please review this PR

@iamareebjamal
Copy link
Member

Is the discount code ticket limit and maximum cutoff taken into account?

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal currently they are being checked when we send a POST request to create order.

@iamareebjamal
Copy link
Member

But the amount will be wrong if you don't check it here

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Aug 20, 2019

Agreed. Will apply checks here too

@shreyanshdwivedi
Copy link
Member Author

Was caught up in finalizing work product and other high priority pending PRs. Will start to finalize this now

@fossasia fossasia deleted a comment Aug 27, 2019
@shreyanshdwivedi shreyanshdwivedi force-pushed the orderAmount branch 2 times, most recently from 8fea2a5 to cb31c21 Compare August 29, 2019 16:36
@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal I've included checks for overconsumption and service fees. Please have a look to updated response in the PR description
I checked for discount_code consumption. I set max discount tickets to be 1 and then tried for 2 tickets, error was produced as expected.
And the ticket service fee levied is 90% on INR with maximum_fee of 90. Please check

… code

adds function to order helper

adds checks for donation tickets

adds discount code overconsumption check

included ticket service fee calculation
@iamareebjamal
Copy link
Member

Error should not be produced. Discount code should only be applied to 1 ticket

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal actually the current implementation while ordering tickets follow the same thing and produces error in case of overconsumption.
Can we handle this in a follow up issue?

@iamareebjamal iamareebjamal merged commit 31c0b86 into fossasia:development Aug 30, 2019
@shreyanshdwivedi shreyanshdwivedi deleted the orderAmount branch August 30, 2019 10:11
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.

API for ticket pricing with discount code and tax included

4 participants