Improve attachment handling and sync_labels_api robustness#66
Conversation
…set attachment limit
Two bugs in the old per-attachment loop:
1. frappe.db.get_value("File", file_doc_name, "file_url") returns None
when the File row has been deleted. The function then wrote
attachment["file_url"] = None and returned it. The timeline_message_box
template renders a.file_url.split("/") to derive the filename, which
crashed with "Cannot read properties of null" — the whole form
timeline failed to render.
2. The frappe.db.get_value call was inside the per-attachment loop,
so a thread with N attachments did N round-trips against tabFile.
Now a single frappe.db.get_all collects every referenced file_url, and
attachments whose File row is gone are dropped from the returned list
so the template never sees a null file_url.
There was a problem hiding this comment.
Pull request overview
This PR improves the Gmail thread integration by optimizing attachment URL resolution for timeline rendering, making the sync_labels_api endpoint more flexible/robust, and enhancing inbound email initialization with account context.
Changes:
- Batch-fetch
File.file_urlvalues when building attachment metadata and skip missing files to avoid template crashes. - Update
sync_labels_apito accept both JSON-string and dict inputs, with stronger argument validation. - Extend
GmailInboundMailinitialization to accept an optional account context and pass the Gmail account fromcreate_new_email.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| frappe_gmail_thread/utils/helpers.py | Passes account context into GmailInboundMail during email creation and adjusts initialization behavior. |
| frappe_gmail_thread/frappe_gmail_thread/doctype/gmail_account/gmail_account.py | Expands sync_labels_api input handling (string/dict) and adds validation paths. |
| frappe_gmail_thread/api/activity.py | Refactors attachment URL resolution to avoid N+1 queries and filters invalid/missing attachments for safe rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not hasattr(self.email_account, "attachment_limit"): | ||
| self.email_account.attachment_limit = None | ||
|
|
||
| super().__init__(content, email_account=self.email_account) |
There was a problem hiding this comment.
I see that the Email class does not accept any email_account or kwargs arguments. Reference: https://github.com/frappe/frappe/blob/fcdc612120cd68cc21e756ace9b941df025ccc0e/frappe/email/receive.py#L399
Can you confirm if this is tested and working (and how)?
There was a problem hiding this comment.
Approving this, and closing #47 as both tries to solves the same problem.
Description
This pull request introduces several improvements and bug fixes to the Gmail thread integration, focusing on more efficient attachment handling, improved API flexibility, and enhanced email object initialization. The most significant changes are grouped below:
Attachment Handling Optimization:
get_attachments_datainactivity.pyto batch-fetch all referencedFileURLs in a single database query, eliminating the previous N+1 query pattern and skipping missing files to avoid rendering errors.API Usability Improvements:
sync_labels_apito accept both string and dictionary arguments, and added validation to ensure adoc_nameis provided, improving robustness and flexibility of the API.Email Object Initialization Enhancements:
GmailInboundMailto accept an optionalemail_accountparameter and set itsattachment_limittoNoneduring initialization, allowing for more flexible handling of email account properties.create_new_emailto pass thegmail_accounttoGmailInboundMail, ensuring the email object is initialized with the correct account context.Relevant Technical Choices
Testing Instructions
Additional Information:
Screenshot/Screencast
Checklist
Fixes #