[29.0] New Header & footer designs and Report Themes for Composite Layouts - #11126
[29.0] New Header & footer designs and Report Themes for Composite Layouts #11126MSlenejennum wants to merge 13 commits into
Conversation
…10489) [AB#647452](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/647452) upgrade Adds the out-of-box Composite Layout parts (11 header/footer designs, 3 themes) and the install/upgrade pass that writes them into the shared pool under Tenant Report Defaults. The page and lookup helper are already in `main`; this supplies their content. **New:** `codeunit 9667 "Composite Report Parts Mgt."`, `codeunit 104064 "Upgrade Composite Report Parts"`, 14 resources under `.resources/ReportParts/`. **Changed:** `BaseAppInstall` seeds on install; `UpgradeTagDefinitions` (W1/BE/IT/RU) registers the per-database tag; `CompositeLayoutTests` now 42 tests. - Parts are stored under this app's own App ID, resolved via `NavApp.GetCurrentModuleInfo`. That ID is part of the `Tenant Report Layout` key and of the `<guid>::<name>` reference every assignment stores. - Seeding deletes and re-inserts, because the platform will not let an existing layout's type and content be modified. - **The pass is not gated by its upgrade tag — it runs on every upgrade and overwrites tenant edits to the shipped parts.** Deliberate: these are app content, not tenant data. Copy a part to make a variant. - A part the app no longer ships is removed and its assignments cleared, so dropping a `SeedPart` call actually takes it out of the pool. - A part whose resource cannot be read is logged to telemetry and skipped, not raised — an uncaught error here would abort install or upgrade. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: MSlenejennum <276599928+MSlenejennum@users.noreply.github.com> Co-authored-by: Morten Rasmussen (ABAKION AS) <v-mortenr@microsoft.com> (cherry picked from commit a8b4069)
github-actions
Bot
added
the
Team: Integrations
MSlenejennum
changed the title
MSlenejennum
changed the title
MSlenejennum
changed the title
Permission elevation applies to the object executing the operation, not its callers. "Upgrade Composite Report Parts" declared the elevated tabledata permission but never touches the table itself - it just calls "Composite Report Parts Mgt.", which does the actual Insert/Delete. Move the elevation there so seeding succeeds for a user without direct Tenant Report Layout permissions. (cherry picked from commit 1bc2883)
a82f0c5
Uses Permissions Mock to run the company-open seeding path under a user with Execute-only access (no table data permissions), overriding this codeunit's default TestPermissions = Disabled for just this test so permission enforcement is actually exercised. Fails on the old placement of the elevated permission and passes with the fix. (cherry picked from commit 5c5151e)
…ning SeedPart now does Get + Modify-or-Insert instead of unconditionally deleting any existing row first. This keeps the same retry-after- partial-failure safety (no duplicate-key error re-seeding a part that already succeeded in an earlier aborted pass) without ever deleting a row. Also drops PruneRetiredParts and its now-orphaned helpers (ClearAssignments, IsShippedPart, RemovePart). Tracing the seeding gate showed it runs at most once per database, ever - on this release there is nothing pre-existing to prune, and after the first run the upgrade tag never reopens, so the pruning pass could never clean up a future retirement either. Removing it drops the last delete from this feature entirely. (cherry picked from commit b16eec7)
The removed PruneRetiredParts was gated by the same one-time seeding tag as SeedDefaultParts, so it could never have run on a database that already completed seeding under an earlier version - it was unreachable in every real scenario, not just the one this PR removes. Document the actual gap so a future part retirement adds its own upgrade tag instead of assuming this pass will clean it up. (cherry picked from commit 32e6a69)
Permissions was still "rid" (Read/Insert/Delete) from before the upsert change - it never covered Modify, which SeedPart now calls for every part that already exists, and no longer needs Delete since nothing in this codeunit deletes anymore. Changed to "rim". Also updated the retirement guidance: the platform's report-layout lifecycle feature already gives "Layout Status" a Retired value, so a future retirement's dedicated upgrade step should flip that status rather than delete the row - keeping this feature delete-free even when a part eventually needs retiring. (cherry picked from commit ab8d3c0)
SeedDefaultParts ran every part straight through SeedPart, so a missing or unreadable resource on any one of the 14 - a build defect - raised out of the per-database upgrade trigger and failed the whole upgrade tag, not just that one part. Added SeedPartOrLogFailure, which wraps SeedPart in a TryFunction and logs the failure to telemetry instead of propagating it, so the remaining parts still get seeded and the tag still gets written. SeedPart itself is unchanged and still raises for direct callers. (cherry picked from commit 29f0269)
ShippedThemeResourceTok was only used by the two tests removed with PruneRetiredParts; CI's AA0137 (unused variable) failed the Clean build on it. (cherry picked from commit 64eb670)
Moving the tabledata grant off "Upgrade Composite Report Parts" also took its InherentPermissions = X with it - but that was granting inherent Execute, which its own comment says the company-open subscriber needs, and which the docs call out for upgrade codeunits. Restored, and added the matching InherentEntitlements = X to "Composite Report Parts Mgt." so the write survives a low-entitlement license too, mirroring what the platform's "Upgrade Tag" codeunit carries for the same kind of company-open write. The new permission test could not have caught this: All Objects grants codeunit * = X, so execute elevation is never exercised. Test hygiene: Permissions Mock is SingleInstance and a lowering test that fails never reaches its own Stop(), which in this shared, non-isolated suite would fail every later test. Initialize now resets it. Also asserted the part is really absent before seeding, so the final assertion cannot pass on a pre-existing row. (cherry picked from commit a1e5366)
Session.LogMessage ships the message string to telemetry verbatim regardless of DataClassification, and GetLastErrorText can carry customer content such as file paths. The message is now a static label and every variable part - PartName, ResourceFile and the error text - goes into the custom dimensions instead. (cherry picked from commit d04b2ae)
The new permission test set the seeding upgrade tag and then cleared it only after its closing assertion, so a failing assertion would have left the tag set for whatever ran next. Same shape as the Permissions Mock leak: cleanup of shared state must not be gated on the assertion passing. Both are now restored before the assert. (cherry picked from commit 571ea57)
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis PR adds shipped composite report header/footer designs and report themes, then seeds them under Tenant Report Defaults during install, upgrade, and company open. The main flow is targeted, the resource names in the seeding code match the shipped files, and the tests cover the normal seeding path and minimal-permission company-open path. The remaining problem is the completion marker: the code can still record the database tag even when one or more parts were not written. Problem-solution fitFit: Partial The change delivers the requested seeded parts and includes a clear install/upgrade path. It does not fully protect the persistent state when a part fails to seed, because the database can be marked as complete while shipped content is missing. SuggestionsS1 (🔴 High): Do not mark failed seeding complete Risk assessment and necessityRisk: The regression surface is the shared Tenant Report Defaults pool and the install, upgrade, and company-open seeding paths. If the tag is set after a partial seed, a tenant can stay permanently missing a shipped theme or header/footer design until a later dedicated upgrade step fixes it. Necessity: The backport is needed so the composite report parts are available on 29.0. The scope is right, but the tag must mean that the full seeding step completed.
|
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThis round changes the seeding flow so Status of previous suggestions
New observations (commits since round 1)None beyond the still-open previous suggestion. Risk assessment and necessityRisk: The regression surface is the shared Tenant Report Defaults pool and install, upgrade, and company-initialization paths. If one shipped resource cannot be read, automatic tag stamping can make later install, upgrade, or company-open runs exit before retrying, leaving a shipped part missing. Necessity: The backport is needed so the composite report parts are available on 29.0. The scope is right, but the completion tag must only mean that all shipped parts were seeded.
|
5 participants
Backport on PR #10489 and #11137
AB#649263