Skip to content

Expense Agent: Add VAT reclaim approval workflow - #10062

Open
Alexander Yakunin (Alexander-Ya) wants to merge 3 commits into
mainfrom
migrate/ado-pr-251251
Open

Expense Agent: Add VAT reclaim approval workflow#10062
Alexander Yakunin (Alexander-Ya) wants to merge 3 commits into
mainfrom
migrate/ado-pr-251251

Conversation

@Alexander-Ya

@Alexander-Ya Alexander Yakunin (Alexander-Ya) commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What & why

Problem

Expense Agent did not support accountant approval or rejection of VAT reclaim requests on individual expense report VAT specification lines. VAT reclaim status was not surfaced consistently in expense report pages and statistics.

Solution

This PR adds the VAT reclaim approval workflow for Expense Agent:

  • Adds the Expense Reclaim Status enum and reclaim status tracking.
  • Adds approval and rejection actions for expense report VAT specification lines.
  • Adds VAT specification pages for current and posted expense reports.
  • Extends expense report statistics with:
    • VAT amount
    • Refundable amount
    • Approved reclaim VAT
    • VAT specification details
  • Updates expense report posting so VAT reclaim amounts are applied only when reclaim status is Approved.
  • Integrates VAT specification and statistics views into expense report, manager approval, and accountant pages.
  • Updates Expense Agent permissions and application metadata.

Migration Notes

  • Source Azure DevOps PR: 251251
  • Migrated to the current BCApps layout under src/Apps/W1/ExpenseAgent.
  • Legacy App/Internal/Apps/ExpenseAgent paths were removed.

Validation

  • Resolved path, rename, and content conflicts.
  • Removed all merge conflict markers.
  • Focused diagnostics passed for the repaired VAT specification page.
  • git diff --check passed.
  • Migration branch is clean and pushed to GitHub.

Linked work

Fixes AB#626087

How I validated this

  • I read the full diff and it contains only changes I intended.
  • I built the affected app(s) locally with no new analyzer warnings.
  • I ran the change in Business Central and confirmed it behaves as expected.
  • I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)

Risk & compatibility

Copilot AI left a comment

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.

Pull request overview

Adds VAT reclaim approval workflow and improved VAT visibility for Expense Agent expense reports, including VAT specification views and updated posting/statistics behavior.

Changes:

  • Introduces reclaim-status tracking for VAT spec lines and adds approve/reject actions.
  • Adds/updates VAT specification and statistics pages for current and posted expense reports.
  • Updates posting logic to apply reclaim-related VAT handling based on reclaim status.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/Apps/W1/ExpenseAgent/app/src/Permissions/ExpenseManagementObjects.permissionset.al Updates object permissions to align with the new/renamed VAT specification and statistics pages.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Tables/ExpenseReportLineVATSpec.Table.al Adjusts reclaim-status behavior when reclaimability/% changes and updates reclaim-status validation flow.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpRepVATSpecification.Page.al Introduces/renames a posted VAT specification ListPart used from posted statistics.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpReportStatistics.Page.al Reworks posted statistics page layout and adds VAT specification subpage integration.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpReportFactBox.Page.al Adds a posted expense report FactBox page.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpRepLineVATSpec.Page.al Removes obsolete reclaim-approved field from posted VAT spec line page.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpenseReport.Page.al Updates FactBox part and statistics action to point to updated posted statistics page.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/ExpenseReportVATSpec.Page.al Renumbers/adjusts VAT spec ListPart used as a FactBox for current reports.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/ExpenseReportStatistics.Page.al Reworks current report statistics page layout and integrates VAT specification subpage.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/ExpenseReports.Page.al Updates FactBox and statistics action target; removes list-page posting UI.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/ExpenseReportList.Page.al Removes obsolete statistics FactBox part.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/ExpenseReportLineVATSpec.Page.al Tidies promoted action refs and removes obsolete reclaim-approved field.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/ExpenseReportLines.Page.al Switches VAT specification FactBox to the updated VAT spec ListPart page.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/ExpenseReportFactBox.Page.al Renumbers the expense report FactBox page.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/ExpenseReport.Page.al Updates FactBox and statistics action; reorganizes release/reopen actions.
