MSlenejennum/647454/sales internal new body layouts and obsolete current - #10495
MSlenejennum/647454/sales internal new body layouts and obsolete current#10495MSlenejennum wants to merge 9 commits into
Conversation
…urrent Word layouts Cherry-picked from BCApps/features/643211/composite-layout-with-pr10134: - Report 107 Customer - Order Summary - Report 114 Salesperson - Sales Statistics - Report 115 Salesperson - Commission Each report gets a new WordBody layout (Subtype = Body) with its *Body.docx file, and the existing Word layout is marked ObsoleteState = Pending with ObsoleteTag 30.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s/643211/migrate-existing-word-report-layouts-to-composite-document-report-structure Takes the 6 files (3 .Report.al + 3 *Body.docx) verbatim from that branch, tip 8443b74, for: - Report 107 Customer - Order Summary - Report 114 Salesperson - Sales Statistics - Report 115 Salesperson - Commission Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ejennum/647454/Sales-Internal-New-Body-Layouts-And-Obsolete-Current
MSlenejennum
changed the title
…ejennum/647454/Sales-Internal-New-Body-Layouts-And-Obsolete-Current
Agentic PR Review - Round 1Recommendation: Accept with SuggestionsWhat this PR doesThis PR adds a body-only The change is a coherent, additive layout increment: it introduces the new body layouts and obsoletes the old Word ones without removing them, so partners with derived Word layouts get a compile warning rather than a break. The three reports are handled the same way, which is the right consistency. The layout content itself is inside binary SuggestionsS1 - Obsolete tag and guard do not match the description Risk assessment and necessityRisk: Low. This is report layout metadata only, with no posting, amount, or data logic. Because the default rendering layout is unchanged and the old layouts are obsoleted (not removed), existing users see no behavior change; the only external effect is a compile warning for partners with derived Word layouts, which is the intended deprecation signal. The main open items are the tag/guard mismatch above and the fact that the Necessity: Reasonable as an incremental step toward the Document Report Experience. The description notes the header/footer and theme parts are deliberately deferred until AB#647452 lands, which keeps this PR appropriately scoped. No automated test is warranted for a layout addition.
|
Agentic PR Review - Round 2Recommendation: Accept with SuggestionsWhat this PR doesThis PR adds a body-only The only change since round 1 is the commit Status of previous suggestions
S1 - Obsolete tag and guard do not match the description New observations (commits since round 1)None. The single new commit changes only the binary Risk assessment and necessityRisk: Low, and unchanged from round 1. This is report layout metadata only, with no posting, amount, or data logic. The default rendering layout stays the same and the old layouts are obsoleted (not removed), so existing users see no behavior change; the only external effect is a compile warning for partners with derived Word layouts. The open items are the tag/guard mismatch above and the fact that the Necessity: Unchanged from round 1. Reasonable as an incremental step toward the Document Report Experience, and appropriately scoped. No automated test is warranted for a layout addition.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ejennum/647454/Sales-Internal-New-Body-Layouts-And-Obsolete-Current
|
In src/Layers/APAC/BaseApp/Sales/Reports/SalespersonSalesStatistics.Report.al (report 114 "Salesperson - Sales Statistics"), the pre-existing rendering layout was named layout(RDLCLayout) with Type = RDLC. This PR keeps the identifier RDLCLayout but changes it to Type = Excel (new .xlsx file), and moves the actual legacy RDLC content into a brand-new, differently-named layout(RDLC). Report layout identifiers are the stable key used by Report Layout Selection / per-tenant custom layout configuration; any customer or partner setup that already selects report 114's built-in "RDLCLayout" will, after this upgrade, silently start receiving an Excel-formatted output instead of the RDLC output it previously produced — with no ObsoleteState/warning on the renamed element to signal the change. Compare with the sibling, unmodified W1 file src/Layers/W1/BaseApp/Sales/Reports/SalespersonSalesStatistics.Report.al (same report 114), which already keeps these as three distinctly-named layouts: layout(Excel), layout(Word), layout(RDLC). The APAC file should follow the same pattern: keep the existing RDLCLayout identifier bound to the RDLC content (optionally obsoleting it on its own timeline) and introduce the new Excel layout under a new, distinct identifier (e.g. layout(Excel)) instead of repurposing the pre-existing one. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4 |
Agentic PR Review - Round 3Recommendation: Request ChangesWhat this PR doesThis PR adds body-only Word layouts for Sales report migration to the Document Report Experience and obsoletes the existing Word layouts. Since round 2, the new commit adds the RU Customer - Order Summary metadata and changes the APAC Salesperson - Sales Statistics rendering block. The RU change matches the existing W1 pattern. The APAC change does not: it keeps the old layout(RDLCLayout) name but changes that layout from RDLC to Excel, while moving the RDLC file into a new layout(RDLC). This breaks the stated goal that default and existing layout behavior stay unchanged. Status of previous suggestions
New observations (commits since round 2)S2 - Do not reuse RDLCLayout for Excel Risk assessment and necessityRisk: The W1 and RU body-layout additions are metadata-only and low risk. The APAC report change is high risk because it changes the output format behind an existing layout identifier and the report still has DefaultRenderingLayout = RDLCLayout. Necessity: The body-layout migration is a valid slice, but it must preserve existing layout selections. Fixing the APAC layout identity is required before this can merge safely; no automated test is required for the body-layout metadata itself.
|
| { | ||
| layout(RDLCLayout) | ||
| { | ||
| Caption = 'Salesperson Sales Statistics Excel'; |
There was a problem hiding this comment.
Existing APAC layout name changed from RDLC to Excel. The layout RDLCLayout was RDLC and now becomes Type = Excel with an .xlsx file, and a new layout named RDLC is added. Report 114 still has DefaultRenderingLayout = RDLCLayout, so APAC users get Excel output by default instead of RDLC, and any saved layout selection that points to RDLCLayout also switches. Please keep RDLCLayout as the RDLC layout and add only the new body layout.
| LayoutFile = '.\Sales\Reports\CustomerOrderSummary.docx'; | ||
| Type = Word; | ||
| Summary = 'Built in layout for the Customer Order Summary word report.'; | ||
| ObsoleteState = Pending; |
There was a problem hiding this comment.
Obsoleted Word layout is still referenced, so Clean builds fail. RU report 107 keeps DefaultRenderingLayout = Word while this PR marks that Word layout ObsoleteState = Pending. The Clean pipelines for about 20 countries fail with AS0105 The ReportLayout 'Word' cannot be referenced ... ObsoleteTag '30.0'. Please move the default to a layout that is not obsolete, so no object references the obsoleted layout.
| Type = Word; | ||
| LayoutFile = './Sales/Reports/SalespersonSalesStatistics.docx'; | ||
| Summary = 'Report layout made for print. Use a Word editor to modify the layout.'; | ||
| ObsoleteState = Pending; |
There was a problem hiding this comment.
The PR description does not match the code. It says ObsoleteTag = '32.0' behind #if not CLEAN32 and that the default rendering layout stays Excel, but the code uses ObsoleteTag = '30.0', has no CLEAN guard, and the RU and APAC defaults are not Excel. Please update the description and confirm the obsolete tag matches the target release.
Predrag Maricic (PredragMaricic)
left a comment
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
Recommendation: Request Changes
The W1 and RU body-layout additions are coherent, additive changes, but the APAC update introduces a compatibility regression and the obsolete lifecycle remains inconsistent with the declared cleanup plan.
S1 - Align the obsolete lifecycle with the planned cleanup version
The PR description says the old Word layouts use ObsoleteTag = '32.0' behind #if not CLEAN32, but every changed report currently uses ObsoleteTag = '30.0' unconditionally. Please make the implementation and declared cleanup plan consistent so the deprecation is introduced and removed in the intended release.
S2 - Preserve the APAC RDLCLayout identity and default output
In the APAC report, RDLCLayout was the existing RDLC layout and remains the value of DefaultRenderingLayout. This PR rebinds that same identifier to an Excel file and moves RDLC to a new RDLC identifier. Existing default and saved layout selections can therefore silently produce Excel output instead of RDLC. Keep RDLCLayout bound to the RDLC layout and add Excel under a distinct layout identifier.
The W1 and RU changes are otherwise low-risk layout metadata additions. The APAC identifier reuse is blocking because it changes output format behind an existing default layout key.
|
In src/Layers/RU/BaseApp/Sales/Reports/CustomerOrderSummary.Report.al, the report property Suggested fix (apply manually — could not be anchored as a one-click suggestion): DefaultRenderingLayout = WordBody;Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4 |
| LayoutFile = './Sales/Reports/SalespersonSalesStatistics.xlsx'; | ||
| Summary = 'Report layout primarily made for data analysis. Use an Excel editor to modify the layout.'; | ||
| } | ||
| #if not CLEAN30 |
There was a problem hiding this comment.
The APAC variant of "Salesperson - Sales Statistics" (report 114) previously had only an RDLCLayout; this PR introduces a brand-new layout(Word) wrapped in #if not CLEAN30 and marks it ObsoleteState = Pending at the moment of its introduction. This ships new report-layout behavior that is already scheduled for removal: any tenant setup, report selection, or extension that starts depending on this Word layout before CLEAN30 flips is being pointed at a transient, already-deprecated contract with no real usable lifetime. Prefer shipping only the supported Excel/WordBody layouts here, or introduce the Word layout without the immediate obsoletion if it needs a real (non-transient) lifetime.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
Good Sense Reviewer - Round 4Recommendation: Request ChangesWhat changed since round 3Only the APAC report
Status of previous suggestions
New observations (commits since round 3)S3 (🔴 High): The #if not CLEAN30
layout(Word)
{
...
ObsoleteTag = '30.0';
#endif
}
layout(RDLCLayout)The #if not CLEAN30
layout(Word)
{
...
ObsoleteTag = '30.0';
}
#endif
layout(RDLCLayout)Since removability at CLEAN30 is the whole purpose of adding the guard, this needs to be fixed before merge. Risk assessment and necessityRisk: The layout-identity fix (S2) removes the round-3 high risk - existing selections render RDLC again, and the Excel layout has its own name. The remaining risk is the malformed guard: it does not break runtime or today's CI, but it will break the CLEAN30 cleanup build, which is exactly the scenario the guard exists to support. Necessity: The body-layout migration and the APAC identity fix are valid and now correct. Only the one-line
|
4 participants
What & why
all reports run with default theme and header footer design internal.
Each report gets a new
WordBodylayout with a body-only*Body.docx, and its existingWordlayout is marked
ObsoleteState = Pending/ObsoleteTag = '30.0'.DefaultRenderingLayoutstaysExcel, and Excel/RDLC layouts are untouched, so nothing changesfor existing users.
DefaultHeaderFooterPart/DefaultThemePartare commented out until theparts from 647452 land.
Linked work
AB#647454
How I validated this
What I tested and the outcome
Risk & compatibility
No default behavior change. The old Word layouts are obsoleted, not removed — partners with
derived layouts get a compile warning and should move to the body layout before the 32.0 cleanup.
Follow-up: enable the header/footer and theme parts once 647452 is merged.
Attacnhed you find the