diff --git a/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.bad.al b/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.bad.al new file mode 100644 index 0000000..256a6ae --- /dev/null +++ b/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.bad.al @@ -0,0 +1,22 @@ +// BAD: only the changed row is validated (From < To), and there is NO OnDelete check. +// Deleting an interior band opens a gap that nothing rejects; amounts in that range +// then fall through to the wrong tier at calculation time. +table 50100 "Bad Tier Band" +{ + fields + { + field(1; "Plan Code"; Code[20]) { } + field(2; "From Amount"; Decimal) { } + field(3; "To Amount"; Decimal) { } + field(4; "Rate %"; Decimal) { } + } + keys { key(PK; "Plan Code", "From Amount") { } } + + trigger OnInsert() + begin + if (Rec."To Amount" <> 0) and (Rec."To Amount" <= Rec."From Amount") then + Error('To must exceed From.'); // per-row only: ignores gaps/overlaps across the set + end; + + // No OnModify / OnDelete contiguity check -> a deleted interior band silently opens a gap. +} diff --git a/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.good.al b/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.good.al new file mode 100644 index 0000000..479a042 --- /dev/null +++ b/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.good.al @@ -0,0 +1,49 @@ +// GOOD: every insert/modify/delete re-validates the WHOLE ordered band set for +// contiguity: first band at 0, each From = the prior To (no gap, no overlap), only the +// last band open-ended. (Build the post-change set with the stage-siblings/overlay-Rec +// pattern so the in-flight or being-deleted row is reflected.) +table 50101 "Good Tier Band" +{ + fields + { + field(1; "Plan Code"; Code[20]) { } + field(2; "From Amount"; Decimal) { } + field(3; "To Amount"; Decimal) { } + field(4; "Rate %"; Decimal) { } + } + keys { key(PK; "Plan Code", "From Amount") { } } + + trigger OnInsert() begin ValidateContiguity(Rec."Plan Code"); end; + + trigger OnModify() begin ValidateContiguity(Rec."Plan Code"); end; + + trigger OnDelete() begin ValidateContiguityAfterDelete(Rec); end; + + local procedure ValidateContiguity(PlanCode: Code[20]) + var + Band: Record "Good Tier Band"; // in practice: the staged post-change set (siblings + overlaid Rec) + ExpectedFrom: Decimal; + IsFirst: Boolean; + begin + Band.SetRange("Plan Code", PlanCode); + Band.SetCurrentKey("Plan Code", "From Amount"); + IsFirst := true; + if Band.FindSet() then + repeat + if IsFirst then begin + if Band."From Amount" <> 0 then + Error('The first band must start at 0.'); + IsFirst := false; + end else + if Band."From Amount" <> ExpectedFrom then + Error('Bands must be contiguous: no gaps or overlaps.'); + ExpectedFrom := Band."To Amount"; // next band must start where this one ends + until Band.Next() = 0; + end; + + local procedure ValidateContiguityAfterDelete(var DeletedBand: Record "Good Tier Band") + begin + // Re-validate the set with DeletedBand removed so an interior delete that opens a gap is rejected. + ValidateContiguity(DeletedBand."Plan Code"); + end; +} diff --git a/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.md b/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.md new file mode 100644 index 0000000..11b89d5 --- /dev/null +++ b/custom/knowledge/reliability/interval-bands-must-be-contiguous-no-gaps-or-overlaps.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: reliability +keywords: [tier, bands, breakpoints, intervals, contiguous, gaps, overlaps, from-to, validation] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Interval/breakpoint bands must be validated for contiguity (no gaps, no overlaps) across the whole set + +## Description + +When a feature models an ordered set of **breakpoint bands** — tier rate bands with a From/To amount, aging buckets, quantity breaks — the bands must form a contiguous cover: the first starts at the floor (0), each band's From equals the previous band's To, no two bands overlap, and only the highest band may be open-ended. A per-row check (`From < To`) is **not enough**: a gap or overlap is a property of the *set*, and the most dangerous case is a **delete** that removes an interior band and silently opens a gap, so amounts in that range fall through to the wrong band (or no band) at calculation time. The integrity must be re-validated on every insert, modify, AND delete, against the whole ordered set. + +## Best Practice + +On insert/modify/delete, validate the full ordered band set in one pass: sort by From; require the first From = the floor; require each subsequent From to equal the prior band's To (no gap, no overlap); allow only the last band to be open-ended (blank/max To). Build the post-change set with the stage-committed-siblings-and-overlay-Rec pattern so the in-flight row (or the row being deleted, staged out) is reflected — see `stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers`. Reject the change with a clear error rather than letting a gap/overlap reach the calculation. + +See sample: `interval-bands-must-be-contiguous-no-gaps-or-overlaps.good.al`. + +## Anti Pattern + +A band table that validates only the changed row (e.g. `TestField`/`From < To`) and has no whole-set contiguity check, or that has an OnInsert/OnModify check but **no OnDelete** check. Detection signal: From/To band rows where calculation assumes a contiguous cover but nothing rejects a gap (deleted interior band) or an overlap (a new band straddling two existing ones). The defect surfaces only at calc time for amounts landing in the gap/overlap. + +See sample: `interval-bands-must-be-contiguous-no-gaps-or-overlaps.bad.al`. + +## See also + +Originates from commissions-management Plan Builder (PB-3) tier breakpoint bands: From/To rate bands validated for no gaps/overlaps, first band at 0, highest open-ended, and a deleted interior band rejected for opening a gap. Pairs with `stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers` (how to see the in-flight set in the trigger). diff --git a/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.bad.al b/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.bad.al new file mode 100644 index 0000000..b85bd5c --- /dev/null +++ b/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.bad.al @@ -0,0 +1,21 @@ +// BAD: matches the credit memo to its shipment via "Order No.", which is BLANK on a +// return-order-sourced credit memo. The loop finds no lines, so the source shipment's +// commission is never reversed. +codeunit 50100 "Bad Return Match Example" +{ + procedure ReverseShipmentCommission(var SalesCrMemoHeader: Record "Sales Cr.Memo Header") + var + SalesCrMemoLine: Record "Sales Cr.Memo Line"; + SalesShipmentLine: Record "Sales Shipment Line"; + begin + SalesCrMemoLine.SetRange("Document No.", SalesCrMemoHeader."No."); + SalesCrMemoLine.SetRange(Type, SalesCrMemoLine.Type::Item); + SalesCrMemoLine.SetFilter("Order No.", '<>%1', ''); // empty on a return-order credit memo -> no rows + if SalesCrMemoLine.FindSet() then + repeat + SalesShipmentLine.SetRange("Order No.", SalesCrMemoLine."Order No."); + SalesShipmentLine.SetRange("Order Line No.", SalesCrMemoLine."Order Line No."); + // ... reverse the shipment commission (never reached) + until SalesCrMemoLine.Next() = 0; + end; +} diff --git a/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.good.al b/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.good.al new file mode 100644 index 0000000..d0652e3 --- /dev/null +++ b/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.good.al @@ -0,0 +1,21 @@ +// GOOD: resolves the source shipment through the item application that survives a +// return-of-goods flow: the credit-memo line's "Appl.-from Item Entry" is the shipment's +// outbound Item Ledger Entry, whose "Document No." is the posted shipment. +codeunit 50101 "Good Return Match Example" +{ + procedure ReverseShipmentCommission(var SalesCrMemoHeader: Record "Sales Cr.Memo Header") + var + SalesCrMemoLine: Record "Sales Cr.Memo Line"; + ItemLedgerEntry: Record "Item Ledger Entry"; + begin + SalesCrMemoLine.SetRange("Document No.", SalesCrMemoHeader."No."); + SalesCrMemoLine.SetRange(Type, SalesCrMemoLine.Type::Item); + SalesCrMemoLine.SetFilter("Appl.-from Item Entry", '<>%1', 0); + if SalesCrMemoLine.FindSet() then + repeat + if ItemLedgerEntry.Get(SalesCrMemoLine."Appl.-from Item Entry") then + if ItemLedgerEntry."Document Type" = ItemLedgerEntry."Document Type"::"Sales Shipment" then + ; // reverse the commission sourced to ItemLedgerEntry."Document No." (the posted shipment) + until SalesCrMemoLine.Next() = 0; + end; +} diff --git a/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.md b/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.md new file mode 100644 index 0000000..c41b766 --- /dev/null +++ b/custom/knowledge/reliability/match-return-to-source-via-item-application-not-order-link.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: reliability +keywords: [credit-memo, return-order, appl-from-item-entry, item-ledger-entry, order-no, sales-shipment, document-link] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Match a return/credit memo to its source document via item application, not the order link + +## Description + +To reverse or relate a posted **return / credit memo** back to the original document it returns (a posted shipment, the original sale), do **not** key on the posted line's `"Order No."`/`"Order Line No."`. Those fields are **blank** on a return-order-sourced credit memo: the unposted `Sales Line` has no `"Order No."` field to carry, and `Sales-Post` stamps a posted line's `"Order No."` only when posting an **Order** document — a Return Order populates `"Return Order No."`/`"Return Receipt No."` instead. A join on `Sales Cr.Memo Line."Order No."` therefore matches nothing and the link silently never resolves (e.g. the original commission/cost is never reversed). The link that **does** survive a return-of-goods flow is the **item application**: the line's `"Appl.-from Item Entry"` points at the originating outbound `Item Ledger Entry`, whose `"Document No."`/`"Document Type"` identify the posted shipment/invoice. + +## Best Practice + +Resolve a return/credit-memo line to its source document through `"Appl.-from Item Entry"` → `Item Ledger Entry.Get(...)` → `ItemLedgerEntry."Document No."` (gated on the expected `"Document Type"`, e.g. `"Sales Shipment"`). This is the same chain BC uses for exact-cost reversing, so it is present whenever the return is applied to the goods. In tests, establish the application explicitly (copy from the posted shipment, or set `"Appl.-from Item Entry"` to the shipment line's `"Item Shpt. Entry No."`) — a return order copied from a posted shipment does not always carry it for free. + +See sample: `match-return-to-source-via-item-application-not-order-link.good.al`. + +## Anti Pattern + +`SalesCrMemoLine.SetFilter("Order No.", '<>%1', '')` (or `SetRange("Order No.", SomeOrderNo)`) used to find the order/shipment a credit memo reverses. Detection signal: a posted `Sales Cr.Memo Line` (or `Return Receipt Line`) filtered/joined on `"Order No."`/`"Order Line No."` to reach a source Order or Shipment. Those fields are empty for the return-order flow, so the match resolves to nothing and the dependent action (reversal, cost link, traceability) is silently skipped. + +See sample: `match-return-to-source-via-item-application-not-order-link.bad.al`. + +## See also + +Originates from commissions-management #59 (ENG-8): the on-shipment commission reversal matched credit-memo lines on `"Order No."` (blank on a return-order credit memo), so the original shipment commission was never flagged Reversed. Fixed by matching via `"Appl.-from Item Entry"` → the shipment's Item Ledger Entry `"Document No."`. Confirmed against the base-app symbols: the unposted `Sales Line` has no `"Order No."` field. diff --git a/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.bad.al b/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.bad.al new file mode 100644 index 0000000..a01cc9c --- /dev/null +++ b/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.bad.al @@ -0,0 +1,28 @@ +// BAD: Net Payable is derived from Commission Amount, THEN the event fires letting a +// subscriber change Commission Amount, THEN the entry is recorded with no recompute. +// A subscriber that halves the gross leaves Net Payable computed from the old gross. +codeunit 50100 "Bad Mutation Hook Example" +{ + procedure RecordEntry(var CommissionEntry: Record "Commission Ledger Entry") + var + IsHandled: Boolean; + begin + ApplyNetPayable(CommissionEntry); // Net Payable := f(Commission Amount) + + OnBeforeRecordCommissionEntry(CommissionEntry, IsHandled); // subscriber may change Commission Amount + if IsHandled then + exit; + + CommissionEntry.Insert(true); // Net Payable is now stale vs the modified Commission Amount + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeRecordCommissionEntry(var CommissionEntry: Record "Commission Ledger Entry"; var IsHandled: Boolean) + begin + end; + + local procedure ApplyNetPayable(var CommissionEntry: Record "Commission Ledger Entry") + begin + CommissionEntry."Net Payable Amount" := CommissionEntry."Commission Amount"; // (cap/draw omitted) + end; +} diff --git a/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.good.al b/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.good.al new file mode 100644 index 0000000..3d095ec --- /dev/null +++ b/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.good.al @@ -0,0 +1,30 @@ +// GOOD: after the (un-suppressed) mutation event, the derivation is re-run so Net Payable +// always follows the FINAL Commission Amount, whether or not a subscriber changed it. +codeunit 50101 "Good Mutation Hook Example" +{ + procedure RecordEntry(var CommissionEntry: Record "Commission Ledger Entry") + var + IsHandled: Boolean; + begin + ApplyNetPayable(CommissionEntry); + + OnBeforeRecordCommissionEntry(CommissionEntry, IsHandled); + if IsHandled then + exit; + + // The subscriber may have changed the gross; re-derive so the entry is internally consistent. + ApplyNetPayable(CommissionEntry); + + CommissionEntry.Insert(true); + end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeRecordCommissionEntry(var CommissionEntry: Record "Commission Ledger Entry"; var IsHandled: Boolean) + begin + end; + + local procedure ApplyNetPayable(var CommissionEntry: Record "Commission Ledger Entry") + begin + CommissionEntry."Net Payable Amount" := CommissionEntry."Commission Amount"; // (cap/draw omitted) + end; +} diff --git a/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.md b/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.md new file mode 100644 index 0000000..f1c606b --- /dev/null +++ b/custom/knowledge/reliability/re-derive-dependent-fields-after-a-record-mutation-event.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: reliability +keywords: [integration-event, onbefore, mutation-hook, derived-field, stale, ishandled, extensibility] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Re-derive dependent fields after a record-mutation extension event + +## Description + +When code computes a **derived** field from a source field and also raises an `OnBefore…` integration event that lets a subscriber **modify the record** before it is recorded, the order matters. If the derivation runs first and the event fires after it, a subscriber that changes the source field leaves every dependent field computed from the **pre-modification** value — they are silently stale. The record is then inserted (and any side effect, e.g. a balance decrement, applied) with an internally inconsistent source-vs-derived state. This is a BC-specific extensibility trap: the event contract advertises "you may modify amount/status/dimensions," but the platform cannot know that a downstream field was derived from what the subscriber just changed. + +## Best Practice + +Establish a single invariant — *the dependent field is always re-derived from the final source value* — and enforce it relative to the event. Either (a) fire the mutation event **before** the derivation, so the derivation naturally consumes the modified value, or (b) **re-run the derivation after** the (un-suppressed) event returns, just before recording. Option (b) is cheap when the derivation is a pure in-memory recompute. Document that the dependent field is engine-derived (not a value the subscriber sets directly), so the contract is unambiguous. Add a test with a subscriber that modifies only the source field and asserts the dependent field is consistent. + +See sample: `re-derive-dependent-fields-after-a-record-mutation-event.good.al`. + +## Anti Pattern + +`DeriveDependent(Entry)` → `OnBeforeRecord(Entry, IsHandled)` → `Entry.Insert()` with no re-derivation between the event and the insert. Detection signal: an `OnBefore…`/`OnBeforeInsert…` event passing the record `var` (modifiable) that fires **after** a field on that record was computed from another field on the same record, with no recompute before the write. A pure-suppression (`IsHandled`) subscriber is unaffected; only an amount/source-modifying subscriber exposes the stale derived field. + +See sample: `re-derive-dependent-fields-after-a-record-mutation-event.bad.al`. + +## See also + +Originates from commissions-management #51 (Commission Engine close-out): the ENG-2 posting gateway raised `OnBeforeRecordCommissionEntry` after the ENG-9 net-payable derivation, so a subscriber halving `Commission Amount` left `Net Payable`/`Cap Deduction`/`Draw Offset` stale and decremented the draw by a stale offset. Fixed by re-running the net-payable derivation after the event. diff --git a/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.bad.al b/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.bad.al new file mode 100644 index 0000000..e8e7867 --- /dev/null +++ b/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.bad.al @@ -0,0 +1,17 @@ +// BAD: OnAfterApplyCustLedgEntry fires during the apply CALC, before the application +// detailed entries post, so the invoice's Remaining Amount is still pre-payment. +// The cash % reads as 0 and the payment-triggered promotion never fires. +codeunit 50100 "Bad Apply Timing Example" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Gen. Jnl.-Post Line", 'OnAfterApplyCustLedgEntry', '', false, false)] + local procedure OnAfterApply(var CustLedgerEntry: Record "Cust. Ledger Entry") + begin + CustLedgerEntry.CalcFields("Remaining Amount"); // still the PRE-payment remaining here + if CustLedgerEntry."Remaining Amount" = 0 then + PromoteCommission(CustLedgerEntry."Entry No."); // never reached on a paying application + end; + + local procedure PromoteCommission(InvoiceCustLedgerEntryNo: Integer) + begin + end; +} diff --git a/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.good.al b/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.good.al new file mode 100644 index 0000000..70ec565 --- /dev/null +++ b/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.good.al @@ -0,0 +1,23 @@ +// GOOD: react after the application is committed — subscribe to the Detailed Cust. Ledg. +// Entry insert for Entry Type = Application, where the applied invoice's Remaining Amount +// reflects the payment. +codeunit 50101 "Good Apply Timing Example" +{ + [EventSubscriber(ObjectType::Table, Database::"Detailed Cust. Ledg. Entry", 'OnAfterInsertEvent', '', false, false)] + local procedure OnAfterInsertDtldCLE(var Rec: Record "Detailed Cust. Ledg. Entry") + var + InvoiceCustLedgerEntry: Record "Cust. Ledger Entry"; + begin + if Rec."Entry Type" <> Rec."Entry Type"::Application then + exit; + if not InvoiceCustLedgerEntry.Get(Rec."Cust. Ledger Entry No.") then + exit; + InvoiceCustLedgerEntry.CalcFields("Remaining Amount"); // now reflects the applied payment + if InvoiceCustLedgerEntry."Remaining Amount" = 0 then + PromoteCommission(InvoiceCustLedgerEntry."Entry No."); + end; + + local procedure PromoteCommission(InvoiceCustLedgerEntryNo: Integer) + begin + end; +} diff --git a/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.md b/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.md new file mode 100644 index 0000000..396b22a --- /dev/null +++ b/custom/knowledge/reliability/read-cust-ledger-remaining-amount-after-application-detailed-entries-post.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: reliability +keywords: [cust-ledger-entry, remaining-amount, application, detailed-cust-ledg-entry, onafterapplycustledgentry, gen-jnl-post-line, payment] +technologies: [al] +countries: [w1] +application-area: [finance] +--- + +# Read a customer ledger entry's Remaining Amount after the application detailed entries post, not during the apply calc + +## Description + +`Codeunit "Gen. Jnl.-Post Line"."OnAfterApplyCustLedgEntry"` fires during the application **calculation**, BEFORE the application `Detailed Cust. Ledg. Entry` records are posted. At that point the applied invoice's `Cust. Ledger Entry."Remaining Amount"` is still its **pre-payment** value — the payment has not yet reduced it. Code that reacts to this event to decide "how much of the invoice is now paid" (a cash-collected %, a paid-in-full check, a payment-triggered promotion) therefore reads the old remaining and computes 0% collected, firing too early or not at all. The event name suggests "after apply," but it is after the apply *math*, not after the ledger reflects it. + +## Best Practice + +React to the application **after** it is committed to the ledger: subscribe to `Detailed Cust. Ledg. Entry."OnAfterInsertEvent"` and act only on `"Entry Type" = Application`. From the application detailed entry, resolve the applied invoice's `Cust. Ledger Entry` (its `CalcFields("Remaining Amount")` now reflects the payment) and compute the collected amount/percentage there. This is the seam where the post-payment remaining is correct. + +See sample: `read-cust-ledger-remaining-amount-after-application-detailed-entries-post.good.al`. + +## Anti Pattern + +An `[EventSubscriber]` on `"Gen. Jnl.-Post Line"."OnAfterApplyCustLedgEntry"` that reads `CustLedgerEntry."Remaining Amount"` (or `CalcFields` it) to gauge payment. Detection signal: any payment/collection decision keyed on a customer ledger Remaining Amount inside the apply-calc event. The value is pre-application, so the decision uses stale data (typically reading the invoice as still fully open). + +See sample: `read-cust-ledger-remaining-amount-after-application-detailed-entries-post.bad.al`. + +## See also + +Originates from commissions-management #58 (ENG-7 On-Payment Cash % promotion): the Cash % was first computed in `OnAfterApplyCustLedgEntry`, where the invoice Remaining Amount was still pre-payment, so it read 0% and never promoted; CI caught it. Fixed by reacting to `Detailed Cust. Ledg. Entry.OnAfterInsertEvent` (Entry Type = Application). diff --git a/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.bad.al b/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.bad.al new file mode 100644 index 0000000..69b19c3 --- /dev/null +++ b/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.bad.al @@ -0,0 +1,19 @@ +// BAD: after a ship-only post the order is Released; validating "Unit Price" runs +// TestStatusOpen and errors ("Status must be equal to 'Open' ... Current value is 'Released'"). +codeunit 50100 "Bad Reopen Example" +{ + procedure RepriceAfterShip(var SalesHeader: Record "Sales Header"; NewUnitPrice: Decimal) + var + SalesLine: Record "Sales Line"; + LibrarySales: Codeunit "Library - Sales"; + begin + LibrarySales.PostSalesDocument(SalesHeader, true, false); // ship only -> order stays Released + + SalesLine.SetRange("Document Type", SalesHeader."Document Type"); + SalesLine.SetRange("Document No.", SalesHeader."No."); + SalesLine.SetRange(Type, SalesLine.Type::Item); + SalesLine.FindFirst(); + SalesLine.Validate("Unit Price", NewUnitPrice); // TestStatusOpen -> runtime error on a Released doc + SalesLine.Modify(true); + end; +} diff --git a/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.good.al b/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.good.al new file mode 100644 index 0000000..ad7f2bd --- /dev/null +++ b/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.good.al @@ -0,0 +1,23 @@ +// GOOD: reopen the Released order before validating a TestStatusOpen-guarded field; +// the next post re-releases it. ("Qty. to Ship"/"Qty. to Invoice" would NOT need this.) +codeunit 50101 "Good Reopen Example" +{ + procedure RepriceAfterShip(var SalesHeader: Record "Sales Header"; NewUnitPrice: Decimal) + var + SalesLine: Record "Sales Line"; + LibrarySales: Codeunit "Library - Sales"; + begin + LibrarySales.PostSalesDocument(SalesHeader, true, false); // ship only -> order Released + + LibrarySales.ReopenSalesDocument(SalesHeader); // back to Open so the price can change + + SalesLine.SetRange("Document Type", SalesHeader."Document Type"); + SalesLine.SetRange("Document No.", SalesHeader."No."); + SalesLine.SetRange(Type, SalesLine.Type::Item); + SalesLine.FindFirst(); + SalesLine.Validate("Unit Price", NewUnitPrice); // OK: document is Open + SalesLine.Modify(true); + + LibrarySales.PostSalesDocument(SalesHeader, false, true); // invoicing re-releases then posts + end; +} diff --git a/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.md b/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.md new file mode 100644 index 0000000..1e30839 --- /dev/null +++ b/custom/knowledge/reliability/reopen-released-sales-document-before-editing-status-open-fields.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: reliability +keywords: [teststatusopen, released, reopen, sales-header, unit-price, qty-to-ship, validate, status-open] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Reopen a Released sales document before editing fields guarded by TestStatusOpen + +## Description + +Most editable fields on a sales document run `TestStatusOpen` in their `OnValidate` (directly or via the line's modification check), which raises *"Status must be equal to 'Open' … Current value is 'Released'."* once the document has been **Released** — and posting a shipment/receipt leaves an order Released, not Open. So code (or a test) that ships/posts part of a document and then `Validate`s a price, discount, quantity, or dimension on the same document fails at runtime. The trap: a deliberate subset of fields — `"Qty. to Ship"`, `"Qty. to Invoice"`, `"Return Qty. to Receive"` — are intentionally editable on a Released document (you must set them for partial posting), so a partial-ship-then-set-qty step succeeds while a sibling repricing step on the same Released document throws. The difference is per-field, not obvious from the call site. + +## Best Practice + +Reopen the document before editing a `TestStatusOpen`-guarded field on a Released document — `Codeunit "Release Sales Document".Reopen(SalesHeader)` (or `LibrarySales.ReopenSalesDocument` in tests) — then make the change; the subsequent post re-releases it automatically. Do not assume a field is editable just because a nearby field (like `"Qty. to Ship"`) was; check whether the field's validation calls `TestStatusOpen`. When a flow legitimately changes price/quantity between partial postings, model it as reopen → edit → post. + +See sample: `reopen-released-sales-document-before-editing-status-open-fields.good.al`. + +## Anti Pattern + +`SalesLine.Validate("Unit Price", …)` (or `"Line Discount %"`, `Quantity`, a dimension) on a `SalesHeader`/`SalesLine` that has been Released — typically after a ship-only `PostSalesDocument(Header, true, false)` — with no `Reopen` in between. Detection signal: a `Validate` of a status-open-guarded field on a document that a preceding post/release left in `Released` status. Contrast `"Qty. to Ship"`/`"Qty. to Invoice"`, which validate fine on a Released document and so are not a signal. + +See sample: `reopen-released-sales-document-before-editing-status-open-fields.bad.al`. + +## See also + +Originates from commissions-management #59 (ENG-8) tests: a true-up test shipped an order then `Validate`d `"Unit Price"` to bill a different amount, erroring on the Released order; `"Qty. to Ship"` had been editable on the same Released order (so the partial-shipment test passed), which masked the cause. Fixed by reopening the order before repricing. diff --git a/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.bad.al b/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.bad.al new file mode 100644 index 0000000..e699544 --- /dev/null +++ b/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.bad.al @@ -0,0 +1,20 @@ +// BAD: a positive Cap Period duration applied FORWARD lands the window start in the +// future, so SetRange(Posting Date, Start, AnchorDate) is inverted and matches nothing. +// The period cap then sees zero prior consumption and silently never binds. +codeunit 50100 "Bad Rolling Window Example" +{ + procedure ConsumedInPeriod(SalespersonCode: Code[20]; CapPeriod: DateFormula; AnchorDate: Date): Decimal + var + LedgerEntry: Record "Commission Ledger Entry"; + PeriodStart: Date; + begin + // CapPeriod is '1M' (a positive duration). CalcDate('1M', AnchorDate) = AnchorDate + 1 month + // (the FUTURE); stepping back a day is still after AnchorDate. + PeriodStart := CalcDate('<-1D>', CalcDate(CapPeriod, AnchorDate)); + + LedgerEntry.SetRange("Salesperson Code", SalespersonCode); + LedgerEntry.SetRange("Posting Date", PeriodStart, AnchorDate); // Start > AnchorDate -> empty range + LedgerEntry.CalcSums("Net Payable Amount"); + exit(LedgerEntry."Net Payable Amount"); // always 0 for a positive period + end; +} diff --git a/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.good.al b/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.good.al new file mode 100644 index 0000000..0564800 --- /dev/null +++ b/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.good.al @@ -0,0 +1,33 @@ +// GOOD: the duration is applied BACKWARD to build a trailing window ending on the anchor, +// so [Start, Anchor] is a valid range that accumulates the period's prior entries. The +// window math lives in one shared helper so every caller uses the same correct computation. +codeunit 50101 "Good Rolling Window Example" +{ + procedure ConsumedInPeriod(SalespersonCode: Code[20]; CapPeriod: DateFormula; AnchorDate: Date): Decimal + var + LedgerEntry: Record "Commission Ledger Entry"; + PeriodStart: Date; + begin + PeriodStart := TrailingPeriodStart(CapPeriod, AnchorDate); + + LedgerEntry.SetRange("Salesperson Code", SalespersonCode); + LedgerEntry.SetRange("Posting Date", PeriodStart, AnchorDate); + LedgerEntry.CalcSums("Net Payable Amount"); + exit(LedgerEntry."Net Payable Amount"); + end; + + // Negate the duration -> trailing window, inclusive start. A positive '1M' and an + // explicit '-1M' both yield the same trailing month ending on AnchorDate. + procedure TrailingPeriodStart(CapPeriod: DateFormula; AnchorDate: Date): Date + var + Negated: DateFormula; + FormulaText: Text; + begin + FormulaText := Format(CapPeriod); + if FormulaText[1] = '-' then + Negated := CapPeriod + else + Evaluate(Negated, '-' + FormulaText); + exit(CalcDate('<1D>', CalcDate(Negated, AnchorDate))); + end; +} diff --git a/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.md b/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.md new file mode 100644 index 0000000..be05786 --- /dev/null +++ b/custom/knowledge/reliability/rolling-period-window-must-be-trailing-not-forward.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: reliability +keywords: [dateformula, calcdate, rolling-period, trailing-window, cap-period, cumulative, period-filter, negate] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A rolling-period window must apply the period formula backward (trailing), not forward + +## Description + +A "period" `DateFormula` a user enters for a rolling window — a commission/royalty Cap Period, a cumulative-sales window, an aging bucket — is a **positive duration** (`1M`, `1Y`, `3M`). To build the trailing window that ENDS on an anchor date you must apply that duration **backward** from the anchor. Applying it forward with `CalcDate(Period, AnchorDate)` lands the computed start one period in the **future**, after the anchor, so the resulting `SetRange(, Start, Anchor)` is an inverted, empty range. The filter then matches nothing: a period cap sees zero prior consumption and silently never binds; a tiered cumulative resets to the bottom band on every transaction. It is insidious because the code compiles, the happy path (a single transaction, or a blank/default period) looks correct, and only a multi-transaction period in the same window reveals the wrong total. + +## Best Practice + +Treat a user-entered period as a duration and negate it to build a trailing window: step back one period from the anchor, then forward a day for an inclusive start (`CalcDate('<1D>', CalcDate(, Anchor))`). Normalize the sign so a positive `1M` and an explicit `-1M` both yield the same trailing window. Put the window math in ONE shared helper so every caller (the cap consumer, the tier cumulative, the preview) shares the correct computation — duplicated copies turn one fix into an N-site fix. Cover it with a test that spans **two** transactions in one period; a single-transaction test cannot reveal a forward/empty window. + +See sample: `rolling-period-window-must-be-trailing-not-forward.good.al`. + +## Anti Pattern + +`PeriodStart := CalcDate('<-1D>', CalcDate(Period, AnchorDate))` — or any `CalcDate(, AnchorDate)` — used as the START (low bound) of a `[Start, Anchor]` date filter. Detection signal: a `DateFormula` sourced from a user/setup field, applied with `CalcDate` in the forward direction, then used as the low bound of a date range ending at/near the anchor. The range is empty whenever the formula is positive, so a cap/cumulative that should accumulate across the period reads as if the period were empty. + +See sample: `rolling-period-window-must-be-trailing-not-forward.bad.al`. + +## See also + +Originates from commissions-management #51 (Commission Engine close-out): ENG-9 `CapPeriodStart` and three duplicated `TierPeriodWindow` copies applied a positive Cap Period forward, so period caps never bound and tiered plans reset every sale. No test used a non-blank Cap Period, so CI stayed green until a deliberate two-entry test was added. Pairs with the duplication lesson — the fix had to touch four sites. diff --git a/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.bad.al b/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.bad.al new file mode 100644 index 0000000..4a6f23e --- /dev/null +++ b/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.bad.al @@ -0,0 +1,17 @@ +// BAD: each share is rounded independently, so on an odd-cent total the shares don't +// sum to the rounded total. $100.01 at 33.33/33.33/33.34 -> 33.34 x 3 = $100.02. +codeunit 50100 "Bad Split Example" +{ + procedure AllocateShares(LineTotal: Decimal; var TempShare: Record "Allocation Share" temporary) + var + Agent: Record "Allocation Share"; + begin + if Agent.FindSet() then + repeat + TempShare := Agent; + TempShare.Amount := Round(LineTotal * Agent."Split %" / 100); // independent rounding + TempShare.Insert(); + until Agent.Next() = 0; + // No reconciliation: Sum(TempShare.Amount) may be Round(LineTotal) +/- a cent. + end; +} diff --git a/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.good.al b/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.good.al new file mode 100644 index 0000000..e3a5d97 --- /dev/null +++ b/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.good.al @@ -0,0 +1,27 @@ +// GOOD: round every share but the last independently; the last share absorbs the +// residual so the shares always sum EXACTLY to the rounded line total. +codeunit 50101 "Good Split Example" +{ + procedure AllocateShares(LineTotal: Decimal; var TempShare: Record "Allocation Share" temporary) + var + Agent: Record "Allocation Share"; + RoundedTotal: Decimal; + PriorSum: Decimal; + Index: Integer; + AgentCount: Integer; + begin + RoundedTotal := Round(LineTotal); + AgentCount := Agent.Count(); + if Agent.FindSet() then + repeat + Index += 1; + TempShare := Agent; + if Index < AgentCount then begin + TempShare.Amount := Round(LineTotal * Agent."Split %" / 100); + PriorSum += TempShare.Amount; + end else + TempShare.Amount := RoundedTotal - PriorSum; // last share = residual; rows now tie out + TempShare.Insert(); + until Agent.Next() = 0; + end; +} diff --git a/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.md b/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.md new file mode 100644 index 0000000..4ec0b82 --- /dev/null +++ b/custom/knowledge/reliability/split-allocations-reconcile-to-rounded-line-total.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: reliability +keywords: [rounding, split, allocation, largest-remainder, penny-allocation, reconcile, percentage-split] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Split allocations must reconcile to the rounded line total, not round each share independently + +## Description + +When one amount (a line commission, a discount, a charge) is divided across several parties by percentage, rounding **each share independently** — `Round(Total × Pct / 100)` per party — can make the rounded shares fail to sum back to the rounded total. On an odd-cent amount the residual is non-zero: $100.01 split three ways at 33.33/33.33/33.34 gives 33.34 × 3 = $100.02 (a +$0.01 drift), and $0.01 split 50/50 gives $0.01 × 2 = $0.02 (over-allocation). The per-party rows then don't tie to the line, so audit detail and downstream reconciliation are off by a cent. This applies wherever a **single shared total** is apportioned — an equal-rate split, a manual-amount split. It does **not** apply to a per-party independent computation (each party at their own rate with no common total to reconcile against). + +## Best Practice + +Apply largest-remainder (penny) reconciliation when a single total is divided: round each share, sum them, and assign the residual (rounded total − Σ rounded shares) to one share — the largest, or simply the last — so the rows always sum to the rounded total. The simplest correct form: give every share but the last its independently rounded value, and set the last share to `RoundedTotal − Σ(previous shares)`. Reconcile only where a common total exists; leave a genuine per-party different-rate split (no shared total) untouched. + +See sample: `split-allocations-reconcile-to-rounded-line-total.good.al`. + +## Anti Pattern + +A loop that emits `Round(Total * Party.Pct / 100)` for each party with no post-pass reconciling the sum to `Round(Total)`. Detection signal: independent per-share rounding of a shared total where the shares are expected to reconstruct that total (audit rows, a "rows sum to the line" invariant), with no largest-remainder/residual step. The defect only shows on odd-cent totals or splits that don't divide evenly, so even-split tests pass and hide it. + +See sample: `split-allocations-reconcile-to-rounded-line-total.bad.al`. + +## See also + +Originates from commissions-management #71: equal-rate and manual-amount multi-agent splits rounded each agent's share independently, so an odd-cent line commission's shares did not sum to the rounded line total. Fixed with a last-share-absorbs-residual reconciliation, applied only when all rows share one line total (per-agent different-rate splits, UC-13, were left as-is). diff --git a/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.bad.al b/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.bad.al new file mode 100644 index 0000000..97c80e3 --- /dev/null +++ b/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.bad.al @@ -0,0 +1,20 @@ +// BAD: OnInsert scans the live table, which does NOT include the in-flight Rec. +// The split total is computed without the row being inserted, so an invalid set +// can commit (or a valid completing row be rejected). +table 50100 "Bad Split Line" +{ + fields { field(1; "Scope"; Code[20]) { } field(2; "Line No."; Integer) { } field(3; "Split %"; Decimal) { } } + keys { key(PK; "Scope", "Line No.") { } } + + trigger OnInsert() + var + Sibling: Record "Bad Split Line"; + Total: Decimal; + begin + Sibling.SetRange("Scope", Rec."Scope"); + Sibling.CalcSums("Split %"); // the live table does not yet contain Rec + Total := Sibling."Split %"; + if Total <> 100 then + Error('Splits must total 100%%.'); // wrong: Rec's own % is missing from Total + end; +} diff --git a/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.good.al b/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.good.al new file mode 100644 index 0000000..ee576f1 --- /dev/null +++ b/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.good.al @@ -0,0 +1,42 @@ +// GOOD: stage committed siblings (excluding Rec's own key) into a temp record, overlay +// the in-flight Rec, then validate the temp set — so Rec's in-flight value is counted. +table 50101 "Good Split Line" +{ + fields { field(1; "Scope"; Code[20]) { } field(2; "Line No."; Integer) { } field(3; "Split %"; Decimal) { } } + keys { key(PK; "Scope", "Line No.") { } } + + trigger OnInsert() + begin + ValidateScopeTotal(); + end; + + trigger OnModify() + begin + ValidateScopeTotal(); + end; + + local procedure ValidateScopeTotal() + var + TempStaged: Record "Good Split Line" temporary; + Sibling: Record "Good Split Line"; + Total: Decimal; + begin + Sibling.SetRange("Scope", Rec."Scope"); + Sibling.SetFilter("Line No.", '<>%1', Rec."Line No."); // committed siblings, excluding Rec's own key + if Sibling.FindSet() then + repeat + TempStaged := Sibling; + TempStaged.Insert(); + until Sibling.Next() = 0; + + TempStaged := Rec; // overlay the in-flight row + if not TempStaged.Insert() then + TempStaged.Modify(); + + TempStaged.Reset(); + TempStaged.CalcSums("Split %"); + Total := TempStaged."Split %"; + if Total <> 100 then + Error('Splits must total 100%%.'); + end; +} diff --git a/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.md b/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.md new file mode 100644 index 0000000..f226ac6 --- /dev/null +++ b/custom/knowledge/reliability/stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: reliability +keywords: [oninsert, onmodify, in-flight-rec, set-validation, staging, temporary-record, split-total, trigger] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Stage committed siblings and overlay Rec for whole-set validation in OnInsert/OnModify + +## Description + +A validation that must consider a **whole set** of rows — "the splits in this scope total 100%", "these bands are contiguous", "only one row is the default" — cannot scan the live table from inside the row's own `OnInsert`/`OnModify` trigger to get the right answer. During the trigger the in-flight `Rec` is **not yet committed**: a separate `Record` variable that filters and scans the table does **not** see a being-inserted row at all, and sees the **stale** committed image of a being-modified row. So a sum/contiguity/uniqueness check built on a live scan validates against the pre-change set and either rejects a valid change or accepts an invalid one. + +## Best Practice + +Build the post-change image explicitly: scan the committed siblings in the scope **excluding the in-flight row's own primary key** into a temporary record, then **overlay the in-flight `Rec`** (insert it into the temp set), and run the set validation over the temporary set. That way the new/changed row's in-flight value is the one counted, exactly as it will be once committed. For a delete, stage the siblings and omit `Rec` so the set reflects the row being gone. Centralize this staging in one helper that every trigger (OnInsert/OnModify/OnValidate/OnDelete) calls, so the rule is defined once. + +See sample: `stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.good.al`. + +## Anti Pattern + +An `OnInsert`/`OnModify` (or a field `OnValidate`) that does `Sibling.SetRange(); Sibling.CalcSums(...)` / `FindSet` on a fresh table variable and validates the result, expecting the in-flight `Rec` to be included. Detection signal: a whole-set check (total, contiguity, uniqueness) inside a row trigger that reads the live table without excluding-Rec's-key-and-overlaying-Rec. The check passes the existing rows but mis-handles the row currently being inserted/modified. + +See sample: `stage-committed-siblings-and-overlay-rec-for-set-validation-in-triggers.bad.al`. + +## See also + +Originates from commissions-management Plan Builder / Agent Manager (PB-3): the 100%-per-scope split total and tier-band integrity checks ran from OnInsert/OnModify and had to stage committed siblings + overlay Rec rather than scan the live table. Pairs with `tier-bands-must-be-contiguous-no-gaps-or-overlaps`. diff --git a/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.bad.al b/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.bad.al new file mode 100644 index 0000000..2f0d7dc --- /dev/null +++ b/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.bad.al @@ -0,0 +1,14 @@ +// BAD: a List page sets SecurityFiltering = Filtered expecting it to limit each rep to +// their own commission rows. With no configured security filters behind it, the property +// has nothing to apply and every user still sees every row. +page 50100 "Bad Rep Commission List" +{ + PageType = List; + SourceTable = "Commission Ledger Entry"; + SourceTableView = sorting("Entry No."); + // Intent: "reps see only their own rows." Reality: no security filter is configured, + // so this isolates nothing; it only governs HOW existing filters (none) would apply. + // (And SecurityFiltering is not a permissionset property — it cannot live in the role set.) + + layout { area(Content) { repeater(Lines) { field("Salesperson Code"; Rec."Salesperson Code") { ApplicationArea = All; } } } } +} diff --git a/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.good.al b/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.good.al new file mode 100644 index 0000000..f182d72 --- /dev/null +++ b/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.good.al @@ -0,0 +1,28 @@ +// GOOD: when configured security filters are not in scope, isolate at the application +// layer — explicitly filter the source to the current user's salesperson — rather than +// implying isolation a page property does not enforce. +page 50101 "Good Rep Commission List" +{ + PageType = List; + SourceTable = "Commission Ledger Entry"; + Editable = false; + + layout { area(Content) { repeater(Lines) { field("Salesperson Code"; Rec."Salesperson Code") { ApplicationArea = All; } } } } + + trigger OnOpenPage() + var + OwnSalespersonCode: Code[20]; + begin + // Resolve the current user's salesperson (omitted) and scope the source explicitly. + OwnSalespersonCode := CurrentUserSalespersonCode(); + if OwnSalespersonCode <> '' then + Rec.SetRange("Salesperson Code", OwnSalespersonCode); + // For enforced row security (not just a UI filter), configure security filters on the + // user's permission sets; SecurityFiltering then controls how strictly they apply. + end; + + local procedure CurrentUserSalespersonCode(): Code[20] + begin + exit(''); // resolve from the user-setup / salesperson mapping + end; +} diff --git a/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.md b/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.md new file mode 100644 index 0000000..1432073 --- /dev/null +++ b/custom/knowledge/security/securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: security +keywords: [securityfiltering, security-filter, permissionset, row-level-security, data-isolation, rep-scope, page-property] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# SecurityFiltering is not a permission-set property and does not by itself isolate rows per user + +## Description + +Per-user **row-level** data isolation ("each rep sees only their own records") is a common requirement that AL gives no single declarative switch for, and two plausible-looking approaches do not deliver it. First, `SecurityFiltering` is **not** a `permissionset` property — a permission set grants object/tabledata permissions (RIMD) and cannot be authored to filter rows; writing `SecurityFiltering` in a permissionset object does not compile. Second, `SecurityFiltering` on a page/query is a property that governs **how the user's already-configured security filters are applied** (Filtered/Validated/Ignored/Disallowed) — it does not, on its own, create those filters, so adding it to a List page without configured security filters restricts nothing. Teams reach for one of these to scope a salesperson/rep to their own commission, sales, or ledger rows and find the rows are not actually isolated. + +## Best Practice + +Decide the isolation mechanism explicitly. True per-user row security in BC requires **configured security filters** (the security-filter setup tied to the user's permission sets) — `SecurityFiltering` on the page/query then controls how strictly those filters apply. If configured security filters are not in scope, isolate at the application layer (filter the source by the user's salesperson/responsibility, or expose the scoped data through a dedicated access layer/portal) and do not imply isolation that is not enforced. Either way, do not treat a permission set or a bare page `SecurityFiltering` property as row-level security. + +See sample: `securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.good.al`. + +## Anti Pattern + +A `SecurityFiltering` property added to a List page (or a permission set) with the intent of limiting a user to their own rows, but with no configured security filters behind it — the rows are not isolated. Detection signal: a row-isolation requirement met only by a page `SecurityFiltering` property or a permission-set grant, with no configured security filter or explicit source filtering. Also flag any attempt to place `SecurityFiltering` in a `permissionset` object (it is not a valid property there). + +See sample: `securityfiltering-is-not-per-user-row-isolation-or-a-permissionset-property.bad.al`. + +## See also + +Originates from commissions-management: rep data isolation (each rep seeing only their own commissions) could not be met by `SecurityFiltering` or the permission sets alone and was deferred to a dedicated portal access layer (BL-07). The role-based permission sets scope *object* access, not rows.