src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Codeunits/ExpenseReportPost.Codeunit.al Changes posting behavior for VAT spec lines to respect reclaim status during posting.
src/Apps/W1/ExpenseAgent/app/src/Expense/Pages/AccountantExpenseReports.Page.al Removes obsolete statistics FactBox part and updates statistics action target.
src/Apps/W1/ExpenseAgent/app/src/Common/Enums/ExpenseReclaimStatus.Enum.al Updates reclaim status enum values (pending/approved/rejected semantics).
src/Apps/W1/ExpenseAgent/app/src/Approval/Pages/ManagerExpenseReports.Page.al Removes obsolete statistics FactBox part.
src/Apps/W1/ExpenseAgent/app/src/Approval/Pages/ManagerExpenseReport.Page.al Removes obsolete statistics FactBox part and updates statistics action target.
Suppressed comments (2)

src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpRepVATSpecification.Page.al:11

  • Page object ID 7093 is already used by page "Posted Expense Report VAT Spec" (PostedExpenseReportVATSpec.Page.al). Keeping this at 7093 will cause a duplicate page ID compile error.
    src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Codeunits/ExpenseReportPost.Codeunit.al:744
  • VAT base/amount fields are only populated when Reclaim Status is Approved. For Rejected (i.e., Reclaim % = 0) this posts a net-only amount with 0 VAT, understating the expense and losing the VAT entry. Populate VAT fields for both Approved and Rejected, and use Non-Deductible VAT % (based on Reclaim %) to control deductibility.
        if ExpenseReportLineVATSpec."Reclaim Status" = ExpenseReportLineVATSpec."Reclaim Status"::"Approved" then begin
            GenJournalLine."VAT Base Amount" := ExpenseReportLineVATSpec."VAT Base Amount";
            GenJournalLine."VAT Base Amount (LCY)" := ExpenseReportLineVATSpec."VAT Base Amount (LCY)";
            GenJournalLine."VAT Amount" := ExpenseReportLineVATSpec."VAT Amount";
            GenJournalLine."VAT Amount (LCY)" := ExpenseReportLineVATSpec."VAT Amount (LCY)";

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

// ------------------------------------------------------------------------------------------------
namespace Microsoft.ExpenseAgent;

page 7094 "Posted Exp. Report FactBox"
Comment on lines +14 to 17
value(0; " ")
{
Caption = 'Pending';
Caption = ' ';
}
Comment on lines 179 to 182
if not Reclaimable then begin
Validate("Reclaim %", 0);
"Reclaim Status" := "Reclaim Status"::Rejected;
"Reclaim Status" := "Reclaim Status"::" ";
end;
namespace Microsoft.ExpenseAgent;

page 7098 "Expense Report FactBox"
page 7092 "Expense Report FactBox"
Comment on lines 201 to 203
if "Reclaim %" <> xRec."Reclaim %" then
"Reclaim Status" := "Reclaim Status"::"Pending";
"Reclaim Status" := "Reclaim Status"::" ";

Comment on lines +709 to +710
if ExpenseReportLineVATSpec."Reclaim Status" = ExpenseReportLineVATSpec."Reclaim Status"::" " then
error(NotApprovedForVATReclaimErr, ExpenseReportLineVATSpec."Expense Category", ExpenseReportLineVATSpec."Expense Subcategory");
Comment on lines 613 to 614
protected var
SubmitEnabled: Boolean;
Extensible = false;

value(0; "Pending")
value(0; " ")

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\ —\ Agent}$

