feat: Separate ticket for each attendee #7458

codedsun · 2020-11-19T17:18:15Z

Fixes #7397 feat: Ticket pdf path made with attendee identifier

Short description of what this resolves:

Changes proposed in this pull request:

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.

codedsun · 2020-11-19T17:22:44Z

Directory Structure

Ticket Holder

Ticket purchaser

codecov · 2020-11-20T17:23:24Z

Codecov Report

Merging #7458 (a3cf1a1) into development (8a65edc) will decrease coverage by 0.00%.
The diff coverage is 28.00%.

@@               Coverage Diff               @@
##           development    #7458      +/-   ##
===============================================
- Coverage        65.39%   65.38%   -0.01%     
===============================================
  Files              265      265              
  Lines            13234    13249      +15     
===============================================
+ Hits              8654     8663       +9     
- Misses            4580     4586       +6

Impacted Files	Coverage Δ
app/api/helpers/order.py	`63.50% <0.00%> (+0.46%)`	⬆️
app/api/helpers/storage.py	`63.93% <ø> (ø)`
app/models/order.py	`88.50% <0.00%> (ø)`
app/api/custom/orders.py	`66.30% <21.42%> (-4.43%)`	⬇️
app/api/custom/invoices.py	`41.30% <33.33%> (+3.00%)`	⬆️
app/models/ticket_holder.py	`78.08% <60.00%> (-1.33%)`	⬇️
app/api/schema/microlocations.py	`100.00% <0.00%> (ø)`
app/models/microlocation.py	`95.65% <0.00%> (+0.19%)`	⬆️

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 8a65edc...5558cda. Read the comment docs.

iamareebjamal · 2020-11-20T17:28:55Z

app/api/custom/orders.py

+@order_blueprint.route('/attendees/<int:attendee_id>.pdf')
+@jwt_required
+def ticket_attendee_pdf(attendee_id):
+    if current_user:


No need of this check

iamareebjamal · 2020-11-20T17:29:15Z

app/api/custom/orders.py

+@jwt_required
+def ticket_attendee_pdf(attendee_id):
+    if current_user:
+        try:


Too broad try block

iamareebjamal · 2020-11-20T17:30:27Z

app/api/custom/orders.py

+                except FileNotFoundError:
+                    return NotFoundError({'source': ''}, 'File Not found')
+            else:
+                raise ForbiddenError({'source': ''}, 'Unauthorized Access')


Use early return

iamareebjamal · 2020-11-20T17:30:56Z

app/api/custom/orders.py

+                try:
+                    return send_from_directory('../', file_path, as_attachment=True)
+                except FileNotFoundError:
+                    return NotFoundError({'source': ''}, 'File Not found')


This will fail if the file doesn't exist

What shall be the behaviour?

iamareebjamal · 2020-11-21T06:37:45Z

app/api/custom/orders.py

+        try:
+            return send_from_directory('../', file_path, as_attachment=True)
+        except FileNotFoundError:
+            return NotFoundError({'source': ''}, 'File Not found')


Should generate the file and send if file not found

iamareebjamal · 2020-11-21T11:00:28Z

app/api/custom/orders.py

+    ):
+
+        raise ForbiddenError({'source': ''}, 'Unauthorized Access')
+    else:


No need of else

iamareebjamal · 2020-11-21T11:01:42Z

app/api/custom/orders.py

+        if os.path.exists(file_path):
+            return send_from_directory('../', file_path, as_attachment=True)
+        else:
+            create_pdf_tickets_for_holder(ticket_holder.order)
+            return send_from_directory('../', file_path, as_attachment=True)


Suggested change

if os.path.exists(file_path):

return send_from_directory('../', file_path, as_attachment=True)

else:

create_pdf_tickets_for_holder(ticket_holder.order)

return send_from_directory('../', file_path, as_attachment=True)

if not os.path.isfile(file_path):

create_pdf_tickets_for_holder(ticket_holder.order)

return send_from_directory('../', file_path, as_attachment=True)

Think of the simplest way to achieve a solution which minimizes duplication

And this change needs to be in other endpoint as well

