Skip to content

[FI] Delocalize Depreciation Differences into a first-party extension - #9326

Open
Milica Đukić (djukicmilica) wants to merge 70 commits into
mainfrom
bugs/622873
Open

Milica Đukić (djukicmilica) wants to merge 70 commits into
mainfrom
bugs/622873

Conversation

@djukicmilica

@djukicmilica Milica Đukić (djukicmilica) commented Jul 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Delocalizes the Finland Depreciation Differences regulatory feature from the FI BaseApp layer into a new first-party extension app, and obsoletes the in-place BaseApp implementation behind CLEAN30 / CLEANSCHEMA33 guards and a feature key (DepreciationDifferencesFI).

New extension app -- src/Apps/FI/DepreciationDifferencesFI

Type Object
TableExt FA Posting Group, FA Ledger Entry, Source Code Setup
PageExt FA Posting Group Card, FA Posting Groups, Source Code Setup, Finance Manager Role Center
Report Calc. and Post Depr. Diff. FI (+ RDLC)
Table Depr. Diff. Posting Buffer FI
Codeunit FI Depreciation Diff. Feature, Dep Diff FI Subscribers, Dep Diff FI Feature Data Upd., Upgrade Depreciation Diff. FI, Dep Diff FI Upgrade Tag, Dep Diff FI Helper Procedures, Dep Diff FI Data Class.
Enum/Perm Feature To Update enum ext, PermissionSet ext
Test Depreciation difference calculation tests

Transition-only feature-management objects (the feature codeunit, subscribers, Feature Data Update implementation, and enum registration) compile only while CLEAN30 is not defined. In CLEAN30 builds, the Upgrade Depreciation Diff. FI codeunit (Subtype = Upgrade) performs the field migration directly. The field-copy logic is shared by both migration paths through the always-compiled Dep Diff FI Helper Procedures codeunit, avoiding duplicated migration code. Legacy schema remains available until CLEANSCHEMA33.

BaseApp obsoletions (ObsoleteState: Pending, 30.0 / removed 33.0, schema removed under CLEANSCHEMA33)

  • Obsolete depreciation-difference fields on FA Posting Group / FA Ledger Entry / Source Code Setup, the legacy report (Calc. and Post Depr. Diff.), and the legacy Depr. Diff. Posting Buffer table.
  • Wrap legacy references in BaseApp pages, permission sets, data classification, demo data, and tests behind the CLEAN30 guard. BaseApp production objects no longer depend on the feature key; transitional runtime checks remain in the extension, demo-data producers, and test infrastructure while CLEAN30 is not defined.
  • Move posting-buffer data classification from BaseApp to the app-side Dep Diff FI Data Class. subscriber.
  • Exclude legacy BaseApp tests and helpers under CLEAN30; equivalent depreciation-difference tests are included in the extension test app.

Integration and migration compatibility

  • Register the app and test app in build/projects.json, build/groups.json, and the Apps FI AL-Go project settings.
  • Add the Depreciation Differences FI dependency to Contoso Coffee Demo Dataset FI and route demo-data initialization to legacy or extension fields according to feature state.
  • Preserve the FI Hybrid BC Last field mapping until CLEANSCHEMA33 and update its transformation test for the intentionally obsolete source field.
  • Update FI cloud-migration table tests to account for the legacy object lifecycle.

Verification

Implementation and build registration are complete and the app is correctly wired into the BCApps build registry. Local full compile/publish/test was not run because the local enlistment's Base Application symbols were not provisioned. PR CI has completed successfully with 170 successful checks and 2 skipped checks.

Fixes: AB#622873

Linked Platform PR for feature key

https://microsoft.ghe.com/bic/BC-Platform/pull/47112

@djukicmilica Milica Đukić (djukicmilica) changed the title Draft deloc Draft test deloc Jul 11, 2026
@github-actions github-actions Bot added the Build: scripts & configs Build scripts and configuration files label Jul 11, 2026
Move the Finland "Depreciation Differences" regulatory feature out of the
FI BaseApp layer into a new first-party extension app at
src/Apps/FI/DepreciationDifferencesFI, and obsolete the in-place BaseApp
implementation behind CLEAN29 / CLEANSCHEMA32 guards and a feature flag,
following the pattern of PR #9197.

