Skip to content

Align SalesTaxCalculate across countries#9704

Open
Alexander-Ya wants to merge 3 commits into
mainfrom
bugs/Align-SalesTaxCalculate-across-countries
Open

Align SalesTaxCalculate across countries#9704
Alexander-Ya wants to merge 3 commits into
mainfrom
bugs/Align-SalesTaxCalculate-across-countries

Conversation

@Alexander-Ya

@Alexander-Ya Alexander-Ya commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What & why

Linked work

Fixes AB#636629

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

@Alexander-Ya
Alexander-Ya requested a review from a team July 23, 2026 18:00
@github-actions github-actions Bot added the Finance GitHub request for Finance area label Jul 23, 2026
TaxAmount := DesiredTaxAmount;
end;
if (TaxAmount <> DesiredTaxAmount) and (Abs(TaxAmount - DesiredTaxAmount) <= 0.01) then
if TempTaxDetail.FindSet(true) then begin

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}$

This branch updates only the first TempTaxDetail row and never calls Next(), so FindSet(true) is doing multi-row iteration setup for a single-record access. Use FindFirst() here and keep Modify() on that one record.

Suggested change
if TempTaxDetail.FindSet(true) then begin
if TempTaxDetail.FindFirst() then begin

Knowledge:

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

Caption = 'Calculate Tax on Tax';
Editable = false;
}
field(10010; "Expense/Capitalize"; Boolean)

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

The new "Expense/Capitalize" field is a Normal table field with no explicit DataClassification, so it remains implicitly unclassified at field scope. BCQuality's privacy guidance and AS0016 require every Normal field to declare its own non-"ToBeClassified" classification.

Knowledge:

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

#pragma warning disable AA0074
#pragma warning disable AA0470
Text000: Label '%1 in %2 %3 must be filled in with unique values when %4 is %5.';
Text001: Label 'The sales tax amount for the %1 %2 and the %3 %4 is incorrect. ';

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}$

Both renamed labels still contain multiple placeholders but no Comment, so translators have no canonical meaning for %1-%6 when localizing these errors.

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

        MissingTaxAreaValuesErr: Label '%1 in %2 %3 must be filled in with unique values when %4 is %5.', Comment = '%1 = Calculation Order field caption, %2 = Tax Area table caption, %3 = Tax Area code, %4 = Calculate Tax on Tax field caption, %5 = Boolean value of Calculate Tax on Tax.';
        SalesTaxAmountIncorrectErr: Label 'The sales tax amount for the %1 %2 and the %3 %4 is incorrect. The calculated sales tax amount is %5, but was supposed to be %6.', Comment = '%1 = Tax Area Code value, %2 = Tax Area Code field caption, %3 = Tax Group Code value, %4 = Tax Group Code field caption, %5 = Calculated sales tax amount, %6 = Expected sales tax amount.';

Knowledge:

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

@github-actions

Copy link
Copy Markdown
Contributor

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

In InitSalesTaxLines, the newly added maximum-tracking setup wraps its own 'if TaxLiable then begin' around the pre-existing 'if TaxLiable then begin' that starts the below/above-maximum calculation, producing two nested begin blocks with only one matching 'end' before the enclosing then-block closes. This leaves the procedure's begin/end structure unbalanced. The underlying impact is a compile-breaking defect (a 'blocker' in effect), not a minor one; it should be fixed immediately and, if this shape recurs, considered for a dedicated knowledge rule.

Recommendation:

  • remove the duplicate inner 'if TaxLiable then begin' so the Maximums setup and the below/above-maximum calculation share a single if block.

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

                    if TaxLiable then begin
                        // This code uses a temporary table to keep track of Maximums.
                        // This temporary table should be cleared before the first call
                        // to this routine.  All subsequent calls will use the values in
                        // that get put into this temporary table.

                        TempTaxDetailMaximums := TaxDetail;
                        if not TempTaxDetailMaximums.Find() then
                            TempTaxDetailMaximums.Insert();

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

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

else begin
then begin
AddedTaxAmount := TaxBaseAmount * TaxDetail."Tax Below Maximum" / 100;
TempTaxDetailMaximums."Maximum Amount/Qty." := TempTaxDetailMaximums."Maximum Amount/Qty." - Quantity;

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}$

In the Sales Tax (amount-based) branch of InitSalesTaxLines, the running 'Maximum Amount/Qty.' balance is decremented by Quantity instead of TaxBaseAmount. The sibling Excise Tax (quantity-based) branch correctly decrements by Quantity, and the analogous logic in CalculateTax decrements the Sales Tax case by TaxBaseAmount, confirming this is a copy-paste mismatch rather than an intentional choice. This causes the tracked remaining maximum to drift using the wrong unit (units instead of currency amount), corrupting subsequent tax-maximum calculations across postings that share the temporary table. The underlying impact is a real calculation defect (a 'major' concern), capped here at minor per agent-finding rules.

Recommendation:

  • decrement by TaxBaseAmount to match the amount-based nature of this branch.
Suggested change
TempTaxDetailMaximums."Maximum Amount/Qty." := TempTaxDetailMaximums."Maximum Amount/Qty." - Quantity;
TempTaxDetailMaximums."Maximum Amount/Qty." := TempTaxDetailMaximums."Maximum Amount/Qty." - TaxBaseAmount;

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

@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant