Skip to content

Hidden breaking change and instability on sales/purchase line insert#9688

Open
JakovljevicDusan wants to merge 5 commits into
mainfrom
bugs/main-HiddenBreakingChangeOnSalesPurchaseLineInsert
Open

Hidden breaking change and instability on sales/purchase line insert#9688
JakovljevicDusan wants to merge 5 commits into
mainfrom
bugs/main-HiddenBreakingChangeOnSalesPurchaseLineInsert

Conversation

@JakovljevicDusan

@JakovljevicDusan JakovljevicDusan commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

What & why

ISSUE
Existing check is introduced in v28 and is intended to prevent inserting sales/purchase document lines if header does not exist at the time when record is actually inserted. And should skip this check in certain scenarios when this is allowed by design (that are followed soon by inserting "Sales Header").
The existing check was introduced in v28 to prevent the insertion of sales and purchase document lines when the corresponding header does not exist at the time the record is actually inserted. The check is intentionally bypassed in specific scenarios where this behavior is allowed by design, such as when the corresponding "Sales Header" is created immediately afterward.
However, the check has proven to be unreliable in several scenarios and can raise false-positive errors for operations that are valid and expected to succeed.

SOLUTION
Changes in "Sales Line":

  • Rename variable HasSalesHeader -> SuppressSalesHeaderExistsVerification, to make it clear it is not about (stale) cache state, but switch whether to skip verification.
    • Caveat is that OnBeforeGetSalesHeader() publisher retains parameter named HasSalesHeader, to avoid breaking changes.
    • Add procedure SetSuppressSalesHeaderExistsVerification(), so using SetSalesHeader()/ClearSalesHeader() is not mandatory to change this switch.
    • Default value of SuppressSalesHeaderExistsVerification switch is false. State of the SuppressSalesHeaderExistsVerification can be changed via: SetSalesHeader(), ClearSalesHeader(), SetSuppressSalesHeaderExistsVerification(), OnBeforeGetSalesHeader() event.
  • Add explicit "if SalesHeader.Get()" check in VerifySalesHeaderExists() procedure, do not rely on flaky (for this purpose) GetSalesHeader().
  • Skips SalesHeaderExistsVerification for temporay "Sales Line".

Symmetrical changes in "Purchase Line".

One of the scenarios when Base app suppresses this check is when recurring sales lines are added during validation of the "Sell-to Customer No." in the "Sales Header".

Linked work

Fixes AB#643231

@github-actions github-actions Bot added the SCM GitHub request for SCM area label Jul 23, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 23, 2026
@JakovljevicDusan
JakovljevicDusan marked this pull request as ready for review July 23, 2026 12:07
@JakovljevicDusan
JakovljevicDusan requested a review from a team July 23, 2026 12:07
@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Data\ Modeling}$

SuppressPurchaseHeaderExistsVerification is set once by SetPurchHeader/SetSuppressPurchaseHeaderExistsVerification, but GetPurchHeader no longer resets it to false when a subsequent lookup fails to find the header (the removed 'HasPurchHeader := false' in the else branch was not replaced). Once a caller has called SetPurchHeader (or explicitly suppressed the check), the flag stays true for the lifetime of the record variable even if the referenced Purchase Header is later deleted, so VerifyPurchaseHeaderExists() on a later Insert of the same Purchase Line instance will silently skip the 'cannot insert line without header' guard instead of re-validating. Under the prior implementation this path was covered because GetPurchHeader kept the flag in sync with the last known lookup result. This same pattern is replicated identically in every localized copy of Purchase Line (APAC, BE, CH, DACH, ES, FI, GB, IT, NA, NL, NO, RU, SE, W1). Reset the flag when the header lookup fails so the guard cannot be bypassed by a stale flag.

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

            end else begin
                Clear(PurchHeader);
                SuppressPurchaseHeaderExistsVerification := false;
            end;

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.14.4

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Data\ Modeling}$

SuppressSalesHeaderExistsVerification is set once by SetSalesHeader/SetSuppressSalesHeaderExistsVerification, but GetSalesHeader no longer resets it to false when a subsequent lookup fails to find the header (the removed 'HasSalesHeader := false' in the else branch was not replaced). Once a caller has called SetSalesHeader (or explicitly suppressed the check), the flag stays true for the lifetime of the record variable even if the referenced Sales Header is later deleted, so VerifySalesHeaderExists() on a later Insert of the same Sales Line instance will silently skip the 'cannot insert line without header' guard instead of re-validating. Under the prior implementation this path was covered because GetSalesHeader kept the flag in sync with the last known lookup result. This same pattern is replicated identically in every localized copy of Sales Line (APAC, BE, CH, DACH, ES, FI, GB, IT, NA, NL, NO, RU, SE, W1). Reset the flag when the header lookup fails so the guard cannot be bypassed by a stale flag.

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

            end else begin
                Clear(SalesHeader);
                SuppressSalesHeaderExistsVerification := false;
            end;

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.14.4

begin
IsHandled := false;
OnBeforeGetPurchHeader(Rec, PurchHeader, IsHandled, Currency, HasPurchHeader);
OnBeforeGetPurchHeader(Rec, PurchHeader, IsHandled, Currency, SuppressPurchaseHeaderExistsVerification);

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

The published integration event OnBeforeGetPurchHeader still declares 'var HasPurchHeader: Boolean' unchanged, but the call site now feeds it from SuppressPurchaseHeaderExistsVerification and GetPurchHeader no longer writes back to it on a successful or failed lookup. The parameter's meaning silently changed from 'a header was found for this line' to 'suppress the OnInsert header-existence check', with the same name and type. A dependent extension subscribing to this event and setting/reading the var per the old semantics will now unknowingly toggle the new insert-time verification instead of the old lookup cache, since the platform only protects name/type/var-mode, not meaning. Recommend renaming the parameter via a new event/overload, or explicitly documenting the behavior change for existing subscribers.

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

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

IsHandled: Boolean;
begin
OnBeforeGetSalesHeader(Rec, SalesHeader, IsHandled, Currency, HasSalesHeader);
OnBeforeGetSalesHeader(Rec, SalesHeader, IsHandled, Currency, SuppressSalesHeaderExistsVerification);

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

The published integration event OnBeforeGetSalesHeader still declares 'var HasSalesHeader: Boolean' unchanged, but the call site now feeds it from SuppressSalesHeaderExistsVerification and GetSalesHeader no longer writes back to it on a successful or failed lookup. The parameter's meaning silently changed from 'a header was found for this line' to 'suppress the OnInsert header-existence check', with the same name and type. A dependent extension subscribing to this event and setting/reading the var per the old semantics will now unknowingly toggle the new insert-time verification instead of the old lookup cache, since the platform only protects name/type/var-mode, not meaning. Recommend renaming the parameter via a new event/overload, or explicitly documenting the behavior change for existing subscribers.

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

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

@JakovljevicDusan JakovljevicDusan changed the title Hidden breaking change on sales/purchase line insert Hidden breaking change and instability on sales/purchase line insert Jul 23, 2026
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.

1 participant