T3333 letter report fixes - #2158
Merged
Merged
Conversation
get_attachments_per_page() crashed with AttributeError on any attachment whose mimetype is NULL, hit via /b2s_image. It ran unconditionally on every letter because the report template's t-foreach was compiled before its guarding t-if (both directives were on the same node) - nest them so the condition actually gates the call.
…ent_ids The "Attached images" field picked up sponsor_letter_scan/zip_file attachments (res_field set), which is where the NULL-mimetype rows crashing get_attachments_per_page() come from. Restrict the domain to res_field = False so it only sees actual attached images.
…tatus report_needs_original_text was gated on "not yet sent to GMC", using that as a proxy for "no image exists yet". For S2B letters that were sent but never got a photo back, this skipped the text overlay too, rendering a silently blank page despite the sponsor's text sitting in the database. Gate it on whether an image source is actually known instead, via a new _has_page_image_source() that only reads stored URL columns and never triggers an external Cloudinary/Connect fetch.
spread_text_to_pages() measures whether a page's text overflows its template text box and pushes the excess onto a new page, but it was only wired up as a manual backend button. The normal letter-generation flow never called it, so a sufficiently long sponsor letter would overflow its text box and bleed into the next page's rendering area instead of being split across pages.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
_has_page_image_source() used .filtered() + bool(), which is true as soon as any single page has a known image. Since the letter-wide overlay flag gates every page uniformly, a multi-page letter where only some pages have a source would suppress the text overlay everywhere, leaving the uncovered pages with neither an image nor the sponsor's text - the same blank-page failure this fix was meant to close, just on a narrower case. Found by greptile review on PR #2158.
ecino
approved these changes
Sep 17, 2026
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
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.
T3333 — MyCompassion "write a letter" quality-test failures
Original ask
A quality test on "MyCompassion: write a letter" failed on 2026-07-22 with several defects reported from a single manual walkthrough:
Related PRs
What changed
compassion-modules(sbc_compassion)efd3e51b— the 500.get_attachments_per_page()crashed withAttributeError: 'bool' object has no attribute 'startswith'on any attachment with a NULLmimetype, reached via/b2s_image. It ran unconditionally on every letter because the report template'st-if/t-foreachsat on the same node — QWeb compilest-foreachbeforet-if, so the guard never actually gated the call. Nested the directives and guarded the mimetype check. Reproduced the exact crash live onstage18_neutralizedby temporarily reverting the fix, then confirmed 8 UUIDs (5 previously-crashing + 3 controls) all return 200 afterward with zero occurrences of the error in the log.1a7cd55c— narrowedoriginal_attachment_ids's domain tores_field = False, removing the actual source of the NULL-mimetype rows (attachment-backed binary fields likesponsor_letter_scan) from a field meant only for attached images. Separable from the crash fix per the original plan.11cb201f— the blank-PDF issue (bigger than the reported ticket). While investigating the 500, found thatreport_needs_original_textgated the letter-text overlay on "not yet sent to GMC" as a proxy for "no image exists yet" — false for any S2B letter that was sent but never got a scanned photo back. Those letters render as a valid but completely empty PDF: no image, no text, despite the sponsor's message sitting untouched in the database. Replaced the gate with a real check (_has_page_image_source(), reading only stored URL columns, never triggering an external Cloudinary/Connect fetch). Verified on realstage18_neutralizeddata: a previously blank letter now renders its full text; a letter with a real returned image still correctly shows no overlay (no regression); a not-yet-sent letter renders exactly as before.2e0937b8— found while manually testing the above with a long letter:spread_text_to_pages()(which measures whether a page's text overflows its template text box and pushes the excess onto a new page) existed but was only wired up as a manual "Spread text to pages" backend button — the normal S2B letter-generation flow never called it, so a long sponsor letter would silently overflow its text box into the next page's rendering area instead of being paginated. Now called automatically after letter creation (draft and real generation both).