New app:
- Table extensions for FA Posting Group, FA Ledger Entry, Source Code Setup
- Page extensions for FA Posting Group Card / List
- Calc. and Post Depreciation Difference report (+ RDLC) and posting buffer table
- Feature, subscribers, upgrade and upgrade-tag codeunits; permission set ext

BaseApp obsoletions (Pending 29.0 / Removed 32.0):
- Obsolete depreciation-difference fields, report and buffer table
- Wrap references in pages, permissions, demo data and tests
- Register the app and its test in the BCApps build registry

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 731eaef8-bab7-4b1c-840e-f409b4de0195
@djukicmilica Milica Đukić (djukicmilica) changed the title Draft test deloc [FI] Delocalize Depreciation Differences into a first-party extension Jul 13, 2026
@JesperSchulz Jesper Schulz-Wedde (JesperSchulz) added the Team: Finance GitHub request for Finance area label Jul 16, 2026
…de Setup

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c9e418b3-19a8-4eaa-ba20-8aa03d619fb7
@github-actions github-actions Bot added Team: Other GitHub request for other area than SCM, Finance or Integration Ownership: Needs Review Ownership is Other, low confidence, or needs manual correction and removed Team: Finance GitHub request for Finance area labels Jul 26, 2026
@djukicmilica Milica Đukić (djukicmilica) added the Team: Finance GitHub request for Finance area label Aug 7, 2026
@github-actions github-actions Bot removed the Team: Finance GitHub request for Finance area label Aug 7, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 17

Recommendation: Request Changes

What this PR does

This moves Finnish depreciation-difference calculation, posting, setup, migration, and tests into a first-party extension. The latest commit removes the invalid demo-data event subscriber and restores the legacy field assignments under CLEAN30.

That removes one invalid reference, but it does not complete the demo-data compatibility fix. LegacyDemoGeneratorInitializesExtensionAccounts still resolves an unavailable Create FA Posting Group codeunit, and the current default and clean FI builds both fail with AL0185. The legacy generator also no longer has an extension-owned path that initializes the new fields when the feature is enabled.

Status of previous suggestions
ID Title Status Author response
S1 Keep other companies pending for migration Addressed Completion remains company-specific; other companies retain their pending update.
S2 Guard removed objects in the CLEAN30 build Addressed Runtime references and transition objects remain guarded; legacy schema survives until its separate cleanup.
S3 Avoid duplicate role center report actions Addressed The legacy and extension actions remain mutually exclusive.
S4 Add rendered report layout screenshots Disputed The layout is stated to be unchanged, but the current description still has no embedded rendered screenshot.
S5 Posting tests omit required company setup Addressed The tests retain general posting setup, an FA class, and document number series.
S6 Preserve the public demo codeunit in its original app Addressed The original public codeunit remains available with pending obsoletion before CLEAN30.
S7 Bind subscriber to the intended publisher Not addressed The invalid subscriber was removed, but the regression test still resolves an unavailable codeunit and both current FI builds fail with AL0185.
New observations (commits since round 16)

None - the latest commit targets S7, which remains open.

Problem-solution fit

Fit: Partial

The delocalization remains a coherent feature increment, and the latest commit removes an invalid subscriber. The demo-data compatibility path still does not compile or provide a verified extension-owned integration for the legacy generator.

Risk assessment and necessity

Risk: The change affects G/L posting setup, migrated ledger flags, per-company upgrade tags, and demo-data initialization. The current default and clean FI builds fail with AL0185 in the extension test app. The legacy generator can also leave the new extension posting-group fields uninitialized after feature enablement. The changed RDLC still has no rendered screenshot.

Necessity: Delocalization is needed to remove Finland-specific functionality from the base application while preserving existing data and upgrade behavior. The demo-data compatibility path must compile and initialize the extension fields before this change is safe to merge.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=9326 round=17 by=alexei-dobriansky at=2026-10-13T13:13:22.9400451Z lastSha=d850de5d87e2c630a4465c7edcdf5a1b180934a1 reviewKey=09afdbf3a81cbd36cf844420c378a276875679d1937dfef26fd938f4dcce1eff suggestions=S1@13016d35:addressed,S2@73049526:addressed,S3@be6cf7a6:addressed,S4@f79aa9bf:disputed,S5@4c2d0d1e:addressed,S6@5d57b380:addressed,S7@fd1ef691:notaddressed parentRound=16

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 99f6f898-8482-413c-ba3c-ada5389ae15a
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 18

