Skip to content

[Master]-Inconsistent Validation of Non-Deductible VAT % for Purchase Lines Sharing the Same VAT Identifier - #10521

Open
neeleshsinghal wants to merge 3 commits into
mainfrom
bugs/Bug-647053-NonDeductible-VAT-Purch-Lines-Same-VAT-Identifier-v4
Open

[Master]-Inconsistent Validation of Non-Deductible VAT % for Purchase Lines Sharing the Same VAT Identifier#10521
neeleshsinghal wants to merge 3 commits into
mainfrom
bugs/Bug-647053-NonDeductible-VAT-Purch-Lines-Same-VAT-Identifier-v4

Conversation

@neeleshsinghal

@neeleshsinghal neeleshsinghal commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@neeleshsinghal
neeleshsinghal requested a review from a team August 21, 2026 14:36
@neeleshsinghal
neeleshsinghal requested review from a team as code owners August 21, 2026 14:36
@github-actions github-actions Bot added the SCM GitHub request for SCM area label Aug 21, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Aug 21, 2026
begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

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

begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

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

begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

The same issue exists in these regional copies — apply the equivalent fix in each:

  • src/Layers/ES/BaseApp/Purchases/Document/PurchaseLine.Table.al:3695 (ES)
  • src/Layers/NO/BaseApp/Purchases/Document/PurchaseLine.Table.al:3695 (NO)

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

begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

The same issue exists in these regional copies — apply the equivalent fix in each:

  • src/Layers/SE/BaseApp/Purchases/Document/PurchaseLine.Table.al:3692 (SE)

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

begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

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

begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

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

begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

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

begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

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

begin
NonDeductibleVAT.CheckPrepmtWithNonDeductubleVATInPurchaseLine(Rec);
NonDeductibleVAT.CheckNonDeductibleVATPctIsAllowed(Rec);
CheckNonDedVATPctConsistencyForZeroLine();

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

This adds a new error path to the shipped OnValidate trigger of field 6200 "Non-Deductible VAT %": existing extensions, integrations, or data-migration code that previously called Validate("Non-Deductible VAT %", 0) successfully can now fail when another line on the same document has the same VAT Identifier with a different percentage. Preserve the released field-trigger behavior or move the stricter consistency check behind a compatible opt-in path; BCQuality has no knowledge-backed rule for trigger-behavior breaks yet, so this functional breaking change can only be emitted as an agent finding.

The same issue exists in these regional copies — apply the equivalent fix in each:

  • src/Layers/DACH/BaseApp/Purchases/Document/PurchaseLine.Table.al:3691 (DACH)
  • src/Layers/NL/BaseApp/Purchases/Document/PurchaseLine.Table.al:3691 (NL)

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

exit("Matched Inv./Cr. Memo Lines" > 0);
end;

local procedure CheckNonDedVATPctConsistencyForZeroLine()

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

The new CheckNonDedVATPctConsistencyForZeroLine() helper, added identically to all 14 country-layer PurchaseLine.Table.al copies, throws a hard Error() directly from the table's "Non-Deductible VAT %" OnValidate trigger instead of going through the existing NonDeductibleVAT/NonDedVATImpl facade. That facade already owns the near-identical non-zero-percent consistency check (CheckNonDeductibleVATPctIsAllowed) and already exposes an OnBeforeCheckNonDeductibleVATPctIsAllowed(PurchaseLine, var IsHandled) integration event, so extensions that customize or suppress that validation cannot intercept this new zero-percent branch. Bypassing the facade also duplicates the SetRange/SetFilter/FindFirst query and error text as a second maintenance point directly in the table object (copied into all 14 layers) instead of centralizing the rule in the codeunit that already owns it. Route the new check through the event-enabled facade (e.g. add the zero-percent branch to NonDedVATImpl.CheckNonDeductibleVATPctIsAllowed, guarded by the same or a new IsHandled event) instead of hard-wiring a duplicate Error() in the table trigger.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

The same issue exists in these regional copies — apply the equivalent fix in each:

  • src/Layers/SE/BaseApp/Purchases/Document/PurchaseLine.Table.al:10312 (SE)

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

if not NonDeductibleVAT.IsNonDeductibleVATEnabled() then
exit;

PurchLine.SetRange("Document Type", "Document Type");

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

Knowledge:

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

exit("Matched Inv./Cr. Memo Lines" > 0);
end;

local procedure CheckNonDedVATPctConsistencyForZeroLine()

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

CheckNonDedVATPctConsistencyForZeroLine() only reads "VAT Bus. Posting Group" and "VAT Prod. Posting Group", but its FindFirst() on the wide "Purchase Line" table does not call SetLoadFields(), so each validation loads a full row just to build the error text. Add SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group") before the read.

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

