Skip to content

[SOA]: Bugbash for releases 28.x - Contact unable to find due to Qasim map to Megan, different names - #10036

Open
tomasevicst wants to merge 9 commits into
mainfrom
bugs/640657_SOA_Mapped_Contact_Enhancement
Open

[SOA]: Bugbash for releases 28.x - Contact unable to find due to Qasim map to Megan, different names#10036
tomasevicst wants to merge 9 commits into
mainfrom
bugs/640657_SOA_Mapped_Contact_Enhancement

Conversation

@tomasevicst

@tomasevicst tomasevicst commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What & why

Fixes contact lookup and reply routing for Sales Order Agent tasks when the sender’s email address is mapped to a different contact.

When an unknown sender is handled with Use another contact once, the selected contact is stored in SOA Task Contact Override. The Contact List now exposes a find-record event that is handled only during Sales Order Agent sessions. The handler resolves the override for the latest active input message and surfaces the mapped contact to the agent, even when its name or email address differs from the original sender.

When Use another contact always is selected, the sender’s address is stored in the contact’s E-Mail 2 field. Contact lookup, security filtering, known-sender classification, and message recipient resolution now consider both E-Mail and E-Mail 2. The additional email field is displayed on the Contact List only during an active Sales Order Agent session.

Reply routing also respects these mappings. Replies remain in the original email thread but are addressed to the mapped contact’s primary email address, while preserving the original CC and BCC recipients. When no mapping exists, the existing Reply All behavior remains unchanged.

The contact-search subscriber is manually bound from SOA Session Events, so it affects only Sales Order Agent sessions. Agent instructions were also updated to accept the mapped contact as authoritative and continue quote creation without requesting assistance solely because the selected contact does not match the sender’s identity.

Linked work

Fixes AB#640657

How I validated this

  • I read the full diff and it contains only changes I intended.
  • I built the affected app(s) locally with no new analyzer warnings.
  • I ran the change in Business Central and confirmed it behaves as expected.
  • I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)

Risk & compatibility

The change is limited to Sales Order Agent sessions. The contact-search subscriber is manually bound during SOA session initialization, and the E-Mail 2 field is visible on the Contact List only while an SOA agent task is active. Standard Contact List behavior remains unchanged for non-agent sessions.

The new Contact List OnBeforeFindRecord integration event is added to both W1 and APAC Base App implementations. When no subscriber handles the event, the page continues using its existing Rec.Find(Which) behavior.

Contact matching now includes E-Mail 2. Existing contacts without an alternate email retain their current behavior. If the same email matches multiple contacts, the existing duplicate-contact handling remains in effect rather than selecting a mapped recipient ambiguously.

Reply routing changes only when an exact task-message override exists or the sender uniquely matches a contact through E-Mail 2. In that case, the reply is intentionally addressed to the mapped contact’s primary email while preserving the original thread and CC/BCC recipients. Without a mapping, the existing Reply All path is unchanged.

The Contact List override is resolved against the latest active input message using the full (Task ID, Task Message ID) identity. This avoids selecting an older or unrelated mapping in tasks containing multiple messages.

- Updated instructions for selecting contacts and customers in the Sales Order Agent.
- Introduced a new codeunit for contact search implementation.
- Refactored email message handling to utilize mapped contact emails.
- Added support for alternative email addresses in contact list and filters.
- Implemented event triggers for contact search in the Contact List page.
auto-merge was automatically disabled August 7, 2026 09:39

Pull request was closed

@github-actions github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label Aug 7, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Aug 7, 2026
@github-actions github-actions Bot added the SCM GitHub request for SCM area label Aug 7, 2026
Comment thread src/Apps/W1/SalesOrderAgent/app/src/PageExtention/SOAContactListExt.PageExt.al Outdated
Comment thread src/Apps/W1/SalesOrderAgent/app/src/Integration/SOAContactSearchImpl.Codeunit.al Outdated
Comment thread src/Apps/W1/SalesOrderAgent/app/src/Integration/SOATaskMessage.Codeunit.al Outdated
Comment thread src/Apps/W1/SalesOrderAgent/app/src/Integration/SOAContactSearchImpl.Codeunit.al Outdated
Comment thread src/Apps/W1/SalesOrderAgent/app/src/Validation/SOAFiltersImpl.Codeunit.al Outdated
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This PR fixes contact lookup and reply routing in the Sales Order Agent when the incoming sender's email is mapped to a different contact. When a user chooses "Use another contact once", the selected contact is stored in SOATaskContactOverride and surfaced to the agent through a new OnBeforeFindRecord event on the Contact List page. When "Use another contact always" is chosen, the sender's address is stored in the contact's E-Mail 2 field. Security filtering, known-sender classification, and reply routing are updated to consider both E-Mail and E-Mail 2.

The core design is correct. SOAContactSearchImpl subscribes to the new OnBeforeFindRecord event only in agent sessions (via manual binding), which prevents the override logic from leaking into non-agent sessions. The FindContactByEmail refactor correctly deduplicates contacts that match on both email fields. Reply routing in SOASendReply correctly prefers the override contact's primary email while preserving the original CC and BCC recipients from the source email. The OnFindRecord trigger and OnBeforeFindRecord event were added to both W1 and APAC ContactList.Page.al consistently.

