Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
JacobCoffee
force-pushed
the
svg-spons-fix
branch
from
a9b2ecf to
ecec8d3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {% if sponsorship.sponsor.web_logo.name|lower|slice:"-4:" == ".svg" %} | ||
| <img src="{{ sponsorship.sponsor.web_logo.url }}" alt="{{ sponsorship.sponsor.name }} logo" style="max-height:150px;max-width:150px;height:auto;width:auto;"> | ||
| {% else %} | ||
| {% thumbnail sponsorship.sponsor.web_logo "x150" format="PNG" quality=100 as im %} | ||
| <img src="{{ im.url }}" alt="{{ sponsorship.sponsor.name }} logo" style="max-height:150px;max-width:150px;height:auto;width:auto;"> | ||
| {% endthumbnail %} | ||
| {% endif %} |
There was a problem hiding this comment.
The SVG detection / rendering logic is duplicated in both the download and jobs sections. To reduce the chance of future drift (e.g., adding another placement/size), consider extracting this into a small reusable include or a custom template filter/tag (e.g., render_sponsor_logo sponsorship.sponsor.web_logo size=150).
| @admin.display(description="Web Logo") | ||
| def get_sponsor_web_logo(self, obj): | ||
| """Render and return the sponsor's web logo as a thumbnail image.""" | ||
| html = "{% load thumbnail %}{% thumbnail sponsor.web_logo '150x150' format='PNG' quality=100 as im %}<img src='{{ im.url}}'/>{% endthumbnail %}" | ||
| img = obj.sponsor.web_logo | ||
| if not img: | ||
| return "---" | ||
| if img.name and img.name.lower().endswith(".svg"): | ||
| return format_html( | ||
| '<img src="{}" style="max-width:150px;max-height:150px"/>', | ||
| img.url, |
There was a problem hiding this comment.
These methods’ docstrings say they always return a “thumbnail image”, but the SVG branch returns the original asset (just CSS-constrained). Consider updating the docstrings (and/or the @admin.display description) to reflect the mixed behavior so future maintainers don’t assume everything is thumbnailed/rasterized.
| @admin.display(description="Web Logo") | ||
| def get_sponsor_web_logo(self, obj): | ||
| """Render and return the sponsor's web logo as a thumbnail image.""" | ||
| html = "{% load thumbnail %}{% thumbnail sponsor.web_logo '150x150' format='PNG' quality=100 as im %}<img src='{{ im.url}}'/>{% endthumbnail %}" | ||
| img = obj.sponsor.web_logo | ||
| if not img: | ||
| return "---" | ||
| if img.name and img.name.lower().endswith(".svg"): | ||
| return format_html( | ||
| '<img src="{}" style="max-width:150px;max-height:150px"/>', | ||
| img.url, | ||
| ) | ||
| html = "{% load thumbnail %}{% thumbnail img '150x150' format='PNG' quality=100 as im %}<img src='{{ im.url}}'/>{% endthumbnail %}" | ||
| template = Template(html) | ||
| context = Context({"sponsor": obj.sponsor}) | ||
| html = template.render(context) | ||
| return mark_safe(html) # noqa: S308 | ||
| context = Context({"img": img}) | ||
| return mark_safe(template.render(context)) # noqa: S308 | ||
|
|
||
| @admin.display(description="Print Logo") | ||
| def get_sponsor_print_logo(self, obj): | ||
| """Render and return the sponsor's print logo as a thumbnail image.""" | ||
| img = obj.sponsor.print_logo | ||
| html = "---" | ||
| if img: | ||
| template = Template( | ||
| "{% load thumbnail %}{% thumbnail img '150x150' format='PNG' quality=100 as im %}<img src='{{ im.url}}'/>{% endthumbnail %}" | ||
| if not img: | ||
| return "---" | ||
| if img.name and img.name.lower().endswith(".svg"): | ||
| return format_html( | ||
| '<img src="{}" style="max-width:150px;max-height:150px"/>', | ||
| img.url, | ||
| ) | ||
| context = Context({"img": img}) | ||
| html = mark_safe(template.render(context)) # noqa: S308 | ||
| return html | ||
| template = Template( | ||
| "{% load thumbnail %}{% thumbnail img '150x150' format='PNG' quality=100 as im %}<img src='{{ im.url}}'/>{% endthumbnail %}" | ||
| ) | ||
| context = Context({"img": img}) | ||
| return mark_safe(template.render(context)) # noqa: S308 |
There was a problem hiding this comment.
There’s no automated coverage around the new SVG-vs-non-SVG branching for admin logo rendering. Given this was added to prevent failures/OOMs, consider adding a unit test that exercises both branches (e.g., .svg returns raw img.url via format_html, non-SVG goes through the thumbnail path) so regressions are caught early.
| {% if sponsorship.sponsor.web_logo.name|lower|slice:"-4:" == ".svg" %} | ||
| <img src="{{ sponsorship.sponsor.web_logo.url }}" alt="{{ sponsorship.sponsor.name }} logo" style="max-height:150px;max-width:150px;height:auto;width:auto;"> | ||
| {% else %} | ||
| {% thumbnail sponsorship.sponsor.web_logo "x150" format="PNG" quality=100 as im %} | ||
| <img src="{{ im.url }}" alt="{{ sponsorship.sponsor.name }} logo" style="max-height:150px;max-width:150px;height:auto;width:auto;"> | ||
| {% endthumbnail %} |
There was a problem hiding this comment.
This template now has conditional behavior based on file extension (SVG served directly vs raster thumbnailed). Consider adding a regression test that renders this partial (via list_sponsors) with a .svg web_logo and asserts it does not invoke the thumbnail tag / does not try to generate a PNG, since that’s the behavior intended to avoid OOMs.
| {% if sponsorship.sponsor.web_logo.name|lower|slice:"-4:" == ".svg" %} | ||
| <img src="{{ sponsorship.sponsor.web_logo.url }}" alt="{{ sponsorship.sponsor.name }} logo" style="max-height:150px;max-width:150px;height:auto;width:auto;"> | ||
| {% else %} | ||
| {% thumbnail sponsorship.sponsor.web_logo "x150" format="PNG" quality=100 as im %} | ||
| <img src="{{ im.url }}" alt="{{ sponsorship.sponsor.name }} logo" style="max-height:150px;max-width:150px;height:auto;width:auto;"> | ||
| {% endthumbnail %} |
There was a problem hiding this comment.
This change serves uploaded SVGs directly for sponsor.web_logo (bypassing rasterization). Note that Sponsor.web_logo is an ImageField with help_text indicating PNG/JPG only, so allowing/rendering SVG here creates a mismatch with the model/form expectations and can also introduce SVG-specific security concerns (e.g., untrusted SVG content being rendered in browsers). Consider either (a) enforcing/repairing data so web_logo is always raster (and keep thumbnailing), or (b) formally supporting SVG by updating validation/docs and adding SVG sanitization and/or converting SVG->PNG at upload time in a controlled way.
| {% if sponsorship.sponsor.web_logo.name|lower|slice:"-4:" == ".svg" %} | ||
| <img src="{{ sponsorship.sponsor.web_logo.url }}" alt="{{ sponsorship.sponsor.name }} logo" style="max-height:100px;max-width:100px;height:auto;width:auto;"> | ||
| {% else %} | ||
| {% thumbnail sponsorship.sponsor.web_logo "x100" format="PNG" quality=100 as im %} | ||
| <img src="{{ im.url }}" alt="{{ sponsorship.sponsor.name }} logo" style="max-height:100px;max-width:100px;height:auto;width:auto;"> | ||
| {% endthumbnail %} |
There was a problem hiding this comment.
Same concern as the download sponsors block: serving web_logo SVGs directly here diverges from the ImageField/PNG-JPG expectation for Sponsor.web_logo and may require explicit validation/sanitization or an upload-time conversion approach to avoid rendering untrusted SVG content.
Description
Works locally to load, hoping this fixes issue with svg load then oom