Recommendation: Request Changes

What this PR does

This moves Finnish depreciation-difference calculation, posting, setup, migration, and tests into a first-party extension. The latest commit adds the Contoso Coffee Demo Dataset as a dependency of the extension test app so Create FA Posting Group can be resolved.

The dependency resolves that codeunit, but its public API does not contain InsertData. Both the default and clean FI builds now fail with AL0132 at the regression test. The test also calls the generator directly instead of running the Contoso Demo Tool lifecycle that binds the extension-owned subscriber, so it still does not verify the intended integration path.

Status of previous suggestions
ID Title Status Author response
S1 Keep other companies pending for migration Addressed Completion remains company-specific; other companies retain their pending update.
S2 Guard removed objects in the CLEAN30 build Addressed Runtime references and transition objects remain guarded; legacy schema survives until its separate cleanup.
S3 Avoid duplicate role center report actions Addressed The legacy and extension actions remain mutually exclusive.
S4 Add rendered report layout screenshots Disputed The layout is stated to be unchanged, but the current description still has no embedded rendered screenshot.
S5 Posting tests omit required company setup Addressed The tests retain general posting setup, an FA class, and document number series.
S6 Preserve the public demo codeunit in its original app Addressed The original public codeunit remains available with pending obsoletion before CLEAN30.
S7 Bind subscriber to the intended publisher Not addressed The new dependency resolves a different codeunit whose API has no InsertData; both FI builds fail with AL0132, and the test still bypasses the subscriber-binding lifecycle.
New observations (commits since round 17)

None - the latest commit targets S7, which remains open.

Problem-solution fit

Fit: Partial

The delocalization remains a coherent feature increment. The demo-data integration has a plausible extension-owned design, but its regression test still does not compile or exercise the lifecycle that activates the subscriber.

Risk assessment and necessity

Risk: The change affects G/L posting setup, migrated ledger flags, per-company upgrade tags, and demo-data initialization. The current default and clean FI builds fail with AL0132 in the extension test app. The intended demo-data subscriber path therefore remains unverified. The changed RDLC still has no rendered screenshot.

Necessity: Delocalization is needed to remove Finland-specific functionality from the base application while preserving existing data and upgrade behavior. The regression test must compile and exercise the supported demo-data generation path before this change is safe to merge.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=9326 round=18 by=alexei-dobriansky at=2026-10-13T19:49:54.0190588Z lastSha=075dc4fc13b4e495c70b0624464fbd3e350951a6 reviewKey=1996e88762126b77b8d02a7fbaafb64bd93765bc02b317e98e55f89aa7e27562 suggestions=S1@13016d35:addressed,S2@73049526:addressed,S3@be6cf7a6:addressed,S4@f79aa9bf:disputed,S5@4c2d0d1e:addressed,S6@5d57b380:addressed,S7@fd1ef691:notaddressed parentRound=17

Exercise the Contoso setup workflow for enabled and disabled feature states across all five fixed asset posting groups. Add the FI demo-data dependency and grant the test app access to Contoso module state.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2d71e872-f1f6-4046-b6ed-80b37c198ee1
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 19

Recommendation: Accept with Suggestions

What this PR does

This moves Finnish depreciation-difference calculation, posting, setup, migration, and tests into a first-party extension. The latest commit replaces the invalid direct demo-generator call with tests that run the public Contoso setup workflow for both feature states.

The tests now select the Fixed Asset module, invoke setup generation through the page action, and let the extension bind its posting-group subscriber around the real generation lifecycle. They verify all five posting groups in both the transitional and clean behavior, while the manifest changes provide the required FI dependency and internal test access. This addresses the remaining demo-data integration issue, and both FI app build variants compile successfully.

