Port NAV Extract-ES-Cartera changes from bugs branch#9002
Port NAV Extract-ES-Cartera changes from bugs branch#9002Alexander-Ya wants to merge 28 commits into
Conversation
Alexander-Ya
added
the
Delocalization
Copilot PR ReviewIteration 15 · Outcome: completed Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630 Findings by domainFindings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).
Totals: 9 knowledge-backed · 0 agent findings. Orchestrator pre-filter (13 file(s) excluded)
Findings produced by the Copilot CLI agent against BCQuality at |
This comment was marked as outdated.
This comment was marked as outdated.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Agentic PR Review - Round 2Recommendation: Request ChangesWhat this PR doesThis PR extracts the Spanish Cartera code into Correctness of the event wiring is good: the publishers the ES subscribers use ( Status of previous suggestions
New observations (commits since round 1)S4 - Publisher propagation to country layers Risk assessment and necessityRisk: High. The diff touches Sales and Purchase posting, journal checks, customer/vendor ledger entries, payment methods, permissions, and pages across the W1 base and many country layers. The event contract itself now looks self-contained inside this PR, which lowers the earlier concern. The remaining risk is concentrated in the failing ES build: until the ES app compiles, the extracted behavior cannot be validated at all, and a broken layer build blocks the whole change. Necessity: The linked work item AB#636627 is a Deliverable for extracting ES Cartera to object extensions, so the large scope is expected for that goal. The change is not mergeable yet because the ES app build fails; once it compiles, confirm that the existing ES Cartera tests still exercise the extracted objects (bill groups, payment orders, settlement, blocked-settlement checks), since this is sensitive posting logic that moved.
|
| { | ||
| actions | ||
| { | ||
| addlast(reporting) |
There was a problem hiding this comment.
The Small Business Owner role center originally placed the Cartera reporting block right after Customer - Due Payments, but this extension re-adds it with addlast(reporting), moving the Cartera reports away from the related due-payments actions.
Re-anchor the block after Customer - Due Payments to preserve the original layout.
| addlast(reporting) | |
| addafter("Customer - Due Payments") |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
PostSettlementForPostedPmtOrder and PostSettlementForPostedBillGroup (new procedures added by this PR) call GenJournalLine.FindSet() on an unfiltered Gen.Journal Line record. Neither caller applies SetRange/SetFilter before invoking these procedures, so each call iterates every visible journal line instead of only the settlement lines for the current posted payment order or bill group. Knowledge: Posting this finding as an issue comment because inline comment placement failed. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why |
| VendLedgEntry.SetFilter("Document Situation", '<>%1', VendLedgEntry."Document Situation"::" "); | ||
| VendLedgEntry.SetRange("Vendor No.", PurchHeader."Pay-to Vendor No."); | ||
| VendLedgEntry.SetRange(Open, true); | ||
| if VendLedgEntry.FindSet() then |
There was a problem hiding this comment.
The new Vendor Ledger Entry scan iterates a production-scale table and only reads a small subset of fields (Document Situation, Document Status, Document Type, Document No.), but it never calls SetLoadFields(...) before FindSet().
That makes each iteration materialize the full ledger-entry row unnecessarily; add SetLoadFields before the read.
| VendLedgEntry.SetFilter("Document Situation", '<>%1', VendLedgEntry."Document Situation"::" "); | |
| VendLedgEntry.SetRange("Vendor No.", PurchHeader."Pay-to Vendor No."); | |
| VendLedgEntry.SetRange(Open, true); | |
| if VendLedgEntry.FindSet() then | |
| VendLedgEntry.SetFilter("Document Situation", '<>%1', VendLedgEntry."Document Situation"::" "); | |
| VendLedgEntry.SetRange("Vendor No.", PurchHeader."Pay-to Vendor No."); | |
| VendLedgEntry.SetRange(Open, true); | |
| VendLedgEntry.SetLoadFields("Document Situation", "Document Status", "Document Type", "Document No."); | |
| if VendLedgEntry.FindSet() then |
Knowledge:
- microsoft/knowledge/performance/use-setloadfields-for-partial-records.md
- microsoft/knowledge/performance/production-scale-tables-warrant-extra-analysis.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| CustLedgEntry.SetCurrentKey("Document No.", "Document Type", "Customer No."); | ||
| CustLedgEntry.SetFilter("Document Type", '%1|%2', CustLedgEntry."Document Type"::Invoice, | ||
| CustLedgEntry."Document Type"::Bill); | ||
| CustLedgEntry.SetFilter("Document Situation", '<>%1', CustLedgEntry."Document Situation"::" "); |
There was a problem hiding this comment.
The new Customer Ledger Entry scan iterates a production-scale table and only reads a small subset of fields (Document Situation, Document Status, Document Type, Document No.), but it never calls SetLoadFields(...) before reading rows.
That makes each iteration materialize the full ledger-entry row unnecessarily; add SetLoadFields before the read.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
CustLedgEntry.SetFilter("Document Situation", '<>%1', CustLedgEntry."Document Situation"::" ");
CustLedgEntry.SetRange("Customer No.", SalesHeader."Bill-to Customer No.");
CustLedgEntry.SetRange(Open, true);
CustLedgEntry.SetLoadFields("Document Situation", "Document Status", "Document Type", "Document No.");
if CustLedgEntry.Find('-') thenKnowledge:
- microsoft/knowledge/performance/use-setloadfields-for-partial-records.md
- microsoft/knowledge/performance/production-scale-tables-warrant-extra-analysis.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| PurchPost: Codeunit "Purch.-Post"; | ||
| #endif | ||
| begin | ||
| if PaymentMethod.Get(PurchHeader."Payment Method Code") then |
There was a problem hiding this comment.
PaymentMethod.Get(...) runs before the Invoice guard.
On non-invoice purchase postings this lookup is unused, so the branch pays an avoidable database round-trip. Move the cheap PurchHeader.Invoice guard ahead of the Get.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
if PurchHeader.Invoice and PaymentMethod.Get(PurchHeader."Payment Method Code") then
if (PaymentMethod."Create Bills" or PaymentMethod."Invoices to Cartera") and
(not CarteraSetup.ReadPermission)
then
Error(CannotCreateCarteraDocErr);Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| #endif | ||
| begin | ||
| // Create Bills | ||
| if PaymentMethod.Get(SalesHeader."Payment Method Code") then |
There was a problem hiding this comment.
PaymentMethod.Get(...) runs before the Invoice guard.
On non-invoice sales postings this lookup is unused, so the branch pays an avoidable database round-trip. Move the cheap SalesHeader.Invoice guard ahead of the Get.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
if SalesHeader.Invoice and PaymentMethod.Get(SalesHeader."Payment Method Code") then
if (PaymentMethod."Create Bills" or PaymentMethod."Invoices to Cartera") and
(not CarteraSetup.ReadPermission)
then
Error(CannotCreateCarteraDocErr);Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| TotalVATAmount: Decimal; | ||
|
|
||
| Text1100000Err: Label 'The Bill Group does not exist.'; | ||
| Text1100001Err: Label 'This Bill Group has already been printed. Proceed anyway?'; |
There was a problem hiding this comment.
Text1100001Err and Text1100004Err are used in Confirm() prompts, so their Err suffix is misleading.
Per AA0074, confirmation text should use the Qst suffix to match how the label is consumed.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| Text1100010Err: Label '%1 must be of a type that creates bills.', Comment = '%1=Field Name'; | ||
| Text1100011Err: Label 'A grouped document cannot be settled from a journal. Remove it from its group or payment order and try again.'; | ||
| Text1100012Err: Label 'cannot be filtered when posting recurring journals'; | ||
| Text1100013Err: Label 'Do you want to post the journal lines and print the posting report?'; |
There was a problem hiding this comment.
Labels Text1100013Err and Text1100014Err are passed to Confirm(), and Text1100016Err and Text1100017Err are passed to Message().
The Err suffix contradicts their actual usage; per AA0074, Confirm/StrMenu text should use Qst and Message text should use Msg.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| UntitledLbl: Label 'untitled'; | ||
| CollExpensesLbl: Label 'CollExpenses'; | ||
| OutOfRangeLbl: Label 'Out of Range'; |
There was a problem hiding this comment.
OutOfRangeLbl is raised with Error(), so its Lbl suffix contradicts the way the text is consumed.
Per AA0074, text passed to Error() should use the Err suffix.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| Text1100000Err: Label 'No bills have been found that can be settled. \'; | ||
| Text1100001Err: Label 'Please check that at least one open bill was selected.'; | ||
| Text1100002Err: Label 'Related to Bill,Related to Bill Group'; |
There was a problem hiding this comment.
Text1100002Err supplies the option text for StrMenu(), so the Err suffix does not match its actual use.
Per AA0074, labels consumed by Confirm/StrMenu should use the Qst suffix.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| SourceCodeSetup: Record "Source Code Setup"; | ||
| CarteraReportSelection: Record "Cartera Report Selections"; | ||
| Text1100001: Label 'Cartera Journal'; | ||
| CARJNLTok: Label 'CARJNL'; |
There was a problem hiding this comment.
CARJNLTok is a source-code token, not natural-language UI text.
Tokens like this should be declared with Locked = true so translation cannot rewrite the value.
| CARJNLTok: Label 'CARJNL'; | |
| CARJNLTok: Label 'CARJNL', Locked = true; |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Agentic PR Review - Round 4Recommendation: Request ChangesWhat this PR doesThis PR extracts Spanish Cartera code into The in-repo publisher check still resolves for the new subscribers I sampled, including Status of previous suggestions
New observations (commits since round 3)S5 - Current unit-test checks still fail Risk assessment and necessityRisk: High. The extraction touches posting-adjacent Cartera behavior, VAT entry data, ledger application, reversal, Navigate, and country-layer BaseApp hooks. A regression here can change posted amounts, blocked validations, or ledger navigation, and the current failing unit-test jobs show the branch is not yet proven safe. Necessity: AB#636627 is a Deliverable for extracting ES Cartera, so the broad extraction is expected. The change remains necessary, but it needs a green PR status and real automated coverage for the moved posting and Cartera flows before merge.
|
Agentic PR Review - Round 5Recommendation: Request ChangesWhat this PR doesThis PR extracts Spanish Cartera code into The sampled event wiring still lines up: Status of previous suggestions
New observations (commits since round 4)S6 - Payable unapply error text is reversed Risk assessment and necessityRisk: High. The extraction touches posting-adjacent Cartera behavior, VAT entry data, ledger application, reversal, Navigate, and country-layer BaseApp hooks. A regression here can change posted amounts, blocked validations, or ledger navigation, and the current failing unit-test jobs still show the branch is not proven safe. Necessity: AB#636627 is a Deliverable for extracting ES Cartera, so the broad slice is expected and the direction is still justified. The PR still needs green checks and real automated coverage for the moved Cartera posting flows before merge.
|
| keys | ||
| { | ||
| key(Key7000000; "Bill No.") | ||
| { | ||
| } | ||
| key(Key7000001; "Document Situation", "Document Status") | ||
| { | ||
| SumIndexFields = "Remaining Amount (LCY) stats.", "Amount (LCY) stats."; | ||
| } | ||
| } |
There was a problem hiding this comment.
The ES Cartera split moves Cust. Ledger Entry additions into CRTCustLedgerEntry.TableExt.al, but the extension only recreates keys on Bill No. and Document Situation/Document Status. The same diff still contains SetCurrentKey("Customer No.", "Document Type", "Document Situation", "Document Status") in the moved statistics pages and SetCurrentKey("Applies-to ID", "Document Type", "Document Situation", "Document Status") in CRTCustEntryApplyPostedEnt.Codeunit.al. Restore those composite keys on the tableextension so the moved Cartera flows keep the key shapes they still request.
| keys | |
| { | |
| key(Key7000000; "Bill No.") | |
| { | |
| } | |
| key(Key7000001; "Document Situation", "Document Status") | |
| { | |
| SumIndexFields = "Remaining Amount (LCY) stats.", "Amount (LCY) stats."; | |
| } | |
| } | |
| keys | |
| { | |
| key(Key7000000; "Bill No.") | |
| { | |
| } | |
| key(Key7000001; "Document Situation", "Document Status") | |
| { | |
| SumIndexFields = "Remaining Amount (LCY) stats.", "Amount (LCY) stats."; | |
| } | |
| key(Key7000002; "Customer No.", "Document Type", "Document Situation", "Document Status") | |
| { | |
| SumIndexFields = "Remaining Amount (LCY) stats.", "Amount (LCY) stats."; | |
| } | |
| key(Key7000003; "Applies-to ID", "Document Type", "Document Situation", "Document Status") | |
| { | |
| } | |
| } |
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
| @@ -9778,7 +9778,7 @@ codeunit 12 "Gen. Jnl.-Post Line" | |||
| end; | |||
|
|
|||
| [IntegrationEvent(false, false)] | |||
| local procedure OnInsertVATOnAfterAssignVATEntryFields(GenJnlLine: Record "Gen. Journal Line"; var VATEntry: Record "VAT Entry"; CurrExchRate: Record "Currency Exchange Rate") | |||
There was a problem hiding this comment.
OnInsertVATOnAfterAssignVATEntryFields is a shipped local integration event, so its existing parameters are still a subscriber contract. This change correctly adds VATPostingSetup, but it also changes the existing GenJnlLine parameter from by-value to var, which is subscriber-breaking even on a local publisher.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
local procedure OnInsertVATOnAfterAssignVATEntryFields(GenJnlLine: Record "Gen. Journal Line"; var VATEntry: Record "VAT Entry"; CurrExchRate: Record "Currency Exchange Rate"; var VATPostingSetup: Record "VAT Posting Setup")Knowledge:
- microsoft/knowledge/events/treat-local-and-internal-events-as-subscriber-contracts.md
- microsoft/knowledge/events/add-new-event-parameters-at-the-end.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
| /// <summary> | ||
| /// Event raised by OnSumSalesLines2OnAfterDivideAmount. | ||
| /// </summary> | ||
| /// <param name="OldSalesLine"></param> |
There was a problem hiding this comment.
This PR adds many event XML doc tags with empty parameter descriptions, for example /// <param name="OldSalesLine"></param> here. The same pattern repeats throughout the canonical W1 posting codeunits (498 added empty <param> tags in SalesPost.Codeunit.al and 281 in PurchPost.Codeunit.al) and in the ES/FR/APAC localization copies spot-checked. Empty tags do not explain the subscriber-facing contract; replace them with real parameter descriptions or omit the tag until it can be documented properly.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
| @@ -580,6 +581,13 @@ table 25 "Vendor Ledger Entry" | |||
| { | |||
| Caption = 'Prepayment'; | |||
| } | |||
| field(91; "Payment Terms Code"; Code[10]) | |||
There was a problem hiding this comment.
The new Payment Terms Code Normal field is missing a DataClassification property, so it falls back to ToBeClassified; table-level DataClassification = CustomerContent does not satisfy AS0016. The same omission is repeated in 11 localization copies of VendorLedgerEntry.Table.al.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
| @@ -853,6 +854,13 @@ table 21 "Cust. Ledger Entry" | |||
| Caption = 'Prepayment'; | |||
| ToolTip = 'Specifies if the related payment is a prepayment.'; | |||
| } | |||
| field(91; "Payment Terms Code"; Code[10]) | |||
There was a problem hiding this comment.
The new Payment Terms Code Normal field is missing a DataClassification property, so it falls back to ToBeClassified; table-level DataClassification = CustomerContent does not satisfy AS0016. The same omission is repeated in 10 localization copies of CustLedgerEntry.Table.al.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
| @@ -325,6 +325,10 @@ codeunit 90 "Purch.-Post" | |||
| OnRunOnBeforeMakeInventoryAdjustment(PurchHeader, GenJnlPostLine, ItemJnlPostLine, PreviewMode, PurchRcptHeader, PurchInvHeader, IsHandled); | |||
| if not IsHandled then | |||
| MakeInventoryAdjustment(); | |||
|
|
|||
| Clear(GenJnlPostLine); | |||
There was a problem hiding this comment.
This PR adds new posting-path behavior in W1 Purch.-Post/Sales-Post by clearing GenJnlPostLine and introducing OnAfterProcessPostingLines, and the same pattern is replicated into the 9 other changed PurchPost copies and 11 other changed SalesPost copies. The diff contains no Test*.Codeunit.al or other test changes, so add regression tests that post purchase and sales documents through the new hook and verify subscribers observe the intended state after GenJnlPostLine is cleared.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
2 participants
What & why
Linked work
Fixes AB#636627
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Risk & compatibility