[SOA] Show Cc recipients on email messages - #10247
attilatoury wants to merge 7 commits into
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.
|
|
|
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
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
|
Incoming email opened from the SOA's timeline: 
Outgoing email opened from the SOA's timeline: 
|
Return blank Cc when mapped-recipient calculation fails instead of showing the unfiltered source list on the outgoing card. Preserve successful mapped and ordinary reply previews and leave sending unchanged. Full SOA build passed with CodeCop and UICop: no errors or warnings, four existing informational diagnostics. This failure-path guard has not been runtime-tested; the preceding inbox permission fix and Cc UI were validated in the private deployment.
attilatoury
added
Team: SCM
Good Sense Reviewer - Round 5Recommendation: Accept with SuggestionsWhat this PR doesThe new commits add Email Inbox read permissions to the codeunits that read Cc data and change mapped-reply Cc preview failures to show a blank value instead of falling back to the raw incoming Cc list. The full diff adds Cc display to the email details card, uses the same semicolon formatting as the email viewer, and keeps send behavior unchanged. The logic now handles incoming messages, unmapped outgoing replies, mapped replies, and source or preview failures without persisting data or changing the recipient list used by send. Status of previous suggestionsNo previous suggestions. New observations (commits since round 4)S1 (🟠 Moderate): Cover mapped Cc preview failure Risk assessment and necessityRisk: The regression surface is limited to the Sales Order Agent email details card and the SOA Email lookup. The new permissions allow read access to Email Inbox from the display helpers, and the mapped failure guard changes only preview text; it does not send, create, authorize, or store email recipients. No BaseApp publisher dependency was found. Necessity: The change is needed because Cc recipients can already be part of the mail flow while the card did not show them. The final guard is also needed so the card does not display a Cc list that does not match the mapped reply when the mapped preview cannot be resolved.
|
|
| if AgentTaskMessage.Type = AgentTaskMessage.Type::Output then begin | ||
| if not SourceAgentTaskMessage.Get(AgentTaskMessage."Task ID", AgentTaskMessage."Input Message ID") then | ||
| exit(''); |
There was a problem hiding this comment.
GetMessageCcRecipients fetches the full Agent Task Message row via SourceAgentTaskMessage.Get(...) when Type = Output, but only the From field (used indirectly by TryGetMappedReplyCcRecipients -> GetMappedContactEmail) plus the primary-key fields (already loaded regardless) are needed from that record in this code path. Add SetLoadFields(From) before the Get call to avoid materializing the rest of the record unnecessarily, matching the partial-record-load guidance already applied elsewhere in this codeunit (e.g. SOAEmail.SetLoadFields("Email Inbox ID")).
| if AgentTaskMessage.Type = AgentTaskMessage.Type::Output then begin | |
| if not SourceAgentTaskMessage.Get(AgentTaskMessage."Task ID", AgentTaskMessage."Input Message ID") then | |
| exit(''); | |
| if AgentTaskMessage.Type = AgentTaskMessage.Type::Output then begin | |
| SourceAgentTaskMessage.SetLoadFields(From); | |
| if not SourceAgentTaskMessage.Get(AgentTaskMessage."Task ID", AgentTaskMessage."Input Message ID") then | |
| exit(''); |
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 a read-only Cc field to Email Details, with the same Additional importance and semicolon-separated formatting as the Business Central Email Viewer. The field remains present but blank when there are no Cc recipients or the required source data cannot be resolved.
The net PR changes four SOA files: the email card, task-message helper, send-reply helper, and SOA Email table. It also adds a nonunique lookup key on (Task ID, Task Message ID) and narrowly scoped Email Inbox read permissions to both inbox-reading codeunits. Those declarations fix the license-level Email Inbox: Read error encountered when opening cards from the agent timeline.
Linked work
Fixes AB#632233
How I validated this
e0b5a627e9ce24328d8e6be4f644c24c1cecb3cd.What was tested and the outcome
6c017100e6810d15f3e669fcac43ba636fbebee1) passed with AL compiler 18.0.41.8481, CodeCop, and UICop: 0 errors, 0 warnings, and four existing AW0006 informational page diagnostics. The build used the local Application 30 / platform 29 symbol cache.git diff --checkpassed. VS Code reported no errors in the final changed helper.Automated test coverage
No automated tests are added by this GitHub PR. The Sales Order Agent internal test app is maintained in NAV, so companion coverage requires a separate change there. Coverage for missing-source fallbacks, 0/1/multiple recipients, mapped exclusions/deduplication, and mapped-calculation failure remains a follow-up; successful compilation and manual validation do not replace it.
Risk & compatibility
Low and localized to Cc display, the supporting lookup index, and indirect inbox read access. This PR does not change recipient selection during sending, reply creation, or send authorization. It adds no email/task data writes. Failed mapped-recipient calculation no longer presents the original incoming list as the outgoing Cc list.
The outgoing value is resolved from source email and current mapping data; it is not a persisted receipt of historical delivery. Long lists follow the existing Email Viewer behavior; no separate recipient-list UI is introduced.