637301 Report layout override lifecycle - #9428
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 |
Jesper Schulz-Wedde (JesperSchulz)
added
the
Integration
|
V-CWurtzen
changed the title
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.
8257d10 to
e17c5ff
Compare
CI compile (CodeCop/analyzers) rejects two report layouts sharing one LayoutFile: AL0835 "The LayoutFile '…TestReportLayoutsReport.rdl' is already being used." MYLAYOUT2 (added for mixed-scope test coverage) reused MYLAYOUT's rdl. Point it at a dedicated copy, Layouts/TestReportLayoutsReport2.rdl.
|
Address the Copilot review findings on the extension-layout override edit path: A (major usability) — stop the silent, invisible all-companies override: - Add a dedicated "Override for all companies" control (page 9661), shown only for extension layouts; do NOT reuse "Available in All Companies" (layout availability) for override scope. Default = current company. - EditReportLayout: write an override only for fields the user actually changed (no-op OK writes nothing); Confirm before any all-companies (global) override. B — copy can be renamed: checking "Save Changes to a Copy" now re-enables the Layout Name field. C/D — telemetry: comment out the override Log for now (it reused event id 0000N0H with a changed schema and shipped the free-text description/PII); re-enable later with a dedicated event id and no content dimension. Tests: switch edit handlers to the new control, add ConfirmHandler to the global-edit test, add TestEditExtensionLayoutNoOpWritesNoOverride.
Manual step-4 finding: answering No to the all-companies confirmation exited EditReportLayout, so the Description the user had typed was silently discarded with no feedback. - ReportLayoutsImpl: run the edit dialog in a repeat..until loop. On a declined global confirmation, reopen the dialog with the entered values and the scope reset to the current company, so the edit can still be saved company-scoped (or cancelled deliberately). - Confirm text now states the consequence: "Choose No to go back to the dialog with your changes, where you can apply them to the current company only." - ReportLayoutEditDialog: add SetOverrideValues(Description, IsObsolete, ScopeIsGlobal) to re-seed the reopened dialog without SetupDialog (which would wipe the edits); IsObsolete only re-applied while that field is editable. - Tests: add TestEditExtensionLayoutDeclineGlobalKeepsEditsAndSavesCompanyScope (+ ConfirmHandlerNo). The modal handler runs twice: pass 1 types a description and asks for global; pass 2 asserts the description survived and the scope was reset, then accepts. Asserts a company override with the typed description and NO global override. Note: the post-loop body is kept inside a plain begin..end block so the existing override/copy/in-place paths stay as they were (small diff); can be flattened if preferred.
AL does not short-circuit `and`, so if AvailableInAllCompanies and (not Confirm(GlobalOverrideConfirmQst, false)) raised the prompt on every extension-layout edit, including company-scoped ones — TestEditExtensionLayoutWritesCompanyObsoleteOverride failed with "Unhandled UI: Confirm". Restored the nested ifs with a comment explaining why they must stay nested.
Two manual-test findings.
Step 4 — declining the all-companies confirm gave no feedback. Reopening the
dialog (the earlier 1+3 attempt) is impossible: an AL Page variable cannot be
RunModal'd twice ("The object has been run. Use the function CLEAR(Page)") and
Clear would discard the entries we wanted to keep. That loop passed the automated
test — a ModalPageHandler never exercises the real page lifecycle — but failed
manually, so it is reverted. Now the confirm says No discards the changes (and how
to apply them to this company instead) and declining shows an explicit
"No changes were applied" message. Removed the now-unused SetOverrideValues.
Step 10e — "Save Changes to a Copy" silently created a company-scoped copy that
never appeared in other companies: Available in All Companies was hidden for
extension layouts and the override toggle was fed into the copy path. Restore the
shipped default (copy = all companies), show/enable the field once Copy is ticked,
and read the override control only when not copying.
Tests: TestEditExtensionLayoutDeclineGlobalWritesNothing (no override at either
scope) and TestCopyOfExtensionLayoutKeepsAllCompaniesScope (copy lands with empty
Company Name, no override row) — the copy path's scope had no coverage before.
AL evaluates a field's Visible property at page initialisation and does not re-evaluate it when a variable changes, so Visible = (not OverrideMode) or CreateCopy never showed the field after ticking Copy. Keep the field always visible and let Editable — which is re-evaluated — carry the meaning: read-only Yes while overriding an extension layout (a true statement), editable once the user opts into a copy, whose company scope it controls. Override scope remains a separate control, so the two meanings stay distinct.
A copy writes a tenant layout and no override, so leaving "Override for all companies" live there offered a control that silently did nothing. Ticking Copy now locks and clears it, and unlocks "Available in All Companies" (scope of the copy); unticking restores the reverse. Tooltip states when each applies.
The company user is the audience: extension layouts join lifecycle management, and the tenant-wide dimension is hidden from the everyday path. Scope is a governance decision — in the sunrise case a company user usually has neither the permission nor the judgement for it, and in the sunset case (obsoleting is one-way) forcing all companies is the gatekeeper's act, not a side effect of an edit dialog. Guarding the choice (default + confirm + a second toggle) kept adding UI without removing the confusion; removing the choice removes the class. - Edit dialog: drop the "Override for all companies" control (field, vars, accessor). An in-place edit of an extension layout always writes a CURRENT-COMPANY override; no prompt, no "no changes applied" message needed. - "Available in All Companies" keeps its own meaning and serves only the copy path, so nothing is overloaded. - Kept: one-way obsolete, only-changed-field writes, the copy escape hatch with rename, and the status path's derive+confirm as a safety net — a global override can still exist (e.g. seeded by a vendor install codeunit) and must never be flipped silently. - Tests: TestEditExtensionLayoutWritesGlobalDescriptionOverride becomes TestEditExtensionLayoutWritesCompanyDescriptionOverride and now also asserts an everyday edit NEVER creates a global override; the decline test is removed (unreachable by design).
Review follow-up — all four findings addressedThanks — the review caught a real defect and two correctness issues. Summary of what changed since the review, plus one item deliberately left open. 🟡 A — silent all-companies override on Edit info ✅ fixed, by removing the choiceThis was worse than reported: because the dialog hard-coded all-companies scope and the override We first guarded it (explicit scope control defaulting to the current company + a confirmation), but So the everyday path no longer offers the choice at all:
A gatekeeper affordance for deliberate tenant-wide acts is a candidate follow-up. Note it would need a 🟡 B — copy could not be renamed ✅ fixed
Manual testing then found a related regression we introduced: while the scope toggle existed, the 🟠 C + 🟡 D — telemetry ⏸ deliberately parked (not an oversight)Both are valid. The override path's
Question for a maintainer: what's the process for allocating a new telemetry event ID for BaseApp? Verification
Known caveatThe simplification landed after the manual matrix run, so the trimmed dialog is compile-clean and |
…00RTQ Allocated a fresh tag from the number series rather than reusing 0000N0H, whose dimension schema (Old/New layout name and description) doesn't fit this path and is consumed by existing queries. Dimensions are metadata only - ReportId, LayoutName, DescriptionChanged, ObsoleteSet, OverrideScope (stable non-localized token via ScopeDimension) - so no user-entered text reaches telemetry. Closes review findings C and D.
|
The new in-place "Edit info" path for extension-installed layouts now reaches code that inserts or modifies "Tenant Report Layout Override" rows using the codeunit's elevated 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.32.4 |
|
| Warnings | File | In this PR? |
|---|---|---|
4 × AL0607 (with deprecated) |
APIV1CustomersE2E.Codeunit.al |
no |
3 × AL0607 |
APIV1VendorsE2E.Codeunit.al |
no |
2 × AL0607 |
APIV1CompanyInfoE2E.Codeunit.al |
no |
1 × AL0607 |
APIV1ShipmentMethodsE2E.Codeunit.al |
no |
3 × AA0139, 5 × AA0217 |
ReportLayoutsTest.Codeunit.al |
yes — mine |
This PR changes six files, all under Foundation/Reporting and Tests/Report. None of the APIV1*E2E codeunits is among them.
What changed: the head commit 3adc5c7636 is unchanged since 7 August and the full matrix passed on it (170 green) on 9 and 10 August. It began failing after the BCArtifact bump to 29.0.53514.0 (#10121). The source did not move; the compiler did.
Not transient: two complete matrix runs nine hours apart, with main having advanced in between, produce byte-identical annotations.
▎ mirkobiasini — is the warning baseline being rebuilt against the new artifact, or are PRs expected to absorb the new compiler's warnings?
I have eight genuine warnings of my own in ReportLayoutsTest.Codeunit.al and will fix them — but since the gate fails on the union, fixing only mine still leaves the ten AL0607 and the job still red. I would rather push that fix once, when the gate can actually pass, than spend a matrix run per attempt.
Happy to be told I have misread this.
Joshua (mynjj)
left a comment
Joshua (mynjj)
left a comment
There was a problem hiding this comment.
This is a very verbose PR, I recommend removing most of the comments, for doc comments local procedure shouldn't need them. I haven't gone through everything, but there is at least one unnecessary procedure (overly verbose code).
Predrag Maricic (PredragMaricic)
left a comment
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
Request changes:
S1 - Copy-to-override toggle displays the wrong scope
In ReportLayoutEditDialog.page.al, switching Save Changes to a Copy back off only disables Available in All Companies; it does not restore its value to true. If the user selected company-only while copy mode was enabled and then returns to override mode, the dialog displays a read-only No, while EditReportLayout silently forces AvailableInAllCompanies := true and writes a tenant-wide override. The displayed scope can therefore contradict the actual write.
Please restore the value to true when CreateCopy = false in override mode and add a regression test covering toggle on, select company-only, toggle off, and save.
S2 - Copying an obsolete extension layout cannot clear obsolete
For an extension layout already marked obsolete, SetupDialog sets IsObsoleteEditable := false. Selecting Save Changes to a Copy does not re-enable the field, so the new user-defined copy is forced to remain obsolete. This regresses the previous copy flow, where the copy's metadata was editable.
Copy mode should restore normal tenant-layout editability, while switching back to override mode should reapply the one-way obsolete lock. Please add coverage for copying an obsolete extension layout.
S3 - Current head does not pass validation
The current head SHA fails the PR build because it introduces three AA0139 warnings and five AA0217 warnings. There is also an accepted unresolved test issue in this PR: assertions inside two new modal handlers can be swallowed, so those tests do not reliably pin the obsolete-lock and default-scope behavior.
The core override and field-granular scope implementation is thoughtfully covered, but these UI transition defects and the current validation failures should be resolved before merge.
Mirko and Joshua reviewed the comment density and both landed on the same point: implementation comments are a second thing to maintain and go out of sync, and too many of them read as generated. Trimmed across all four files on one rule - keep a comment only where it records why and the reason cannot be recovered from the code, delete anything restating what. The note on OverrideCompany survives because the fallback looks like belt-and-braces without it, but at two lines rather than six. Telling Niels-Henrik earlier that this was prototype quality and the comments would come out later does not survive the app team joining, so they come out now. Inline ScopeDimension, which had a single consumer, and drop the parentheses around single conditions - both flagged as unidiomatic. The parenthesised lines predate this branch but a re-indent had already pulled them into the diff, so cleaning them adds no attribution that was not there. Convert the two modal handlers this pull request introduced. An assertion that fails inside a handler can be swallowed by the calling UI operation, so both tests could have stayed green while the obsolete lock or the default scope regressed - which defeats the reason they were written. The handlers now record the observed value and the tests assert it after the modal closes, which is stronger than asserting in the handler at all. The pre-existing-handler argument that deferred this last round does not apply to handlers added here. Fix the eight warnings the new compiler reports in the test codeunit: CopyStr around CompanyName for the three Text to Text[30] assignments, and labels for the five StrSubstNo format strings. Verified: test project compiles clean with CodeCop and no AA0139 or AA0217 remain in the codeunit; BaseApp has no diagnostics in Foundation/Reporting; codeunit 139595 is 31/31 including both converted handlers.
8270309
…pass Predrag filed these as requested changes on 13 August. His approval an hour later replaced the review state, so the body stopped showing in every summary view and nobody actioned it for two days. S1 - unticking Save Changes to a Copy disabled the availability field without restoring its value, so a user who had chosen company-only saw a read-only No while the in-place edit wrote an all-companies override. The dialog contradicted what was written. Both the scope and the obsolete value are now restored when copy mode is left, so a locked field never displays something the write denies. S2 - the one-way obsolete lock was applied once at setup and never revisited, so taking a copy of an obsolete extension layout produced a copy that could not be un-obsoleted. The lock now belongs to the override path only: copy mode restores normal tenant-layout editability and switching back reapplies it, which is the shape he asked for. Remembering the metadata value in ObsoleteInMetadata is what makes reapplying possible. Two regression tests written to his sequences: toggle on, choose company-only, toggle off, save; and copying an obsolete layout. 33/33. Finish the comment pass. Joshua's review body asked for most comments removed and said local procedures should not carry doc comments, so the last doc block on a local procedure is gone along with a duplicate of the SetLayoutStatus summary and four comments that restated their own code. Comment density on added BaseApp lines is 10 percent, against 24 at the start of the pass; the implementation codeunit sits at 6 percent where it was 5 before this branch existed. Close the queue-lifecycle finding the way its own knowledge article prescribes: Clear when a test starts, AssertEmpty when it ends, so a leaked value cannot cascade and every enqueued interaction is proven to have fired exactly once.
|
The 'Edit info' action (EditLayout) on page 9660 "Report Layouts" now unconditionally calls ReportLayoutsImpl.EditReportLayout for both user-defined and extension-installed layouts. Previously, editing a non-user-defined layout required a Dialog.Confirm warning and only ever created a separately-owned copy. Now, confirming OK on the edit dialog for a non-user-defined layout calls UpsertLayoutOverride, writing a global (Company Name = '') 'Tenant Report Layout Override' record that changes Description/Obsolete status of a vendor-supplied layout for every company on the tenant, with no confirmation and no AccessByPermission gate on the action. By contrast, the sibling status-change actions (SetApproved, SetDraft, SetPendingApproval, SetRetired) that mutate the same class of data via the same internal codeunit are explicitly gated with AccessByPermission = tabledata "Tenant Report Layout" = M. EditLayout has no equivalent gate, widening the mutation's blast radius without a matching authorization boundary. Recommend adding an AccessByPermission guard on EditLayout consistent with the other mutation actions, and/or restoring an explicit confirmation before writing a global override record. 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.33.4 |
|
SetLayoutStatusBatch's new mixed-scope guard only classifies scope for non-user-defined (extension-installed) rows; user-defined rows are skipped by the classification loop entirely even though a user-defined Tenant Report Layout also has a company scope of its own. The new test suite only exercises batches made up entirely of extension-installed layouts — there is no test mixing a user-defined layout with company-scoped/global-scope extension-installed layouts in one batch, so intended behavior for that combination is unverified. Recommend adding such a test. 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.33.4 |
All review points addressed —
|
6 participants
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. Scope is resolved per field — from theOverride Layout Statusflag rather than from the mere existence of a row — so a description-only or obsolete-only row never drags a status change into the wrong scope.Telemetry for the override path uses its own allocated event id
0000RTQwith metadata-only dimensions; the layout description is reported as a changed/not-changed flag, never as content.Tests
Report Layouts Test(codeunit 139595) — 33 tests, covering the all-companies default for both the status and edit paths, company precedence where a company already owns the status, field-granular scope resolution in both directions, no-op edits writing nothing, one-wayIsObsolete, the copy escape hatch, both the success and mixed-scope-rejection paths of a multi-layout batch run, and the pre-existing user-defined regression suite. A second shipped layout (MYLAYOUT2) was added to the test report so multi-layout scope scenarios are exercisable.Two of those cover the dialog transitions raised in review: unticking Save Changes to a Copy after selecting company-only must restore the all-companies scope, and taking a copy of an obsolete extension layout must re-enable the obsolete field so the copy is not stuck obsolete.
Modal handlers record what they observe and the test asserts it after the dialog closes, rather than asserting inside the handler where a failure can be swallowed by the calling UI operation.
LibraryVariableStorageis cleared at the start of every test and asserted empty at the end, so a leaked value cannot cascade and each dialog is proven to have fired exactly once.The tests that cover the all-companies paths deliberately register no
ConfirmHandler, so an unexpected prompt fails them — that is what pins the removal of the former confirmation.Scope behaviour is additionally checked by hand across two companies, using a helper extension that surfaces the override scope the product page does not display.
Notes for reviewers
AccessByPermission = tabledata "Tenant Report Layout" = Mis deliberate:BaseSystemPermissionSetgrants the tenant layout and override tables together, so it correctly represents "may manage layout status" and matches the page's own permission.StatusChangedMessageHandlernot inspecting the message it consumes (pre-existing), and the parameter shape ofUpsertLayoutOverride(changes again with the company-scoped option).AB#637301