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.

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

feat:ticket pdf associated with attendee id #7400

Closed
wants to merge 1 commit into from

Conversation

codedsun
Copy link
Contributor

Fixes #7397 ticket pdf associated with attendee id

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.

@auto-label auto-label bot added the feature label Oct 31, 2020
@iamareebjamal
Copy link
Member

This doesn't change anything

@codedsun
Copy link
Contributor Author

codedsun commented Nov 1, 2020

@iamareebjamal - What i understood by the task was to made the ticket-pdf downloadable by attendee_id.

@codedsun
Copy link
Contributor Author

codedsun commented Nov 1, 2020

@iamareebjamal - Currently the api serves the file from generated/tickets/orders/tickets/pdf/ It's the ticket for individual attendee. Can you just reference me which ticket pdf you are referring to. Also, I see another folder generated/invoices/orders which is a order invoice and imo doesn't need to be served from this API.

@iamareebjamal
Copy link
Member

What difference does it make when it is still downloading an order ticket PDF? The end result is the same. The goal is to have individual PDF of each attendee. This is single PDF for all attendees

)
or order.is_attendee(current_user)
):
key = UPLOAD_PATHS['pdf']['tickets_all'].format(identifier=order_identifier)
key = UPLOAD_PATHS['pdf']['tickets_all'].format(identifier=order.identifier)
Copy link
Member

Choose a reason for hiding this comment

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

This is taking order identifier as key, how can it ever be for an attendee. Please understand the issue first

Copy link
Contributor Author

@codedsun codedsun Nov 1, 2020

Choose a reason for hiding this comment

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

Can I know the path of the attendee ticket pdf?

