From aebba7be44496cbfcd84c0dbce120b888afdd6fa Mon Sep 17 00:00:00 2001 From: Quinten Steenhuis Date: Fri, 18 Sep 2026 14:21:39 -0400 Subject: [PATCH 1/2] Fix attachment question saves and empty field mappings --- docassemble/ALWeaver/attachment_editor.py | 38 +++++++++--- docassemble/ALWeaver/data/static/editor.js | 29 ++++++--- docassemble/ALWeaver/document_bundles.py | 36 +++++++---- docassemble/ALWeaver/editor_utils.py | 25 ++++++++ .../ALWeaver/test_attachment_editor.py | 33 +++++++++- docassemble/ALWeaver/test_document_bundles.py | 60 +++++++++++++++++++ .../ALWeaver/test_editor_attachments.js | 49 +++++++++++++++ .../test_editor_source_preservation.py | 33 ++++++++++ 8 files changed, 274 insertions(+), 29 deletions(-) diff --git a/docassemble/ALWeaver/attachment_editor.py b/docassemble/ALWeaver/attachment_editor.py index 65cfabd5..a41cb8a0 100644 --- a/docassemble/ALWeaver/attachment_editor.py +++ b/docassemble/ALWeaver/attachment_editor.py @@ -8,6 +8,28 @@ from yaml.nodes import MappingNode, ScalarNode, SequenceNode +def attachment_matches(variable_name: Any, document_name: str) -> bool: + """True when an attachment's ``variable name`` assigns this document. + + Callers that select attachments and callers that remove them must agree + about this: a block one selects and the other declines to touch would be + rewritten forever. + """ + return bool( + re.match( + r"^" + re.escape(document_name) + r"(?:\[|\.|$)", + str(variable_name or "").strip(), + ) + ) + + +def _blank(node) -> bool: + """True when a key is absent or written with no value at all (``fields:``).""" + return node is None or ( + isinstance(node, ScalarNode) and node.tag == "tag:yaml.org,2002:null" + ) + + def _mapping(node): if not isinstance(node, MappingNode) or (node.flow_style and node.value): raise ValueError( @@ -48,7 +70,9 @@ def attachment_mappings(source: str) -> List[Dict[str, Any]]: template = props.get("pdf template file", props.get("docx template file")) fields = props.get("fields") rows = [] - if fields is not None: + if not _blank(fields): + if not isinstance(fields, (MappingNode, SequenceNode)): + raise ValueError("Use YAML mode for computed attachment fields.") groups = fields.value if isinstance(fields, SequenceNode) else [fields] seen = set() for group in groups: @@ -118,12 +142,12 @@ def update_attachment_mappings(source: str, updates: list) -> str: props = _mapping(node) fields = props.get("fields") entries = {} - if fields is not None: - groups = fields.value if isinstance(fields, SequenceNode) else [fields] + if not _blank(fields): if not isinstance(fields, (MappingNode, SequenceNode)) or ( fields.flow_style and fields.value ): - raise ValueError("Use YAML mode for flow-style fields.") + raise ValueError("Use YAML mode for computed or flow-style fields.") + groups = fields.value if isinstance(fields, SequenceNode) else [fields] for group in groups: for name, value in _mapping(group).items(): if name in entries: @@ -150,7 +174,7 @@ def update_attachment_mappings(source: str, updates: list) -> str: replacement += "\n" patches.append((old.start_mark.index, old.end_mark.index, replacement)) if additions: - if fields is not None and not fields.value: + if fields is not None and (_blank(fields) or not fields.value): field_key = next( key for key, value in node.value if key.value == "fields" ) @@ -215,8 +239,8 @@ def remove_attachment(source: str, document_name: str) -> str: targets = [] for node in nodes: variable = _mapping(node).get("variable name") - if isinstance(variable, ScalarNode) and re.match( - r"^" + re.escape(document_name) + r"(?:\[|\.|$)", variable.value + if isinstance(variable, ScalarNode) and attachment_matches( + variable.value, document_name ): targets.append(node) if not targets: diff --git a/docassemble/ALWeaver/data/static/editor.js b/docassemble/ALWeaver/data/static/editor.js index 9a96e19e..13aa69ce 100644 --- a/docassemble/ALWeaver/data/static/editor.js +++ b/docassemble/ALWeaver/data/static/editor.js @@ -8047,13 +8047,23 @@ return fallbackId ? getBlockById(fallbackId) : null; } + function isQuestionEditorBlock(block) { + return Boolean( + block && + (block.type === 'question' || + (block.type === 'attachment' && + block.data && + block.data.question !== undefined)), + ); + } + function getBlockYamlForSave(block) { if (!block) return ''; - if (state.questionEditMode === 'preview' && block.type === 'attachment') - return block.yaml; - if (state.questionEditMode === 'preview' && block.type === 'question') { + if (state.questionEditMode === 'preview' && isQuestionEditorBlock(block)) { return serializeQuestionBlockToYaml(block); } + if (state.questionEditMode === 'preview' && block.type === 'attachment') + return block.yaml; if (state.questionEditMode === 'preview' && block.type === 'code') { return serializeCodeToYaml(block); } @@ -9458,7 +9468,7 @@ return; } - if (block.type === 'question') { + if (isQuestionEditorBlock(block)) { renderQuestionBlock(block); } else if (block.type === 'review') { renderReviewBlock(block); @@ -15173,9 +15183,11 @@ } var attachmentMappingContext = null; - document - .getElementById('attachment-mappings-modal') - .addEventListener('hide.bs.modal', function (event) { + var attachmentMappingsModal = document.getElementById( + 'attachment-mappings-modal', + ); + if (attachmentMappingsModal) + attachmentMappingsModal.addEventListener('hide.bs.modal', function (event) { if (attachmentMappingContext && attachmentMappingContext.saving) event.preventDefault(); else attachmentMappingContext = null; @@ -15268,7 +15280,7 @@ (row.missing ? 'true' : 'false') + '" data-symbol-role="variable"' + (row.editable ? '' : ' disabled') + - '>' + + '>\n' + esc(row.value) + ''; }); @@ -15292,6 +15304,7 @@ return { index: attachment.index, values: Object.create(null) }; }); document + .getElementById('attachment-mappings-body') .querySelectorAll('[data-attachment-field]') .forEach(function (input) { if ( diff --git a/docassemble/ALWeaver/document_bundles.py b/docassemble/ALWeaver/document_bundles.py index 771bdbfd..46b7a973 100644 --- a/docassemble/ALWeaver/document_bundles.py +++ b/docassemble/ALWeaver/document_bundles.py @@ -533,16 +533,18 @@ def set_bundle_elements( if name not in cleaned: cleaned.append(name) block_id, declaration, entry = _find_declaration(raw_yaml, bundle_name) - try: - current = ast.parse( - declaration_keyword(declaration, "elements"), mode="eval" - ).body - except SyntaxError as exc: - raise ValueError("Use YAML mode for computed bundle elements.") from exc - if not isinstance(current, ast.List) or any( - not isinstance(item, ast.Name) for item in current.elts - ): - raise ValueError("Use YAML mode for computed bundle elements.") + # A bundle that has not listed its elements yet just gains the keyword. + # Only an existing list that is not plain variable names is beyond us. + existing = declaration_keyword(declaration, "elements") + if existing: + try: + current = ast.parse(existing, mode="eval").body + except SyntaxError as exc: + raise ValueError("Use YAML mode for computed bundle elements.") from exc + if not isinstance(current, ast.List) or any( + not isinstance(item, ast.Name) for item in current.elts + ): + raise ValueError("Use YAML mode for computed bundle elements.") updated = with_declaration_keyword( declaration, "elements", "[" + ", ".join(cleaned) + "]" ) @@ -558,6 +560,8 @@ def remove_document(raw_yaml: str, name: str) -> str: Template files and questions are retained, including questions carrying several attachments. """ + from .attachment_editor import attachment_matches, remove_attachment + model = interview_documents(raw_yaml) if name not in {document.name for document in model.documents}: raise ValueError(f"{name} is not a document in this interview.") @@ -577,17 +581,23 @@ def remove_document(raw_yaml: str, name: str) -> str: target = None for entry in parse_interview_yaml(raw_yaml)["blocks"]: data = entry.get("data") or {} + # A commented-out block assembles nothing, and its source is hash + # marks rather than the YAML the attachment editor would patch. + if data.get("_commented"): + continue attachment = data.get("attachment", data.get("attachments")) attachments = attachment if isinstance(attachment, list) else [attachment] matching = any( isinstance(item, dict) - and reference_root(item.get("variable name")) == name + and attachment_matches(item.get("variable name"), name) for item in attachments ) if matching: - from .attachment_editor import remove_attachment - replacement = remove_attachment(entry["yaml"], name) + if replacement == entry["yaml"]: + # Nothing came out, so rewriting this block would find it + # again on the next pass and never terminate. + continue remaining = yaml.safe_load(replacement) or {} if any( key in remaining diff --git a/docassemble/ALWeaver/editor_utils.py b/docassemble/ALWeaver/editor_utils.py index cffd05f3..a12ee16d 100644 --- a/docassemble/ALWeaver/editor_utils.py +++ b/docassemble/ALWeaver/editor_utils.py @@ -1139,6 +1139,31 @@ def update_block_in_yaml( edited_body = new_block_yaml.strip("\r\n") replacement: Optional[str] = None if preserve_unchanged_annotations: + # The question controls do not serialize attachments. Retain their + # exact source when saving a question carrying one or more documents. + original_data = _block.get("data") or {} + edited_data = yaml.safe_load(edited_body) + if ( + "question" in original_data + and isinstance(edited_data, dict) + and "question" in edited_data + ): + original_node = yaml.compose(original_body) + if isinstance(original_node, yaml.MappingNode): + for index, (key, _value) in enumerate(original_node.value): + if ( + key.value not in ("attachment", "attachments") + or key.value in edited_data + ): + continue + property_end = ( + original_node.value[index + 1][0].start_mark.index + if index + 1 < len(original_node.value) + else len(original_body) + ) + edited_body += "\n" + original_body[ + key.start_mark.index : property_end + ].rstrip("\r\n") replacement = _merge_changed_mapping_values(original_body, edited_body) if replacement is None: leading_len = len(original_body) - len(original_body.lstrip("\r\n")) diff --git a/docassemble/ALWeaver/test_attachment_editor.py b/docassemble/ALWeaver/test_attachment_editor.py index 77347471..48aea7e6 100644 --- a/docassemble/ALWeaver/test_attachment_editor.py +++ b/docassemble/ALWeaver/test_attachment_editor.py @@ -137,7 +137,7 @@ def test_no_changes_is_exact_noop(self): ) def test_empty_fields_and_block_scalar_comments(self): - for empty in ("[]", "{}"): + for empty in ("[]", "{}", "null", "Null", "NULL", "~"): source = ( "attachment:\n pdf template file: a.pdf\n fields: " + empty @@ -147,6 +147,7 @@ def test_empty_fields_and_block_scalar_comments(self): source, [{"index": 0, "values": {"new": "${ value }"}}] ) self.assertIn("# keep", updated) + self.assertEqual(yaml.safe_load(updated)["attachment"]["name"], "Test") self.assertEqual( yaml.safe_load(updated)["attachment"]["fields"], [{"new": "${ value }"}] ) @@ -156,3 +157,33 @@ def test_empty_fields_and_block_scalar_comments(self): ) self.assertIn("# keep header", updated) self.assertEqual(yaml.safe_load(updated)["attachment"]["fields"]["x"], "new") + + def test_a_fields_key_with_no_value_is_readable_and_fillable(self): + """A stub attachment is exactly what the dialog exists to fill in.""" + source = ( + "attachment:\n" + " pdf template file: a.pdf\n" + " fields:\n" + " editable templates: True\n" + ) + self.assertEqual(attachment_mappings(source)[0]["rows"], []) + updated = update_attachment_mappings( + source, [{"index": 0, "values": {"signature": "${ users[0] }"}}] + ) + self.assertEqual( + yaml.safe_load(updated)["attachment"]["fields"], + [{"signature": "${ users[0] }"}], + ) + self.assertIn("editable templates: True", updated) + + def test_a_computed_fields_value_says_so(self): + source = "attachment:\n pdf template file: a.pdf\n fields: chosen_fields\n" + for call in ( + lambda: attachment_mappings(source), + lambda: update_attachment_mappings( + source, [{"index": 0, "values": {"x": "y"}}] + ), + ): + with self.subTest(call=call), self.assertRaises(ValueError) as caught: + call() + self.assertIn("computed", str(caught.exception)) diff --git a/docassemble/ALWeaver/test_document_bundles.py b/docassemble/ALWeaver/test_document_bundles.py index 8734b258..5a7852fc 100644 --- a/docassemble/ALWeaver/test_document_bundles.py +++ b/docassemble/ALWeaver/test_document_bundles.py @@ -265,6 +265,66 @@ def test_an_element_that_is_not_a_variable_name_is_refused(self): EXISTING_INTERVIEW, "al_user_bundle", ["petition; rm -rf /"] ) + def test_a_bundle_without_elements_yet_gains_the_keyword(self): + """An unlisted bundle is not a computed one; importing a template fills it.""" + source = """--- +objects: + - petition: ALDocument.using(filename="petition", enabled=True) + - al_user_bundle: ALDocumentBundle.using(filename="bundle", title="All") +""" + updated = set_bundle_elements(source, "al_user_bundle", ["petition"]) + self.assertEqual(interview_documents(updated).bundles[0].elements, ["petition"]) + + def test_a_computed_element_list_is_still_refused(self): + source = """--- +objects: + - petition: ALDocument.using(filename="petition", enabled=True) + - al_user_bundle: ALDocumentBundle.using(elements=chosen_documents, filename="b") +""" + with self.assertRaises(ValueError): + set_bundle_elements(source, "al_user_bundle", ["petition"]) + + def test_an_attachment_sharing_a_name_prefix_is_left_alone(self): + """`petition_copy` is a different variable, and must not loop forever.""" + source = """--- +objects: + - petition: ALDocument.using(filename="petition", enabled=True) +--- +attachment: + variable name: petition_copy[i] + pdf template file: copy.pdf +--- +attachment: + variable name: petition[i] + pdf template file: petition.pdf +""" + updated = remove_document(source, "petition") + self.assertIn("petition_copy[i]", updated) + self.assertNotIn("petition.pdf", updated) + self.assertNotIn("ALDocument.using", updated) + + def test_a_commented_out_attachment_does_not_block_deletion(self): + source = """--- +objects: + - petition: ALDocument.using(filename="petition", enabled=True) + - affidavit: ALDocument.using(filename="affidavit", enabled=True) +--- +# attachment: +# variable name: petition[i] +# pdf template file: old.pdf +--- +attachment: + variable name: petition[i] + pdf template file: petition.pdf +""" + updated = remove_document(source, "petition") + self.assertIn("# pdf template file: old.pdf", updated) + self.assertNotIn("pdf template file: petition.pdf", updated) + self.assertEqual( + [document.name for document in interview_documents(updated).documents], + ["affidavit"], + ) + if __name__ == "__main__": unittest.main() diff --git a/docassemble/ALWeaver/test_editor_attachments.js b/docassemble/ALWeaver/test_editor_attachments.js index 85341c99..ad909e8a 100644 --- a/docassemble/ALWeaver/test_editor_attachments.js +++ b/docassemble/ALWeaver/test_editor_attachments.js @@ -39,3 +39,52 @@ context.deleteDocumentFromInterview('affidavit'); assert.deepStrictEqual(Array.from(context.state.documents.removed), ['affidavit']); assert.strictEqual(context.state.documents.documents.length, 1); assert.strictEqual(context.state.documents.bundles[0].elements.length, 0); + +// A screen carrying both a question and an attachment keeps the question +// editor, which is where the "Edit attachment field mappings" button lives. +// A standalone attachment block still gets the plain attachment card. +const routed = []; +const canvas = { + state: {project: 'p', questionEditMode: 'preview'}, + canvasContent: {innerHTML: ''}, + selected: null, + renderProjectSelector: () => { routed.push('project'); }, + renderQuestionBlock: () => { routed.push('question'); }, + renderReviewBlock: () => { routed.push('review'); }, + renderCommentedBlock: () => { routed.push('commented'); }, + renderCodeBlock: () => { routed.push('code'); }, + renderObjectsBlock: () => { routed.push('objects'); }, + renderGenericBlock: () => { routed.push('generic'); }, + emptyCanvasHtml: () => '', + esc: (value) => String(value), +}; +canvas.getSelectedBlock = () => canvas.selected; +vm.createContext(canvas); +const helperStart = source.indexOf(' function isQuestionEditorBlock('); +vm.runInContext(source.slice(helperStart, source.indexOf('\n }', helperStart) + 4), canvas); +const canvasStart = source.indexOf(' function renderBlockCanvas('); +vm.runInContext(source.slice(canvasStart, source.indexOf('\n }', canvasStart) + 4), canvas); +canvas.selected = {type: 'question', title: 'Plain', data: {}}; +canvas.renderBlockCanvas(); +canvas.selected = {type: 'attachment', title: 'Your documents', + data: {question: 'Your documents', attachment: {'variable name': 'petition[i]'}}}; +canvas.renderBlockCanvas(); +assert.deepStrictEqual(routed, ['question', 'question']); +canvas.selected = {type: 'attachment', title: 'Petition', + data: {attachment: {'variable name': 'petition[i]'}}}; +canvas.renderBlockCanvas(); +assert.deepStrictEqual(routed, ['question', 'question']); +assert.ok(canvas.canvasContent.innerHTML.includes('data-edit-attachment-mappings')); + +// Saving must follow the same dispatch as rendering, including in YAML mode. +const saveStart = source.indexOf(' function getBlockYamlForSave('); +vm.runInContext(source.slice(saveStart, source.indexOf('\n }', saveStart) + 4), canvas); +canvas.serializeQuestionBlockToYaml = () => 'question: Edited question\nfields:\n - Name: users[0].name'; +canvas.getSourceEditorValue = () => 'question: Edited in YAML'; +for (const type of ['question', 'attachment']) { + const block = {type, yaml: 'question: Original', data: {question: 'Original', attachment: {}}}; + assert.ok(canvas.getBlockYamlForSave(block).includes('question: Edited question')); +} +assert.strictEqual(canvas.getBlockYamlForSave({type: 'attachment', data: {}, yaml: 'attachment: original'}), 'attachment: original'); +canvas.state.questionEditMode = 'yaml'; +assert.strictEqual(canvas.getBlockYamlForSave({type: 'attachment', data: {question: 'Original'}, yaml: 'original'}), 'question: Edited in YAML'); diff --git a/docassemble/ALWeaver/test_editor_source_preservation.py b/docassemble/ALWeaver/test_editor_source_preservation.py index fec1d722..8c59e6c8 100644 --- a/docassemble/ALWeaver/test_editor_source_preservation.py +++ b/docassemble/ALWeaver/test_editor_source_preservation.py @@ -43,6 +43,39 @@ class TestEditorSourcePreservation(unittest.TestCase): + def test_graphical_question_save_preserves_attachment_source(self): + for key, value in ( + ( + "attachment", + " pdf template file: \"form.pdf\" # keep\n fields:\n name: '${ users[0] }'\n", + ), + ( + "attachments", + " - pdf template file: \"form.pdf\" # keep\n fields:\n name: '${ users[0] }'\n", + ), + ): + with self.subTest(key=key): + attachment = key + ":\n" + value + source = "id: output\nquestion: Original\n" + attachment + edited = ( + "id: output\nquestion: Edited\nfields:\n - Name: users[0].name\n" + ) + updated = update_block_in_yaml( + source, "output", edited, preserve_unchanged_annotations=True + ) + self.assertEqual(yaml.safe_load(updated)["question"], "Edited") + self.assertEqual( + yaml.safe_load(updated)["fields"], [{"Name": "users[0].name"}] + ) + self.assertIn(attachment.rstrip("\n"), updated) + self.assertEqual( + yaml.safe_load(updated)[key], yaml.safe_load(source)[key] + ) + # Explicit edits in YAML mode must still be able to remove it. + self.assertNotIn( + key, yaml.safe_load(update_block_in_yaml(source, "output", edited)) + ) + def test_parser_returns_exact_block_yaml(self): question = next( block From f3e223fbea6a7b73409641323399ac6070b2c5e8 Mon Sep 17 00:00:00 2001 From: Quinten Steenhuis Date: Fri, 18 Sep 2026 18:12:38 -0400 Subject: [PATCH 2/2] Mark attachment editor tests do not pre-load --- docassemble/ALWeaver/test_attachment_editor.py | 1 + 1 file changed, 1 insertion(+) diff --git a/docassemble/ALWeaver/test_attachment_editor.py b/docassemble/ALWeaver/test_attachment_editor.py index 48aea7e6..b0251205 100644 --- a/docassemble/ALWeaver/test_attachment_editor.py +++ b/docassemble/ALWeaver/test_attachment_editor.py @@ -1,3 +1,4 @@ +# do not pre-load """Regression coverage for lossless graphical attachment field edits.""" import unittest