[HOLD for payment 2023-12-12] [$500] Distance – Inconsistency when editing waypoints

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.3.90.1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @paultsimura
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698158967101059

Action Performed:

1. Click FAB - Request Money - Distance
2. Fill in Start and Finish points
3. Create the request
4. Navigate to the created request
5. Click "distance" to open "Edit distance" modal
6. Add a waypoint and click "save"
7. Click "distance" again and remove any of the waypoints
8. Click "Save"
9. Click "Amount" and modify the amount
10. Reload the page or switch to another chat and back

Expected Result:

After the amount is modified manually, it should persist.

Actual Result:

The modified amount is reverted to the previous value

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

Screen.Recording.2023-10-24.at.18.11.20.mov

iOS: Native

RPReplay_Final1698162445.MP4

iOS: mWeb Safari

RPReplay_Final1698162580.MP4

MacOS: Chrome / Safari

Screen.Recording.2023-10-24.at.16.55.23.mp4

Recording.5141.mp4

MacOS: Desktop

Screen.Recording.2023-10-24.at.16.55.23.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~014a1428f6ef24d6ec
• Upwork Job ID: 1716905913390137344
• Last Price Increase: 2023-11-07
• Automatic offers:
• DylanDylann | Contributor | 27653898
• paultsimura | Reporter | 27653901

[melvin-bot]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~014a1428f6ef24d6ec

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[kbecciv]

Proposal by: @paultsimura
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1698158967101059

Proposal

Please re-state the problem that we are trying to solve in this issue.

When the distance request is modified: added, removed waypoint, and manually changed amount, it's reverted, and the removed waypoint is back.

What is the root cause of that problem?

When executing the UpdateDistanceRequest API call, we build the Onyx data with waypoints of the updated transaction here:

App/src/libs/TransactionUtils.ts

Lines 148 to 151 in b1ae0c2

	if (Object.hasOwn(transactionChanges, 'waypoints')) {
	updatedTransaction.modifiedWaypoints = transactionChanges.waypoints;
	shouldStopSmartscan = true;
	}

Let's take a look at what happens when we remove one waypoint.
The simple merge operation for the following object:

{
    "comment": {
        "type": "customUnit",
        "customUnit": {
            "name": "Distance"
        },
        "waypoints": {
            "waypoint0": { wp-0 },
            "waypoint1": { wp-1 },
            "waypoint2": { wp-2 }
        }
    }
}

To the following object:

{
    "comment": {
        "type": "customUnit",
        "customUnit": {
            "name": "Distance"
        },
        "waypoints": {
            "waypoint0": { wp-0 },
            "waypoint1": { wp-1 },
        }
    }
}

...will result in the same as pre-merge:

{
    "comment": {
        "type": "customUnit",
        "customUnit": {
            "name": "Distance"
        },
        "waypoints": {
            "waypoint0": { wp-0 },
            "waypoint1": { wp-1 },
            "waypoint2": { wp-2 }
        }
    }
}

This is because the waypoint2 merge key was not passed to Onyx, so it's ignored, as Onyx replaces values key-by-key to the latest nested object. So this merge operation modifies only waypoint0 and waypoint1, leaving the waypoint2 untouched.
These waypoints are later used to build the next optimistic transaction when modifying the request, which causes the buggy behavior.

What changes do you think we should make in order to solve the problem?

We should explicitly mark the deleted waypoints as null when passing them to Onyx.
First, create a new function in TransactionUtils:

function getReplacementWaypoints(transaction: Transaction, updatedWaypoints: WaypointCollection): WaypointCollection {
    const existingWaypoints = getWaypoints(transaction) ?? {};
    if (Object.keys(updatedWaypoints).length >= Object.keys(existingWaypoints).length) {
        return updatedWaypoints;
    }
    return lodashReduce(existingWaypoints, (result, value, key) => ({
        ...result,
        // Explicitly set the non-existing waypoints to null so that Onyx can remove them
        [key]: updatedWaypoints[key] ?? null,
    }), {});
}

