[SOA] Show Cc recipients on email messages - #10247
Conversation
github-actions
Bot
added
AL: Apps (W1)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Agentic PR Review - Round 1Recommendation: AcceptWhat this PR doesThis fixes the Sales Order Agent Cc visibility bug by adding a read-only Cc field to the email details page and loading it from the original inbox email. The code resolves outgoing messages back to their input message, uses the same semicolon format as the Email Viewer, and returns blank when the source email cannot be found. The change is read-only and does not change recipient selection or email sending. SuggestionsNone. Risk assessment and necessityRisk: The regression surface is limited to the Sales Order Agent email card. The new field reads recipients from Email Inbox and Email Message; it does not write data, change reply creation, or affect posting. There is no BaseApp event or publisher dependency. Necessity: The linked bug has a clear repro: reviewers cannot see Cc recipients in the incoming or outgoing SOA email cards, even though the email thread includes them. The scoped UI addition is justified and matches the existing Email Viewer display pattern.
|
|
Agentic PR Review - Round 2Recommendation: AcceptWhat this PR doesThe new commit only changes Status of previous suggestionsNo previous suggestions. New observations (commits since round 1)None - the new commit is a mechanical TextBuilder replacement and does not introduce a new correctness, data-integrity, security, concurrency, or compatibility issue. Risk assessment and necessityRisk: The round-2 change is limited to Necessity: The linked bug describes missing Cc visibility on SOA email review screens. The PR remains a narrow UI/read-only fix for that problem, and the new commit improves string construction without changing the intended scope.
|
|
Predrag Maricic (PredragMaricic)
left a comment
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
Approved with suggestions.
S1: For output messages, the displayed Cc list is read from the raw inbound email, while the send path excludes the original sender and agent mailbox and deduplicates Cc recipients against To recipients. Please align the displayed outgoing Cc list with the normalized recipients that are actually sent.
S2: Add or link companion Sales Order Agent tests covering incoming and outgoing Cc display, multiple recipients, and the exclusion/deduplication case.
S3: Consider adding and using a key on SOA Email for ("Task ID", "Task Message ID"), because the new lookup runs on every page refresh and no existing key supports that filter pair.
34567c6
|
alexei-dobriansky Predrag Maricic (@PredragMaricic) The addressed feedback is now in |
Good Sense Reviewer - Round 4Recommendation: AcceptWhat this PR doesThe new commit aligns the Cc value shown for output replies with the same recipient-building path used when a mapped reply is sent. It adds a lookup key on SOA Email, exposes an internal try wrapper for mapped-reply Cc calculation, and has SOA Task Message use that mapped Cc list before falling back to the source message. The changed path remains read-only and does not change email sending, stored data, or the review flow. Status of previous suggestionsNo previous suggestions. New observations (commits since round 3)None - the new commit keeps the display path aligned with the mapped-reply send logic, including recipient filtering and de-duplication. The added key matches the existing Task ID + Task Message ID lookup, and git diff --check passed for the current net PR diff. Risk assessment and necessityRisk: The regression surface is limited to the Sales Order Agent email details card for output messages and to the new SOA Email lookup key. The code reuses the same mapped-recipient calculation as send; if that cannot resolve the source email or account, the display falls back to the existing source-Cc path instead of changing send behavior. There is no BaseApp event or publisher dependency. Necessity: The reported bug is a visibility gap: Cc recipients can already be part of the mail flow, but the Sales Order Agent card did not show them. The latest commit is necessary because mapped replies can filter or move recipients compared with the raw inbound Cc list, so the card should show the Cc list that matches the sent reply.
|
|
Pull request was closed
github-actions
Bot
added
Ownership: Needs Review
|
PR approved but waiting for the private deployment from https://dynamicssmb2.visualstudio.com/Dynamics%20SMB/_git/NAV/pullrequest/253790 to be ready for manual testing. The previous one had failed |
There was a problem hiding this comment.
🟡 Changes recommended
The mapped-reply Cc behavior appears inconsistent with the stated “complete inbox Cc” requirement, and the new Task/Message index should be actively used to avoid inefficient lookups.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/Apps/W1/SalesOrderAgent/app/src/Integration/SOATaskMessage.Codeunit.al:146
SOAEmailis filtered by "Task ID"/"Task Message ID" and this PR adds Key3 for that lookup, but the query here never sets the current key. WithoutSetCurrentKey("Task ID", "Task Message ID"), the server may still use the clustered key ("Email Inbox ID") which can degrade performance as the table grows.
SOAEmail.SetLoadFields("Email Inbox ID");
SOAEmail.SetRange("Task ID", SourceAgentTaskMessage."Task ID");
SOAEmail.SetRange("Task Message ID", SourceAgentTaskMessage.ID);
if not SOAEmail.FindFirst() then
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
| SourceAgentTaskMessage := AgentTaskMessage; | ||
| if AgentTaskMessage.Type = AgentTaskMessage.Type::Output then begin | ||
| if not SourceAgentTaskMessage.Get(AgentTaskMessage."Task ID", AgentTaskMessage."Input Message ID") then | ||
| exit(''); | ||
| if SOASendReply.TryGetMappedReplyCcRecipients(SourceAgentTaskMessage, CcRecipients, IsMappedReply) and IsMappedReply then | ||
| exit(RecipientsToText(CcRecipients)); | ||
| end; |
Declare read-only Email Inbox permissions in SOA Task Message and SOA Send Reply. Fixes the license Read error when opening incoming/outgoing email cards from the agent timeline, including mapped-contact recipient resolution. Validated full Sales Order Agent compilation with CodeCop and UICop: no errors or warnings; four AW0006 informational page diagnostics. Verified both declarations in compiled metadata. Private ORGTIE runtime retest pending.
e0b5a62
| exit(true); | ||
| end; | ||
|
|
||
| internal procedure GetMessageCcRecipients(AgentTaskMessage: Record "Agent Task Message"): Text |
There was a problem hiding this comment.
GetMessageCcRecipients (SOATaskMessage.Codeunit.al) uses the codeunit's new Permissions = tabledata "Email Inbox" = r; grant to read CC recipients from Email Inbox/Email Message and, for mapped replies, delegates to SOA Send Reply.TryGetMappedReplyCcRecipients, without re-running SOA Send Reply's existing ValidateMessageAccess check (Agent User Security ID / authorized-user check) before surfacing that data. Access = Internal and the tabledata permission grant are API/data-access hygiene, not an authorization boundary; if this preview path can be reached for a message the current user does not otherwise own, the CC addresses are disclosed without the same authorization check the actual send path enforces. Recommend running the same authorization check (or documenting why the page's existing record-level access control already covers this) before returning CC data.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6
What & why
Sales Order Agent email cards did not show Cc recipients, so reviewers could not see everyone included in incoming and outgoing correspondence.
This change adds the standard read-only, Additional-importance Cc field to the email details. It resolves the complete Cc list from the original inbox email for both incoming messages and their outgoing replies, using the same semicolon-separated formatting as the Business Central Email Viewer. The field remains present but blank when there are no Cc recipients.
Linked work
Fixes AB#632233
How I validated this
What I tested and the outcome
git diff --checkpassed, and the committed diff contains only the two intended Sales Order Agent files.Manual Agent Task Creation Type/IAgentManualTaskCreationsymbols and the missingContact List.OnBeforeFindRecordevent introduced by another current-main change.Risk & compatibility
Low. The change is read-only and does not alter email creation, recipient selection, sending, or stored data. Missing inbox/message records produce a blank Cc value. Multiple recipients use the standard semicolon-separated Email Viewer format. Very long lists follow the existing Business Central Email Viewer behavior; no separate recipient-list UX is introduced by this minimal fix.
Fixes AB#632233