feat: add a resend email route for organizers #6163

uds5501 · 2019-07-09T05:06:59Z

Fixes #6162

Short description of what this resolves:

This adds an endpoint which triggers re sending email confirmation for tickets

Changes proposed in this pull request:

Ensures that only organisers and co organisers have ability to resend the emails.

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.

app/api/orders.py

uds5501 · 2019-07-09T05:14:09Z

@mrsaicharan1 @prateekj117 @shreyanshdwivedi Please review

codecov · 2019-07-09T05:29:47Z

Codecov Report

Merging #6163 into development will decrease coverage by 0.1%.
The diff coverage is 31.57%.

@@               Coverage Diff               @@
##           development    #6163      +/-   ##
===============================================
- Coverage        66.18%   66.08%   -0.11%     
===============================================
  Files              288      288              
  Lines            14460    14497      +37     
===============================================
+ Hits              9571     9580       +9     
- Misses            4889     4917      +28

Impacted Files	Coverage Δ
app/api/auth.py	`24.39% <31.57%> (+0.26%)`	⬆️
app/api/tickets.py	`47.61% <0%> (-5.8%)`	⬇️
app/models/event.py	`79.04% <0%> (-0.22%)`	⬇️

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 2bfa553...6d93fc7. Read the comment docs.

niranjan94 · 2019-07-09T06:35:46Z

app/api/orders.py

        return redirect(make_frontend_url('orders/{}/view'.format(order_identifier)))
+
+
+@order_misc_routes.route('/orders/<string:order_identifier>/resend-email', methods=['POST'])


Ensure this endpoint has rate limiting

mrsaicharan1 · 2019-07-09T09:41:55Z

app/api/orders.py