Enum 6988 "Expense Reclaim Status" value 0 was renamed from Pending (Caption 'Pending') to " " (Caption ' '). The "Reclaim Status" field on ExpenseReportLineVATSpec.Page.al still carries the ToolTip 'Specifies whether the VAT reclaim for this row is pending, approved, or rejected.' — but a row in the pending state will now render as a blank field instead of showing 'Pending', contradicting the field's own documented meaning and leaving users unable to visually distinguish an unprocessed row from an empty/uninitialized one (the field also drives StyleExpr = ReclaimStatusStyle, which likely also loses its ability to visually flag the pending state). Give the enum's blank member a real, descriptive caption (e.g. 'Pending') to match the field's ToolTip and any style logic that depends on distinguishing the state.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

    value(0; "Pending")
    {
        Caption = 'Pending';
    }

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

ObsoleteReason = 'Replaced by page Expense Report FactBox';
ObsoleteState = Pending;
ObsoleteTag = '29.0';
DeleteAllowed = false;

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{🟠\ High\ Severity\ —\ Breaking\ Changes}$

Page 6982 "Expense Report Statistics" was already marked obsolete on origin/main (wrapped in #if not CLEAN29, ObsoleteState = Pending, ObsoleteTag = '29.0', replaced by "Expense Report FactBox"). This PR removes that obsolete marking and repurposes the same object as the new live statistics page (PageType changed to ListPlus with all-new fields/groups). Obsolete objects are one-way migration shims that should only shrink toward removal, not be revived and repurposed with new behavior — extensions or customizations that relied on the object being frozen/obsolete during the deprecation window can break. Keep the obsolete object frozen and put the new statistics implementation on a fresh, non-obsolete object.

Knowledge:

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

/// Used as a FactBox on the Expense Report Lines page.
/// </summary>
page 7096 "Expense Report VAT Spec."
page 7086 "Expense Report VAT Spec."

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{🟠\ High\ Severity\ —\ Breaking\ Changes}$

The rename collapses two existing page identities into one. On origin/main, page 7086 was "Expense Report Line VATFactBox" and a separate page 7096 area held "Expense Report VAT Spec."; this diff renames/repurposes the file so only page 7086 "Expense Report VAT Spec." survives, with no obsolete shim left for dependents of the previous public page. Any extension referencing the previous object breaks immediately on upgrade. Keep the old page object present with ObsoleteState = Pending (or equivalent) and introduce the replacement under a new, unused page ID instead of repurposing an existing public ID in place.

Knowledge:

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

page 6978 "Posted Exp. Report Statistics"
{
PageType = CardPart;
Caption = 'Posted Expense Report Statistics';

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{🟠\ High\ Severity\ —\ Breaking\ Changes}$

Page 6978 "Posted Exp. Report Statistics" was already marked obsolete on origin/main behind #if not CLEAN29 (ObsoleteState = Pending, ObsoleteTag = '29.0'). This PR strips the obsolete marking and repurposes the same object as the live posted-statistics page with new fields/groups. As with the sibling "Expense Report Statistics" page, building new behavior onto an object already on the deletion path is a breaking-change anti-pattern. Keep the obsolete page unchanged and move the new implementation to a non-obsolete object.

Knowledge:

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

Extensible = false;

value(0; "Pending")
value(0; " ")

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\ —\ Breaking\ Changes}$

Enum 6988 "Expense Reclaim Status" (Extensible = false) renames value 0 from the named member Pending to a blank/space-named member " ". Confirmed by direct inspection that src/Apps/W1/ExpenseAgent/test/src/ExpensePostingVATTest.Codeunit.al:828 still references ExpenseReportLineVATSpec."Reclaim Status"::"Pending", a symbol that no longer exists after this rename. Severity is capped at minor as an agent finding, but the real impact is build-breaking/blocker-level for that caller, and more generally this is a breaking rename of a published enum member on a non-extensible enum. Either keep the Pending member name (only changing its Caption if a blank display value is desired) or update every caller in the same change.

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

namespace Microsoft.ExpenseAgent;

page 7098 "Expense Report FactBox"
page 7092 "Expense Report FactBox"

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\ —\ Breaking\ Changes}$

