Repository navigation
Conversation
Lets users parse a Communication's attachments on demand. The dialog is pre-filled from Transaction Parser Settings (matched email account, party emails, default AI model, one-document toggle) and every value can be changed before parsing. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| doc = frappe.get_doc("Communication", communication) | ||
| doc.check_permission("read") | ||
|
|
||
| frappe.has_permission(TRANSACTION_MAP[transaction], "create", throw=True) |
There was a problem hiding this comment.
parse_communication only requires read access to the Communication, but its job writes is_processed_by_transaction_parser with doc.db_set. A caller with read access and permission to create the target document can therefore change a Communication they cannot edit. Check write permission before queuing the job, and add a test for a read-only caller.
How this was verified: The endpoint checks only read access before queuing _parse_attachments, which directly writes the selected Communication without another permission check.
| frappe.enqueue( | ||
| "transaction_parser.transaction_parser.overrides.communication._parse_attachments", | ||
| doc=doc, | ||
| country=_get_country(company), | ||
| transaction_type=transaction, | ||
| attachments=[supported_attachments[url] for url in file_urls], |
There was a problem hiding this comment.
The same attachments can be queued twice before the first job finishes. After the request returns, the dialog closes and the form reloads while the attachments are still present, so the user can click Parse again. Neither the endpoint nor the worker reserves those files or checks their current attachment before parsing.
_parse finds the files by URL even after the first job moves them. Both jobs can create drafts, and the second job moves the files off the first draft. Reserve the selected files when queuing and recheck their ownership in the worker.
📝 WalkthroughWalkthroughThe change adds automatic and manual parsing for Communication attachments. Shared helpers handle account and party matching, attachment filtering, and country mapping. Whitelisted endpoints return parsing defaults and validate manual parsing requests before enqueueing them. The client adds a Parse dialog for saved Communications. Boot information exposes whether parsing is enabled. Tests cover defaults, parsing modes, invalid attachments, and automatic enqueue arguments. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to The new Parse dialog displays attachment filenames without escaping them. A file with a crafted name could inject script into the page of a user who opens the dialog. On sites with developer mode enabled, parsing also runs inside the web request rather than in the background queue. The filename escaping should be fixed before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
afea25e0-41ec-4b05-a6b6-b423ad1217d6
📒 Files selected for processing (5)
transaction_parser/boot.pytransaction_parser/hooks.pytransaction_parser/public/js/communication.jstransaction_parser/tests/test_communication.pytransaction_parser/transaction_parser/overrides/communication.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| label: attachment.file_name || attachment.file_url, | ||
| value: attachment.file_url, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect the defaults payload and the project's declared Frappe version.
rg -n -C 8 'def get_parse_defaults|file_name|attachments' transaction_parser/transaction_parser/overrides/communication.py
rg -n -C 3 'frappe|framework' pyproject.tomlRepository: resilient-tech/transaction-parser
Length of output: 8391
🏁 Script executed:
git diff --no-ext-diff --unified=15 86a6f0b514105b55279a20b8021851ece67d3a8e fb878e68f6e1c5e0a1ff06b1c4a4813c603cf952 -- transaction_parser/public/js/communication.js
printf '\\n--- current source ---\\n'
nl -ba transaction_parser/public/js/communication.js | sed -n '55,135p'
printf '\\n--- dependency declaration ---\\n'
nl -ba pyproject.toml | sed -n '40,50p'Repository: resilient-tech/transaction-parser
Length of output: 6973
🌐 Web query:
frappe version-15 frappe/public/js/frappe/form/controls/multicheck.js option label value HTML template source
💡 Result:
For **Frappe `version-15`**, the option HTML is built in `get_checkbox_element(option)` in [`multicheck.js`](https://github.com/frappe/frappe/blob/version-15/frappe/public/js/frappe/form/controls/multicheck.js):
```html
<div class="checkbox unit-checkbox">
<label title="${option.description || ""}" style="display: flex; align-items: center;">
<input type="checkbox" data-unit="${option.value}" style="flex-shrink: 0;">
<span class="label-area" data-unit="${option.value}">${option.label}</span>
</label>
</div>
```
Each option is an object with `label` and `value`; `checked` is also used to initialize selection. The `value` goes into `data-unit` and is returned as the selected value; the `label` is displayed in the span. ([github.com](https://github.com/frappe/frappe/blob/version-15/frappe/public/js/frappe/form/controls/multicheck.js))
Citations:
- 1: https://github.com/frappe/frappe/blob/version-15/frappe/public/js/frappe/form/controls/multicheck.js
Escape attachment options before passing them to MultiCheck.
get_parse_defaults returns attachment.file_name unchanged. Frappe 15's MultiCheck inserts option labels and values into an HTML template without escaping them. A filename containing HTML can execute when the dialog opens. Escape both label and value for the template.
| frappe.enqueue( | ||
| "transaction_parser.transaction_parser.overrides.communication._parse_attachments", | ||
| doc=doc, | ||
| country=_get_country(company), | ||
| transaction_type=transaction, | ||
| attachments=[supported_attachments[url] for url in file_urls], | ||
| ai_model=ai_model or None, | ||
| user=frappe.session.user, | ||
| party=party or None, | ||
| company=company, | ||
| process_one_document=cint(process_one_document), | ||
| page_limit=cint(page_limit) or None, | ||
| queue="long", | ||
| now=frappe.conf.developer_mode, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not run parsing synchronously based on developer_mode.
now=frappe.conf.developer_mode makes the whole AI parsing pipeline run inside the HTTP request on any site with developer mode enabled. Developer mode can be on in staging and on-prem deployments. On those sites, large PDFs or slow AI responses can exceed the request timeout, and the user receives an error instead of a queued job. The job also uses frappe.session.user and doc inside the request transaction, so a parse failure behaves differently from the queued path. The automatic hook does not set now, so the two paths differ. Remove now. Tests already patch frappe.enqueue.
🐛 Proposed fix
page_limit=cint(page_limit) or None,
queue="long",
- now=frappe.conf.developer_mode,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| frappe.enqueue( | |
| "transaction_parser.transaction_parser.overrides.communication._parse_attachments", | |
| doc=doc, | |
| country=_get_country(company), | |
| transaction_type=transaction, | |
| attachments=[supported_attachments[url] for url in file_urls], | |
| ai_model=ai_model or None, | |
| user=frappe.session.user, | |
| party=party or None, | |
| company=company, | |
| process_one_document=cint(process_one_document), | |
| page_limit=cint(page_limit) or None, | |
| queue="long", | |
| now=frappe.conf.developer_mode, | |
| ) | |
| frappe.enqueue( | |
| "transaction_parser.transaction_parser.overrides.communication._parse_attachments", | |
| doc=doc, | |
| country=_get_country(company), | |
| transaction_type=transaction, | |
| attachments=[supported_attachments[url] for url in file_urls], | |
| ai_model=ai_model or None, | |
| user=frappe.session.user, | |
| party=party or None, | |
| company=company, | |
| process_one_document=cint(process_one_document), | |
| page_limit=cint(page_limit) or None, | |
| queue="long", | |
| ) |
🧰 Tools
🪛 GitHub Actions: Linters / 0_linters.txt
[error] 233-242: semgrep ci --config ./frappe-semgrep-rules/rules --config r/python.lang.correctness failed: Semgrep rule frappe-semgrep-rules.rules.security.whitelisted-side-effect-on-get reports parse_communication permits GET requests while enqueuing a job, bypassing CSRF checks. Restrict the whitelisted method to POST (or another unsafe method).
🪛 GitHub Actions: Linters / linters
[error] 233-233: semgrep ci --config ./frappe-semgrep-rules/rules --config r/python.lang.correctness failed: Semgrep rule whitelisted-side-effect-on-get flags parse_communication: it can accept GET requests while enqueueing work outside the request rollback. Restrict the whitelist to POST (or another unsafe method).
- mark the communication processed when parsing is enqueued, so a second click before the job finishes asks for confirmation - set the processed flag without touching `modified`, so an open form can still be saved after the job finishes - validate company, party and AI model in parse_communication - hide the Parse button and transaction options the user cannot create - start with a blank transaction and hide the party field until one is chosen when no incoming email account matches - show or hide the button when attachments change, without a reload - send supported extensions, party types and creatable transactions in bootinfo instead of duplicating them in JS Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Fixes the blocking frappe semgrep rules missing-argument-type-hint and whitelisted-side-effect-on-get. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Summary
Adds a Parse button to the Communication form. It opens a dialog pre-filled from Transaction Parser Settings. The user can change any value and parse the selected attachments, using the same
_parse_attachmentsjob as the incoming-email hook.What the dialog pre-fills
Page Limit starts empty. PDF Processor is not in the dialog and still comes from settings.
Behaviour
is_processed_by_transaction_parseras soon as parsing is queued, without changingmodified. If it is already marked, the user confirms before it is parsed again, so a second click while the job is still running also asks first. Parsed files are moved to the generated document by the existingTransaction._attach_file. So the button only stays visible, and the prompt only appears, while some attachments are left unparsed.parse_communicationis POST-only and type-hinted. It checks read permission on the Communication, create permission on the target doctype, and that the company exists and is readable. It also checks that the party exists for the transaction's party type, and that the AI model is supported. It rejects any file that is not a supported attachment of that Communication.Changes
overrides/communication.pyon_updateand two new whitelisted methods:get_parse_defaultsandparse_communication._parse_attachmentsaccepts optionalprocess_one_documentandpage_limit, falling back to settings as before. It now sets the processed flag withupdate_modified=False, so an open form doesn't hit a "modified after you opened it" error.on_updatestill enqueues the same arguments.public/js/communication.js: the button and the dialog.boot.pyandhooks.py: addfrappe.boot.transaction_parser(enabled flag, supported extensions, party types, transactions the user can create), so the form script needs no server call or copied constants, and register the form script.public/js/transaction_parser_dialog.js: exportsTRANSACTION_LABEL_MAPso both dialogs use the same labels.tests/test_communication.py: tests for the defaults, attachment and input validation, the per-file vs combined parse, the early processed flag, bootinfo, and regression checks foron_update.Notes for review
frappe.boot.transaction_parserlives in bootinfo, which Frappe caches per session. After Enabled is toggled in settings, the button appears or disappears only after the user's cache is cleared or they log in again. If the button is still visible after the parser was disabled, clicking it shows "Please enable Transaction Parser Settings".The automatic
on_updatehook still doesn't checkis_processed_by_transaction_parser, so it can parse an email again when it is saved later. This is existing behaviour and is left for a separate change.Testing
bench run-tests --app transaction_parser --module transaction_parser.tests.test_communication: 12 tests pass. The AI parser and the background queue are mocked.AIParser._make_api_callwas stubbed to return canned Sales Order JSON.Visual QA
Recorded on a local test site with test data only.
communication-parse-dialog.webm
.txtfile attached.PO-1001.pdfattached.🤖 Generated with Claude Code