Status of previous suggestions
ID Title Status Author response
S1 Keep other companies pending for migration Addressed Completion remains company-specific; other companies retain their pending update.
S2 Guard removed objects in the CLEAN30 build Addressed Runtime references and transition objects remain guarded; legacy schema survives until its separate cleanup.
S3 Avoid duplicate role center report actions Addressed The legacy and extension actions remain mutually exclusive.
S4 Add rendered report layout screenshots Disputed The layout is stated to be unchanged, but there is still no embedded rendered screenshot.
S5 Posting tests omit required company setup Addressed The tests retain general posting setup, an FA class, and document number series.
S6 Preserve the public demo codeunit in its original app Addressed The original public codeunit remains available with pending obsoletion before CLEAN30.
S7 Bind subscriber to the intended publisher Addressed The tests now invoke the Contoso Demo Tool lifecycle, and both FI app build variants compile.
New observations (commits since round 18)

None - the latest commit addresses S7 without introducing another review finding.

Problem-solution fit

Fit: Strong

The delocalization is a coherent feature increment. The updated tests now cover the intended demo-data integration in both enabled and disabled transition states, matching the implementation path.

Risk assessment and necessity

Risk: The change affects G/L posting setup, migrated ledger flags, per-company upgrade tags, and demo-data initialization. The demo-data regression now exercises the real subscriber lifecycle and validates every standard fixed-asset posting group. The changed RDLC still has no rendered screenshot, so its visual result remains unverified.

Necessity: Delocalization is needed to remove Finland-specific functionality from the base application while preserving existing data and upgrade behavior. The latest tests provide the missing integration coverage; adding rendered layout evidence remains a non-blocking review request.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=9326 round=19 by=alexei-dobriansky at=2026-10-14T14:04:33.9788028Z lastSha=5814f3d2d579b07ae8cdb39a94d43d9880137053 reviewKey=7ecf78377767c84c7d7cb357ed6648871d7e43bb9c5049b453293379544c0500 suggestions=S1@13016d35:addressed,S2@73049526:addressed,S3@be6cf7a6:addressed,S4@f79aa9bf:disputed,S5@4c2d0d1e:addressed,S6@5d57b380:addressed,S7@fd1ef691:addressed parentRound=18

djukicmilica and others added 3 commits October 15, 2026 10:35
Keep standalone CLEAN tests on the completed-upgrade path and verify migration through a documented disposable-database upgrade check. Guard obsolete-field mapping logic and imports consistently for CLEANSCHEMA33.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 44f5f2d5-b573-449b-9d7e-40a06615f665
Keep the migration verification harness local rather than shipping it with the extension tests. Preserve the AL test and schema-guard corrections.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 44f5f2d5-b573-449b-9d7e-40a06615f665
@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ Error\ Handling}$

The report raises plain errors when the depreciation-difference accounts are missing from a named FA Posting Group, even though the user can be taken to that related setup record to correct the configuration. Use an ErrorInfo with a Show-it navigation action targeting the identified FA Posting Group.

Knowledge:

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 20

Recommendation: Request Changes

What this PR does

This moves Finnish depreciation-difference calculation, posting, setup, migration, and tests into a first-party extension. The latest commits adjust clean-schema guards, stabilize the Contoso tests, and replace the forced-upgrade migration test with an idempotency test.

The clean-schema guards and Contoso test isolation are consistent with the current object lifecycle. However, the new forced-upgrade test sets the migration tag before it calls the upgrade codeunit, so the production procedure exits before any legacy field is copied. The CLEAN30 financial-data migration is therefore no longer tested.

Status of previous suggestions
ID Title Status Author response
S1 Keep other companies pending for migration Addressed Completion remains company-specific; other companies retain their pending update.
S2 Guard removed objects in the CLEAN30 build Addressed Runtime references and transition objects remain guarded; legacy schema survives until its separate cleanup.
S3 Avoid duplicate role center report actions Addressed The legacy and extension actions remain mutually exclusive.
S4 Add rendered report layout screenshots Disputed The layout is stated to be unchanged, but there is still no embedded rendered screenshot.
S5 Posting tests omit required company setup Addressed The tests retain general posting setup, an FA class, and document number series.
S6 Preserve the public demo codeunit in its original app Addressed The original public codeunit remains available with pending obsoletion before CLEAN30.
S7 Bind subscriber to the intended publisher Addressed The tests continue to invoke the Contoso Demo Tool lifecycle.
New observations (commits since round 19)