PurchLine.SetLoadFields("VAT Bus. Posting Group", "VAT Prod. Posting Group");
PurchLine.SetRange("Document Type", "Document Type");
PurchLine.SetRange("Document No.", "Document No.");
PurchLine.SetFilter("Line No.", '<>%1', "Line No.");
PurchLine.SetRange("VAT Identifier", "VAT Identifier");
PurchLine.SetFilter("Non-Deductible VAT %", '<>%1', "Non-Deductible VAT %");
if PurchLine.FindFirst() then

Knowledge:

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

InvoiceOrOrderDocTypeErr: Label '%1 must be either %2 or %3.', Comment = '%1 - Document Type; %2, %3 - Purchase Document Type, Invoice or Order';
CannotInsertPurchLineWithoutHeaderErr: Label 'You cannot insert a purchase line without a purchase header.';
MustSpecifyErr: Label 'You must either specify %1 or %2.', Comment = '%1 = Field Caption; %2 = Field Caption';
DifferentNonDedVATRatesSameVATIdentifierErr: Label 'You cannot set different Non-Deductible VAT % for the combinations of business and product groups with the same VAT identifier.\The following combination with the same VAT identifier has different Non-Deductible VAT %: business group %1, product group %2', Comment = '%1 = VAT business posting group code, %2 = VAT product posting group code';

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

The new DifferentNonDedVATRatesSameVATIdentifierErr label is copied into all 14 changed src/Layers/*/BaseApp/Purchases/Document/PurchaseLine.Table.al objects even though src/Layers/W1/BaseApp/Finance/VAT/Calculation/NonDedVATImpl.Codeunit.al already owns the identical message. BCQuality's style guidance treats shared messages as single-owner translation assets; introducing a second owner for the same text already splits the placeholder metadata ('%1, %2 - codes' in the existing owner versus the new detailed comment here) and makes future localization drift more likely. Keep one owning object for this shared error text and reuse it through shared logic instead of duplicating the label into each table copy.

Knowledge:

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

exit("Matched Inv./Cr. Memo Lines" > 0);
end;

local procedure CheckNonDedVATPctConsistencyForZeroLine()

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

All 14 changed PurchaseLine.Table.al copies add CheckNonDedVATPctConsistencyForZeroLine() as an unguarded table-39 runtime validation that now throws a new Error(...) when an existing purchase line is revalidated to "Non-Deductible VAT %" = 0. That is a new upgrade-time failure surface for legacy documents: if upgrade code or partner migration code revalidates this field directly, or revalidates VAT posting groups through the existing NonDeductibleVAT.SetNonDeductiblePct path, the upgrade can now abort on data that previously upgraded successfully. Per the referenced guidance, this runtime-only consistency check should be skipped when GetExecutionContext() = ExecutionContext::Upgrade.

Knowledge:

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

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Web\ Services}$

This PR adds a new Error() path to the published Purchase Line validation contract without introducing a new API version. table 39 "Purchase Line" backs already-published routes such as the v2.0 purchaseOrderLine API, so the new CheckNonDedVATPctConsistencyForZeroLine() call can now turn existing POST/PATCH requests into new API error responses when another line on the document shares the VAT identifier but has a different "Non-Deductible VAT %". Published API behavior is part of the stable contract; keep existing routes behavior-compatible, or ship this rule behind a new API version instead of changing it in place.

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

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR fixes the Non-Deductible VAT purchase-line case where one line can be set to 0 while another line with the same VAT Identifier keeps a non-zero Non-Deductible VAT %. The existing validation already rejects the reverse order, and the new tests cover both entry orders. The rule itself is correct, but the new table-level call is placed outside the existing handled validation path.

Suggestions

S1 - Keep the check inside the existing validation
The new table-level call runs after OnBeforeCheckNonDeductibleVATPctIsAllowed, so subscribers that set IsHandled can still get this new error when the value is 0. Move the zero-value branch into NonDedVATImpl.CheckNonDeductibleVATPctIsAllowed after the existing IsHandled check, or add an equivalent handled event before this new error. Apply the same shape to the layer copies.

Risk assessment and necessity

Risk: This is purchase-line VAT validation, so a wrong change can block purchase documents and API writes for a financial field. The same check was added consistently to W1 and the regional BaseApp Purchase Line copies, and no new BaseApp publisher dependency was added; I verified the existing handled event in NonDeductibleVAT.Codeunit.al. CI still had some unit-test jobs running and two integration jobs marked failed without logs available from the in-progress run, so those results should be checked before merge.

Necessity: The scenario is valid: without the change, a document can mix 0 and non-zero Non-Deductible VAT % values under the same VAT Identifier depending on line entry order. The scope is targeted and the tests cover both order directions, but the fix should preserve the existing handled validation contract.


[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10521 round=1 by=alexei-dobriansky at=2026-08-21T18:11:27Z lastSha=5794e0cf14cf2215060b59024d444a53150b1f16 reviewKey=da273117122186a6f1c90c3e24db6b21fa5624b9f3c8ffe342b8e5fdf2463867 suggestions=S1@44c8aee0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

SCM GitHub request for SCM area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants