Skip to content

Conversation

@shreyanshdwivedi
Copy link
Member

Fixes #5874

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:

Currently their is no server side validation of data sent on creating/editing a discount code.

Changes proposed in this pull request:

  • Adds check for discount amount and discount %

@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #5880 into development will decrease coverage by <.01%.
The diff coverage is 73.33%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5880      +/-   ##
===============================================
- Coverage        66.17%   66.17%   -0.01%     
===============================================
  Files              285      285              
  Lines            14133    14118      -15     
===============================================
- Hits              9353     9343      -10     
+ Misses            4780     4775       -5
Impacted Files Coverage Δ
.../integration/api/validation/test_discount_codes.py 98.52% <100%> (+1.09%) ⬆️
app/api/schema/discount_codes.py 59.25% <44.82%> (-3.95%) ⬇️
app/api/helpers/auth.py 89.47% <0%> (-2.53%) ⬇️
app/api/auth.py 21.3% <0%> (-2.05%) ⬇️
app/models/ticket.py 71.73% <0%> (-1.18%) ⬇️
tests/all/integration/api/helpers/test_auth.py 97.22% <0%> (-0.7%) ⬇️
config.py 87.32% <0%> (-0.52%) ⬇️
app/models/setting.py 88.59% <0%> (-0.45%) ⬇️
app/__init__.py 86.78% <0%> (-0.16%) ⬇️
app/api/schema/settings.py 100% <0%> (ø) ⬆️
... and 5 more

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 6d69104...e5b08ba. Read the comment docs.

@shreyanshdwivedi shreyanshdwivedi force-pushed the checkDiscount branch 3 times, most recently from 2b47e7f to a8ba942 Compare May 14, 2019 00:30
@fossasia fossasia deleted a comment May 14, 2019
@fossasia fossasia deleted a comment May 14, 2019
@fossasia fossasia deleted a comment May 14, 2019
@fossasia fossasia deleted a comment May 14, 2019
@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal I've added a new file to add new tests in unit test section. Please review
@mrsaicharan1 @uds5501 please review

@iamareebjamal
Copy link
Member

Please handle percent by marshmallow schema

@fossasia fossasia deleted a comment Jun 4, 2019
@fossasia fossasia deleted a comment Jun 4, 2019
@shreyanshdwivedi
Copy link
Member Author

@CosmicCoder96 @iamareebjamal I've finalized my PR and included checks and tests for free tickets.
Idk why travis is failing. Such errors are occuring in other PRs as well. Please review and let me know if the travis is failing due to my PR.

@iamareebjamal
Copy link
Member

iamareebjamal commented Jun 5, 2019

The error was straightforward and right there in the clear:

  File "/home/travis/build/fossasia/open-event-server/app/api/schema/discount_codes.py", line 172, in validate_value
    for ticket in data['tickets']:
KeyError: 'tickets'

let me know if the travis is failing due to my PR.

Yes

@shreyanshdwivedi shreyanshdwivedi force-pushed the checkDiscount branch 2 times, most recently from 2f7e1b6 to a989538 Compare June 5, 2019 12:28
@fossasia fossasia deleted a comment Jun 5, 2019
added tests

Updated tests

adds check for negative discount amount and percent

removes validation for event discount code

adds check for free tickets
@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal @uds5501 @CosmicCoder96 I've finalized the PR and fix the build. Please review

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal please review

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal @niranjan94 please review

@fossasia fossasia deleted a comment Jun 17, 2019
Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

LGTM !!

@abhinavk96 abhinavk96 requested a review from iamareebjamal June 18, 2019 17:11
@fossasia fossasia deleted a comment Jun 18, 2019
@bhaveshAn bhaveshAn requested a review from pradeepgangwar June 19, 2019 19:28
if 'tickets' in data:
for ticket in data['tickets']:
ticket_object = Ticket.query.filter_by(id=ticket).one()
if not ticket_object.price:
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it be compared to zero ? for clarity?

Copy link
Member Author

Choose a reason for hiding this comment

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

@CosmicCoder96 actually we are allowing none for price attribute of ticket. Though for front-end we send 0 in case ticket price is not set but server is also for android. Maybe there are tickets present with None as ticket price so it may cause discrepancies
price = fields.Float(validate=lambda n: n >= 0, allow_none=True)
I checked it just now. Should I compare explicitly?

Copy link
Contributor

Choose a reason for hiding this comment

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

@shreyanshdwivedi okay, cool then don't change it.

@abhinavk96 abhinavk96 requested review from iamareebjamal and removed request for iamareebjamal June 22, 2019 12:25
@mariobehling mariobehling merged commit 6d0f957 into fossasia:development Jun 22, 2019
iamareebjamal pushed a commit to iamareebjamal/open-event-server that referenced this pull request Aug 2, 2019
added tests

Updated tests

adds check for negative discount amount and percent

removes validation for event discount code

adds check for free tickets
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.

Add checks on creating/editing discount codes

8 participants