S8 (🔴 High): Restore forced migration field-copy coverage
The test sets the upgrade tag before calling UpgradeDepreciationDifferencesFI(), so the procedure exits before any DataTransfer runs. Restore a CLEAN30 test that writes the four legacy fields and verifies they are copied, because this is the forced financial-data migration path.

Problem-solution fit

Fit: Partial

The delocalization remains a coherent feature increment, and the latest schema and Contoso changes fit it. The replacement upgrade test no longer proves that retained legacy financial fields are migrated in the CLEAN30 path.

Risk assessment and necessity

Risk: The change migrates fixed-asset posting accounts, a ledger-entry flag, and the source code used by financial processing. A defect in the forced CLEAN30 migration can leave extension fields empty or wrong, and the current test cannot detect it because it exercises only the already-tagged exit path. The changed RDLC also still has no rendered screenshot.

Necessity: Delocalization is needed to remove Finland-specific functionality from the base application while preserving existing data and upgrade behavior. The schema-guard and test-isolation changes are useful, but direct coverage of the forced migration must remain before this financial upgrade path is safe.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=9326 round=20 by=alexei-dobriansky at=2026-10-15T13:30:31.2913395Z lastSha=1ef503db54ac6b1f0102408ea4858e79924fe2e5 reviewKey=c446d263088f6cc5458f781bc0a7cd44667a6006ca789c595063e6c4144beb7e suggestions=S1@13016d35:addressed,S2@73049526:addressed,S3@be6cf7a6:addressed,S4@f79aa9bf:disputed,S5@4c2d0d1e:addressed,S6@5d57b380:addressed,S7@fd1ef691:addressed,S8@ad871390:new parentRound=19

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: aced1f3f-c55d-4177-bc2d-301bf59ea88b
@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The AL source file name local.permissionset.al does not use the required PascalCase <ObjectName>.<ObjectType>.al convention for the LOCAL permission set. Rename it to LOCAL.PermissionSet.al so the file name unambiguously identifies the object and type.

Knowledge:

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

The AL source file name localread.permissionset.al does not use the required PascalCase <ObjectName>.<ObjectType>.al convention for the LOCAL READ permission set. Rename it to LOCALREAD.PermissionSet.al so the file name unambiguously identifies the object and type.

Knowledge:

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 21

Recommendation: Request Changes

What this PR does

This moves Finnish depreciation-difference calculation, posting, setup, migration, and tests into a first-party extension. The latest commit initializes the Contoso demo setup record before rebuilding localization account mappings, which supplies the FI defaults that the single-instance mapping needs.

The test initialization change is correctly placed and does not alter production behavior. It does not change the CLEAN30 migration test, so the forced field-copy path remains uncovered.

Status of previous suggestions
ID Title Status Author response
S1 Keep other companies pending for migration Addressed Completion remains company-specific; other companies retain their pending update.
S2 Guard removed objects in the CLEAN30 build Addressed Runtime references and transition objects remain guarded; legacy schema survives until its separate cleanup.
S3 Avoid duplicate role center report actions Addressed The legacy and extension actions remain mutually exclusive.
S4 Add rendered report layout screenshots Disputed The layout is stated to be unchanged, but no rendered screenshot is embedded.
S5 Posting tests omit required company setup Addressed The tests retain general posting setup, an FA class, and document number series.
S6 Preserve the public demo codeunit in its original app Addressed The original public codeunit remains available with pending obsoletion before CLEAN30.
S7 Bind subscriber to the intended publisher Addressed The tests continue to invoke the Contoso Demo Tool lifecycle.
S8 Restore forced migration field-copy coverage Not addressed The latest commit changes only Contoso test initialization. The forced-upgrade test still sets the migration tag before calling the upgrade codeunit.
New observations (commits since round 20)

None - the latest commit only initializes the Contoso setup record before refreshing test mappings.

Problem-solution fit

Fit: Partial

