Sales Order API: restore Completely Shipped after redistributing invoice discounts

[jeffreybulanadi]

Summary

Fixes #29071

Root Cause

In OnAfterGetRecord of both APIV1 - Sales Orders (page 20000) and APIV2 - Sales Orders (page 30028), calling GraphMgtSalesOrderBuffer.RedistributeInvoiceDiscounts(Rec) has an unintended side effect on Rec.\Completely Shipped\`.

The call chain is:

1. RedistributeInvoiceDiscounts calls SalesCalcDiscountByType.ResetRecalculateInvoiceDisc(SalesHeader)
2. Which calls SalesLine.ModifyAll(\Recalculate Invoice Disc.\, false) with a filter on Recalculate Invoice Disc. = Yes
3. This fires OnAfterModifySalesLine on codeunit 5496, which calls UpdateCompletelyShipped(Rec) where Rec still carries the Recalculate Invoice Disc. = Yes filter
4. UpdateCompletelyShipped calls SearchSalesLine.CopyFilters(SalesLine), inheriting the spurious filter
5. SearchSalesLine.IsEmpty() returns rue because no lines have Recalculate Invoice Disc. = Yes after the ModifyAll
6. Rec.\Completely Shipped\ := SearchSalesLine.IsEmpty() is set to rue incorrectly

Fix

Save the value of Rec.\Completely Shipped\` from the database before invoking RedistributeInvoiceDiscounts and restore it afterward. The invoice discount redistribution function has no business modifying shipping status; this ensures the API returns the actual shipment state of the sales lines.

Files Changed

• Apps/W1/APIV2/app/src/pages/APIV2SalesOrders.Page.al
• Apps/W1/APIV1/app/src/pages/APIV1SalesOrders.Page.al

How to Test

1. Create a Sales Order with one or more sales lines (do not ship any lines).
2. Call GET /api/v2.0/companies({id})/salesOrders({id}).
3. Verify ullyShipped is alse.
4. Before this fix, ullyShipped would incorrectly return rue.

Fixes AB#632884

[jeffreybulanadi]

Updated the fix to use Rec.Find() after RedistributeInvoiceDiscounts, as suggested. This re-reads the entire Sales Header from the database, which addresses both concerns raised:

1. No risk of overwriting legitimate future updates - the database is the source of truth, and Rec.Find() always reflects the actual persisted state.
2. All fields are restored, not just Completely Shipped - any other header field that may have been corrupted in-memory by the call chain is also corrected.

SetCalculatedFields() sets only page-level Text variables (BillingPostalAddressJSONText, ShippingPostalAddressJSONText, CurrencyCodeTxt, PartialOrderShipping) - not Rec fields - so Rec.Find() does not undo that work.

[JesperSchulz]

Thank you for your contribution and for taking the time to submit this pull request.

At this stage, we’re not able to accept the change. We’d like to revisit this proposal once the migration to BCApps is complete though. At that point, we’ll be in a better position to properly evaluate and validate changes like this.

We appreciate your contribution and encourage you to resubmit or revisit this after the transition has been finalized.

[nikolakukrika]

Please open a BC idea for this and lets get votes. One of the issues here is that we are re-reading the record from the OnAfterGetRecord trigger which is not something we do and can lead to the issues down the line. If we are refactoring this code, it would be better to introduce a more stabile solution. The best would be if we could redistribute discounts always, or maybe from a different trigger/background job or a different system. Patching from the OnAfterGetRecord is a workaround.