Merge pull request Expensify#85493 from mukhrr/fix/83781

Allowing distance to be edited while editing

Author: youssef-lr

src/CONST/index.ts

@@ -6303,6 +6303,7 @@ const CONST = {
         RECEIPT_TAB_ID: 'ReceiptTab',
         IOU_REQUEST_TYPE: 'iouRequestType',
         DISTANCE_REQUEST_TYPE: 'distanceRequestType',
+        DISTANCE_EDIT_TYPE: 'distanceEditType',
         SPLIT_EXPENSE_TAB_TYPE: 'splitExpenseTabType',
         SPLIT: {
             AMOUNT: 'amount',

src/components/ReportActionItem/ReportActionItemImage.tsx

@@ -118,7 +118,13 @@ function ReportActionItemImage({
     const isMapDistanceRequest = !!transaction && isDistanceRequest(transaction) && !isManualDistanceRequest(transaction);
     const hasPendingWaypoints = transaction && isFetchingWaypointsFromServer(transaction);
     const hasErrors = !isEmptyObject(transaction?.errors) || !isEmptyObject(transaction?.errorFields?.route) || !isEmptyObject(transaction?.errorFields?.waypoints);
-    const showMapAsImage = isMapDistanceRequest && (hasErrors || hasPendingWaypoints);
+    // After a distance/rate edit the BE regenerates the receipt and invalidates the prior URL, but
+    // the local `receipt.source` only refreshes when the Pusher push arrives. Render `ConfirmedRoute`
+    // (which draws the map from `routes.coordinates`, independent of the URL) while any of these
+    // edits are pending so the thumbnail doesn't briefly try to load the now-404'd URL.
+    const pf = transaction?.pendingFields as Record<string, unknown> | undefined;
+    const hasPendingReceiptRegeneration = !!pf && (!!pf.distance || !!pf.merchant || !!pf.customUnitRateID);
+    const showMapAsImage = isMapDistanceRequest && (hasErrors || !!hasPendingWaypoints || hasPendingReceiptRegeneration);
 
     if (showMapAsImage) {
         return (

src/libs/ReportUtils.ts

@@ -4589,7 +4589,6 @@ function getTransactionDetails(
     }
 
     const report = getReportOrDraftReport(transaction?.reportID, undefined, 'report' in transaction ? transaction.report : undefined);
-    const isManualDistanceRequest = isManualDistanceRequestTransactionUtils(transaction);
     const isFromExpenseReport = (!isEmptyObject(report) && isExpenseReport(report)) || isPaidGroupPolicyPolicyUtils(policy);
 
     return {
@@ -4616,7 +4615,7 @@ function getTransactionDetails(
         convertedAmount: getConvertedAmount(transaction, isFromExpenseReport, transaction?.reportID === CONST.REPORT.UNREPORTED_REPORT_ID, allowNegativeAmount, disableOppositeConversion),
         postedDate: getFormattedPostedDate(transaction),
         transactionID: transaction.transactionID,
-        ...(isManualDistanceRequest && {distance: transaction.comment?.customUnit?.quantity ?? undefined}),
+        ...(isDistanceRequest(transaction) && {distance: transaction.comment?.customUnit?.quantity ?? undefined}),
     };
 }
 

src/libs/TransactionUtils/getDistanceInMeters.ts

@@ -5,16 +5,19 @@ import type {Unit} from '@src/types/onyx/Policy';
 // Get the distance in meters from the transaction.
 // This function is placed in a separate file to avoid circular dependencies.
 function getDistanceInMeters(transaction: OnyxInputOrEntry<Transaction>, unit: Unit | undefined) {
-    // If we are creating a new distance request, the distance is available in routes.route0.distance and it's already in meters.
-    if (transaction?.routes?.route0?.distance) {
-        return transaction.routes.route0.distance;
-    }
-
     // If the request is completed, transaction.routes is cleared and comment.customUnit.quantity holds the new distance in the selected unit.
     // We need to convert it from the selected distance unit to meters.
+    // This check takes priority because after a manual distance edit, routes.route0.distance may still
+    // hold a stale route-calculated value while quantity reflects the user's intended distance.
     if (transaction?.comment?.customUnit?.quantity && unit) {
         return DistanceRequestUtils.convertToDistanceInMeters(transaction.comment.customUnit.quantity, unit);
     }
+
+    // If we are creating a new distance request, the distance is available in routes.route0.distance and it's already in meters.
+    if (transaction?.routes?.route0?.distance) {
+        return transaction.routes.route0.distance;
+    }
+
     return 0;
 }
 

src/libs/TransactionUtils/index.ts

@@ -191,6 +191,17 @@ function hasGPSWaypoints(transaction: OnyxEntry<Transaction>) {
     return !!waypoint?.keyForList?.startsWith('gps');
 }
 
+/**
+ * Compare two waypoint collections by their addresses only (ignoring coordinates/names), which is
+ * the meaningful signal for "did the user change the route?". Numeric fields like lat/lng can drift
+ * due to rounding in transaction backups, so they're excluded.
+ */
+function haveWaypointAddressesChanged(oldWaypoints: WaypointCollection | undefined, newWaypoints: WaypointCollection | undefined): boolean {
+    const toAddresses = (collection: WaypointCollection | undefined) =>
+        Object.fromEntries(Object.entries(collection ?? {}).map(([key, waypoint]) => [key, waypoint && 'address' in waypoint ? waypoint.address : undefined]));
+    return !deepEqual(toAddresses(oldWaypoints), toAddresses(newWaypoints));
+}
+
 function isMapDistanceRequest(transaction: OnyxEntry<Transaction>): boolean {
     // This is used during the expense creation flow before the transaction has been saved to the server
     if (transaction && Object.hasOwn(transaction, 'iouRequestType')) {
@@ -719,16 +730,22 @@ function getUpdatedTransaction({
         }
         shouldStopSmartscan = true;
 
-        if (!transactionChanges.routes?.route0?.geometry?.coordinates) {
+        // A manual-distance edit re-sends unchanged waypoints; when they truly didn't change, leave
+        // `amount`/`modifiedAmount` to the sibling `distance` branch instead of zeroing them here.
+        const waypointsActuallyChanged = !deepEqual(transactionChanges.waypoints, transaction?.comment?.waypoints);
+
+        if (waypointsActuallyChanged && !transactionChanges.routes?.route0?.geometry?.coordinates) {
             // The waypoints were changed, but there is no route – it is pending from the BE and we should mark the fields as pending
             updatedTransaction.amount = CONST.IOU.DEFAULT_AMOUNT;
             updatedTransaction.modifiedAmount = CONST.IOU.DEFAULT_AMOUNT;
             updatedTransaction.modifiedMerchant = translateLocal('iou.fieldPending');
-        } else {
+        } else if (transactionChanges.routes?.route0?.geometry?.coordinates) {
             const mileageRate = DistanceRequestUtils.getRate({transaction: updatedTransaction, policy});
             const {unit, rate} = mileageRate;
 
-            const distanceInMeters = getDistanceInMeters(transaction, unit);
+            // Use route distance directly since waypoints changed and the route was recalculated.
+            // getDistanceInMeters prefers quantity which may hold a stale manually-edited value.
+            const distanceInMeters = transactionChanges.routes?.route0?.distance ?? getDistanceInMeters(transaction, unit);
             const amount = DistanceRequestUtils.getDistanceRequestAmount(distanceInMeters, unit, rate ?? 0);
             const updatedAmount = isFromExpenseReport || isUnReportedExpense ? -amount : amount;
             const updatedMerchant = DistanceRequestUtils.getDistanceMerchant(
@@ -746,6 +763,13 @@ function getUpdatedTransaction({
             updatedTransaction.amount = updatedAmount;
             updatedTransaction.modifiedAmount = updatedAmount;
             updatedTransaction.modifiedMerchant = updatedMerchant;
+
+            // Sync `customUnit.quantity` to the new route distance. Without this the prior manual
+            // quantity (set when the user edited distance manually before changing waypoints) would
+            // linger and drive `getDistanceInMeters`, since that helper prefers quantity over routes.
+            if (unit) {
+                lodashSet(updatedTransaction, 'comment.customUnit.quantity', roundToTwoDecimalPlaces(DistanceRequestUtils.convertDistanceUnit(distanceInMeters, unit)));
+            }
         }
     }
 
@@ -861,8 +885,11 @@ function getUpdatedTransaction({
 
     if (Object.hasOwn(transactionChanges, 'distance') && typeof transactionChanges.distance === 'number') {
         const distance = roundToTwoDecimalPlaces(transactionChanges.distance ?? 0);
+        // Capture before mutating quantity below; needed by the fallback amount computation.
+        const previousDistanceInMeters = getDistanceInMeters(transaction, transaction?.comment?.customUnit?.distanceUnit);
 
         lodashSet(updatedTransaction, 'comment.customUnit.quantity', distance);
+        lodashSet(updatedTransaction, 'routes.route0.distance', null);
         shouldStopSmartscan = true;
 
         const updatedMileageRate = DistanceRequestUtils.getRate({transaction: updatedTransaction, policy, useTransactionDistanceUnit: false});
@@ -884,9 +911,20 @@ function getUpdatedTransaction({
             isManualDistanceRequest(transaction),
         );
 
-        updatedTransaction.modifiedAmount = amount;
-        updatedTransaction.modifiedMerchant = updatedMerchant;
-        updatedTransaction.modifiedCurrency = updatedCurrency;
+        // No locally resolvable rate (e.g. track expense without policy loaded) → scale the previous
+        // amount by the distance ratio so the optimistic value isn't 0. `modifiedAmount` is `""` for
+        // unedited transactions, so coerce via Number() and fall through to `amount`.
+        const previousAmount = Number(transaction?.modifiedAmount) || transaction?.amount || 0;
+        const useFallback = !rate && !!previousDistanceInMeters && !!previousAmount && !!distanceInMeters;
+        if (useFallback) {
+            updatedTransaction.modifiedAmount = Math.round(previousAmount * (distanceInMeters / previousDistanceInMeters));
+            updatedTransaction.modifiedMerchant = updatedMerchant;
+            // Leave currency alone — without a resolvable rate we don't know the target currency.
+        } else {
+            updatedTransaction.modifiedAmount = amount;
+            updatedTransaction.modifiedMerchant = updatedMerchant;
+            updatedTransaction.modifiedCurrency = updatedCurrency;
+        }
     }
 
     if (Object.hasOwn(transactionChanges, 'odometerStart') && typeof transactionChanges.odometerStart === 'number') {
@@ -2881,6 +2919,7 @@ export {
     isReceiptBeingScanned,
     didReceiptScanSucceed,
     getValidWaypoints,
+    haveWaypointAddressesChanged,
     isDistanceRequest,
     isMapDistanceRequest,
     isGPSDistanceRequest,

src/libs/actions/IOU/UpdateMoneyRequest.ts

@@ -28,6 +28,7 @@ import {
     getClearedPendingFields,
     getMerchant,
     getUpdatedTransaction,
+    haveWaypointAddressesChanged,
     isDistanceRequest as isDistanceRequestTransactionUtils,
     isFetchingWaypointsFromServer,
     isOnHold,
@@ -530,7 +531,10 @@ function updateMoneyRequestDistance({
         // Don't sanitize waypoints here - keep all fields for Onyx optimistic data (e.g., keyForList)
         // Sanitization happens when building API params
         ...(waypoints && {waypoints}),
-        routes,
+        // Only include routes when the caller explicitly provided them. Including `routes: undefined`
+        // would make the optimistic merge wipe the existing route, briefly blanking the map thumbnail
+        // and report preview before the server response restores it.
+        ...(routes !== undefined && {routes}),
         ...(distance && {distance}),
         ...(odometerStart !== undefined && {odometerStart}),
         ...(odometerEnd !== undefined && {odometerEnd}),
@@ -975,20 +979,46 @@ function getUpdateMoneyRequestParams(params: GetUpdateMoneyRequestParamsType): U
         >
     > = [];
 
-    // Step 1: Set any "pending fields" (ones updated while the user was offline) to have error messages in the failureData
-    const pendingFields: OnyxTypes.Transaction['pendingFields'] = Object.fromEntries(Object.keys(transactionChanges).map((key) => [key, CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE]));
+    // Step 1: Get the transaction being updated
+    const transaction = getAllTransactions()?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
+
+    // The manual-distance submit path always sends waypoints to keep the BE in sync, even when the user
+    // only edited the distance number. Detect whether the addresses actually changed so we can skip the
+    // optimistic side effects (pending field, route clearing, render-path swap to interactive map) that
+    // would otherwise make the parent map briefly disappear on a pure distance edit.
+    const hasWaypointAddressesChanged = 'waypoints' in transactionChanges && haveWaypointAddressesChanged(transaction?.comment?.waypoints, transactionChanges.waypoints);
+    const shouldSuppressWaypointsAsPending = 'waypoints' in transactionChanges && !hasWaypointAddressesChanged;
+
+    // Step 2: Set any "pending fields" (ones updated while the user was offline) to have error messages in the failureData
+    const pendingFields: OnyxTypes.Transaction['pendingFields'] = Object.fromEntries(
+        Object.keys(transactionChanges)
+            .filter((key) => !(shouldSuppressWaypointsAsPending && key === 'waypoints'))
+            .map((key) => [key, CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE]),
+    );
+    // Flag `merchant` as pending on any edit that causes the BE to regenerate the receipt
+    // (waypoints / distance / rate). `merchant` isn't in `transactionChanges`, so the success-data
+    // merge won't clear it via `clearedPendingFields` — it persists through the gap between API ack
+    // and the Pusher push that delivers the new `receipt.source`. The Pusher push then clears all
+    // pendingFields atomically together with the new URL, eliminating the broken-image flash.
+    // It also drives the Distance row's offline-feedback strikethrough for pure distance edits.
+    if ('waypoints' in transactionChanges || 'distance' in transactionChanges || 'customUnitRateID' in transactionChanges) {
+        pendingFields.merchant = CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE;
+    }
     const clearedPendingFields = getClearedPendingFields(transactionChanges);
     const errorFields = Object.fromEntries(Object.keys(pendingFields).map((key) => [key, getMicroSecondOnyxErrorWithTranslationKey('iou.error.genericEditFailureMessage')]));
 
-    // Step 2: Get all the collections being updated
-    const transaction = getAllTransactions()?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
-
     const isTransactionOnHold = isOnHold(transaction);
     const isFromExpenseReport = isExpenseReport(iouReport) || isInvoiceReportReportUtils(iouReport);
+    // Drop the waypoints from the changes fed to getUpdatedTransaction when they didn't actually change,
+    // so we skip the waypoints branch that flips isLoading and clobbers amount/merchant before being
+    // re-overridden by the distance branch.
+    const transactionChangesForOptimisticMerge: TransactionChanges = shouldSuppressWaypointsAsPending
+        ? Object.fromEntries(Object.entries(transactionChanges).filter(([key]) => key !== 'waypoints'))
+        : transactionChanges;
     const updatedTransaction: OnyxEntry<OnyxTypes.Transaction> = transaction
         ? getUpdatedTransaction({
               transaction,
-              transactionChanges,
+              transactionChanges: transactionChangesForOptimisticMerge,
               isFromExpenseReport,
               isSplitTransaction,
               policy,
@@ -1003,6 +1033,12 @@ function getUpdateMoneyRequestParams(params: GetUpdateMoneyRequestParamsType): U
 
     const dataToIncludeInParams: Partial<TransactionDetails> = Object.fromEntries(Object.entries(transactionDetails ?? {}).filter(([key]) => key in transactionChanges));
 
+    // Preserve the caller's full-precision distance so the server doesn't fire `increasedDistance`
+    // when the rounded display value drifts above the exact route distance.
+    if ('distance' in transactionChanges && typeof transactionChanges.distance === 'number') {
+        dataToIncludeInParams.distance = transactionChanges.distance;
+    }
+
     const apiParams: UpdateMoneyRequestParams = {
         ...dataToIncludeInParams,
         reportID: iouReport?.reportID,
@@ -1017,6 +1053,9 @@ function getUpdateMoneyRequestParams(params: GetUpdateMoneyRequestParamsType): U
     // For split transactions, the merchant and amount are already computed in transactionChanges,
     // so we can build a valid optimistic MODIFIED_EXPENSE even when waypoints are pending.
     const hasSplitDistanceMessageFields = !!isSplitTransaction && hasModifiedMerchant && hasModifiedAmount;
+    // When distance is provided alongside waypoints (route was already calculated), we have valid
+    // merchant/amount data to build the optimistic report action instead of waiting for the server.
+    const hasDistanceWithWaypoints = hasPendingWaypoints && 'distance' in transactionChanges;
     if (transaction && updatedTransaction && (hasPendingWaypoints || hasModifiedDistanceRate)) {
         // Delete the draft transaction when editing waypoints when the server responds successfully and there are no errors
         successData.push({
@@ -1052,7 +1091,11 @@ function getUpdateMoneyRequestParams(params: GetUpdateMoneyRequestParamsType): U
     const updatedReportAction = shouldBuildOptimisticModifiedExpenseReportAction
         ? buildOptimisticModifiedExpenseReportAction(transactionThreadReport, transaction, transactionChanges, isFromExpenseReport, policy, updatedTransaction, allowNegative)
         : null;
-    if ((!hasPendingWaypoints || hasSplitDistanceMessageFields) && !(hasModifiedDistanceRate && isFetchingWaypointsFromServer(transaction)) && updatedReportAction) {
+    if (
+        (!hasPendingWaypoints || hasSplitDistanceMessageFields || hasDistanceWithWaypoints) &&
+        !(hasModifiedDistanceRate && isFetchingWaypointsFromServer(transaction)) &&
+        updatedReportAction
+    ) {
         apiParams.reportActionID = updatedReportAction.reportActionID;
 
         optimisticData.push({
@@ -1264,15 +1307,22 @@ function getUpdateMoneyRequestParams(params: GetUpdateMoneyRequestParamsType): U
         apiParams.attendees = JSON.stringify(apiParams?.attendees);
     }
 
-    // Clear out the error fields and loading states on success
+    // Clear out the error fields and loading states on success.
+    // Only clear `routes` when waypoints/rate changed (the server will push a fresh route via Pusher).
+    // For pure distance edits the route is unchanged, and clearing it would make the map briefly disappear.
+    // When the caller already supplied a valid optimistic route (waypoint edit with route pre-fetched
+    // locally), keep it so the receipt thumbnail and ConfirmedRoute don't flicker between success and
+    // the Pusher route push.
+    const hasValidOptimisticRoute = !!transactionChanges.routes?.route0?.geometry?.coordinates?.length;
+    const shouldClearRoutes = (hasWaypointAddressesChanged || hasModifiedDistanceRate) && !hasValidOptimisticRoute;
     successData.push({
         onyxMethod: Onyx.METHOD.MERGE,
         key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
         value: {
             pendingFields: clearedPendingFields,
             isLoading: false,
             errorFields: null,
-            routes: null,
+            ...(shouldClearRoutes && {routes: null}),
         },
     });
 
@@ -1341,9 +1391,13 @@ function getUpdateMoneyRequestParams(params: GetUpdateMoneyRequestParamsType): U
         if (hasPendingWaypoints) {
             optimisticViolations = optimisticViolations.filter((violation) => violation.name !== CONST.VIOLATIONS.NO_ROUTE);
         }
-        if (hasModifiedDistanceRate || hasModifiedDistance) {
+        if (hasModifiedDistanceRate || hasModifiedDistance || hasPendingWaypoints) {
+            // Clear stale distance-related violations while the server reprocesses.
+            // The server will re-evaluate and re-add any that legitimately apply.
             optimisticViolations = optimisticViolations.filter(
-                (violation) => !(violation.name === CONST.VIOLATIONS.MODIFIED_AMOUNT && violation.data?.type === CONST.MODIFIED_AMOUNT_VIOLATION_DATA.DISTANCE),
+                (violation) =>
+                    !(violation.name === CONST.VIOLATIONS.MODIFIED_AMOUNT && violation.data?.type === CONST.MODIFIED_AMOUNT_VIOLATION_DATA.DISTANCE) &&
+                    violation.name !== CONST.VIOLATIONS.INCREASED_DISTANCE,
             );
         }