fix(email): delegate the inbound fetch to the framework - #3695
Open
chdecultot wants to merge 1 commit into
Open
Conversation
`CustomEmailAccount.get_inbound_mails` is a full copy of
`EmailAccount.get_inbound_mails`. Only two things in it are helpdesk's:
skipping auto-generated mails, and threading through `CustomInboundMail`.
Everything else -- the IMAP folder loop, the sync rule, the connection
handling -- is duplicated framework code, so any change the framework makes
to the fetch loop is silently dropped on every site where helpdesk is
installed.
The copy also assumes the fetched messages are bytes:
_msg = message_from_string(message.decode("utf-8", errors="replace"))
When they are not, the `except Exception` around it swallows the
`AttributeError` per message and the account quietly yields zero mails.
frappe's own `test_email_account` helpers build `messages` from `str`, which
is how I ran into this.
Do both customisations on top of `super().get_inbound_mails()` instead.
`InboundMail` already carries everything needed to re-wrap it: `raw_message`,
`uid`, `seen_status` and `append_to`.
Verified on a bench with helpdesk installed: the
`frappe.email.doctype.email_account` test module goes from 1 failure and 9
errors to a single unrelated SMTP error. Note this bench runs Dodock, a
Frappe fork, so the exact counts may differ upstream -- but the duplication
and the bytes assumption are the same in both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Tick the box to add this pull request to the merge queue (same as
|
Contributor
Confidence Score: 5/5The PR appears safe to merge with no actionable changed-code defect identified. The delegated results retain the raw message, UID, seen status, and append target needed by the custom inbound-mail wrapper, while per-message filtering and error handling remain intact. Reviews (1): Last reviewed commit: "fix(email): delegate the inbound fetch t..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
CustomEmailAccount.get_inbound_mailsis a full copy ofEmailAccount.get_inbound_mails. Only two things in it are actually helpdesk's:X-Auto-Generated)CustomInboundMailEverything else — the IMAP folder loop, the sync rule, the connection handling, the error path — is duplicated framework code. Any change the framework makes to the fetch loop is silently dropped on every site where helpdesk is installed.
The copy also assumes the fetched messages are bytes:
When they are not, the
except Exceptionaround it swallows theAttributeErroronce per message and the account quietly yields zero mails. frappe's owntest_email_accounthelpers buildmessagesfromstr, which is how I ran into it.Fix
Do both customisations on top of
super().get_inbound_mails().InboundMailalready carries everything needed to re-wrap it:raw_message,uid,seen_statusandappend_to.Net effect: -66 lines, and helpdesk inherits framework fixes to the fetch loop from now on.
Verification
On a bench with helpdesk installed, the
frappe.email.doctype.email_accounttest module goes from 1 failure + 9 errors to a single unrelated SMTP error (test_sendmail).One caveat in the interest of transparency: that bench runs Dodock, a Frappe fork, so the exact counts may differ against upstream frappe. The duplication and the bytes assumption are identical in both, and the 9 errors were all this override swallowing every message. Happy to re-run against upstream frappe if you'd like the numbers from there.
How I found it: on a multi-folder IMAP account, mail filed into a folder mapped to
append_toproduced no document unless it happened to be the newest message in that folder at poll time. The framework's per-folder UID cursor was handling this correctly; the override's copy of the loop predates it and never ran that code.🤖 Generated with Claude Code