fix: resolve PDF download issues #1256
Open
+19
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fix duplicate .pdf extension in ticket filenames by removing redundant provider identifier - Add defensive null checks for order_position to prevent HTTP 500 errors - Apply consistent filename format across presale, control, and API views - Filenames now follow clean pattern: EVENT-CODE-POSITION.pdf Fixes fossasia#1245: HTTP 500 error on confirmation page PDF download Fixes fossasia#1247: Duplicate .pdf extension in ticket filename
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSimplify PDF filename generation by removing provider identifiers and strengthening null-safe logic around order positions and subevents across presale, API, and control ticket download endpoints. Sequence diagram for PDF ticket download filename generation (with null-safe logic)sequenceDiagram
participant User
participant "OrderView"
participant "FileResponse"
User->>OrderView: Request PDF ticket download
OrderView->>OrderView: Check if order_position and subevent exist
alt order_position and subevent
OrderView->>FileResponse: Generate filename with event, code, position, subevent date, extension
else order_position only
OrderView->>FileResponse: Generate filename with event, code, position, extension
else no order_position
OrderView->>FileResponse: Generate filename with event, code, extension
end
OrderView->>User: Return PDF file with generated filename
Class diagram for FileResponse and filename generation logicclassDiagram
class OrderView {
+order_position
+order
+request
+success(value)
}
class FileResponse {
+file
+type
}
OrderView --> FileResponse : generates
OrderView : +success(value)
OrderView : filename generation logic
OrderView : null checks for order_position and subevent
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ArnavBallinCode
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `app/eventyay/presale/views/order.py:1105` </location>
<code_context>
+ value.extension,
+ )
+ else:
+ # Fallback if order_position is not available
+ resp['Content-Disposition'] = 'attachment; filename="{}-{}{}}"'.format(
+ self.request.event.slug.upper(),
</code_context>
<issue_to_address>
**issue (bug_risk):** Extra closing brace in fallback filename format string.
The format string should be corrected to '"{}-{}{}"' to avoid malformed filenames.
</issue_to_address>
### Comment 2
<location> `app/eventyay/api/views/order.py:296-304` </location>
<code_context>
@action(detail=True, url_name='download', url_path='download/(?P<output>[^/]+)')
def download(self, request, output, **kwargs):
provider = self._get_output_provider(output)
order = self.get_object()
if order.status != Order.STATUS_PAID:
raise PermissionDenied('Downloads are not available for unpaid orders.')
ct = CachedCombinedTicket.objects.filter(order=order, provider=provider.identifier, file__isnull=False).last()
if not ct or not ct.file:
generate.apply_async(args=('order', order.pk, provider.identifier))
raise RetryException()
else:
if ct.type == 'text/uri-list':
resp = HttpResponse(ct.file.file.read(), content_type='text/uri-list')
return resp
else:
resp = FileResponse(ct.file.file, content_type=ct.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
order.code,
ct.extension,
)
return resp
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>
### Comment 3
<location> `app/eventyay/api/views/order.py:1093-1102` </location>
<code_context>
@action(detail=True, url_name='download', url_path='download/(?P<output>[^/]+)')
def download(self, request, output, **kwargs):
provider = self._get_output_provider(output)
pos = self.get_object()
if pos.order.status != Order.STATUS_PAID:
raise PermissionDenied('Downloads are not available for unpaid orders.')
if not pos.generate_ticket:
raise PermissionDenied('Downloads are not enabled for this product.')
ct = CachedTicket.objects.filter(order_position=pos, provider=provider.identifier, file__isnull=False).last()
if not ct or not ct.file:
generate.apply_async(args=('orderposition', pos.pk, provider.identifier))
raise RetryException()
else:
if ct.type == 'text/uri-list':
resp = HttpResponse(ct.file.file.read(), content_type='text/uri-list')
return resp
else:
resp = FileResponse(ct.file.file, content_type=ct.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
pos.order.code,
pos.positionid,
ct.extension,
)
return resp
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>
### Comment 4
<location> `app/eventyay/control/views/orders.py:510` </location>
<code_context>
def success(self, value):
if 'ajax' in self.request.POST or 'ajax' in self.request.GET:
return JsonResponse(
{
'ready': True,
'success': True,
'redirect': self.get_success_url(value),
'message': str(self.get_success_message(value)),
}
)
if isinstance(value, CachedTicket):
if value.type == 'text/uri-list':
resp = HttpResponseRedirect(value.file.file.read())
return resp
else:
resp = FileResponse(value.file.file, content_type=value.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
self.order_position.positionid,
value.extension,
)
return resp
elif isinstance(value, CachedCombinedTicket):
resp = FileResponse(value.file.file, content_type=value.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
value.extension,
)
return resp
else:
return redirect(self.get_self_url())
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string [×2] ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Fixes syntax error where fallback filename had an extra '}' character causing malformed filenames.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `app/eventyay/api/views/order.py:296-304` </location>
<code_context>
@action(detail=True, url_name='download', url_path='download/(?P<output>[^/]+)')
def download(self, request, output, **kwargs):
provider = self._get_output_provider(output)
order = self.get_object()
if order.status != Order.STATUS_PAID:
raise PermissionDenied('Downloads are not available for unpaid orders.')
ct = CachedCombinedTicket.objects.filter(order=order, provider=provider.identifier, file__isnull=False).last()
if not ct or not ct.file:
generate.apply_async(args=('order', order.pk, provider.identifier))
raise RetryException()
else:
if ct.type == 'text/uri-list':
resp = HttpResponse(ct.file.file.read(), content_type='text/uri-list')
return resp
else:
resp = FileResponse(ct.file.file, content_type=ct.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
order.code,
ct.extension,
)
return resp
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>
### Comment 2
<location> `app/eventyay/api/views/order.py:1093-1102` </location>
<code_context>
@action(detail=True, url_name='download', url_path='download/(?P<output>[^/]+)')
def download(self, request, output, **kwargs):
provider = self._get_output_provider(output)
pos = self.get_object()
if pos.order.status != Order.STATUS_PAID:
raise PermissionDenied('Downloads are not available for unpaid orders.')
if not pos.generate_ticket:
raise PermissionDenied('Downloads are not enabled for this product.')
ct = CachedTicket.objects.filter(order_position=pos, provider=provider.identifier, file__isnull=False).last()
if not ct or not ct.file:
generate.apply_async(args=('orderposition', pos.pk, provider.identifier))
raise RetryException()
else:
if ct.type == 'text/uri-list':
resp = HttpResponse(ct.file.file.read(), content_type='text/uri-list')
return resp
else:
resp = FileResponse(ct.file.file, content_type=ct.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
pos.order.code,
pos.positionid,
ct.extension,
)
return resp
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>
### Comment 3
<location> `app/eventyay/control/views/orders.py:510` </location>
<code_context>
def success(self, value):
if 'ajax' in self.request.POST or 'ajax' in self.request.GET:
return JsonResponse(
{
'ready': True,
'success': True,
'redirect': self.get_success_url(value),
'message': str(self.get_success_message(value)),
}
)
if isinstance(value, CachedTicket):
if value.type == 'text/uri-list':
resp = HttpResponseRedirect(value.file.file.read())
return resp
else:
resp = FileResponse(value.file.file, content_type=value.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
self.order_position.positionid,
value.extension,
)
return resp
elif isinstance(value, CachedCombinedTicket):
resp = FileResponse(value.file.file, content_type=value.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
value.extension,
)
return resp
else:
return redirect(self.get_self_url())
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string [×2] ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>
### Comment 4
<location> `app/eventyay/presale/views/order.py:1085` </location>
<code_context>
def success(self, value):
if 'ajax' in self.request.POST or 'ajax' in self.request.GET:
return JsonResponse(
{
'ready': True,
'success': True,
'redirect': self.get_success_url(value),
'message': str(self.get_success_message(value)),
}
)
if isinstance(value, CachedTicket):
if value.type == 'text/uri-list':
resp = HttpResponseRedirect(value.file.file.read())
return resp
else:
resp = FileResponse(value.file.file, content_type=value.type)
if self.order_position and self.order_position.subevent:
# Subevent date in filename improves accessibility e.g. for screen reader users
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
self.order_position.positionid,
self.order_position.subevent.date_from.strftime('%Y_%m_%d'),
value.extension,
)
elif self.order_position:
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
self.order_position.positionid,
value.extension,
)
else:
# Fallback if order_position is not available
resp['Content-Disposition'] = (
'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
value.extension,
)
)
return resp
return resp
elif isinstance(value, CachedCombinedTicket):
resp = FileResponse(value.file.file, content_type=value.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
value.extension,
)
return resp
else:
return redirect(self.get_self_url())
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string [×4] ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
The entire codebase uses .format() - changing just these 3 files would be inconsistent |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `app/eventyay/api/views/order.py:296-304` </location>
<code_context>
@action(detail=True, url_name='download', url_path='download/(?P<output>[^/]+)')
def download(self, request, output, **kwargs):
provider = self._get_output_provider(output)
order = self.get_object()
if order.status != Order.STATUS_PAID:
raise PermissionDenied('Downloads are not available for unpaid orders.')
ct = CachedCombinedTicket.objects.filter(order=order, provider=provider.identifier, file__isnull=False).last()
if not ct or not ct.file:
generate.apply_async(args=('order', order.pk, provider.identifier))
raise RetryException()
else:
if ct.type == 'text/uri-list':
resp = HttpResponse(ct.file.file.read(), content_type='text/uri-list')
return resp
else:
resp = FileResponse(ct.file.file, content_type=ct.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
order.code,
ct.extension,
)
return resp
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>
### Comment 2
<location> `app/eventyay/api/views/order.py:1093-1102` </location>
<code_context>
@action(detail=True, url_name='download', url_path='download/(?P<output>[^/]+)')
def download(self, request, output, **kwargs):
provider = self._get_output_provider(output)
pos = self.get_object()
if pos.order.status != Order.STATUS_PAID:
raise PermissionDenied('Downloads are not available for unpaid orders.')
if not pos.generate_ticket:
raise PermissionDenied('Downloads are not enabled for this product.')
ct = CachedTicket.objects.filter(order_position=pos, provider=provider.identifier, file__isnull=False).last()
if not ct or not ct.file:
generate.apply_async(args=('orderposition', pos.pk, provider.identifier))
raise RetryException()
else:
if ct.type == 'text/uri-list':
resp = HttpResponse(ct.file.file.read(), content_type='text/uri-list')
return resp
else:
resp = FileResponse(ct.file.file, content_type=ct.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
pos.order.code,
pos.positionid,
ct.extension,
)
return resp
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>
### Comment 3
<location> `app/eventyay/control/views/orders.py:510` </location>
<code_context>
def success(self, value):
if 'ajax' in self.request.POST or 'ajax' in self.request.GET:
return JsonResponse(
{
'ready': True,
'success': True,
'redirect': self.get_success_url(value),
'message': str(self.get_success_message(value)),
}
)
if isinstance(value, CachedTicket):
if value.type == 'text/uri-list':
resp = HttpResponseRedirect(value.file.file.read())
return resp
else:
resp = FileResponse(value.file.file, content_type=value.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
self.order_position.positionid,
value.extension,
)
return resp
elif isinstance(value, CachedCombinedTicket):
resp = FileResponse(value.file.file, content_type=value.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
value.extension,
)
return resp
else:
return redirect(self.get_self_url())
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string [×2] ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>
### Comment 4
<location> `app/eventyay/presale/views/order.py:1085` </location>
<code_context>
def success(self, value):
if 'ajax' in self.request.POST or 'ajax' in self.request.GET:
return JsonResponse(
{
'ready': True,
'success': True,
'redirect': self.get_success_url(value),
'message': str(self.get_success_message(value)),
}
)
if isinstance(value, CachedTicket):
if value.type == 'text/uri-list':
resp = HttpResponseRedirect(value.file.file.read())
return resp
else:
resp = FileResponse(value.file.file, content_type=value.type)
if self.order_position and self.order_position.subevent:
# Subevent date in filename improves accessibility e.g. for screen reader users
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
self.order_position.positionid,
self.order_position.subevent.date_from.strftime('%Y_%m_%d'),
value.extension,
)
elif self.order_position:
resp['Content-Disposition'] = 'attachment; filename="{}-{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
self.order_position.positionid,
value.extension,
)
else:
# Fallback if order_position is not available
resp['Content-Disposition'] = (
'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
value.extension,
)
)
return resp
return resp
elif isinstance(value, CachedCombinedTicket):
resp = FileResponse(value.file.file, content_type=value.type)
resp['Content-Disposition'] = 'attachment; filename="{}-{}{}"'.format(
self.request.event.slug.upper(),
self.order.code,
value.extension,
)
return resp
else:
return redirect(self.get_self_url())
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Replace call to format with f-string [×4] ([`use-fstring-for-formatting`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-formatting/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
mariobehling
requested changes
Nov 13, 2025
Member
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Thanks! Please resolve AI comments.
Copilot finished reviewing on behalf of
mariobehling
November 13, 2025 15:19
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses two bugs in the PDF ticket download functionality by removing the provider identifier from filenames and adding defensive null checks for order_position attributes.
- Simplified filename format by removing provider identifier parameter across all download endpoints
- Added null safety checks in presale view to handle cases where
order_positionmight not be available - Maintained consistent filename patterns across presale, control, and API modules
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/eventyay/presale/views/order.py | Updated filename format strings to remove provider identifier; added defensive null checks for order_position and order_position.subevent with fallback handling for order-level downloads |
| app/eventyay/control/views/orders.py | Simplified filename format strings by removing provider identifier from both position-based and combined ticket downloads |
| app/eventyay/api/views/order.py | Removed provider identifier from filename format strings in both order-level and position-level download endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
This return statement is necessary. The code has multiple conditions:
If we remove the return at line 1107, the The code structure is:
Each branch needs its own return statement. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mariobehling
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix PDF Ticket Download Issues
Summary
This PR fixes two related bugs in the PDF ticket download functionality:
Fix for #1247
Removed the provider identifier from the filename format string across all download endpoints:
Fix for #1245
Added defensive null checks before accessing
order_positionattributes:Summary by Sourcery
Fix PDF ticket download issues by simplifying filename format and adding null checks
Bug Fixes:
Summary by Sourcery
Fix PDF ticket download issues by removing the provider identifier from filenames and adding null checks to prevent attribute errors
Bug Fixes:
fixes #1247
fixes #1245