Second, use it here when building the updated transaction:

    if (Object.hasOwn(transactionChanges, 'waypoints')) {
-       updatedTransaction.modifiedWaypoints = transactionChanges.waypoints;
+       updatedTransaction.modifiedWaypoints = getReplacementWaypoints(updatedTransaction, transactionChanges.waypoints!!);
        shouldStopSmartscan = true;
    }

Third, we are also passing these waypoints to MapBox on the API call. We pass them as a stringified JSON, and there we should remove the null-ish waypoints:

App/src/libs/actions/IOU.js

Line 697 in 3b86da6

waypoints: JSON.stringify(transactionDetails.waypoints),

We should use the following:

waypoints: JSON.stringify(TransactionUtils.getValidWaypoints(transactionDetails.waypoints, true)),

Similar to what we do in the createDistanceRequest:

App/src/libs/actions/IOU.js

Line 649 in 3b86da6

waypoints: JSON.stringify(TransactionUtils.getValidWaypoints(transaction.comment.waypoints, true)),

Result:

Screen.Recording.2023-10-20.at.11.23.52.mov

What alternative solutions did you explore? (Optional)

Alternatively, we can only modify the Transaction.removeWaypoint function to set not only waypoints, but also the modifiedWaypoints:

    let newTransaction: Transaction = {
        ...transaction,
        comment: {
            ...transaction.comment,
            waypoints: reIndexedWaypoints,
        },
        modifiedWaypoints: reIndexedWaypoints
    };

App/src/libs/actions/Transaction.ts

Lines 125 to 134 in 01016dc

	// Onyx.merge won't remove the null nested object values, this is a workaround
	// to remove nested keys while also preserving other object keys
	// Doing a deep clone of the transaction to avoid mutating the original object and running into a cache issue when using Onyx.set
	let newTransaction: Transaction = {
	...transaction,
	comment: {
	...transaction.comment,
	waypoints: reIndexedWaypoints,
	},
	};

This solution is simpler and will have the same effect, but I'm not sure if we are OK with modifying the modifiedWaypoints field before the actual API call – would appreciate C+ input on this.

[DylanDylann]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Distance – Inconsistency when editing waypoints

What is the root cause of that problem?

For example, on ONYX we have:

waypoints: {
   "waypoint1": {before},
   "waypoint2": {before},
   "waypoint3:" {before }
}

When remove a waypoint we will update waypoints field like this
method: Merge
optimisticData:

waypoints: {
   "waypoint1": {after},
   "waypoint2": {after},
}

As how ONYX.merge work, the result will be

waypoints: {
   "waypoint1": {after},
   "waypoint2": {after},
   "waypoint3:" {before }.  //**still remain**
}

This problem happens with transaction.modifiedWaypoints that causes this bug

What changes do you think we should make in order to solve the problem?

Note that: the same problem doesn't happen with transaction.comment.waypoint field because we are using SET method instead of MERGE method

App/src/libs/actions/Transaction.ts

Line 154 in cd3c863

Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, newTransaction);

So I suggest 2 solutions for this bug, for consistency we need to confirm which solution we should come up with and then using that solution in both places removeWaypoint and getUpdatedTransaction:

1. When removing the waypoint we will use SET method instead of MERGE method like we did here

   App/src/libs/actions/Transaction.ts

   Line 154 in cd3c863

   Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, newTransaction);

2. When removing the waypoint we reset waypoint to null if transactionChanges.waypoints is not exist
   in here
   

   App/src/libs/TransactionUtils.ts

   Line 149 in a8482e2

   updatedTransaction.modifiedWaypoints = transactionChanges.waypoints;

   
   update like this