Suggestions

S1 - Page customization overrides dynamic E-Mail 2 visibility
SOAContactList.PageCust.al adds modify("E-Mail 2") { Visible = true; }, which hard-codes the field as always visible for any user with the SOA profile. The page extension SOAContactListExt already controls this field's visibility with Visible = IsAgentSession, but profile page customizations are applied after extensions, so the hard-coded true wins for SOA profile users. The PR description says E-Mail 2 is shown only during an active agent session, but that is not accurate for users of the SOA profile. Remove the modify("E-Mail 2") block from the page customization; the page extension already handles it correctly.

S2 - No test files in the diff
The PR touches new security filtering logic (FindContactByEmail across two fields), a new integration event on Contact List, and reply rerouting. The "What I tested" section in the PR description is empty. These are sensitive, agent-facing behaviors and warrant at minimum a unit test covering the FindContactByEmail deduplication logic and the GetMappedContactEmail fallback path.

S3 - Comment in SelectContactAndUpdateEmail references the wrong field
The code comment reads "Direct assignment is intentional: ContactEmail originates from an incoming email's From address... Validate() is skipped to avoid rejecting valid but non-standard addresses". The diff now assigns to E-Mail 2 instead of E-Mail. The comment still says E-Mail implicitly. Update it to say the assignment targets E-Mail 2 so future readers understand the intent.

Risk assessment and necessity

Risk: Security filtering is the most sensitive area. GetContactsInvolvedInTask now also includes contacts whose E-Mail 2 matches the sender. Since E-Mail 2 is a standard Contact field editable by any user with contact write rights, a contact's E-Mail 2 could in principle be set outside the SOA flow. In practice, the risk is bounded by existing contact permissions; E-Mail 2 is only set through the "Use another contact always" SOA action, and the SOA security filter is already limited to agent sessions. Reply routing changes are also scoped to agent sessions via the SOATaskContactOverride lookup and the FindContactByEmail2 single-match guard (ContactCount = 1), so existing Reply All behavior is unchanged when no mapping exists. The new OnBeforeFindRecord integration event on the Contact List page has no default behavior (empty event body) and only fires when a subscriber is bound, so non-agent pages are unaffected. The change ships in both W1 and APAC Contact List, which is correct.

Necessity: The scenario is valid and important: a mapped contact (Qasim → Megan) caused the agent to stall at contact selection because the selected contact did not match the sender identity. The fix cleanly addresses the root cause by extending the lookup and filtering to honor both the task override and the E-Mail 2 mapping, rather than patching the agent instructions alone. The scope is appropriate.


[AI-PR-REVIEW] version=1 promptVersion=1 system=github pr=10036 round=1 by=alexei-dobriansky at=2026-08-07T11:38:40Z lastSha=b613f0ffc838d2e05abe2bfbf1eb2b53f73d3155 reviewKey=a828463e1652765064d7b40beba4c0aa2e5f1f4fdc620bfb76305eead93f43a7 suggestions=S1@7a5fe4b8,S2@1d2615e9,S3@bd89f492

Comment thread src/Apps/W1/SalesOrderAgent/app/src/Integration/SOASendReply.Codeunit.al Outdated
Comment thread src/Apps/W1/SalesOrderAgent/app/src/Integration/SOASendReply.Codeunit.al Outdated
Comment thread src/Apps/W1/SalesOrderAgent/app/src/Integration/SOASendReply.Codeunit.al Outdated
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling}$

When an override or uniquely-matched contact exists but its primary email is blank, GetSentMessageToAddress falls back to SentAgentTaskMessage.From and still returns true. That makes the message history and page show the original sender as the reply target, even though SOASendReply later rejects the same mapping with MappedContactEmailMissingErr. Reuse the same mapped-contact validation here and surface the mapping problem instead of silently displaying a different address than what would actually be used.

Agent judgement — not directly backed by a BCQuality knowledge article.

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4

Comment thread src/Apps/W1/SalesOrderAgent/app/src/Integration/SOASendReply.Codeunit.al Outdated
Comment thread src/Apps/W1/SalesOrderAgent/app/src/Validation/SOAFiltersImpl.Codeunit.al Outdated
…g contact validation, and refining email reply logic
Comment thread src/Apps/W1/SalesOrderAgent/app/src/Validation/SOAFiltersImpl.Codeunit.al Outdated
…, refining email handling, and improving error messaging
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ Performance}$

The new OnBeforeFindRecord subscriber (FindRecordContact) issues Agent Task Message and SOA Task Contact Override database reads on every Contact List find/navigation once AgentTaskID is set, with no caching of the resolved contact or short-circuit for repeated calls within the same page instance. Because OnFindRecord/OnBeforeFindRecord is invoked on every list navigation step (paging, search, refresh), this re-runs the same lookup chain repeatedly instead of resolving once per page open.

Knowledge:

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4