Confirmed by direct inspection: this diff changes "Expense Report FactBox" from page 7098 to page 7092, but page 7092 already exists, unchanged, as "Expense Projects API" (src/Apps/W1/ExpenseAgent/app/src/APIs/Projects/ExpenseProjectsAPI.Page.al) in the same app. Two objects now declare the same page ID. Severity is capped at minor because this is an agent finding without a dedicated knowledge-file citation, but the real impact is build-breaking/blocker-level: the app cannot compile until one of the two objects uses a different, unused ID. This defect was independently reported (and cross-verified) by both the breaking-changes and upgrade review passes.

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

// ------------------------------------------------------------------------------------------------
namespace Microsoft.ExpenseAgent;

page 7094 "Posted Exp. Report FactBox"

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\ —\ Breaking\ Changes}$

Confirmed by direct inspection: this diff adds a new page 7094 "Posted Exp. Report FactBox" (src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpReportFactBox.Page.al), but page 7094 already exists, unchanged, as "Posted Expense Report FactBox" (src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpenseReportFactBox.Page.al). Two objects now declare page ID 7094. Severity is capped at minor as an agent finding, but the real impact is build-breaking/blocker-level: the ID collision prevents compilation until one object is renumbered or the older duplicate is removed/obsoleted.

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

/// Used as a part on the Posted Expense Report Statistics page.
/// </summary>
page 7086 "Expense Report Line VATFactBox"
page 7093 "Posted ExpRep VATSpecification"

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\ —\ Breaking\ Changes}$

Confirmed by direct inspection: this diff renames page 7086 "Expense Report Line VATFactBox" to page 7093 "Posted ExpRep VATSpecification" (git-detected rename, 62% similarity). Page 7093 already exists, unchanged, as "Posted Expense Report VAT Spec" (src/Apps/W1/ExpenseAgent/app/src/ExpenseReport/Pages/PostedExpenseReportVATSpec.Page.al). Two objects now declare page ID 7093. Severity is capped at minor as an agent finding, but the real impact is build-breaking/blocker-level: one of the two objects must be renumbered, renamed, or the pre-existing duplicate removed/obsoleted before the app can compile.

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

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling}$

In CreateGenJournalLineFromSpec (ExpenseReportPost.Codeunit.al), the previous version branched on Reclaim Status into an Approved path and an explicit non-approved/Rejected path (which posted the gross amount including VAT, used Automatic VAT Entry, and cleared VAT posting groups). The new version only special-cases Reclaim Status::Approved and has no else branch at all; nothing upstream filters out Rejected rows before this call (verified: no SetRange/SetFilter on "Reclaim Status" anywhere between the FindSet loops and this procedure). For a Rejected VAT-spec row, the journal line is therefore left with only the unconditional net "VAT Base Amount" as Amount, non-blank VAT posting groups, VAT Posting = Manual VAT Entry, but VAT Amount/VAT % left at zero — silently dropping the VAT portion from the GL posting instead of the old gross-amount handling, with no error and no equivalent compensating logic. This looks like a genuine, concrete loss of correctness for rejected VAT-reclaim lines. Restore an explicit branch (or equivalent handling) for Reclaim Status::Rejected.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        if ExpenseReportLineVATSpec."Reclaim Status" = ExpenseReportLineVATSpec."Reclaim Status"::"Approved" then begin
            GenJournalLine."VAT Base Amount" := ExpenseReportLineVATSpec."VAT Base Amount";
            GenJournalLine."VAT Base Amount (LCY)" := ExpenseReportLineVATSpec."VAT Base Amount (LCY)";
            GenJournalLine."VAT Amount" := ExpenseReportLineVATSpec."VAT Amount";
            GenJournalLine."VAT Amount (LCY)" := ExpenseReportLineVATSpec."VAT Amount (LCY)";
            GenJournalLine."VAT %" := ExpenseReportLineVATSpec."VAT %";

            // VAT Reclaim % is used to route the non-deductible portion of VAT to the correct accounts via VAT Posting Setup.
            if (ExpenseReportLineVATSpec."Reclaim %" <> 100) and (ExpenseReportLineVATSpec."VAT Amount" <> 0) then begin
                VATSetup.Get();
                VATSetup.TestField("Non-Deductible VAT Is Enabled");
                GenJournalLine.Validate("Non-Deductible VAT %", 100 - ExpenseReportLineVATSpec."Reclaim %");
            end;
        end else begin
            // VAT is not reclaimable: include VAT in the expense amount (gross) and do not create a VAT entry.
            GenJournalLine.Amount := ExpenseReportLineVATSpec."VAT Base Amount" + ExpenseReportLineVATSpec."VAT Amount";
            GenJournalLine."Amount (LCY)" := ExpenseReportLineVATSpec."VAT Base Amount (LCY)" + ExpenseReportLineVATSpec."VAT Amount (LCY)";

            GenJournalLine."VAT Posting" := GenJournalLine."VAT Posting"::"Automatic VAT Entry";
            GenJournalLine."VAT %" := 0;
            GenJournalLine."VAT Amount" := 0;
            GenJournalLine."VAT Amount (LCY)" := 0;
            GenJournalLine."VAT Base Amount" := 0;
            GenJournalLine."VAT Base Amount (LCY)" := 0;
            GenJournalLine."VAT Bus. Posting Group" := '';
            GenJournalLine."VAT Prod. Posting Group" := '';
            GenJournalLine."Gen. Posting Type" := GenJournalLine."Gen. Posting Type"::" ";
        end;

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

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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

This PR removes the published 'Post' action from 'Expense Reports' together with its 'Category_Posting' split-button promotion. Removing an existing promotion/action on a shipped list page is a breaking UI change, and this list page no longer exposes any same-page batch/manual posting entry point for selected reports (the 'Expense Report Batch Post Mgt.' codeunit permission entry was also dropped, corroborating the entry point is gone, not just moved).

Knowledge:

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

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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

Enum 6988 "Expense Reclaim Status" renames value(0) from 'Pending' to a blank identifier ' '. Even with Extensible = false, any dependent AL code or automation compiled against Enum::"Expense Reclaim Status"::Pending will stop compiling. The Expense Agent app.json declares version 29.0.0.0 '(Preview)', which suggests the app has not yet shipped and this rename is likely low-risk in practice, but this could not be conclusively confirmed from the diff alone; also, using a literal space as both the enum member's caption and identifier is an unusual pattern that reduces readability (e.g., in code review, debugger output, and telemetry) compared to keeping a named value with a blank/neutral caption assigned separately.

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

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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

IMPACT NOTE: this would otherwise be rated Critical/blocker — flagged as minor only per agent-finding severity cap. In CreateGenJournalLineFromSpec (Expense Report-Post codeunit), the previous 'else' branch that handled non-reclaimable ('Rejected'/not-Approved) VAT spec rows was deleted without replacement. That branch used to: (1) set GenJournalLine.Amount/Amount(LCY) to the GROSS amount (VAT Base + VAT), (2) set VAT Posting to 'Automatic VAT Entry', (3) zero out VAT %/VAT Amount/VAT Base Amount fields, and (4) clear VAT Bus./Prod. Posting Group and Gen. Posting Type. In the new code, 'VAT Posting' := 'Manual VAT Entry', 'VAT Calculation Type', and both VAT Bus./Prod. Posting Groups are now set UNCONDITIONALLY (outside the 'if Approved' guard), while GenJournalLine.Amount/Amount(LCY) are set only to the net VAT Base Amount (never gross) and the VAT Amount/VAT Base Amount/VAT% fields are only populated inside the 'if Approved' branch. For any row whose Reclaim Status is not exactly 'Approved' (i.e. Rejected, and now also the renamed blank/pending value never reaches this point because it errors earlier), the resulting Gen. Journal Line will post only the net amount (excluding VAT) under 'Manual VAT Entry' with valid-looking VAT posting groups but zero VAT Amount/VAT% — silently dropping the VAT portion from the posted expense amount and posting with inconsistent/incomplete VAT entry metadata instead of the previous, intentional gross no-VAT-entry treatment. This is a genuine posting/financial-correctness regression, not a refactor-neutral change, and should be fixed by restoring an explicit non-Approved branch before this PR merges.

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

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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

