Skip to content

feat(attachments): multi-annex embed for Sales Invoice (part 1)#263

Open
dafrose wants to merge 17 commits into
alyf-de:version-16-hotfixfrom
dafrose:feat/embed_attachments_16
Open

feat(attachments): multi-annex embed for Sales Invoice (part 1)#263
dafrose wants to merge 17 commits into
alyf-de:version-16-hotfixfrom
dafrose:feat/embed_attachments_16

Conversation

@dafrose

@dafrose dafrose commented Jun 15, 2026

Copy link
Copy Markdown
Member

Summary

Part 1 of selectable PDF annexes for Sales Invoice (#261, coordination #76). This PR is supposed to only add support for multiple attachments via the same embedding path as before. File attachments are still embedded into XML (which may be embedded into PDF). It does not provide support for embedding additional files directly into PDF. This is planned for a follow-up PR.

When Multiple Attachment Embedding is enabled on E Invoice Settings, annexes are selected from the new einvoice_attachments child table and embedded in the e-invoice XML as 916 supporting documents (same _embed_attachments loop as today). When the setting is off, behaviour stays on the legacy einvoice_embedded_document attach field.

  • einvoice_attachments child table on Sales Invoice with Desk attach button and first-row-wins dedup on validate
  • multi_attachment_embed_enabled checkbox on E Invoice Settings
  • get_embed_attachments branches between table and legacy field; shared module sales_invoice_attachments.py
  • Migration on enable: per-save move of legacy field → table (orange msgprint); bulk RQ job via Desk button; legacy field hidden + read_only while multi-embed is on
  • Embed refactor: get_legacy_embed_attachment + _embed_attachments URL loop (no change to hybrid PDF mechanics yet — annex bytes still travel inside embedded Factur-X XML)
  • Tests: New tests cover embedding behaviour of the legacy path as well as the table path, migration from legacy field to table and table-specific validations.

Not in this PR (possible follow-ups): PDF/A-3 factur-x pipeline with N PDF file attachments (#262), profile-aware 916 encoding, MIME allowlist, v15/develop backports.

Settings:
image

Migration Button UI:
image

Sales Invoice EInvoice tab:
image

Test plan

  • bench --site <site> run-tests --app eu_einvoice --lightmode — 22 tests green
  • E Invoice Settings: enable Multiple Attachment Embedding → legacy attach field hidden/read-only
  • Sales Invoice with legacy einvoice_embedded_document only → save with setting on → row appears in Embedded Documents, legacy field cleared, orange migration message
  • Sales Invoice with broken legacy URL → save with setting on → field unchanged, Error Log entry (no blocking save)
  • E Invoice SettingsMigrate attachments to table → RQ job clears legacy fields site-wide
  • Sales Invoice with two rows in einvoice_attachments → submit/download PDF → annex payloads present in embedded Factur-X XML (Desk spot-check or XML download)

Notes for reviewers

  • Hybrid PDF path unchanged: attach_xml_to_pdf still embeds invoice XML only; new integration test verifies table annex content survives the PDF round-trip inside that XML.
  • Target branch is version-16-hotfix per rollout plan; develop backport differs (patch migration drops legacy field).

Ticket Ref

LMK-16

dafrose added 7 commits June 12, 2026 11:38
Pin legacy embed behaviour with YAML scenarios (empty field, local 916,
https and /api/method/ remote uri_id) and Drafthorse assertions on mocked
find_file_by_url. Ignore .vscode/ in the app repo.
Replace _embed_attachment with get_legacy_embed_attachment and
_embed_attachments so create_einvoice passes a URL list. Add integration
test and before_tests fixtures so embed tests use governed master data
instead of site-specific rows.
…ation

Introduce E Invoice Attachment Row, einvoice_attachments on Sales Invoice,
multi_attachment_embed_enabled setting, first-row-wins dedup on validate,
and Desk attach-button UX.
Branch create_einvoice on get_embed_attachments; move attachment
resolution to sales_invoice_attachments; consolidate tests and helpers.
Move einvoice_embedded_document into einvoice_attachments when
multi-attachment embed is enabled, with per-save migration on Sales
Invoice validate and a deduplicated bulk RQ job from E Invoice
Settings. Lock the legacy attach field when multi-embed is on, fail
embed on missing File rows, and add integration coverage for
migration, lockdown, and broken links.
…units

Add an integration test that embeds a table annex through attach_xml_to_pdf
and verifies the payload survives extraction from the hybrid Factur-X PDF.
Introduce shared helpers for minimal PDF bytes and CII AttachmentBinaryObject
assertions.

Remove unit tests for deduplicate_attachment_rows, get_legacy_embed_attachment,
and mocked get_table_embed_attachments; those paths are already covered by
integration tests.
Regenerate POT/PO from the attachment migration strings and translate
23 new and 1 adapted msgid in de.po.
@dafrose dafrose force-pushed the feat/embed_attachments_16 branch from 3e495c6 to 53520f4 Compare June 15, 2026 12:17
@dafrose

dafrose commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

@greptileai

@greptile-apps

greptile-apps Bot commented Jun 16, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

Safe to merge; the core embed logic, migration paths, and permission gates are all correct and well-tested.

The attachment routing, per-save migration, bulk migration (including the cancelled-invoice fix), and permission gate on the whitelisted endpoint are all implemented correctly. The two DocType JSON observations (field array order mismatch and a no-op flag on a Single DocType) are cosmetic and do not affect runtime behaviour. The potential duplicate-row scenario for submitted invoices in bulk migration is an edge case that only matters if someone manually pre-populated the child table before running migration while keeping the legacy field set — an unlikely combination in practice.

eu_einvoice/european_e_invoice/doctype/e_invoice_attachment_row/e_invoice_attachment_row.json — field order inconsistency between field_order and fields array suggests a manual JSON edit that should be re-exported via the Desk.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Sales Invoice save / submit] --> B[validate_doc]
    B --> C{multi_attachment_embed_enabled?}
    C -- Yes --> D[migrate_legacy_embed_to_table]
    D --> E{einvoice_embedded_document set?}
    E -- Yes, file found --> F[append row to einvoice_attachments\nclear legacy field]
    E -- Yes, broken URL --> G[log Error Log\nleave field unchanged]
    E -- No --> H[skip]
    C -- No --> I[skip migration]
    B --> J{einvoice_attachments rows?}
    J -- Yes --> K[deduplicate_attachment_rows]
    K --> L[validate_einvoice]
    J -- No --> L
    L --> M[create_einvoice]
    M --> N[get_embed_attachments]
    N --> O{multi_attachment_embed_enabled?}
    O -- Yes --> P[get_table_embed_attachments\nread file_url per row]
    O -- No --> Q[get_legacy_embed_attachment\nread einvoice_embedded_document]
    P --> R[_embed_attachments loop]
    Q --> R
    R --> S[CII ARD 916 node per file_url]
    T[Bulk Migration Button] --> U[migrate_attachments_to_table]
    U --> V{System Manager?}
    V -- No --> W[PermissionError]
    V -- Yes --> X[enqueue bulk_migrate_legacy_embed_attachments]
    X --> Y{docstatus == 0?}
    Y -- Yes --> Z[_persist_legacy_embed_migration_on_save\ninvoice.save]
    Y -- No --> AA[_persist_legacy_embed_migration_db\ndb.set_value]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Sales Invoice save / submit] --> B[validate_doc]
    B --> C{multi_attachment_embed_enabled?}
    C -- Yes --> D[migrate_legacy_embed_to_table]
    D --> E{einvoice_embedded_document set?}
    E -- Yes, file found --> F[append row to einvoice_attachments\nclear legacy field]
    E -- Yes, broken URL --> G[log Error Log\nleave field unchanged]
    E -- No --> H[skip]
    C -- No --> I[skip migration]
    B --> J{einvoice_attachments rows?}
    J -- Yes --> K[deduplicate_attachment_rows]
    K --> L[validate_einvoice]
    J -- No --> L
    L --> M[create_einvoice]
    M --> N[get_embed_attachments]
    N --> O{multi_attachment_embed_enabled?}
    O -- Yes --> P[get_table_embed_attachments\nread file_url per row]
    O -- No --> Q[get_legacy_embed_attachment\nread einvoice_embedded_document]
    P --> R[_embed_attachments loop]
    Q --> R
    R --> S[CII ARD 916 node per file_url]
    T[Bulk Migration Button] --> U[migrate_attachments_to_table]
    U --> V{System Manager?}
    V -- No --> W[PermissionError]
    V -- Yes --> X[enqueue bulk_migrate_legacy_embed_attachments]
    X --> Y{docstatus == 0?}
    Y -- Yes --> Z[_persist_legacy_embed_migration_on_save\ninvoice.save]
    Y -- No --> AA[_persist_legacy_embed_migration_db\ndb.set_value]
