[HOLD for payment 2024-04-15] [$500] Distance - Unable to save second distance edit offline

[kbecciv]

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: 1.4.56-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com.
2. Go to workspace chat.
3. Create a distance request with three waypoints.
4. Go to request details page.
5. Go offline.
6. Click Distance.
7. Drag and drop the waypoints and save it.
8. Repeat Step 6 and 7.

Expected Result:

User will be able to save the second distance edit.

Actual Result:

User is unable to save the second distance edit.

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

Add any screenshot/video evidence

Bug6423379_1711125466153.20240323_001712.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b020e3127d156e8f
• Upwork Job ID: 1772031538674204672
• Last Price Increase: 2024-03-24
• Automatic offers:
• GandalfGwaihir | Contributor | 0

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @srikarparsi (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

We think that this bug might be related to #wave-collect - Release 1

[nkdengineer]

Proposal

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

User is unable to save the second distance edit.

What is the root cause of that problem?

We prevent the user save the distance if the transaction is loading.

App/src/pages/iou/request/step/IOURequestStepDistance.js

Lines 190 to 192 in 7f5cf61

	if (duplicateWaypointsError || atLeastTwoDifferentWaypointsError || hasRouteError || isLoadingRoute || isLoading) {
	setShouldShowAtLeastTwoDifferentWaypointsError(true);
	return;

So in offline, after the first time we edit the distance, we can't edit this anymore.

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

As the old edit distance flow, we should allow user saving the distance if we're offline event the transaction is loading

if (duplicateWaypointsError || atLeastTwoDifferentWaypointsError || hasRouteError || isLoadingRoute || (isLoading && !isOffline)) {
    setShouldShowAtLeastTwoDifferentWaypointsError(true);
    return;
}

App/src/pages/iou/request/step/IOURequestStepDistance.js

Lines 190 to 192 in 7f5cf61

	if (duplicateWaypointsError || atLeastTwoDifferentWaypointsError || hasRouteError || isLoadingRoute || isLoading) {
	setShouldShowAtLeastTwoDifferentWaypointsError(true);
	return;

What alternative solutions did you explore? (Optional)

NA

[ZhenjaHorbach]

Looks like a regression from this PR
#35302

[shubham1206agra]

Tagging @DylanDylann @cubuspl42 for visibility.

[srikarparsi]

This looks external since editing the order of the points does work while offline when creating the request. Opening it up to see if there's any proposals before Monday.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01b020e3127d156e8f

[melvin-bot]

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

[nkdengineer]

@srikarparsi What do you think about my proposal?

[allgandalf]

Proposal

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

Unable to save second distance edit offline

What is the root cause of that problem?

When offline, We call the submit button:

App/src/pages/iou/request/step/IOURequestStepDistance.js

Lines 188 to 192 in 00e0121

	const submitWaypoints = useCallback(() => {
	// If there is any error or loading state, don't let user go to next page.
	if (duplicateWaypointsError || atLeastTwoDifferentWaypointsError || hasRouteError || isLoadingRoute || isLoading) {
	setShouldShowAtLeastTwoDifferentWaypointsError(true);
	return;

But over here we have a isLoading variable which checks if the route is loading, this is required for the very first time when we create a Distance request.

But when editing one, we don't need to check if the distance is in isLoading state, we can simply allow the user to submit mulitple distance requests inspite of the fact that whether the user is offline or online

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

So we need to update the isLoading condition such that we will not show error if the request is in editing mode

        if (duplicateWaypointsError || atLeastTwoDifferentWaypointsError || hasRouteError || isLoadingRoute || (!isEditing && isLoading)) {
            setShouldShowAtLeastTwoDifferentWaypointsError(true);
            return;
        }

i.e. we will allow the user to edit waypoints even when the distance request is loading if the current request is in editing mode

What alternative solutions did you explore? (Optional)

N/A

[srikarparsi]

Ah looks like @DylanDylann is working on a fix

[allgandalf]

but wasn't this external 🤕 , also i see they used my solution to fix it 🌚

[cubuspl42]

I'm not strongly against merging #38987, but I'm also not sure why are you pushing so hard for merging it right-here-right-now. I'm just being cautious and somewhat uncomfortable with merging something where we have to tip-toe to avoid a crash. I'd prefer to have the crash fixed first.

The strongest argument for fixing issues fast is delivering the fix to the user, but I'd assume this won't happen before the crash is fixed.

If I'm missing something here, you can let me know. Maybe there's some personal reason you'd like to have this merged ASAP?

[DylanDylann]

@cubuspl42 Oh sorry if you are uncomfortable about my bump 😄 But this is a deployed blocker and this issue was created 2 weeks ago then I think we should move forward the PR.

I'm just being cautious and somewhat uncomfortable with merging something where we have to tip-toe to avoid a crash. I'd prefer to have the crash fixed first.

In my opinion, we should process this one and don't need to hold as mentioned here

But if you think holding this issue is necessary, I am fine with that. But this PR will be delayed in a long time

[cubuspl42]

I'm not uncomfortable about the bump. I'm uncomfortable with merging PRs whose functionality closely relates to an area with a serious issue (crashing). I know that we didn't introduce the crash (if we did, we'd be fixing it, not holding!), but I'd like to re-test the behavior after the mentioned crashing is fixed, if possible.

But this is a deployed blocker

This is not a deploy blocker anymore; if it was one, we'd be rushing to merge it indeed.

The discovered issue should be a deploy blocker, though, in my opinion.

[cubuspl42]

After rethinking this, I approved the PR, leaving the final decision to the internal engineer.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix/38814: Distance - Unable to save second distance edit offline #38987

If no regressions arise, payment will be issued on 2024-04-15. 🎊

For reference, here are some details about the assignees on this issue:

• @abdulrahuman5196 requires payment (Needs manual offer from BZ)

[allgandalf]

Gentle bump,

My proposed solution was indeed used to fix,

So I guess it makes me eligible for payment here according to comment

c.c. @mallenexpensify

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[abdulrahuman5196]

There is no C+ payment since it's regression fix. And I didn't review the PR.

[mallenexpensify]

@GandalfGwaihir can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01b020e3127d156e8f

Compensation is 50% of the job price, $250, because authoring a PR and testing wasn't involved.

[allgandalf]

@mallenexpensify accepted the offer thanks :)

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[melvin-bot]

📣 @GandalfGwaihir 🎉 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 📖

[mallenexpensify]

Contributor: @GandalfGwaihir paid $250 via Upwork

Thanks!

[allgandalf]

I will decline the autogenerated offer by Melvin on Upwork, I was paid already, thanks for the paying @mallenexpensify :)