This PR renumbers page objects 7098->7070 (Expense Report FactBox), 7096->7086 (Expense Report VAT Spec.), and 7094->7072 (Posted Expense Report FactBox), while also reusing freed IDs 7086 and 7094 for different pages in the same change. Expense Agent's app.json shows version 29.0.0.0 '(Preview)', suggesting the app is pre-release, which would make this renumbering low-risk; if any customer or partner code already depends on the old IDs, this would break immediately upon upgrade.

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

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

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

IMPACT NOTE: this would otherwise be rated High — flagged as minor only per agent-finding severity cap; this finding was independently produced by the al-data-modeling-review sub-skill and is preserved here. The simplified 'Reclaim Status' OnValidate trigger on 'Expense Report Line VAT Spec.' no longer restores the selected status after its recursive Rec.Validate("Reclaim %", ...) call. 'Reclaim %' OnValidate resets 'Reclaim Status' to the blank value whenever the percentage changes ('if "Reclaim %" <> xRec."Reclaim %" then "Reclaim Status" := "Reclaim Status"::" ";'). Selecting 'Rejected' calls Rec.Validate("Reclaim %", 0), which — whenever the row's reclaim % was previously non-zero — changes the percentage and immediately resets 'Reclaim Status' back to blank, silently undoing the user's rejection. The previous implementation captured the intended status in a local variable and reassigned it after the case statement specifically to survive this recursive reset; that reassignment was removed in this PR without an equivalent safeguard.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

trigger OnValidate()
var
    NewReclaimStatus: Enum "Expense Reclaim Status";
begin
    NewReclaimStatus := "Reclaim Status";
    case "Reclaim Status" of
        "Reclaim Status"::Approved:
            Rec.Validate("Reclaim %");
        "Reclaim Status"::Rejected:
            Rec.Validate("Reclaim %", 0);
    end;
    "Reclaim Status" := NewReclaimStatus;
    Rec."Reclaim Approved By" := CopyStr(UserId(), 1, MaxStrLen(Rec."Reclaim Approved By"));
    Rec."Reclaim Approved At" := CurrentDateTime();
end;

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

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ Breaking\ Changes}$

Page 6982 "Expense Report Statistics" was already marked obsolete (ObsoleteState=Pending, ObsoleteTag='29.0') and wrapped in '#if not CLEAN29' on the baseline, but this PR removes that cleanup path and repurposes it from an obsolete CardPart into a live ListPlus statistics page reused by multiple other pages. This revives a published object that consumers were told to migrate away from, instead of leaving the obsolete contract frozen until removal.

Knowledge:

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

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ Breaking\ Changes}$

Page 6978 "Posted Exp. Report Statistics" was obsolete-pending-removal on the baseline, but this PR strips the '#if not CLEAN29' / obsolescence lifecycle and turns it into a new live ListPlus page. Reusing an object that was already on a removal path adds fresh supported behavior to a deprecated API instead of keeping the obsolete object unchanged until cleanup.

Knowledge:

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

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

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling}$

The VAT reclaim-status check was moved out of the upfront posting validation and into CreateGenJournalLineFromSpec(), so a rejectable condition is now discovered only after posting has already allocated the posted document number and started iterating posting work, instead of being rejected pre-flight via the deleted ValidateVATSpecLinesForPosting(). Even though Error() rolls the transaction back, this is a meaningful error-handling regression: users get a deep posting-time failure instead of an early, cheap validation failure, and the check is now duplicated per VAT-spec-row deep inside the posting call graph rather than centralized.

Agent judgement — not directly backed by a BCQuality knowledge article.

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

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

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 ExpenseManagement Integration GitHub request for Integration area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants