Skip to content

[28.1] Multiple runs of email retrieval on the same emails don't persist again - #11617

Open
Joshua (mynjj) wants to merge 2 commits into
releases/28.1from
bugs/emailmoduledupstorage-28.1
Open

Joshua (mynjj) wants to merge 2 commits into
releases/28.1from
bugs/emailmoduledupstorage-28.1

Conversation

@mynjj

@mynjj Joshua (mynjj) commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Backport of #11612.

Fix at the email module layer of the bug causing additional entries to be persisted when retrieving the same email.

If the email had already been retrieved and stored, then such email is returned without further persisting, unless the previous email was stored without attachments, in which case only the attachment is added to such email message.

Tested the behavior against my email inbox locally.

Fix overview

  • A new centralized FindRetrievedEmail on the email module lets connectors check whether an email had been retrieved before, with the idea of centralizing this potentially tricky logic and benefitting other connectors.
  • For adding attachments when the email has already been retrieved, a new overload of AddAttachment was needed, since the Modify on the email message that was run after, triggers a subscriber that errors when modifying a received email (rationale of the error is that the system shouldn't modify emails after they have been received, but in this scenario we are not modifying the EmailMessage, but rather adding an attachment and leaving the message as it was, so we don't want this validation to run)

Note

In this version the Email - Outlook REST API app does not live in BCApps, so this PR carries the System Application part only. The consuming change in the Outlook connector (Email - Outlook API Helper) has to be done in the NAV repository for this branch.

Fixes AB#650461

@mynjj
Joshua (mynjj) requested a review from a team as a code owner September 18, 2026 14:19
@github-actions github-actions Bot added AL: System Application Team: Integrations GitHub request for Integrations area labels Sep 18, 2026
@github-actions github-actions Bot added this to the Version 28.1 milestone Sep 18, 2026
dcenic
dcenic previously approved these changes Sep 18, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Accept

What this PR does

This change adds a central lookup for emails that were already retrieved by account and external message id, then lets a connector return that existing inbox entry instead of creating another one. It also adds a specialized attachment path for messages that were retrieved earlier without attachments.

The lookup is scoped by account and external id, handles temporary result records, and marks the existing row so the existing retrieval flow can return it. The attachment path skips only the email-message Modify() call that rejects retrieved messages; the attachment insert still goes through the existing attachment checks.

Problem-solution fit

Fit: Strong

The reported problem is that retrieving the same provider message again can persist duplicate inbox entries. The new lookup, key, attachment overload, and tests cover the existing-entry, wrong-account/wrong-message, temporary-result, and missing-attachment cases, so the code matches the scenario.

Suggestions

None.

Risk assessment and necessity

Risk: The change touches public email APIs and the Email Inbox lookup path, so the main regression surface is connectors that retrieve into persistent records and extensions that call the new attachment overload. Existing AddAttachment overloads keep the old validation path, and the new lookup is scoped by account plus external id.

Necessity: The change is needed because filtering alone cannot stop a connector from storing the same provider message again. Centralizing duplicate detection in the email module keeps the rule consistent for connectors and allows the missing-attachment case without reopening general edits to retrieved messages.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11617 round=1 by=alexei-dobriansky at=2026-09-18T22:27:00Z lastSha=10af3a5a8276a493fc2ec0b818fbd0cc4b7e14a3 reviewKey=8fc0a4e5271c87d988f7ca60db54282e8cccc6e27701003e8ccfb56252b8a2ab suggestions=none

ventselartur
ventselartur previously approved these changes Sep 21, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept

What this PR does

The PR adds a central lookup for already retrieved emails and a safe attachment path for a retrieved message that was first stored without attachments. Since round 1, the only author change renames local test variables from TempEmailAccount to EmailAccount, which keeps the tests aligned with the analyzer rule and does not change the tested behavior.

Status of previous suggestions

None - round 1 had no suggestions.

New observations (commits since round 1)

None - the round-2 change only updates local test variable names.

Risk assessment and necessity

Risk: The regression surface is the public email API, the Email Inbox lookup by account and external message id, and attachment loading for retrieved messages. The added APIs are additive, the lookup is scoped, and the attachment insert still keeps the existing sent and queued checks.

Necessity: The change is needed to avoid storing duplicate inbox entries when the same provider message is retrieved again. The scope is right for this branch: it adds the shared email-module support and leaves the branch-specific connector consumption to the repository where that connector lives.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11617 round=2 by=alexei-dobriansky at=2026-09-21T22:22:34Z lastSha=77ffc55ff625bb9aeacc3e7578ca651636d28ae7 reviewKey=21164f79b7c2013ed7c2d4f6f83c896fcc7e23193ce1111435fe3adc1601fad5 suggestions=none parentRound=1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: System Application Team: Integrations GitHub request for Integrations area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants