[Vendor] [Quality Management] Agentic review - Error Handling: Commit() Before Posting Without Rollback on Failure

[PredragMaricic]

Describe the issue

ISSUE 6.1: Commit() Before Posting Without Rollback on Failure

Severity: CRITICAL
Category: Transaction Management & Data Integrity
Files: src/Integration/Inventory/QltyItemJournalManagement.Codeunit.al

Problem:
The PostWarehouseJournal procedure uses Commit() before each posting attempt but has no rollback mechanism when posting fails, leading to partial success scenarios with no recovery path.

Code Location (Lines 181-202):

procedure PostWarehouseJournal(...)
var
    ErrorMessage: Text;
    ErrorStack: Text;
    ErroredLinesErrorMessages: List of [Text];
    ConsideredLines: List of [Text];
begin
    // ... setup code ...
   
    if WarehouseJournalLine.FindSet() then begin
        repeat
            ToPostWarehouseJournalLine := WarehouseJournalLine;
            ToPostWarehouseJournalLine.SetRecFilter();
            if not ConsideredLines.Contains(Format(ToPostWarehouseJournalLine.RecordId())) then begin
                Commit();  // DEFECT: Commits before posting - no rollback if posting fails
                if not WhseJnlRegisterBatch.Run(ToPostWarehouseJournalLine) then begin
                    ErrorMessage := GetLastErrorText();  // DEFECT: Gets error but doesn't clear it
                    ErrorStack := GetLastErrorCallStack();
                    ErroredLinesErrorMessages.Add(ErrorMessage);
                    QltyNotificationMgmt.NotifyDocumentCreationFailed(..., ErrorMessage, ...);
                end;
            end;
            ConsideredLines.Add(Format(WarehouseJournalLine.RecordId()))
        until WarehouseJournalLine.Next() = 0;
        AllLinesPosted := ErroredLinesErrorMessages.Count() = 0;
    end;
end;

Critical Issues:

1. No Rollback: Commit() executed before posting - if posting fails, committed changes cannot be rolled back
2. Partial Success: Some lines may post while others fail, leaving system in inconsistent state
3. No Error Recovery: Failed lines are logged but not retried or handled
4. Data Integrity Risk: Test records may be modified before journal lines created/posted

Failure Scenario:

1. User executes disposition with 10 journal lines
2. Commit() executes before line 1 posts
3. Lines 1-5 post successfully
4. Line 6 fails (inventory shortage, locked record, etc.)
5. Lines 7-10 are not processed
6. Result: Partial posting - 5 lines posted, 5 lines not posted
7. Test status may be changed (committed) but disposition incomplete
8. No way to rollback or complete the operation

Recommended Fix:

procedure PostWarehouseJournal(var QltyInspectionTestHeader: Record "Qlty. Inspection Test Header"; var TempInstructionQltyDispositionBuffer: Record "Qlty. Disposition Buffer" temporary; var WarehouseJournalLine: Record "Warehouse Journal Line") AllLinesPosted: Boolean
var
    ToPostWarehouseJournalLine: Record "Warehouse Journal Line";
    WhseJnlRegisterBatch: Codeunit "Whse. Jnl.-Register Batch";
    QltyNotificationMgmt: Codeunit "Qlty. Notification Mgmt.";
    ErrorMessage: Text;
    ErrorStack: Text;
    ErroredLines: List of [RecordId];
    ErroredLinesErrorMessages: Dictionary of [RecordId, Text];
    PostingSucceeded: Boolean;
begin
    ClearLastError();
    AllLinesPosted := false;
   
    if WarehouseJournalLine."Line No." <> 0 then
        WarehouseJournalLine.SetRecFilter();

    if WarehouseJournalLine.GetFilters() = '' then
        Error(NoFiltersWereSuppliedWhenTryingToPostAWarehouseJournalLineErr);

    // Attempt to post all lines in single transaction
    if WarehouseJournalLine.FindSet() then begin
        repeat
            ToPostWarehouseJournalLine := WarehouseJournalLine;
            ToPostWarehouseJournalLine.SetRecFilter();
           
            ClearLastError();
            PostingSucceeded := WhseJnlRegisterBatch.Run(ToPostWarehouseJournalLine);
           
            if not PostingSucceeded then begin
                ErrorMessage := GetLastErrorText();
                ErrorStack := GetLastErrorCallStack();
                ErroredLines.Add(ToPostWarehouseJournalLine.RecordId());
                ErroredLinesErrorMessages.Add(ToPostWarehouseJournalLine.RecordId(), ErrorMessage);
            end;
        until WarehouseJournalLine.Next() = 0;
       
        // If any errors, rollback by not committing
        if ErroredLines.Count() > 0 then begin
            // Notify user of all failed lines
            foreach ErrorLine in ErroredLines do begin
                if ToPostWarehouseJournalLine.GetBySystemId(ErrorLine) then
                    QltyNotificationMgmt.NotifyDocumentCreationFailed(
                        QltyInspectionTestHeader,
                        TempInstructionQltyDispositionBuffer,
                        PostedWarehouseJournalEntryDocumentTypeLbl,
                        ErroredLinesErrorMessages.Get(ErrorLine),
                        ToPostWarehouseJournalLine);
            end;
           
            // Error to rollback transaction
            Error(PartialPostingNotAllowedErr, ErroredLines.Count(), WarehouseJournalLine.Count());
        end;
       
        AllLinesPosted := true;
        Commit();  // Only commit if ALL lines posted successfully
    end;

    OnAfterPostWarehouseJournal(QltyInspectionTestHeader, TempInstructionQltyDispositionBuffer, WarehouseJournalLine, AllLinesPosted);
end;

var
    PartialPostingNotAllowedErr: Label '%1 of %2 warehouse journal lines failed to post. The entire operation has been rolled back. Please correct the errors and try again.', Comment = '%1=failed count, %2=total count';

Same Issue in:

• Line 359: Similar pattern with Commit() before item journal posting
• src/Setup/SetupWizard/QltyManagementSetupWizard.Page.al lines 482, 508, 560, 566, 568, 672
• src/Integration/Warehouse/QltyWhseGenRuleWizard.Page.al line 390
• src/Integration/Receiving/QltyRecGenRuleWizard.Page.al line 693

Expected behavior

Steps to reproduce

Additional context

I will provide a fix for a bug

• I will provide a fix for a bug

[insightworks007]

Before proceeding with the proposed change, I think it’s important to consider the real-world operational implications of this code path.

As indicated by the implementation, and the reason this method exists, is because the current behavior is intentional. The procedure is explicitly designed to allow partial success rather than enforcing an all-or-nothing transactional model.

The practical scenario this supports is common in warehouse and quality workflows. For example:
-A disposition or automation attempts to perform a warehouse reclassification to move failed inventory into a quarantine bin.
-This creates multiple journal lines (e.g., the same lot exists in multiple bins).
-One or more lines may fail due to legitimate reasons such as inventory shortages, locked records, or bin conflicts.

Under the current design:
-Successfully post-able lines are committed.
-Failed lines are left unposted and visible in the journal for a person to address.
-An error is raised for the failing line, allowing a human to investigate and resolve the issue.

This preserves forward progress while still surfacing the problem.

Changing this to a single transactional scope with rollback would introduce undesirable real-world side effects. In particular:
-A single failing line would roll back all successful postings, which is not desirable in this specific context
-That rollback could also undo related state changes, such as test completion or disposition status. (i.e a single line that fails in a warehouse reclass that's a result of an automation from an inspection changing to finished should not prevent the status change to finished).

The net result would be less operational resilience, not more, especially in high-volume or semi-automated environments.

It’s possible this was already considered and previously discussed and the proposed resolution consciously chosen, but my suspicion is that this issue was flagged by a static code analysis tool that detected a Commit() without sufficient awareness of the surrounding business context.