iamareebjamal · 2020-11-21T11:17:56Z

app/api/custom/invoices.py

-            try:
-                return send_from_directory('../', file_path, as_attachment=True)
-            except FileNotFoundError:
+            if not os.path.exists(file_path):


Suggested change

if not os.path.exists(file_path):

if not os.path.isfile(file_path):

@iamareebjamal - this change should be done everywhere then because i read that isfile will return true only when it points to a file while exist can return true for a directory also.

pdf is a file, not a directory

In my past review, I wrote isfile only

This is my bad apologies.

iamareebjamal · 2020-11-21T11:18:12Z

app/api/custom/orders.py

-            try:
-                return send_from_directory('../', file_path, as_attachment=True)
-            except FileNotFoundError:
+            if not os.path.exists(file_path):


Suggested change

if not os.path.exists(file_path):

if not os.path.isfile(file_path):

feat: Ticket pdf path made with attendee identifier

7b26ef6

auto-label bot added the feature label Nov 19, 2020

iamareebjamal reviewed Nov 20, 2020

View reviewed changes

codedsun force-pushed the ticket-attendee branch 2 times, most recently from bccccb8 to a8c1b00 Compare November 21, 2020 06:32

iamareebjamal reviewed Nov 21, 2020

View reviewed changes

codedsun force-pushed the ticket-attendee branch from a8c1b00 to ddfc2c7 Compare November 21, 2020 10:16

iamareebjamal reviewed Nov 21, 2020

View reviewed changes

app/api/custom/orders.py Outdated

):

raise ForbiddenError({'source': ''}, 'Unauthorized Access')

else:

Copy link

Member

iamareebjamal Nov 21, 2020

Choose a reason for hiding this comment

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

No need of else

iamareebjamal reviewed Nov 21, 2020

View reviewed changes

feat: Added endpoint to download attendee pdf

5bfa8d4

codedsun force-pushed the ticket-attendee branch from ddfc2c7 to 9b88b30 Compare November 21, 2020 11:14

iamareebjamal reviewed Nov 21, 2020

View reviewed changes

fix: Added check if file exist in invoices and orders

a3cf1a1

codedsun force-pushed the ticket-attendee branch from 9b88b30 to a3cf1a1 Compare November 21, 2020 11:26

feat: Saved ticket purchaser pdf with order identifier in path

5558cda

iamareebjamal approved these changes Nov 22, 2020

View reviewed changes

iamareebjamal changed the title ~~feat: Ticket pdf path made with attendee identifier~~ feat: Separate ticket for each attendee Nov 22, 2020

iamareebjamal merged commit 4a77516 into fossasia:development Nov 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Separate ticket for each attendee #7458

feat: Separate ticket for each attendee #7458

codedsun commented Nov 19, 2020

codedsun commented Nov 19, 2020

codecov bot commented Nov 20, 2020 •

edited

Loading

iamareebjamal Nov 20, 2020

iamareebjamal Nov 20, 2020

iamareebjamal Nov 20, 2020

iamareebjamal Nov 20, 2020

codedsun Nov 21, 2020

iamareebjamal Nov 21, 2020

iamareebjamal Nov 21, 2020

iamareebjamal Nov 21, 2020

iamareebjamal Nov 21, 2020

iamareebjamal Nov 21, 2020

codedsun Nov 21, 2020 •

edited

Loading

iamareebjamal Nov 21, 2020

iamareebjamal Nov 21, 2020 •

edited

Loading

codedsun Nov 21, 2020

iamareebjamal Nov 21, 2020

	if not os.path.exists(file_path):
	if not os.path.isfile(file_path):

feat: Separate ticket for each attendee #7458

feat: Separate ticket for each attendee #7458

Conversation

codedsun commented Nov 19, 2020

Short description of what this resolves:

Changes proposed in this pull request:

Checklist

codedsun commented Nov 19, 2020

codecov bot commented Nov 20, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedsun Nov 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal Nov 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 20, 2020 •

edited

Loading

codedsun Nov 21, 2020 •

edited

Loading

iamareebjamal Nov 21, 2020 •

edited

Loading