The delocalization remains a coherent feature increment, and the latest test initialization change fits it. The CLEAN30 test still does not prove that retained legacy financial fields are copied by the forced migration path.

Risk assessment and necessity

Risk: The change migrates fixed-asset posting accounts, a ledger-entry flag, and the source code used by financial processing. A defect in the forced CLEAN30 migration can leave extension fields empty or wrong, while the current test exits before the copy runs. The changed RDLC also still lacks rendered visual evidence.

Necessity: Delocalization is needed to remove Finland-specific functionality from the base application while preserving existing data and upgrade behavior. Initializing the Contoso setup record makes the affected tests more reliable, but direct coverage of the forced migration remains necessary before the financial upgrade path is safe.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=9326 round=21 by=alexei-dobriansky at=2026-10-16T02:06:59.7105309Z lastSha=e1f3cae49a53d01d483df871bdabe6556a8079ea reviewKey=1dc451c77d50d091dee507cad13ddee9bc9b708da246897099349ef209e29833 suggestions=S1@13016d35:addressed,S2@73049526:addressed,S3@be6cf7a6:addressed,S4@f79aa9bf:disputed,S5@4c2d0d1e:addressed,S6@5d57b380:addressed,S7@fd1ef691:addressed,S8@ad871390:notaddressed parentRound=20

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b9f1b46-c4a9-45fc-ae54-e0fc9bb6c770
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 22

Recommendation: Request Changes

What this PR does

This moves Finnish depreciation-difference calculation, posting, setup, migration, and tests into a first-party extension. The latest commit changes the Contoso fixed-asset demo tests to generate the selected module and its setup dependencies instead of injecting synthetic account mappings.

The revised test path is sound. Clearing module progress makes the page rebuild its module state, the Fixed Asset filter lets dependency resolution generate the required setup, and the assertions verify both generated G/L accounts and posting-group assignments. This commit does not change the forced CLEAN30 migration test, so the open migration-coverage blocker remains.

Status of previous suggestions
ID Title Status Author response
S1 Keep other companies pending for migration Addressed Completion remains company-specific; other companies retain their pending update.
S2 Guard removed objects in the CLEAN30 build Addressed Runtime references and transition objects remain guarded; legacy schema survives until its separate cleanup.
S3 Avoid duplicate role center report actions Addressed The legacy and extension actions remain mutually exclusive.
S4 Add rendered report layout screenshots Disputed The layout is stated to be unchanged, but no rendered screenshot is embedded.
S5 Posting tests omit required company setup Addressed The tests retain general posting setup, an FA class, and document number series.
S6 Preserve the public demo codeunit in its original app Addressed The original public codeunit remains available with pending obsoletion before CLEAN30.
S7 Bind subscriber to the intended publisher Addressed The tests continue to invoke the Contoso Demo Tool lifecycle.
S8 Restore forced migration field-copy coverage Not addressed The latest commit only changes Contoso demo test setup. The forced-upgrade test still sets the migration tag before calling the upgrade codeunit.
New observations (commits since round 21)

None - the latest commit makes the existing Contoso demo tests use the real dependency-generated setup.

Problem-solution fit

Fit: Partial

The delocalization remains a coherent feature increment, and the latest test change better represents real demo-data generation. The CLEAN30 test still does not prove that retained legacy financial fields are copied by the forced migration path.

Risk assessment and necessity

Risk: The latest commit changes test setup only and reduces reliance on synthetic mappings. The main remaining risk is the forced upgrade of fixed-asset posting accounts, the ledger-entry flag, and the financial source code: the current test exits before any DataTransfer runs. The changed RDLC also still lacks rendered visual evidence.

Necessity: Delocalization is needed to remove Finland-specific functionality from the base application while preserving existing data and upgrade behavior. The more realistic Contoso tests are useful, but direct coverage of the forced financial migration remains necessary before this upgrade path is safe.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=9326 round=22 by=alexei-dobriansky at=2026-10-16T07:42:10.2128890Z lastSha=220a24d86b301c16abad5699678d6f72e2d2a17d reviewKey=d7a7e9fcb6f13eb8d96bb4497ffd47604dc651a0c4d176a27ecd234ff3f95a2a suggestions=S1@13016d35:addressed,S2@73049526:addressed,S3@be6cf7a6:addressed,S4@f79aa9bf:disputed,S5@4c2d0d1e:addressed,S6@5d57b380:addressed,S7@fd1ef691:addressed,S8@ad871390:notaddressed parentRound=21

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f8517b0d-6564-4ee4-bea0-059b6b4a9164
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 23