Loading

Reviews (9): Last reviewed commit: "docs(attachments): add milestone 1 docst..." | Re-trigger Greptile

Comment thread eu_einvoice/european_e_invoice/doctype/e_invoice_settings/e_invoice_settings.py Outdated
Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice_attachments.py
Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice_attachments.py
dafrose added 2 commits June 16, 2026 09:34
Add an Include submitted Sales Invoices option on the E Invoice Settings
migrate button. Draft invoices still migrate via save; submitted invoices
use direct child-row inserts and db.set_value so post-submit restrictions
do not block the job. Replace the bulk migration integration test with a
draft/submitted × include_submitted matrix and cancel submitted fixtures
before teardown.
Throw when an einvoice_attachments row points at a File without a
file_url, load File rows via the normal get_doc path during legacy
embed resolution, and restrict site-wide migration to System Manager.
@dafrose

dafrose commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

@greptileai

@dafrose dafrose marked this pull request as ready for review June 17, 2026 08:10
@dafrose dafrose requested a review from barredterra June 17, 2026 08:11
Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice_attachments.py Outdated
@dafrose dafrose force-pushed the feat/embed_attachments_16 branch from d3710c5 to d0582ca Compare June 17, 2026 09:08
Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice_attachments.py
Restructure attachment test coverage: split get_embed_attachments
branching into subTest cases, drop redundant table-principles unit class,
extract URL-order resolver checks, and restore multi-embed settings in
integration setUp/tearDown. Move set_multi_attachment_embed_enabled and
assert_single_orange_message into tests/helpers.py.

