feat: implement XML attachment file naming functionality#253
feat: implement XML attachment file naming functionality#253dafrose wants to merge 2 commits intoalyf-de:developfrom
Conversation
- Added E Invoice Settings field for global autonaming pattern for XML attachements. - Added `get_xml_attachment_file_base_name` function to resolve file names. - Introduced unit tests for various naming scenarios in `test_xml_attachment_naming.py`. - Updated Sales Invoice logic to utilize the new naming function for XML file attachments. - Enhanced E Invoice Settings to include a new field for auto naming format of XML files.
dafrose
changed the title
Greptile SummaryThis PR introduces a configurable XML attachment file naming pattern for E Invoice Settings, allowing users to define a dot-separated naming series (with date codes and field references) that is resolved at submit time via Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/UX observations with no correctness impact on the happy path. The core logic is correct, the fallback chain is robust, and the test suite covers the main scenarios. Both flagged items are P2: one is a code-organisation nit (redundant cached doc fetch) and the other is an edge-case UX issue with patterns that mix hash segments and separator literals — neither affects correctness for patterns described in the UI documentation. eu_einvoice/xml_attachment_naming.py — dangling separator edge case; eu_einvoice/european_e_invoice/custom/sales_invoice.py — redundant settings fetch in _attach_xml_file. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Sales Invoice submitted] --> B{auto_attach_xml?}
B -- No --> Z[Exit]
B -- Yes --> C[get_cached_doc E Invoice Settings]
C --> D[get_einvoice XML bytes]
D --> E[_attach_xml_file doc, xml, field_name]
E --> F[get_cached_doc E Invoice Settings again]
F --> G[get_xml_attachment_file_base_name doc, pattern]
G --> H{pattern empty?}
H -- Yes --> I[sanitize doc.name]
H -- No --> J[strip hash-only dot segments]
J --> K{pattern now empty?}
K -- Yes --> I
K -- No --> L[parse_naming_series pattern.split dot]
L --> M{exception?}
M -- Yes --> N[log_error + fallback doc.name]
M -- No --> O{base empty?}
O -- Yes --> I
O -- No --> P[sanitize base]
I --> Q[base_name + .xml]
P --> Q
Q --> R[Create File doc + save]
Reviews (2): Last reviewed commit: "refactor: enhance XML attachment naming ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _sanitize_base(base: str) -> str: | ||
| return base.replace("/", "-") |
There was a problem hiding this comment.
_sanitize_base only strips /
File names that include characters like :, *, ?, ", <, >, |, or \ can cause problems on some operating systems or web servers (e.g. Windows-based deployments, certain cloud storage backends). A field value such as "Invoice: 001" or "Q1 > Q2" resolved into the pattern would produce a file name that passes through unmodified. Expanding the sanitization pattern guards against this edge case.
| def _sanitize_base(base: str) -> str: | |
| return base.replace("/", "-") | |
| _UNSAFE_CHARS = str.maketrans({c: "-" for c in r'/\:*?"<>|'}) | |
| def _sanitize_base(base: str) -> str: | |
| return base.translate(_UNSAFE_CHARS) |
There was a problem hiding this comment.
I compared the suggestion to how Frappe does in their exposed API:
Naming resolution does not sanitize arbitrary field text: parse_naming_series / _format_autoname interpolate values as-is, and validate_name only rejects < and > for document names.1
Partial overlap with File: when the attachment is saved, Frappe already rewrites / \ % ? # to _ for the on-disk / URL basename via get_safe_file_name2 (used from save_file_on_filesystem3). It does not cover the full Windows-reserved set (: * ? " < > |, etc.), so broader sanitization in our app would be stricter than core today, not redundant with naming.
Suggested change: reuse get_safe_file_name here so the XML base name matches what File will apply anyway, instead of only replacing /. Prefer adding from frappe.core.doctype.file.utils import get_safe_file_name next to the other imports at the top of this module and keeping _sanitize_base as a thin wrapper (shown below with a local import so the suggestion applies to the existing hunk only).
| def _sanitize_base(base: str) -> str: | |
| return base.replace("/", "-") | |
| def _sanitize_base(base: str) -> str: | |
| from frappe.core.doctype.file.utils import get_safe_file_name | |
| return get_safe_file_name(base) |
Footnotes
There was a problem hiding this comment.
Sounds good, but if the wrapper only calls get_safe_file_name we might as well drop the wrapper and call it directly.
|
Here is, how this looks in Desk:
|
|
@dafrose I think allowing endings like .#### creates new (global) naming series counters in the database and is something that we do not need. Normally a user wants to:
in the .xml name + some string like "Rechnung-", which makes the .xml prefix usually unique per Invoice, so no further numbering is needed |
- Updated `get_xml_attachment_file_base_name` to strip segments consisting only of '#' characters. - Added new test cases in `test_xml_attachment_naming.py` for various naming scenarios. - Improved documentation in the E Invoice Settings JSON for clarity on naming patterns.
|
I removed the mention of |
|
Just thinking of the manual "download"-button. Is there a reason, why, if the admin chooses to rename automatically, we would exclude it on this button? |
| return _sanitize_base(cstr(doc.name)) | ||
|
|
||
| try: | ||
| base = parse_naming_series(pattern.split("."), doc=doc) |
There was a problem hiding this comment.
| base = parse_naming_series(pattern.split("."), doc=doc) | |
| base = parse_naming_series([p.lstrip("#") for p in pattern.split(".")], doc=doc) |
parse naming series checks for parts starting with "#", so it would make sense to filter the exact same pattern and not just parts containing only #. Also condensing this into one line makes _strip_hash_only_dot_segments and the _HASH_ONLY_SEGMENT unneeded
| pattern = _strip_hash_only_dot_segments(pattern) | ||
| pattern = pattern.strip() | ||
| if not pattern: | ||
| return _sanitize_base(cstr(doc.name)) | ||
|
|
There was a problem hiding this comment.
| pattern = _strip_hash_only_dot_segments(pattern) | |
| pattern = pattern.strip() | |
| if not pattern: | |
| return _sanitize_base(cstr(doc.name)) |
See comment below
4 participants
get_xml_attachment_file_base_namefunction to resolve file names.test_xml_attachment_naming.py.closes #245