-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: restricts unverified user for buying free tickets #6140
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
|
@iamareebjamal Please review. |
Codecov Report
@@ Coverage Diff @@
## development #6140 +/- ##
==============================================
- Coverage 66.13% 66.1% -0.03%
==============================================
Files 288 288
Lines 14479 14484 +5
==============================================
Hits 9575 9575
- Misses 4904 4909 +5
Continue to review full report at Codecov.
|
kushthedude
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.
@prateekj117 Please add validation to check if there is a discount code or not, If there is then don't throw 422
| raise UnprocessableEntity( | ||
| {'pointer': '/data/relationships/ticket'}, | ||
| {'pointer': '/data/relationships/order'}, | ||
| "Unverified user cannot buy free tickets" |
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.
Unverified user cant place free orders @prateekj117
|
@kushthedude This check is before the discount code is applied. |
Nope, I can't place free order with the discount code, Please check again . |
@prateekj117 Please fill the template and if not at least fill the issue number it fixes, Makes it easy to automatically close the issue |
iamareebjamal
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.
Each and every change in this PR is wrong
mrsaicharan1
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.
Try to re-iterate this PR. maybe that would help in solving this in a better way.
|
@iamareebjamal Please review. |
|
@iamareebjamal Please review. |
app/api/orders.py
Outdated
|
|
||
| ticket = db.session.query(Ticket).filter_by( | ||
| id=int(ticket_holder_object.ticket_id), deleted_at=None | ||
| ).first() |
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.
What about an order with free & paid tickets for unverified users? They should be blocked as well too right?
|
No they shouldnt , and also change the title of PR yo just free orders ,
not free tickets!
…On Tue, 9 Jul 2019 at 5:55 PM, Saicharan Reddy ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In app/api/orders.py
<#6140 (comment)>
:
> @@ -86,6 +87,16 @@ def before_create_object(self, data, view_kwargs):
raise ConflictException({'pointer': '/data/relationships/attendees'},
"Attendee with id {} does not exists".format(str(ticket_holder)))
+ ticket = db.session.query(Ticket).filter_by(
+ id=int(ticket_holder_object.ticket_id), deleted_at=None
+ ).first()
What about an order with free & paid tickets for unverified users? They
should be blocked as well too right?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6140?email_source=notifications&email_token=AKQMTLX5XQ2LFBAKBERVDCDP6R7UFA5CNFSM4H56BRQKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB53RXUI#pullrequestreview-259464145>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKQMTLVWXN4XXSICUOSZSTLP6R7UFANCNFSM4H56BRQA>
.
|
|
@shreyanshdwivedi @uds5501 @mrsaicharan1 @iamareebjamal Please review. |
| :return: | ||
| """ | ||
|
|
||
| free_ticket_quantity = 0 |
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 is this variable declared so far from its use?
|
Looks fine to me, ask peers to review. Taking 8 days for a PR this trivial is not expected |
|
@shreyanshdwivedi @uds5501 @mrsaicharan1 Please review. |
shreyanshdwivedi
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.
Logic looks fine!!
* Restrict unverified user from making free orders. * Check ticket-type rather than payment-mode * No need to revert the old PR * Check if order contains all free tickets. * Required changes
Fixes #6034
Short description of what this resolves:
It adds a check to restrict an unverified user from placing free order.
Checklist
developmentbranch.