Recommendation: Request Changes

What this PR does

This moves Finnish depreciation-difference calculation, posting, setup, migration, and tests into a first-party extension. The latest commit adds the new posting-buffer table to the LOCAL permission set and adds a regression test for that effective permission.

The new extension restores full buffer access for users assigned LOCAL. It does not preserve the separate read access that the old buffer received through the assignable LOCAL READ permission set, so the permission move is not complete.

Status of previous suggestions
ID Title Status Author response
S1 Keep other companies pending for migration Addressed Completion remains company-specific; other companies retain their pending update.
S2 Guard removed objects in the CLEAN30 build Addressed Runtime references and transition objects remain guarded; legacy schema survives until its separate cleanup.
S3 Avoid duplicate role center report actions Addressed The legacy and extension actions remain mutually exclusive.
S4 Add rendered report layout screenshots Disputed The layout is stated to be unchanged, but no rendered screenshot is embedded.
S5 Posting tests omit required company setup Addressed The tests retain general posting setup, an FA class, and document number series.
S6 Preserve the public demo codeunit in its original app Addressed The original public codeunit remains available with pending obsoletion before CLEAN30.
S7 Bind subscriber to the intended publisher Addressed The tests continue to invoke the Contoso Demo Tool lifecycle.
S8 Restore forced migration field-copy coverage Not addressed The forced-upgrade test still sets the migration tag before calling the upgrade codeunit, so the field-copy path exits without running.
New observations (commits since round 22)

S9 (🟠 Moderate): Preserve access through LOCAL READ
The legacy buffer granted R through the assignable LOCAL READ permission set, but the new extension only extends LOCAL. Add a LOCAL READ permission-set extension for the new table so read-only users keep the access they had before delocalization.

Problem-solution fit

Fit: Partial

The delocalization remains a coherent feature increment, and the latest commit restores normal LOCAL access to the moved buffer. The forced financial migration is still not exercised, and the read-only permission path is not preserved.

Risk assessment and necessity

Risk: Users assigned only LOCAL READ can lose access to the moved posting buffer. The larger remaining risk is the forced upgrade of fixed-asset posting accounts, the ledger-entry flag, and the financial source code because the current test exits before any DataTransfer runs. The changed RDLC also still lacks rendered visual evidence.

Necessity: Delocalization is needed to remove Finland-specific functionality from the base application while preserving existing data, permissions, and upgrade behavior. The LOCAL fix is necessary, but the read-only permission and forced migration paths also need to remain compatible.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=9326 round=23 by=alexei-dobriansky at=2026-10-17T01:25:02.3411329Z lastSha=c099b89c449bff3e2bdec3a83a01ddc47ed3a47e reviewKey=f2fae506d6c2492c6cdfbef846c14d4e6553b6aeebec0d93e0b403526b2a7fa0 suggestions=S1@13016d35:addressed,S2@73049526:addressed,S3@be6cf7a6:addressed,S4@f79aa9bf:disputed,S5@4c2d0d1e:addressed,S6@5d57b380:addressed,S7@fd1ef691:addressed,S8@ad871390:notaddressed,S9@f5d448d0:new parentRound=22

djukicmilica and others added 2 commits October 17, 2026 11:09
This reverts commit c099b89.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f8517b0d-6564-4ee4-bea0-059b6b4a9164
Use a non-writing feature lookup and check the report ID before evaluating the feature. Add regression coverage for unrelated reports and enabled, disabled, and pending feature states.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f8517b0d-6564-4ee4-bea0-059b6b4a9164

This branch was successfully deployed

1 active (outdated) deployment
triage — c79f1d5d Deployed Oct 12, 2026 by djukicmilica via Classify team ownership #7488
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 Build: scripts & configs Build scripts and configuration files Team: Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants