CP-14012 - Show checkbox and selection marks on PDFs - #80
Conversation
What Checkbox, radio, and multi-select choices now appear as checkmarks on preview and completed PDFs, including when the value is a truthy string like "true" from prefill or the API. Stale radio/multi-select option areas no longer crash PDF generation. Why Users could type text and see it on the PDF, but selected checkboxes and options were often missing on the preview and final document, so they had to re-check form data offline. How to test 1. Create a form with a text field, checkbox, radio (option areas), and multi-select (option areas). 2. Fill and complete it; confirm checkmarks for selections on the live page and on the downloaded PDF. 3. Prefill or submit a checkbox as the string "true" and confirm the PDF shows a check. 4. Optional: point a radio area at a missing option UUID and confirm the PDF still generates.
spaulsandhu
left a comment
spaulsandhu
left a comment
There was a problem hiding this comment.
🤖 This is a semi-automated review done by Paul, with assistance from a review agent. Everything is manually signed off on, but if you feel this feedback isn't helpful, please let him know!
This is the right fix and I like that the specs cover both bugs, the stale-option crash and the string truthiness, instead of just the headline one. Approving, nothing here blocks. Couple of things worth a look on the way through: the silent next on a stale option (the guard right above it logs), and whether the count-only assertions would actually catch a check landing on the wrong area.
[Conversational] Not for this PR, but I went looking for where a checkbox value becomes the string 'true' in the first place and it's Submitters::SubmitValues.merge_default_values, which copies field['default_value'] into submitter.values verbatim with no coercion (the API path goes through NormalizeValues and does get boolean-ized).
Compensating at read time like you're doing is the lower-risk move and I'd keep it here. But the other readers of submitter.values have the same gap: check_field_condition treats 'checked' as submitter_values[uuid].present?, so a checkbox defaulted to the string 'false' reads as checked for conditional fields.
And while I was in there, that same method does field['options'].find { ... } then option['value'] with no nil guard, which is exactly the stale-option crash you just fixed on the PDF side.
Worth a ticket to normalize checkbox default values on write? Happy to file it if you'd rather keep this PR focused.
—Paul-bot
| if field['type'].in?(%w[multiple radio]) | ||
| option = field['options']&.find { |o| o['uuid'] == area['option_uuid'] } | ||
|
|
||
| next if option.nil? |
There was a problem hiding this comment.
Good catch on this one, a stale option_uuid taking out PDF generation entirely is nasty.
Curious though, should this log the way the missing-page guard a few lines up does? It's the same failure mode as that one: the doc generates fine but quietly comes out missing a mark the submitter actually made, and nobody finds out until a customer opens the signed PDF.
Also, since we're patching an upstream file, a one-liner comment like the page.nil? one above would save future-us some archaeology on the next upstream merge.
There was a problem hiding this comment.
Good call. Followed the missing-page pattern: we now Rails.logger.warn with submitter/field/option_uuid and added a short comment so the next upstream merge has context. PDF still generates; missing marks are no longer silent.
|
|
||
| value = Array.wrap(value).include?(option_name) | ||
| else | ||
| value = Submitters::NormalizeValues::TRUE_VALUES.include?(value) |
There was a problem hiding this comment.
Reusing NormalizeValues::TRUE_VALUES is the right call here, it's the exact table normalize_value uses for checkboxes so the PDF and the normalized data agree on what "checked" means. 👍🏽
Small thought, not a blocker: any reason not to call Submitters::NormalizeValues.normalize_value(field, value) == true instead of reaching for the constant? Keeps the truth table behind one door if it ever grows. Totally your call, the constant reads fine as-is.
There was a problem hiding this comment.
Agreed — keeping the cast on the PDF path for this PR. Write-time normalization for checkbox defaults (and the related condition / nil-guard gaps) is a good follow-up so we don’t expand scope here.
| expect(image_xobject_count).to eq(1) | ||
|
|
||
| # Reset page for second value | ||
| pdfs_index[attachment_uuid].pages[0].canvas(type: :overlay) # ensure page exists |
There was a problem hiding this comment.
nitpick: this line isn't doing anything, right? You build a fresh HexaPDF::Document on the next line and swap the whole pdfs_index entry, so that overlay canvas gets thrown away immediately.
Would it read better to fold '1' and 'yes' into the same .each loop shape you used in the false/nil example below? Then the doc reset lives in one place instead of two.
There was a problem hiding this comment.
Yep, that canvas line was a no-op. Folded '1' and 'yes' into the same reset-and-assert loop as the false/nil example.
| ) | ||
|
|
||
| fill | ||
| expect(image_xobject_count).to eq(1) |
There was a problem hiding this comment.
More of a question here, and not a blocker: the example says "only on the matching option area", but the assertion is just "the page has one image XObject", which would pass just as happily if we drew the check on the No area instead of Yes.
The two areas are at x 0.1 and 0.3, so the placement math should come out different. Is there a cheap way to assert the check landed in the right spot? The thing most likely to regress in this branch feels like a check on the wrong option, and a count wouldn't catch that one.
There was a problem hiding this comment.
Fair point. Kept the “one check when both areas exist” case, then added a second step: value is Yes but only the No area is present → expect zero checks, so a wrong-option draw would fail.
|
🤖 Heads-up: while reviewing this PR I noticed an unrelated existing finding worth tracking. Just filed as CP-14765. Not blocking this PR. —Paul-bot |
What When a radio or multi-select area points at an option that no longer exists, PDF generation still succeeds and now logs a warning so missing marks are easier to spot. Specs also better prove the check lands on the correct option and clean up truthy-string coverage. Why A silent skip could hide a real selection from the signed PDF until a customer noticed. Logging matches how we already handle missing page areas, and tighter tests catch drawing the check on the wrong option. How to test 1. bundle exec rspec spec/lib/submissions/generate_result_attachments_spec.rb 2. Optional: complete a form with radio/multi-select; confirm checks still appear on the PDF. 3. Optional: force a stale option UUID and confirm the PDF still generates and the warn log appears.
3 participants
CP-14012
What
Checkbox, radio, and multi-select choices now appear as checkmarks on preview and completed PDFs, including when the value is a truthy string like "true" from prefill or the API. Stale radio/multi-select option areas no longer crash PDF generation.
Why
Users could type text and see it on the PDF, but selected checkboxes and options were often missing on the preview and final document, so they had to re-check form data offline.
How to test
Screenshot