file_path = (
'../generated/tickets/{}/{}/'.format(key, generate_hash(key))
+ order_identifier
+ order.identifier
Copy link
Member

Choose a reason for hiding this comment

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

How is this attendee? This is an order

@codedsun
Copy link
Contributor Author

codedsun commented Nov 1, 2020

@iamareebjamal - The generated folder has all the pdf with order identifer, I might be wrong but here attatching the screenshot which causes confusion
image

@iamareebjamal
Copy link
Member

Exactly. That's the issue. Please read the issue again, carefully this time

@codedsun
Copy link
Contributor Author

codedsun commented Nov 1, 2020

Will this path work good? - tickets/pdf/attendee_id/random_key/order_id
@iamareebjamal

@iamareebjamal
Copy link
Member

order ID first and then attendee ID

@codedsun
Copy link
Contributor Author

codedsun commented Nov 2, 2020

image

@iamareebjamal - tthis looks good?

@codecov
Copy link

codecov bot commented Nov 2, 2020

Codecov Report

Merging #7400 into development will increase coverage by 0.22%.
The diff coverage is 19.04%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #7400      +/-   ##
===============================================
+ Coverage        64.59%   64.81%   +0.22%     
===============================================
  Files              262      262              
  Lines            13246    13220      -26     
===============================================
+ Hits              8556     8569      +13     
+ Misses            4690     4651      -39     
Impacted Files Coverage Δ
app/api/helpers/order.py 66.93% <0.00%> (ø)
app/api/helpers/storage.py 63.93% <ø> (ø)
app/api/orders.py 42.31% <0.00%> (+1.01%) ⬆️
app/api/custom/orders.py 40.00% <22.22%> (+2.28%) ⬆️
app/api/helpers/mail.py 37.00% <22.22%> (-1.01%) ⬇️
app/api/tax.py 51.66% <0.00%> (-0.80%) ⬇️
app/api/stripe_authorization.py 43.66% <0.00%> (-0.79%) ⬇️
app/api/user_favourite_events.py 58.92% <0.00%> (-0.73%) ⬇️
app/api/tickets.py 46.15% <0.00%> (-0.52%) ⬇️
app/api/speakers.py 68.57% <0.00%> (-0.30%) ⬇️
... and 6 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 47eb8e2...2d98670. Read the comment docs.

@@ -87,7 +87,9 @@ def resend_emails():
if order.status == 'completed' or order.status == 'placed':
# fetch tickets attachment
order_identifier = order.identifier
key = UPLOAD_PATHS['pdf']['tickets_all'].format(identifier=order_identifier)
key = UPLOAD_PATHS['pdf']['tickets_all'].format(
identifier=order_identifier, attendee_identifier=str(order.user_id)
Copy link
Member

Choose a reason for hiding this comment

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

order.user_id is not attendee

@@ -329,7 +331,9 @@ def complete_order(order_id):
):
order_identifier = order.identifier

key = UPLOAD_PATHS['pdf']['tickets_all'].format(identifier=order_identifier)
key = UPLOAD_PATHS['pdf']['tickets_all'].format(
identifier=order_identifier, attendee_identifier=str(order.user_id)
Copy link
Member

Choose a reason for hiding this comment

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

order.user_id is not attendee

@@ -73,6 +73,7 @@ def create_pdf_tickets_for_holder(order):
UPLOAD_PATHS['pdf']['tickets_all'],
dir_path='/static/uploads/pdf/tickets/',
identifier=order.identifier,
extra_identifiers={'attendee_identifier': str(order.user_id)},
Copy link
Member

Choose a reason for hiding this comment

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

order.user_id is not attendee

@@ -88,6 +89,7 @@ def create_pdf_tickets_for_holder(order):
UPLOAD_PATHS['pdf']['tickets_all'],
dir_path='/static/uploads/pdf/tickets/',
identifier=order.identifier,
extra_identifiers={'attendee_identifier': str(order.user_id)},
Copy link
Member

Choose a reason for hiding this comment

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

order.user_id is not attendee

@@ -121,7 +121,9 @@ def on_order_completed(order):
# fetch tickets attachment
order_identifier = order.identifier

key = UPLOAD_PATHS['pdf']['tickets_all'].format(identifier=order_identifier)
key = UPLOAD_PATHS['pdf']['tickets_all'].format(
identifier=order_identifier, attendee_identifier=str(order.user_id)
Copy link
Member

Choose a reason for hiding this comment

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

order.user_id is not attendee

@@ -525,7 +527,9 @@ def after_update_object(self, order, data, view_kwargs):
# Send email to attendees with invoices and tickets attached
order_identifier = order.identifier

key = UPLOAD_PATHS['pdf']['tickets_all'].format(identifier=order_identifier)
key = UPLOAD_PATHS['pdf']['tickets_all'].format(
identifier=order_identifier, attendee_identifier=str(order.user_id)
Copy link
Member

Choose a reason for hiding this comment

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

order.user_id is not attendee

@codedsun
Copy link
Contributor Author

codedsun commented Nov 3, 2020

image

@iamareebjamal - Done!

if current_user:
try:
order = Order.query.filter_by(identifier=order_identifier).first()
order = Order.query.filter_by(user_id=attendee_id).first()
Copy link
Member

Choose a reason for hiding this comment

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

Not really. As I already told you, Order.user_id != Attendee.id

Attendee == TicketHolder

Attendee != Order.user

You need to understand what is Ticket Buyer and what is an attendee or a ticket holder. Then understand why the issue was raised and what is the actual problem. We discussed this in meeting as well. And then think if the changes actually solve the problem

Copy link
Contributor Author

@codedsun codedsun Nov 3, 2020

Choose a reason for hiding this comment

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

this is my fault, I missed to change this. Will update the PR. Does the rest of the PR seems OK or not? @iamareebjamal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ticket Buyer is the person who is ordering the ticket
Ticket Holder = Can be ticket buyer or some other person

Issue was raised because on frontend download tickets, a random ticket was downloaded - to solve this we will have to save ticket attendee wise.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you removed the code. Why was it necessary. How is it compatible with previous behaviour?

Copy link
Contributor Author

@codedsun codedsun Nov 3, 2020

Choose a reason for hiding this comment

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

I removed the code as the ticket pdf is now created and saved attendee wise so the ticket path needs to be changed for every ticket holder. The same common code is now moved to create pdf function.

Previously the ticket pdf path seems to be constant using order_identifier but now it needs to be ticketholder wise @iamareebjamal

Copy link
Member

Choose a reason for hiding this comment

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

It still wasn't constant. It changed with order. Now it changes with ticket holder. I understand that. But you removed the code of sending the attachments and other things. We need that. It should be adapted to recent changes

Copy link
Contributor Author

@codedsun codedsun Nov 3, 2020

Choose a reason for hiding this comment

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

I have a question, shall the invoice path and ticket path then be sent as attatchement to the function as earlier? If yes then I have to loop for the ticket path from everywhere where the function is used or should I add a parameter attachment in function and append that function parameter value to the invoice, ticket_path, ?

Copy link
Contributor Author

@codedsun codedsun Nov 3, 2020

Choose a reason for hiding this comment

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

Also, I think that send_Email function should not have this code as it's a generic function? What are your views

for holder in order.ticket_holders:

@iamareebjamal

@codedsun
Copy link
Contributor Author

codedsun commented Nov 5, 2020

To be completed and merged after #7412

@codedsun codedsun closed this Nov 23, 2020
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.

Ticket PDF should exist for each attendee
2 participants