+    :param order_identifier:
+    :return: JSON response if the email was succesfully sent
+    """
+    order = safe_query(db, Order, 'identifier', order_identifier, 'identifier')


Sometimes, the order may not be fetched. So I guess a try...except block would be great here :D

The failure is simply handled by a server 500 in the safe_query itself, IMO that should do

mrsaicharan1

Check this too!

app/api/auth.py

shreyanshdwivedi · 2019-07-10T06:01:06Z

app/api/auth.py

+    '3/hour', key_func=lambda: request.json['data']['order'], error_message='Limit for this action exceeded'
+)
+@limiter.limit(
+    '1/minute', key_func=get_remote_address, error_message='Limit for this action exceeded'


IMO 1/minute is very less for this threshold. An organizer can go on clicking the resent invite button on the client side so we should increase this. @iamareebjamal what can be a suitable threshold for this endpoint?

Maybe 3/minute ? @shreyanshdwivedi @iamareebjamal

60/minute for IP
5/minute for user_id

mrsaicharan1 · 2019-07-10T06:07:38Z

app/api/auth.py

+            invoice_path = 'generated/invoices/{}/{}/'.format(key, generate_hash(key)) + order_identifier + '.pdf'
+
+            # send email.
+            send_email_to_attendees(order=order, purchaser_id=current_user.id, attachments=[ticket_path, invoice_path])


I believe that attachments should be empty for placed orders as they're meant for offline tickets?

I think placed orders should have tickets too. They are meant to show this ticket when they reach event venue

I think placed orders should have tickets too. They are meant to show this ticket when they reach event venue

Agreed 👍

niranjan94 · 2019-07-10T12:45:52Z

app/api/auth.py

+            return jsonify(status=True, message="Verification emails for order : {} has been sent succesfully".
+                           format(order_identifier))
+        else:
+            return jsonify(status=False, message="Only placed and complete orders are verified")


This be raised as an error. Not a success response

@niranjan94 did it, please check.

iamareebjamal · 2019-07-11T12:05:26Z

app/api/auth.py

+
+@ticket_blueprint.route('/orders/resend-email', methods=['POST'])
+@limiter.limit(
+    '5/minute', key_func=lambda: request.json['data']['order'], error_message='Limit for this action exceeded'


Where is limit by user?

@iamareebjamal modified order to user key.

iamareebjamal · 2019-07-11T15:36:29Z

Did you test it?

uds5501 · 2019-07-11T16:22:04Z

@iamareebjamal yes, tested it via postman, it's working as expected.

uds5501 · 2019-07-12T05:22:48Z

@mrsaicharan1 @shreyanshdwivedi @prateekj117 Please review

iamareebjamal · 2019-07-12T09:15:26Z

app/api/auth.py

+            return jsonify(status=True, message="Verification emails for order : {} has been sent succesfully".
+                           format(order_identifier))
+        else:
+            return UnprocessableEntityError({'source': 'data/order'},


Why is it returning instead of raising?

@iamareebjamal because these errors are actually a subclass of ErrorResponse and not actual errors. To make them work we need to initiate their respond function (the same error classes have been used all through this file)

iamareebjamal · 2019-07-12T09:15:33Z

app/api/auth.py

+            return UnprocessableEntityError({'source': 'data/order'},
+                                            "Only placed and completed orders have confirmation").respond()
+    else:
+        return ForbiddenError({'source': ''}, "Co-Organizer Access Required").respond()


* add the resend email route * resend emails using ticket route * handle errors with UnprocessableEntry * modify order to user key

auto-label bot added the feature label Jul 9, 2019

houndci-bot reviewed Jul 9, 2019

View reviewed changes

app/api/orders.py Outdated Show resolved Hide resolved

app/api/orders.py Outdated Show resolved Hide resolved

app/api/orders.py Outdated Show resolved Hide resolved

add the resend email route

d71c128

uds5501 force-pushed the resend-emails-route branch from 32b8d44 to d71c128 Compare July 9, 2019 05:10

Merge branch 'development' into resend-emails-route

17b2604

niranjan94 suggested changes Jul 9, 2019

View reviewed changes

mrsaicharan1 reviewed Jul 9, 2019

View reviewed changes

houndci-bot reviewed Jul 10, 2019

View reviewed changes

app/api/auth.py Outdated Show resolved Hide resolved

app/api/auth.py Show resolved Hide resolved

fossasia deleted a comment Jul 10, 2019

uds5501 force-pushed the resend-emails-route branch from 86d3eb8 to 71c510a Compare July 10, 2019 04:45

fossasia deleted a comment Jul 10, 2019

shreyanshdwivedi reviewed Jul 10, 2019

View reviewed changes

mrsaicharan1 suggested changes Jul 10, 2019

View reviewed changes

uds5501 mentioned this pull request Jul 10, 2019

feat: implement resend email confirmation button fossasia/open-event-frontend#3251

Merged

5 tasks

resend emails using ticket route

639a187

uds5501 force-pushed the resend-emails-route branch from 89dbaea to 639a187 Compare July 10, 2019 10:45

fossasia deleted a comment Jul 10, 2019

niranjan94 suggested changes Jul 10, 2019

View reviewed changes

handle errors with UnprocessableEntry

013ed41

fossasia deleted a comment Jul 10, 2019

Merge branch 'development' into resend-emails-route

94d374f

fossasia deleted a comment Jul 11, 2019

iamareebjamal reviewed Jul 11, 2019

View reviewed changes

modify order to user key

6d93fc7

fossasia deleted a comment Jul 11, 2019

iamareebjamal reviewed Jul 12, 2019

View reviewed changes

mrsaicharan1 approved these changes Jul 12, 2019

View reviewed changes

iamareebjamal merged commit 336ec2d into fossasia:development Jul 12, 2019

		return redirect(make_frontend_url('orders/{}/view'.format(order_identifier)))


		@order_misc_routes.route('/orders/<string:order_identifier>/resend-email', methods=['POST'])

feat: add a resend email route for organizers #6163

feat: add a resend email route for organizers #6163

Uh oh!

Conversation

uds5501 commented Jul 9, 2019

Short description of what this resolves:

Changes proposed in this pull request:

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uds5501 commented Jul 9, 2019

Uh oh!

codecov bot commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrsaicharan1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrsaicharan1 Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrsaicharan1 Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamareebjamal commented Jul 11, 2019

Uh oh!

uds5501 commented Jul 11, 2019

Uh oh!

uds5501 commented Jul 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jul 9, 2019 •

edited

Loading

mrsaicharan1 Jul 10, 2019 •

edited

Loading

mrsaicharan1 Jul 10, 2019 •

edited

Loading