Conversation
There was a problem hiding this comment.
Pull request overview
Hotfix release v6.2.4 addressing offline UI font loading, GraphQL report creation failures, and client logo rendering within the Rolodex app.
Changes:
- Replaced Google Fonts
@importwith locally hosted font files + addedfonts.cssand licensing. - Added a new Rolodex endpoint for serving/downloading client logos and updated the client detail template + tests.
- Adjusted Hasura metadata and Django reporting model/migrations to support new defaults and to prevent orphan
Reportrecords.
Reviewed changes
Copilot reviewed 14 out of 27 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| hasura-docker/metadata/databases/default/tables/public_reporting_report.yaml | Allows include_bloodhound_data in report insert permissions. |
| ghostwriter/static/fonts/roboto-mono-v31-latin-regular.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/roboto-mono-v31-latin-300.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/poppins-v24-latin-regular.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/poppins-v24-latin-italic.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/poppins-v24-latin-700italic.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/poppins-v24-latin-700.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/poppins-v24-latin-600italic.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/poppins-v24-latin-600.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/poppins-v24-latin-500italic.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/poppins-v24-latin-500.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/metrophobic-v24-latin-regular.woff2 | Adds local font asset for offline usage. |
| ghostwriter/static/fonts/LICENSES.txt | Documents font licensing/attribution. |
| ghostwriter/static/css/styles.css | Switches to local font CSS import and minor CSS cleanup. |
| ghostwriter/static/css/fonts.css | Defines local @font-face rules for offline font loading. |
| ghostwriter/rolodex/views.py | Adds ClientLogoDownload view for serving client logos. |
| ghostwriter/rolodex/urls.py | Adds URL route for the client logo endpoint. |
| ghostwriter/rolodex/tests/test_views.py | Adds view tests covering the new client logo endpoint. |
| ghostwriter/rolodex/templates/rolodex/client_detail.html | Uses the new logo endpoint as the <img> source. |
| ghostwriter/reporting/models.py | Makes Report.project non-nullable. |
| ghostwriter/reporting/migrations/0064_alter_report_project.py | Migration attempting to enforce non-null Report.project. |
| ghostwriter/reporting/migrations/0063_set_report_field_defaults.py | Sets DB defaults for last_update and include_bloodhound_data (and alters project). |
| config/settings/base.py | Bumps version and release date to 6.2.4 / 24 February 2026. |
| VERSION | Updates version/release date file for 6.2.4. |
| CHANGELOG.md | Adds v6.2.4 changelog entry. |
| return redirect("home:dashboard") | ||
|
|
||
| def get(self, *args, **kwargs): | ||
| obj = self.get_object() |
There was a problem hiding this comment.
obj.logo.path will raise a ValueError if the client has no logo file associated (e.g., direct request to this URL for a client without a logo, or a stale DB value). Add an explicit guard like if not obj.logo: raise Http404 before accessing .path so the view consistently returns 404 instead of 500.
| obj = self.get_object() | |
| obj = self.get_object() | |
| if not obj.logo: | |
| raise Http404 |
| <div class="container d-flex align-items-center justify-content-center"> | ||
| {% if client.logo %} | ||
| <img src="{{ client.logo.url }}" | ||
| <img src="{% url 'rolodex:client_logo_download' client.id %}" |
There was a problem hiding this comment.
This template now points the <img> tag at rolodex:client_logo_download, but the new view returns a download/attachment response. That combination commonly prevents the image from rendering inline. Either switch the view to return the logo inline (no attachment) or use a dedicated inline-serving endpoint for the <img> and reserve the download endpoint for explicit downloads.
| <img src="{% url 'rolodex:client_logo_download' client.id %}" | |
| <img src="{{ client.logo.url }}" |
| migrations.AlterField( | ||
| model_name="report", | ||
| name="project", | ||
| field=models.ForeignKey( | ||
| default=1, | ||
| help_text="Select the project tied to this report", | ||
| on_delete=django.db.models.deletion.CASCADE, | ||
| to="rolodex.project", | ||
| ), | ||
| preserve_default=False, | ||
| ), |
There was a problem hiding this comment.
This migration hard-codes a one-off default project ID (default=1) to backfill existing rows while making the FK non-nullable. That will fail on installs where project ID 1 does not exist, and it can silently attach orphan reports to the wrong project. Prefer a data migration that (a) explicitly fixes or deletes orphan reports, or (b) aborts with a helpful error instructing the admin to remediate, and then alter the field without assuming any specific primary key.
| migrations.AlterField( | ||
| model_name="report", | ||
| name="project", | ||
| field=models.ForeignKey( | ||
| default=5, | ||
| help_text="Select the project tied to this report", | ||
| on_delete=django.db.models.deletion.CASCADE, | ||
| to="rolodex.project", | ||
| ), | ||
| preserve_default=False, | ||
| ), |
There was a problem hiding this comment.
This migration repeats the Report.project alteration again with a different hard-coded default (default=5). Besides duplicating the schema change, hard-coding a PK here will break on most real databases and can incorrectly reassign orphan reports. Consolidate to a single migration and avoid assuming any specific Project ID when removing nullability.
| * Fixed client logos not showing properly on client dashboards | ||
| * Fixed issue with the `last_update` column preventing creation of a report via the GraphQL API (Fixes #828) | ||
|
|
||
| ## [6.2.3] — 5 February 2026 |
There was a problem hiding this comment.
The new release entries use the ## [x.y.z] - <date> format, but this header still uses an em dash (—) instead of -, making the changelog format inconsistent. Consider switching it to match the [6.2.4] - 24 February 2026 style.
| ## [6.2.3] — 5 February 2026 | |
| ## [6.2.3] - 5 February 2026 |
| return FileResponse( | ||
| open(file_path, "rb"), | ||
| as_attachment=True, | ||
| filename=os.path.basename(file_path), | ||
| ) |
There was a problem hiding this comment.
ClientLogoDownload is used as an <img src> in client_detail.html, but this response forces Content-Disposition: attachment via as_attachment=True. Many browsers will not render an attachment response as an image resource, which can keep logos from displaying. Consider returning the file inline (no attachment) and setting an appropriate Content-Type so the <img> tag can render it (and use a separate endpoint if you still need a forced-download behavior).
|
|
||
|
|
||
| class ClientLogoDownload(RoleBasedAccessControlMixin, SingleObjectMixin, View): | ||
| """Return the target :model:`reporting.Evidence` file for download.""" |
There was a problem hiding this comment.
Docstring references :model:reporting.Evidence`` but this view serves a rolodex.Client's logo. Update the docstring to reflect what the view actually returns to avoid confusing future maintainers.
| """Return the target :model:`reporting.Evidence` file for download.""" | |
| """Return the target :model:`rolodex.Client` logo file for download.""" |
| self.assertEquals( | ||
| response.get("Content-Disposition"), | ||
| f'attachment; filename="{os.path.basename(self.client_with_logo.logo.path)}"', |
There was a problem hiding this comment.
These tests currently assert an attachment-style Content-Disposition. If the logo endpoint is intended for inline rendering in <img>, the expectation should be updated accordingly (e.g., no attachment, and ideally asserting an image Content-Type). Otherwise the tests will keep enforcing behavior that breaks logo display.
| self.assertEquals( | |
| response.get("Content-Disposition"), | |
| f'attachment; filename="{os.path.basename(self.client_with_logo.logo.path)}"', | |
| # Ensure the response is serving an image suitable for inline rendering | |
| self.assertTrue( | |
| str(response["Content-Type"]).startswith("image/"), | |
| msg=f"Expected image content type, got {response['Content-Type']!r}", |
|
|
||
| operations = [ | ||
| migrations.RunSQL( | ||
| "ALTER TABLE reporting_report ALTER COLUMN include_bloodhound_data SET DEFAULT true;", |
There was a problem hiding this comment.
This migration sets the DB default for include_bloodhound_data to true, but the Django model field (and factories) default it to False. That creates inconsistent behavior depending on whether a report is created via Django vs. direct SQL/Hasura. Align the DB default with the model default (or update the model/defaults everywhere if true is the intended new default).
| "ALTER TABLE reporting_report ALTER COLUMN include_bloodhound_data SET DEFAULT true;", | |
| "ALTER TABLE reporting_report ALTER COLUMN include_bloodhound_data SET DEFAULT false;", |
ghostwriter/reporting/migrations/0063_set_report_field_defaults.py
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #830 +/- ##
==========================================
+ Coverage 91.37% 91.39% +0.02%
==========================================
Files 368 370 +2
Lines 20933 20993 +60
==========================================
+ Hits 19127 19187 +60
Misses 1806 1806 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CHANGELOG
[6.2.4] - 24 February 2026
Added
Changed
Reportmodel's project ID field to no longer allow null valuesFixed
last_updatecolumn preventing creation of a report via the GraphQL API (Fixes Can't create report via GraphQL API #828)Mintlify
0 threads from 0 users in Mintlify