const waypointKeys = Object.keys(transaction.modifiedWaypoints ?? {})
        const reIndexedWaypoints: WaypointCollection = {};
        waypointKeys.forEach((waypoint, index) => {
            reIndexedWaypoints[`waypoint${index}`] = transactionChanges.waypoints && transactionChanges.waypoints[`waypoint${index}`] ? transactionChanges.waypoints[`waypoint${index}`] : null
        });
       
        updatedTransaction.modifiedWaypoints = reIndexedWaypoints;

Then using getValidWaypoints function to remove null value when calling API

What alternative solutions did you explore? (Optional)

[paultsimura]

@DylanDylann that's basically 1:1 my proposal, just rephrased, isn't it?

[DylanDylann]

@paultsimura There are some differences between 2 proposals:

1. My first solution: using SET method instead of MERGE method (you didn't mentioend, right?)
2. In the second solution I suggest to write some code line instead of make new function (I think It is clean and clearer) and suggest to make code consistency

[paultsimura]

1. My first solution: using SET method instead of MERGE method (you didn't mentioend, right?)

This is exactly what I suggest as an alternative solution 🙂
Anyway, I only wanted to understand the difference, maybe I was missing something. The further is up to C+

[DylanDylann]

@paultsimura in your alternative solution, you suggest update modifiedWaypoints in removeWayPoint function. My proposal suggest using SET method instead of MERGE method for updated transaction in optimistic data. It is different totally

[jliexpensify]

Bump @Santhosh-Sellavel for reviews please!

Also, I did some research and we should allow for editing distance requests, so this is a bug.

[jliexpensify]

Bump @Santhosh-Sellavel for reviews!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[Santhosh-Sellavel]

On my list today

[Santhosh-Sellavel]

Did some investigation today, still need to dig more. Will post an review update tomorrow!

[jliexpensify]

@Santhosh-Sellavel any updates?

[Santhosh-Sellavel]

Sorry no update yet. Will post update before monday

[melvin-bot]

@jliexpensify @Santhosh-Sellavel this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

@jliexpensify, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[jliexpensify]

@rushatgabhane @situchan what's the verdict here? @neil-marcellini - is there a need for you to also weigh in?

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[bondydaa]

brought up a discussion here https://expensify.slack.com/archives/C01GTK53T8Q/p1702656464396679 about potentially reverting #31340 since it's caused a couple of blockers, please chime in there.

[bondydaa]

okay to recap the thread, we are going to revert #31340.

Tim and Neil are still discussing alternative solutions to solve this GH though in the thread so feel free to chime in there.

[jliexpensify]

Thanks Bondy! I'll hold on the conclusion of that thread, but I'm also OOO from the 19th - 28th so will handle payments after then.

[rushatgabhane]

It was said in a couple of places (e.g. here #31340 (review)) that it's quite a risky change but we should be ok considering that we correctly build an updated transaction object

@paultsimura thank you for the detailed explaination

[paultsimura]

According to this thread, we should put this issue on hold until #28358 is done

[melvin-bot]

This issue has not been updated in over 15 days. @neil-marcellini, @rushatgabhane, @jliexpensify, @DylanDylann eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[jliexpensify]

Still on hold, right @paultsimura ?

[paultsimura]

Still on hold, right @paultsimura ?

Yes, the big 1:1:1 refactoring is currently in active development mode

[melvin-bot]

@neil-marcellini, @rushatgabhane, @jliexpensify, @DylanDylann, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

[neil-marcellini]

The refactor completed 3 days ago, so we can take this off hold finally

[jliexpensify]

Wow, old issue! Are @DylanDylann and @rushatgabhane eligible for payment here?

[paultsimura]

Wow, old issue! Are @DylanDylann and @rushatgabhane eligible for payment here?

No, the solution here was reverted as ot caused multiple regressions. After the revert, the issue was held for the 1:1:1 refactor for a second try.

[jliexpensify]

Thank you for confirming! @neil-marcellini can we close this then, since there are no payments required?

[jliexpensify]

Closing this as no payments required.