[HOLD #37560][$500] Distance Expense - Impossible to remove a waypoint when editing a distance request offline

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: v1.4.26-1 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught during regression testing, add the test name, ID and link from TestRail: N/A 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/p1705505264249909 & https://expensify.slack.com/archives/C01GTK53T8Q/p1705484273717249

Action Performed:

1. Create a distance request with 3 waypoints
2. Navigate to the distance request details
3. Disable the internet connection
4. Click on the distance field to edit
5. Click on a waypoint
6. Click on the 3 dot menu > Delete waypoint
7. Hit Save
8. Go back online

Expected Result:

The waypoint should be deleted.

Actual Result:

The waypoint is not deleted after hitting the option to delete. After going back online, the 3 waypoints are still displayed.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/44479856/11060808-8073-4d2a-b140-af19a804c88b

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01979a5064cc9154e6
• Upwork Job ID: 1747739146196148224
• Last Price Increase: 2024-01-24
• Automatic offers:
• alitoshmatov | Reviewer | 28124284
• paultsimura | Contributor | 28124285

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01979a5064cc9154e6

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[isagoico]

Posting @paultsimura proposal here:

Proposal

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

When editing a Distance request offline, the waypoint cannot be removed

What is the root cause of that problem?

This is because we do not handle the Editing flow correctly when passing the isDraft parameter to Transaction.removeWaypoint: we pass true always: https://github.com/Expensify/App/blob/1f72b5f1ae31765183a2a7f51a450f1e9e27b1ff/src/pages/iou/request/step/IOURequestStepWaypoint.js#L145 https://github.com/Expensify/App/blob/1f72b5f1ae31765183a2a7f51a450f1e9e27b1ff/src/pages/iou/request/step/IOURequestStepWaypoint.js#L165 When we are creating a new request, we correctly remove the waypoint from a draft transaction. But when we edit an existing transaction, we should modify the real, not draft transaction.

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

We should pass the isDraft parameter conditionally depending on the IOU action, similar to how we pass it to Transaction.saveWaypoint: action === CONST.IOU.ACTION.CREATE https://github.com/Expensify/App/blob/1f72b5f1ae31765183a2a7f51a450f1e9e27b1ff/src/pages/iou/request/step/IOURequestStepWaypoint.js#L138

What alternative solutions did you explore? (Optional)

[isagoico]

@tgolen confirmed the bug and the proposed fix here: https://expensify.slack.com/archives/C049HHMV9SM/p1705505264249909

[Vlad-AIMaster]

I will take a look at the issue and come up with a solution.

[melvin-bot[bot]]

📣 @Vlad-AIMaster! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

   Contributor details
   Your Expensify account email: <REPLACE EMAIL HERE>
   Upwork Profile Link: <REPLACE LINK HERE>

[DylanDylann]

Proposal

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

The waypoint is not deleted after hitting the option to delete. After going back online, the 3 waypoints are still displayed.

What is the root cause of that problem?

The RCA here

https://github.com/Expensify/App/blob/4da7fdab30857d0934d8a8035f9e1c773385d950/src/pages/iou/request/step/IOURequestStepWaypoint.js#L145

https://github.com/Expensify/App/blob/4da7fdab30857d0934d8a8035f9e1c773385d950/src/pages/iou/request/step/IOURequestStepWaypoint.js#L165

We always pass isDraft: true to removeWaypoint function, so that new updates will be saved to the draft transaction

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

After refactoring, we use action field from the route to indentify if users create or edit their request. So we also need to rely on action field from the route to determine whether we should save new data into a transaction or a draft transaction

So, in these places

https://github.com/Expensify/App/blob/4da7fdab30857d0934d8a8035f9e1c773385d950/src/pages/iou/request/step/IOURequestStepWaypoint.js#L145

https://github.com/Expensify/App/blob/4da7fdab30857d0934d8a8035f9e1c773385d950/src/pages/iou/request/step/IOURequestStepWaypoint.js#L165

We should update isDraft pram to action === CONST.IOU.ACTION.CREATE like we did here

https://github.com/Expensify/App/blob/1f72b5f1ae31765183a2a7f51a450f1e9e27b1ff/src/pages/iou/request/step/IOURequestStepWaypoint.js#L138

One more thing that the above proposal is omitted, we also need to update isDraft pram to action === CONST.IOU.ACTION.CREATE for updateWaypoint function here

https://github.com/Expensify/App/blob/4da7fdab30857d0934d8a8035f9e1c773385d950/src/pages/iou/request/step/IOURequestStepDistance.js#L159

The updateWaypoint function also be used here

https://github.com/Expensify/App/blob/4da7fdab30857d0934d8a8035f9e1c773385d950/src/components/DistanceRequest/index.js#L203

But this component will be removed here and we only use IOURequestStepDistance so that we don't need to fix in here

What alternative solutions did you explore? (Optional)

NA

[paultsimura]

One more thing that the above proposal is omitted

In my defense, this is a minor addition (that doesn't affect the issue, just covers a different flow) that 100% would have been caught during the PR since I always check other similar use cases when making a change.

This looks very similar to the discussion here: https://github.com/Expensify/App/issues/29895#issuecomment-1842771731 https://github.com/Expensify/App/issues/29895#issuecomment-1849271626

Moreover, the change in here makes no UX difference, since this component is used only in the Create flow (not in Edit flow), so action === CONST.IOU.ACTION.CREATE here always will be true:

https://github.com/Expensify/App/blob/4da7fdab30857d0934d8a8035f9e1c773385d950/src/pages/iou/request/step/IOURequestStepDistance.js#L159

[lschurr]

@rushatgabhane could you take a look at this one?

[lschurr]

Bump @rushatgabhane

[melvin-bot[bot]]

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

[alitoshmatov]

I will try to take a look at this today.

[melvin-bot[bot]]

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

[alitoshmatov]

@paultsimura Thank you for proposal. Your RCA is correct. Your solution also works and on point.

[alitoshmatov]

@DylanDylann Thank you for proposal. Your proposal is very similar to @paultsimura 's proposal. I think it would be fair to go with his proposal.

[alitoshmatov]

We can go with @paultsimura 's proposal.

C+ reviewed 🎀 👀 🎀

[melvin-bot[bot]]

Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot[bot]]

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[lschurr]

Any update on this one? Are we waiting on a PR from @paultsimura?

[paultsimura]

Any update on this one? Are we waiting on a PR from @paultsimura?

The PR is in the making. There is a Slack discussion ongoing: https://expensify.slack.com/archives/C01GTK53T8Q/p1706543295176979?thread_ts=1705484273.717249&cid=C01GTK53T8Q

[paultsimura]

After a discussion in Slack, it was decided to (in addition to the FE changes) modify the UpdateMoneyRequestDistance API endpoint on the server to calculate the difference between existing and new waypoints, and explicitly return the removed ones as null in the response in the transaction's merge operation.

For example:

{
  w0: A
  w1: B (remove)
  w2: C
  w3: D (remove)
}

Should turn into:

{
  w0: A
  w1: C
  w2: null
  w3: null
}

cc: @tgolen @alitoshmatov

[melvin-bot[bot]]

@paultsimura @blimpich @lschurr @alitoshmatov 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!

[lschurr]

Just clarifying @paultsimura - are we putting this issue on hold for the other? https://github.com/Expensify/App/issues/35503

[paultsimura]

Just clarifying @paultsimura - are we putting this issue on hold for the other? #35503

Yes, please

[lschurr]

On hold.

[tgolen]

The backend fix for this has been deployed to production so I am taking this off HOLD.

[lschurr]

What's next on this one? @tgolen @blimpich @paultsimura

[paultsimura]

What's next on this one? @tgolen @blimpich @paultsimura

There seems to be a larger Onyx issue here, I'll update today or maximum tomorrow

[paultsimura]

Just posted an update on the PR.

[lschurr]

Where are we at on this one @blimpich @paultsimura?

[paultsimura]

@lschurr we should put this on hold for https://github.com/Expensify/App/issues/37560

[lschurr]

On hold.

[lschurr]

On hold.

[lschurr]

On hold.

[lschurr]

On hold.

[lschurr]

On hold.

[lschurr]

On hold.

[lschurr]

On hold.

[lschurr]

On hold

[lschurr]

Hold

[mallenexpensify]

Posted on the PR linked to https://github.com/Expensify/App/issues/37560 to check on priority (since there are a few issues held on the PR)

[lschurr]

On hold

[lschurr]

On hold

[alitoshmatov]

I couldn't reproduce this issue, it has been a long time and it looks like it was resolved along the way

[paultsimura]

We should wait for the hold anyway - this issue is still not complete without testing the issue we're holding for

[lschurr]

On hold

[lschurr]

On hold