Skip to content

[master] [All-e]Fixed asset report 5607 and 4413 showing different results with Declining Balance#8324

Open
neeleshsinghal wants to merge 10 commits into
microsoft:mainfrom
neeleshsinghal:bugs/Bug-636727-Fixed-asset-report-5607-and-4413-showing-different-results
Open

[master] [All-e]Fixed asset report 5607 and 4413 showing different results with Declining Balance#8324
neeleshsinghal wants to merge 10 commits into
microsoft:mainfrom
neeleshsinghal:bugs/Bug-636727-Fixed-asset-report-5607-and-4413-showing-different-results

Conversation

@neeleshsinghal
Copy link
Copy Markdown
Contributor

@neeleshsinghal neeleshsinghal commented May 27, 2026

Workitem :

Bug 636727: [master] [All-e]Fixed asset report 5607 and 4413 showing different results with Declining Balance

Fixes AB#636727

Issue: Report 4413 (Fixed Asset Projected Value Excel) calculates incorrect depreciation amounts for Declining Balance assets when projections cross a fiscal year boundary, producing different results than the legacy Report 5607.

Cause: In InsertProjectedEntries, EntryAmounts[3] (depreciation in fiscal year) was reset to 0 before accumulating the current period's depreciation, causing the last fiscal period's depreciation to spill into the new fiscal year and shifting the effective book value base back one month.

Solution: Moved AccumulateProjectionEntryAmounts before the fiscal year boundary reset (EntryAmounts[3] := 0) so the outgoing fiscal year's final depreciation is fully accumulated before the counter resets, matching the order used in Report 5607.

@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item labels May 27, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone May 27, 2026
@JesperSchulz JesperSchulz added the Finance GitHub request for Finance area label May 27, 2026
@neeleshsinghal neeleshsinghal marked this pull request as ready for review May 28, 2026 12:05
@neeleshsinghal neeleshsinghal requested a review from a team as a code owner May 28, 2026 12:05
Comment thread src/Apps/W1/ExcelReports/Test/src/FixedAssetExcelReports.Codeunit.al Outdated
djukicmilica
djukicmilica previously approved these changes May 28, 2026
Copy link
Copy Markdown
Contributor

@AleksanderGladkov AleksanderGladkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neeleshsinghal please also add description to the PR

Comment thread src/Apps/W1/ExcelReports/Test/src/FixedAssetExcelReports.Codeunit.al Outdated
Comment thread src/Apps/W1/ExcelReports/Test/src/FixedAssetExcelReports.Codeunit.al Outdated
Comment thread src/Apps/W1/ExcelReports/Test/src/FixedAssetExcelReports.Codeunit.al Outdated
Comment thread src/Apps/W1/ExcelReports/Test/src/FixedAssetExcelReports.Codeunit.al Outdated
Comment thread src/Apps/W1/ExcelReports/Test/src/FixedAssetExcelReports.Codeunit.al Outdated

// [GIVEN] Create Accounting periods with fiscal year.
CleanupFixedAssetData();
AccountingPeriod.DeleteAll();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

AccountingPeriod.DeleteAll() affects global test state

AccountingPeriod.DeleteAll() removes every accounting period from the database at the start of the test. Other tests that expect accounting periods to exist and run concurrently (or after, if a Commit() persists this) may fail or produce wrong results.

Recommendation:

  • Restrict the deletion to only the periods created by this test, or call DeleteAll() only on a filtered record set. Also ensure cleanup is mirrored in a TearDown or within the same transaction boundary.
Suggested change
AccountingPeriod.DeleteAll();
// Filter to only the fiscal year range created for this test:
AccountingPeriod.SetRange("Starting Date", CalcDate('<-1Y>', FiscalYearStartDate),
CalcDate('<' + Format(NumberOfMonths) + 'M>', FiscalYearStartDate));
AccountingPeriod.DeleteAll();

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why


// The next projected month should be based on this closing book value.
ExpectedProjectedDepr := -Round(BookValueAfterDepr * DecliningBalancePct / 100 / 12, 1);
Commit();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Commit() in test breaks isolation

Calling Commit() inside a test procedure persists all changes to the database, bypassing the automatic transaction rollback that test isolation relies on. If a subsequent assertion fails, the FA journal entries, fixed asset, and accounting periods created in the test will remain in the database and can pollute other tests.

Recommendation:

  • Refactor the test to avoid Commit(). If the report runner requires committed data, use a [TransactionModel(TransactionModel::AutoCommit)] attribute or restructure the setup so committed data is cleaned up in a TearDown procedure.
Suggested change
Commit();
// Replace the bare Commit() with explicit cleanup or use TransactionModel:
[Test]
[TransactionModel(TransactionModel::AutoCommit)]
[HandlerFunctions('EXRFixedAssetProjectedHandler')]
procedure ProjectedValueDeclBalShouldUseCorrectBookValueAcrossFiscalYear()

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

BasePostingDate: Date;
DeprStartDate: Date;
FirstReportDeprDate: Date;
FiscalYearStartDate: Date;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

AccountingPeriod.DeleteAll() pollutes test data

AccountingPeriod.DeleteAll() is called inside the test body without being wrapped in a LibrarySetupStorage.Save/Restore or equivalent cleanup. If the test fails mid-way, or if other tests running in the same session rely on existing accounting periods, the global table state is permanently corrupted for that test run.

Recommendation:

  • Move accounting-period setup and teardown into the Initialize / cleanup helpers, or use LibraryFiscalYear to ensure periods are restored after the test. At minimum, add AccountingPeriod.DeleteAll() to CleanupFixedAssetData with an if IsInitialized guard.
Suggested change
FiscalYearStartDate: Date;
// In CleanupFixedAssetData or a dedicated TearDown:
AccountingPeriod.DeleteAll();
LibraryFiscalYear.CreateFiscalYear();

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

end;

[RequestPageHandler]
procedure EXRFixedAssetProjectedHandler(var EXRFixedAssetProjected: TestRequestPage "EXR Fixed Asset Projected")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Missing newline at end of file

The diff shows \ No newline at end of file for FixedAssetExcelReports.Codeunit.al. AL source files should end with a trailing newline for consistent diff output and to avoid spurious whitespace changes in future edits.

Recommendation:

  • Add a single newline character after the closing } of the codeunit.
Suggested change
procedure EXRFixedAssetProjectedHandler(var EXRFixedAssetProjected: TestRequestPage "EXR Fixed Asset Projected")
}

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

⚠️ Stale Status Check Deleted

The Pull Request Build workflow run for this PR was older than 72 hours and has been deleted.

📋 Why was it deleted?

Status checks that are too old may no longer reflect the current state of the target branch. To ensure this PR is validated against the latest code and passes up-to-date checks, a fresh build is required.


🔄 How to trigger a new status check:

  1. 📤 Push a new commit to the PR branch, or
  2. 🔁 Close and reopen the PR

This will automatically trigger a new Pull Request Build workflow run.

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 Finance GitHub request for Finance area From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants