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.

Sign up for GitHub

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

Jump to bottom

fix: enable discount code checks for over consumption #6055

Merged
merged 6 commits into from Aug 3, 2019
Merged

fix: enable discount code checks for over consumption #6055

merged 6 commits into from Aug 3, 2019

Conversation

uds5501
Copy link
Contributor

@uds5501 uds5501 commented Jun 17, 2019 •
edited

Fixes #6102

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests 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)
  • All the functions created/modified in this PR contain relevant docstrings.

Short description of what this resolves:

This PR fixes the existing tests for checking if Discount Code is exhausted or not upon application

@auto-label auto-label bot added the fix label Jun 17, 2019
houndci-bot
houndci-bot reviewed Jun 17, 2019
View reviewed changes
app/api/helpers/ticketing.py Outdated Show resolved Hide resolved
app/api/helpers/ticketing.py Outdated Show resolved Hide resolved
app/api/helpers/ticketing.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 17, 2019 •
edited

Codecov Report

Merging #6055 into development will increase coverage by <.01%.
The diff coverage is 18.18%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6055      +/-   ##
===============================================
+ Coverage        65.54%   65.54%   +<.01%     
===============================================
  Files              286      286              
  Lines            14589    14592       +3     
===============================================
+ Hits              9562     9564       +2     
- Misses            5027     5028       +1
Impacted Files Coverage Δ
app/api/orders.py 29.49% <14.28%> (+0.23%) ⬆️
app/api/helpers/ticketing.py 17.54% <25%> (+0.57%) ⬆️

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 9fc0c20...74bb0d5. Read the comment docs.

@mrsaicharan1 mrsaicharan1 mentioned this pull request Jun 18, 2019
Merged
5 tasks
@uds5501
Copy link
Contributor Author

uds5501 commented Jun 21, 2019

@mrsaicharan1 @shreyanshdwivedi @prateekj117 Please review

mrsaicharan1
mrsaicharan1 suggested changes Jun 22, 2019
View reviewed changes
app/api/helpers/ticketing.py
for holder in ticket_holders:
ticket_holder = TicketHolder.query.filter_by(id=holder).one()
if ticket_holder.ticket.id in discount_code.tickets.split(","):
if ticket_holder.ticket.id in ticket_ids:
qty += 1
if (qty + old_holders) <= discount_code.tickets_number and \
Copy link
Member

Choose a reason for hiding this comment

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

At this condition, what you could do is, suppose the maximum number of tickets on which the discount code can be applied is 3(considering the discounted tickets which have been bought) and you are trying to order 5 tickets, apply the discount only on 3. This is how it's implemented on other platforms, so I believe it would be better if you could do this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrsaicharan1 should I break this into 2 sub-issues?

  • This PR could serve as a check for discount code's over consumption
    while the second sub issue will focus on application of discount code in the way you suggested?

Copy link
Member

Choose a reason for hiding this comment

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

@uds5501 Yeah, that would suffice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrsaicharan1 I have linked it to the appropriate sub-issue. Please take a look

mrsaicharan1
mrsaicharan1 previously approved these changes Jun 30, 2019
View reviewed changes
Copy link
Member

@mrsaicharan1 mrsaicharan1 left a comment

Choose a reason for hiding this comment

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

For the first sub issue of preventing over consumption, this is fine.

kushthedude
kushthedude previously approved these changes Jun 30, 2019
View reviewed changes
iamareebjamal
iamareebjamal previously approved these changes Jun 30, 2019
View reviewed changes
Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

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

Approving this for now, but the logic is very naive. This kind of calculations should not be done in python, but using queries in the DB

Please open an issue to refactor this logic

@uds5501
Copy link
Contributor Author

uds5501 commented Jun 30, 2019

@iamareebjamal Understood, will open an issue regarding this.

@uds5501 uds5501 mentioned this pull request Jul 1, 2019
Closed
prateekj117
prateekj117 previously approved these changes Jul 8, 2019
View reviewed changes
@mrsaicharan1
Copy link
Member

@uds5501 We can merge this and work on DB level queries in a follow-up PR as soon as you resolve the conflicts :D

@iamareebjamal iamareebjamal merged commit 2c05d30 into fossasia:development Aug 3, 2019
mrsaicharan1 pushed a commit to mrsaicharan1/open-event-server that referenced this pull request Aug 9, 2019
@uds5501 @mrsaicharan1
fix: enable discount code checks for over consumption (fossasia#6055)
2753e7a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@prateekj117 prateekj117 prateekj117 left review comments

@abhinavk96 abhinavk96 Awaiting requested review from abhinavk96

@shreyanshdwivedi shreyanshdwivedi Awaiting requested review from shreyanshdwivedi

@iamareebjamal iamareebjamal Awaiting requested review from iamareebjamal

@mrsaicharan1 mrsaicharan1 Awaiting requested review from mrsaicharan1

@kushthedude kushthedude Awaiting requested review from kushthedude

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sub issue : prevent order placement on potential overuse of discount code
7 participants
@uds5501 @mrsaicharan1 @iamareebjamal @houndci-bot @prateekj117 @kushthedude @mariobehling