Comment thread src/Apps/W1/SalesOrderAgent/app/src/Validation/SOAFiltersImpl.Codeunit.al Outdated
/// Ensures that a mapping is changed only for an existing input message by its configured owner or agent.
/// Internal procedures are not an authorization boundary, so every override and alternate-email write path calls this validation.
/// </summary>
local procedure ValidateContactMappingAccess(TaskID: BigInteger; TaskMessageID: Guid)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Agent}$

The owner/agent identity-resolution rule ("use SOA Setup's Owner User Security ID, falling back to User Security ID when null, then compare against the acting identity") is independently re-implemented three times: SOASendReply.Codeunit.al's IsAuthorizedUserSecurityID (line 133), SOAFiltersImpl.Codeunit.al's IsContactOverrideTrusted (line 128), and SOAFiltersImpl.Codeunit.al's ValidateContactMappingAccess (line 307). All three gate security-sensitive actions (sending a redirected reply, trusting a contact override, and changing a contact mapping). Because the logic is copy-pasted rather than centralized, a future change to the ownership/fallback rule (e.g., adding a delegate or co-owner concept) risks being applied to only some call sites, silently reintroducing an authorization bypass in the others. Extract this rule into a single shared procedure (e.g., on "SOA Setup" or a shared helper codeunit) and have all three call sites use it.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 2

Recommendation: Accept with Suggestions

What this PR does

This PR extends the round-1 changes with authorization and provenance guards, BCC-preservation in mapped replies, and a refactored email-lookup layer shared across all callers. The key additions are IsContactOverrideTrusted (validates that the override row was created and last modified by the configured SOA owner or agent) and ValidateContactMappingAccess (authorization gate before every contact override and alternate-email write). FindContactByEmail and FindContactByAlternateEmail are now shared helpers in SOAFiltersImpl; callers that previously inlined the filter pattern now delegate to these.

The round-2 changes are correct. The IsContactOverrideTrusted provenance check closes a gap where overrides created outside the SOA flow could have affected security filtering. ValidateContactMappingAccess is called at both write sites (SelectContactAndSetOverride, SelectContactAndUpdateEmail), preventing unauthorized contact remapping. GetOriginEmailCCRecipients correctly propagates CC recipients from the source email to mapped replies while intentionally omitting BCC. The deduplication in FindContactByEmail (dictionary by contact No.) correctly handles contacts that match on both E-Mail and E-Mail 2. SOASendReply.GetMappedContactEmail errors explicitly when the mapped contact lacks a primary email, so the reply is blocked rather than silently sent to the wrong address.

Status of previous suggestions
ID Title Status Author response
S1 Page customization overrides dynamic E-Mail 2 visibility Not addressed
S2 No test files in the diff Not addressed
S3 Comment in SelectContactAndUpdateEmail references the wrong field Addressed Assignment now targets E-Mail 2; comment is field-agnostic and accurate
New observations (commits since round 1)

No new High-confidence findings. One low-priority observation below.

S4 - Reply fails silently when source email record is deleted
GetOriginEmailCCRecipients throws OriginEmailUnavailableErr if the SOA Email or Email Inbox record no longer exists. A mapped reply cannot be sent in that case, even if the contact mapping and email address are valid. Consider whether a fallback to an empty CC list (and a logged warning) would give a better user experience than an error that blocks the reply entirely.

Risk assessment and necessity

Risk: The provenance check (IsContactOverrideTrusted) and the authorization gate (ValidateContactMappingAccess) are new security controls; if SOASetup.GetBasedOnAgentUserSecurityID returns false or the owner/agent UIDs are not set, overrides are correctly rejected rather than silently trusted. The FindContactByEmail deduplication is correct and the single-match guard (ContactCount = 1) in SOATaskMessage prevents ambiguous routing. The E-Mail 2 field is a standard Contact field; it is set only through the SOA "Use another contact always" action now protected by ValidateContactMappingAccess. Reply routing changes remain scoped to SOA sessions. The SOAContactList.PageCust.al S1 concern (hard-coded Visible = true for the SOA profile) is still open and creates a discrepancy between what the PR description states and what the code delivers for SOA profile users.

Necessity: The round-2 changes are necessary. Without the provenance and authorization guards, a user with contact write rights could create or modify an override row and affect security filtering or reply routing for another user's SOA session. The shared FindContactByEmail helper also removes duplicated filter logic that was otherwise hard to keep in sync across callers.


[AI-PR-REVIEW] version=1 promptVersion=1 system=github pr=10036 round=2 by=alexei-dobriansky at=2026-08-10T07:53:38Z lastSha=4efb73e4efd8028a6472534ad3e57b9b4d269bd8 reviewKey=dfea1f463c8c538489d30e0bdc7abae6d8af7d550a5b06063f06ad83bb124cee suggestions=S1@7a5fe4b8:notaddressed,S2@1d2615e9:notaddressed,S3@bd89f492:addressed,S4@f36a184d:new parentRound=1

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

Labels

AL: Apps (W1) Add-on apps for W1 SCM GitHub request for SCM area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants