Features/637301 report layout override lifecycle#9428
Conversation
Setting the Layout Status of an extension-installed layout (User Defined = false) now writes a Tenant Report Layout Override record instead of a no-op, rather than copying the layout into Tenant Report Layout. - ReportLayoutsImpl: add UpsertLayoutOverride helper (writes only the flagged fields with their Override * flags; IsObsolete is one-way); reroute SetLayoutStatus non-user-defined branch to it; grant RIMD on Tenant Report Layout Override. - ReportLayouts.page: relax CanModifyStatus so extension layouts can use the status actions. - Tests: add TestSetExtensionLayoutStatusWritesOverride (asserts an override is written with no tenant copy); clear overrides in EnsureNewLayoutsAreCleaned. Verified: codeunit 139595 all 19 tests pass.
Editing an extension-installed layout's info now writes a Tenant Report Layout Override record (Description, one-way IsObsolete) instead of copying the layout. - ReportLayoutsImpl.EditReportLayout: for a non-user-defined layout not being copied, write an override via UpsertLayoutOverride and exit; existing copy/in-place logic unchanged. - ReportLayoutEditDialog: override mode for extension layouts - name read-only, Description editable, IsObsolete editable one-way (locked when already obsolete in metadata), AvailableInAllCompanies drives global vs company scope; copy kept as an opt-in escape hatch. - ReportLayouts.page: EditLayout no longer forces the 'edit a copy instead' confirm for extension layouts; removed the now-unused label. - Tests: add global-description and company-obsolete override tests (assert override written, no tenant copy). Verified: codeunit 139595 all 21 tests pass.
Batch status actions (Set Approved/Draft/Pending/Retired) now resolve the override scope per extension-installed layout and keep each run to one scope: - LayoutStatusIsGlobalScope: current-company override wins (company scope); else an existing global override => global scop - SetLayoutStatus: extension layouts override at the classified scope. - SetLayoutStatusBatch: reject mixed-scope select confirm all-global changes (GlobalScopeConfirmQst) before applying; user-defined layouts still update in place and Fresh extension layouts stay company-specific wit shipped user-defined behaviour with minimal user-facing change. Tests: add TestSetGlobalScopeExtensionLayoutStatusConfirmsAndUpdatesGlobal (global override updated after confirm; no compan Mixed-scope Error is not auto-tested (test report ships a single extension layout); manually verifiable.
Wording/permission (ReportLayouts.page): - Retire "user-defined" from the four Set<Status> tooltips; the status actions now cover extension-installed layouts too (via override). - Document why the AccessByPermission gate stays tabledata "Tenant Report Layout" = M: an action takes a single tabledata permission, and the platform's BaseSystemPermissionSet grants Tenant Report Layout and Tenant Report Layout Override together, so this gate represents "may manage layout status" and matches the page's own permission. Mixed-scope test coverage (Tests-Report): - Add TestMixedScopeBatchStatusIsRejected, driving the internal Report Layouts Impl. codeunit directly (Tests-Report is in Bas a filtered Report Layout List spanning two extension layouts, asserting the mixed-scope error. A TestPage cannot multi-sele - Add a second shipped layout (MYLAYOUT2, same rdl) to test report 139595 so multi-layout scope scenarios are exercisable. Verified: codeunit 139595 all 23 tests pass.
| if (CreateCopy) then | ||
| // In override mode (extension layout) the user always chooses global vs company | ||
| // scope, whether they override in place or opt into a copy. | ||
| if OverrideMode then |
There was a problem hiding this comment.
For an extension-installed layout (override mode), SetupDialog hard-codes LayoutNameEditable := false, and the CreateCopy field's OnValidate trigger only toggles AvailableInAllCompaniesEditable when OverrideMode is true -- it never re-enables LayoutNameEditable.
The surrounding comment describes 'Save Changes to a Copy' as an opt-in escape hatch 'to fork the layout content into a user layout', implying the fork can be named, but the Layout Name field stays read-only even after the user checks CreateCopy, so the resulting copy is always inserted under the exact same name as the source extension layout. Before this change the field had no Editable restriction and could always be renamed, so this is a regression, not a new deliberate restriction.
Recommendation:
- also set LayoutNameEditable := CreateCopy in the OverrideMode branch of the CreateCopy field's OnValidate trigger.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
trigger OnValidate()
begin
// In override mode (extension layout) the user always chooses global vs company
// scope, whether they override in place or opt into a copy. Checking CreateCopy
// also re-enables the Layout Name so the forked copy can be given a distinct name.
if OverrideMode then begin
AvailableInAllCompaniesEditable := true;
LayoutNameEditable := CreateCopy;
end else
if (CreateCopy) then
AvailableInAllCompaniesEditable := true
else
if (IsLayoutOwnedByCurrentCompany) then begin
AvailableInAllCompaniesEditable := true;
AvailableInAllCompanies := false;
end else begin
AvailableInAllCompaniesEditable := false;
AvailableInAllCompanies := true;
end;
end;👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| // mutable properties (Description, IsObsolete) instead of copying the layout. The layout | ||
| // name/identity cannot change here, so NewLayoutName is not used. | ||
| if (not SelectedReportLayoutList."User Defined") and (not CreateCopy) then begin | ||
| UpsertLayoutOverride(SelectedReportLayoutList, AvailableInAllCompanies, true, NewDescription, false, Enum::"Report Layout Status"::Draft, true, NewIsObsolete); |
There was a problem hiding this comment.
SetLayoutStatusBatch requires an explicit Confirm(GlobalScopeConfirmQst) before a status change touches a GLOBAL-scope override, because that write affects every company.
EditReportLayout's new override branch (used when editing an extension layout's Description/IsObsolete) writes through the same UpsertLayoutOverride with MakeGlobal := AvailableInAllCompanies but has no equivalent confirmation gate -- and ReportLayoutEditDialog.SetupDialog hard-codes AvailableInAllCompanies := true for every extension-layout edit, regardless of the layout's current override scope. A user who opens 'Edit Info' on an extension layout, changes only the description, and clicks OK without touching the checkbox will silently create or update an override scoped to every company -- the same class of cross-company blast radius the batch-status path treats as important enough to gate behind a confirmation. The real-world impact of this gap is closer to major (silent cross-company effect with no prompt), but it is reported at minor per this pass's agent-finding ceiling; treat it as a candidate for a dedicated knowledge rule or a fix before merge.
Recommendation:
- default AvailableInAllCompanies to the layout's current override scope (mirroring LayoutStatusIsGlobalScope) rather than hard-coding true, and/or add the same Confirm(GlobalScopeConfirmQst) gate in EditReportLayout when MakeGlobal is true.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| CustomDimensions.Add('ReportId', Format(SelectedReportLayoutList."Report ID")); | ||
| CustomDimensions.Add('LayoutName', SelectedReportLayoutList.Name); | ||
| CustomDimensions.Add('NewLayoutDescription', NewDescription); |
There was a problem hiding this comment.
In the new extension-layout override branch of EditReportLayout, the user-typed 'Description' value (NewDescription) is added verbatim to the CustomDimensions dictionary ('NewLayoutDescription') passed to Session.LogMessage/Log, which ships with TelemetryScope::All.
Free-text description/notes fields are exactly the content this guidance says must not travel into telemetry, whether in the message text or a custom dimension. This mirrors a pre-existing pattern used by the sibling 'Edit' (non-override) path, but the new code re-introduces it.
Recommendation:
- drop the 'NewLayoutDescription' dimension (and its counterpart in the pre-existing Edit path if this is revisited) or replace it with a non-content signal such as a boolean 'DescriptionChanged'.
Knowledge:
- microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.md
- microsoft/knowledge/privacy/featuretelemetry-customdimensions-no-pii.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| CustomDimensions.Add('LayoutName', SelectedReportLayoutList.Name); | ||
| CustomDimensions.Add('NewLayoutDescription', NewDescription); | ||
| AddReportLayoutDimensionsAction('EditOverride', CustomDimensions); | ||
| Log('0000N0H', 'Report layout properties overridden by user', CustomDimensions); |
There was a problem hiding this comment.
The new 'EditOverride' action reuses event ID '0000N0H' with message 'Report layout properties overridden by user', which is the exact same event ID already used a few lines away by the unrelated 'Edit' action's 'Report layout properties changed by user' message.
The two describe different code paths (in-place override for extension layouts vs. copy/rename for user-defined layouts) and are only distinguishable downstream by inspecting the 'Action' custom dimension, not the event ID itself.
Recommendation:
- assign 'EditOverride' its own registered event ID rather than reusing '0000N0H'.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Copilot PR ReviewIteration 1 · Outcome: completed Knowledge source: https://github.com/microsoft/BCQuality@be1b92b624679f8c031061602e7d3a3b5f71a688 Findings by domainFindings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).
Totals: 3 knowledge-backed · 1 agent findings. Orchestrator pre-filter (16 file(s) excluded)
Findings produced by the Copilot CLI agent against BCQuality at |
|
@V-CWurtzen please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Summary
Application-side implementation of CP0529-338: lifecycle/property changes to an extension-installed report layout now write a
Tenant Report Layout Overriderecord instead of copying the layout intoTenant Report Layout(which left the original visible with its default values).Tenant Report Layout).The update path is chosen by
Report Layout List."User Defined":true→ in-place update;false→ upsert aTenant Report Layout Overriderecord. Only fields whoseOverride *flag is set are applied.Tests
Report Layouts Test(codeunit 139595): 23/23 pass, covering status/edit overrides, global vs. company scope, the global-scope confirmation, and mixed-scope rejection. A second shipped layout (MYLAYOUT2) was added to the test report so multi-layout scope scenarios are exercisable.Platform dependency (CI compile)
This change references the platform table
Tenant Report Layout Override(2000000248), which first ships in platform 29.0.52163.0. The repo currently pinsartifact: bcinsider/Sandbox/29.0.51936.0, which predates that table, so the Compile Apps step fails withAL0185 Table 'Tenant Report Layout Override' is missing. CI can only go green once the platform artifact pin is advanced to a build that includes object 2000000248. The AL code itself compiles against the newer platform symbols.AB#637301