Add integration coverage for bulk migration of cancelled submitted
invoices and prefer a positive docstatus check in bulk_migrate_legacy_embed_attachments.

Regenerate fixtures to update attachment row schema and enable file id field visibility,
for the row filter to work on Sales Invoice Attachments table.
@dafrose dafrose force-pushed the feat/embed_attachments_16 branch from d0582ca to a5c5022 Compare June 17, 2026 13:27
Share hidden/read_only flags via legacy_embed_field_lockdown_properties()
so get_custom_fields and set_legacy_embed_field_lockdown stay aligned. This
prevents after_install and migrate patches from re-exposing the legacy attach
field when multi-attachment embedding is enabled.
@barredterra barredterra requested a review from 0xD0M1M0 June 19, 2026 00:53
Comment thread eu_einvoice/european_e_invoice/doctype/e_invoice_settings/e_invoice_settings.py Outdated
Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice.js
Give operators explicit feedback when bulk legacy-embed migration hits
unresolvable URLs: skip (Error Log, field unchanged) or remove (clear field,
site warning log). Add a dialog toggle, split the job summary into skipped,
removed, and error counts, and restrict the enqueue API to POST.
@dafrose dafrose force-pushed the feat/embed_attachments_16 branch from 077bfb3 to 49cfcc5 Compare June 19, 2026 09:56
Comment thread eu_einvoice/european_e_invoice/custom/sales_invoice_attachments.py Outdated
dafrose added 2 commits June 19, 2026 12:15
Show an orange msgprint when per-save migration cannot resolve
einvoice_embedded_document, in addition to the existing Error Log.
Guide invoice owners to ask a System Manager to run bulk migration on
E Invoice Settings. Remove unused _log_removed_broken_legacy_embed
(inlined in _handle_broken_legacy_embed). Add tests for the validate
warning and bulk migration summary segments.
@greptile-apps

greptile-apps Bot commented Jun 19, 2026

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Try greploops.

@dafrose

dafrose commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

I tested a simple print format addition for the embedded files.

The code I used:

		{# Embedded e-invoice annexes (916 supporting documents) #}
		{% set multi_embed = frappe.db.get_single_value("E Invoice Settings", "multi_attachment_embed_enabled") %}
		{% if multi_embed and doc.einvoice_attachments %}
		<div class="info-card">
			<div class="title">{{ _("Embedded Documents") }}</div>
			<ul class="small-text" style="margin: 8px 0 0 0; padding-left: 18px;">
				{% for row in doc.einvoice_attachments %}
				{% set file_url = frappe.db.get_value("File", row.file, "file_url") %}
				<li style="margin-bottom: 4px;">
					{% if file_url %}
					<a href="{{ frappe.utils.get_url(file_url) }}">
						{{ row.display_name or row.file_name or file_url }}
					</a>
					{% else %}
					{{ row.display_name or row.file_name or row.file }}
					{% endif %}
				</li>
				{% endfor %}
			</ul>
		</div>
		{% elif not multi_embed and doc.einvoice_embedded_document %}
		<div class="info-card">
			<div class="title">{{ _("Embedded Document") }}</div>
			<p class="small-text" style="margin-top: 8px;">
				<a href="{{ frappe.utils.get_url(doc.einvoice_embedded_document) }}">
					{{ doc.einvoice_embedded_document }}
				</a>
			</p>
		</div>
		{% endif %}

The result:

image

Expand Google-style docstrings on embed resolution, legacy migration,
bulk migrate APIs, and test helpers. Regenerate locale catalogs and
translate milestone 1 user-facing strings in de.po for the multi-annex
embed settings, migration dialog, and Sales Invoice annex grid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants