[29.0] Composite Report Parts: seed via a report extension instead of install/upgrade code - #11126
Conversation
a82f0c5
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.
|
…tension The shipped header/footer designs and report themes no longer need to be written into Tenant Report Layout by install/upgrade code running with elevated permissions. Declaring them as layout() entries on a "Composite Layout" report extension for "Tenant Report Defaults" lets the platform register them natively, so the seeding codeunit, its upgrade pass, the per-database upgrade tag (W1/BE/IT/RU), and their test suite are removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5991e0d to
1bd536d
Compare
…ention Every other report extension in BaseApp uses ReportExt.al (capital R and E). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Good Sense Reviewer - Round 3Recommendation: AcceptWhat this PR doesThis round replaces the app-owned seeding approach with a declarative report extension for Tenant Report Defaults. The extension lists all shipped report themes and header/footer designs as layout entries, and each LayoutFile path points to a file added by the change. Because the current net diff no longer writes Tenant Report Layout records or registers a completion upgrade tag, the earlier partial-seed completion risk is removed. Status of previous suggestions
New observations (commits since round 2)None - the current force-pushed change addresses the previous blocker without adding a new review finding. Risk assessment and necessityRisk: The regression surface is now the platform registration of report extension layouts and the packaging of the 14 shipped files. If a path or file is wrong, a shipped theme or header/footer design would be missing, but there is no longer an install, upgrade, or company-open database write path that can leave a false completion marker behind. Necessity: The change is needed so the shipped composite report parts are available on 29.0. The scope is narrow: it registers the shipped parts declaratively and removes the need for app-owned seeding behavior.
|
…plate references The docx files carried a Word "attached template" relationship pointing at absolute local paths (C:\Users\...\Downloads\Calm.dotx or a depot-absolute path to ReportTheme\Default.dotx). Strip the relationship so the shipped templates don't leak local machine paths.
Good Sense Reviewer - Round 4Recommendation: AcceptWhat this PR doesThe new commit removes local-machine references from the packaged header and footer design files. The declarative registration approach reviewed in the previous round is unchanged. Status of previous suggestions
New observations (commits since round 3)None - the new commit only cleans metadata inside the shipped document templates. Risk assessment and necessityRisk: The incremental change is limited to packaged document binaries. A malformed template could affect a shipped design, but it does not reintroduce the database seeding risk. Necessity: Removing local-machine references makes the shipped templates portable and is an appropriate final cleanup.
|
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
Recommendation: Request Changes
S1 - Restore the actual External Default template
ExternalDefault.docx and ExternalDefaultDetailed.docx are byte-identical (3f0b469a...). The ordinary External Default therefore renders the detailed design, including bank account, giro, and VAT controls, despite its summary promising only contact details. The merged source PR has distinct blobs, and its ordinary template omits all 16 detailed-only bindings. Backport the final source assets rather than registering the duplicated file.
S2 - Link the work item for this feature backport
AB#649843 is the unrelated stress-test bug delivered by PR #11331; its title and repro cover duplicate names, layout status, empty uploads, and part resolution. It does not track introducing the 14 shipped parts through PR #11137. Link the actual 29.0 feature/backport item so validation and servicing traceability point to this change.
S3 - Validate registration and rendering of all shipped parts
This PR introduces 14 platform-registered binary layouts but adds no focused test, and both manual test-plan items remain unchecked. Existing CI is fully green but does not prove that the parts appear under Tenant Report Defaults or render as their declared variants - the duplicated External Default passed all 166 checks. Add automated registration assertions where feasible and record runtime rendering validation for all distinct designs.
The declarative AL file is otherwise byte-identical to the merged source implementation, all referenced resources exist, and the previous imperative-seeding risks are no longer present.
Backport of PR #11137 (merged into main). These files had drifted from main since the backport branch was created — main went through further review rounds (content fixes, additional header/footer sections) that never made it into this backport. Bring all 11 HeaderFooterDesign docx files and Calm.dotx in sync with main's current content.
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
Verified the backport against #11137: the report extension and all 14 layout assets are blob-identical to the merged source. The previously duplicated External Default asset is corrected. No blocking findings.
Good Sense Reviewer - Round 5Recommendation: AcceptWhat this PR doesThe new commit refreshes the shipped header/footer design files and the Calm theme to match the already merged main-branch version. The declarative Tenant Report Defaults registration is unchanged, all Status of previous suggestions
New observations (commits since round 4)None - the new commit only syncs packaged templates with the main version and does not change AL logic. Risk assessment and necessityRisk: The incremental regression surface is limited to the packaged Word template files used by the shipped themes and header/footer designs. A bad template could make one shipped design unusable, but the layout registrations still reference existing files and the updated blobs match the main-branch version. Necessity: The backport remains needed so the shipped composite report parts are available on 29.0 through platform registration. Syncing the template files keeps the release branch aligned with the already accepted main-branch content without reintroducing app-owned seeding behavior.
|
…nto MSlenejennum/649263/New-Header-and-Footer-Layouts-and-Report-Themes
Good Sense Reviewer - Round 6Recommendation: Request ChangesWhat this PR doesThe new commit merges the release branch and bumps the Status of previous suggestions
New observations (commits since round 5)S2 (🔴 High): Do not merge platform bump that breaks builds Risk assessment and necessityRisk: The incremental regression surface is the shared platform package used by every app build. A broken platform bump blocks release validation before the report-layout change can be safely consumed. Necessity: The backport remains needed so shipped composite report parts are available on 29.0 through platform registration. A newer platform package is only safe when the repo also contains any source or mock updates needed to compile against that package.
|
…Footer-Layouts-and-Report-Themes
Summary
CompositeLayout.reportExt.al, extending "Tenant Report Defaults") that lists all 14 parts aslayout()entries."Composite Report Parts Mgt.","Upgrade Composite Report Parts", the per-database upgrade tag (W1/BE/IT/RU), theBaseApp Installseeding trigger, and the composite-report-part tests added toCompositeLayoutTests— none of it is needed once the platform registers the layouts natively instead of the app writing them intoTenant Report Layoutitself..resources/ReportParts/toFoundation/Reporting/ReportParts/, alongside the report extension that references them viaLayoutFile.This is a [29.0] backport of the same rework applied on
mainin #11137.AB#649843
Test plan