[HOLD for payment 2023-09-29] 🟢 [$500] BUG: [distance] request confirmation offline doesn't show TBD

[neil-marcellini]

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

---

Action Performed:

1. Run App on latest main branch (for now)
2. Sign in with an account on the distance requests beta (or enable all betas)
3. Go offline
4. Click green plus, request money
5. Enter start and finish addresses
6. Click next
7. Click show more to see all fields

Expected Result:

The distance request amount should show as TBD. The distance field should use TBD for values that aren't available.

From the design doc:

On this page we will render the PendingMapView without any text. We will set a hasRoute variable based on whether transaction.routes is empty. If we don’t have a route then we can skip all calculations based on the distance and set the amount to “TBD”.

We’ll also pass hasRoute to the translation function for displaying the distance row so that it sets “TBD” as the distance when we don’t have a route.

Additionally, after going back online we should fetch the route so that the values can be filled in.

Actual Result:

The amount and miles show as 0

Workaround:

Ignore it

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: Unreleased
Reproducible in staging?: Not yet
Reproducible in production?: Not yet
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers): None
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Originally reported by
@situchan
in a PR review here
Expensify/Expensify Issue URL: None
Issue reported by: @situchan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693608145841629

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a9f623a65b6c37a2
• Upwork Job ID: 1697743255361699840
• Last Price Increase: 2023-09-01
• Automatic offers:
• Ollyws | Reviewer | 26513871
• paultsimura | Contributor | 26513873
• situchan | Reporter | 26513874

[melvin-bot]

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

[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]

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

[melvin-bot]

Current assignee @garrettmknight is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[pradeepmdk]

@neil-marcellini can give me more details on Expected Result because linked docs is private

[neil-marcellini]

I included all the details I can. The linked part isn't important.

[paultsimura]

Proposal

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

In Offline mode, the MoneyRequestConfirmationList in Distance request should show "TBD" for amount and distance instead of 0.0

Also, the route should be recalculated on coming back online

What is the root cause of that problem?

1. For "TBD" – there is no handling of the "has no route" case, so the value defaults to 0.
2. For the route recalculation – the isOffline is not handled on the MoneyRequestConfirmationList page, so the route doesn't recalculate inside the transaction on the network state change.

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

1. Add the const doesRouteExist = lodashHas(transaction, 'routes.route0.geometry.coordinates'); variable and handle the formattedAmount accordingly: if (!doesRouteExist && shouldCalculateDistanceAmount) – return "TBD" and skip calculations; Same goes for the distance.

2. Add the useNetwork() to handle the isOffline change. On its change, call Transaction.getRoute(...) if !doesRouteExist

What alternative solutions did you explore? (Optional)

[b4s36t4]

Proposal

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

[distance] request map is cut off after going online

What is the root cause of that problem?

Considerably there are two problems here.

1. To show TBD.

Even though we don't have access to how much distance the ride is we're still calculating the distance amount here

App/src/libs/DistanceRequestUtils.js

Line 72 in 7217a18

const getDistanceMerchant = (distanceInMeters, unit, rate, currency, translate) => {

in this function which is returning odd amount.

We can return "TBD" if the distanceInMiles is 0.

The usage of this function is getting refactored in other PR which could give us a way to do this in
MoneyRequestConfirmationList component itself. Above point is only for reference. Also we need to consider the network
status before just giving TBD

We can do same with Amount menu as well. Just some conditionals within MoneyRequestConfirmationList component.

2. Getting Values when going back to online.

On MoneyRequestConfirmationList we're rendering the map and when we go offline in money request page we'll call GetRoutes command which will return us the route and distance etc which we use to calculate distance and then amount based on it.

But while creating if we're offline we won't be having it so the route will be null even if go online which require us to close the modal and go up again.

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

To fix this we need to do two changes.

Here in this useEffect

App/src/components/DistanceRequest.js

Line 154 in 7217a18

if (isOffline || !shouldFetchRoute) {

we need update the logic as below

if ((isOffline && _.isEmpty(waypoints)) || !shouldFetchRoute) {
    return;
}

Which will refetch the request if there are waypoints and we've updated our online status.

But this only requires us to come back to the first page again, which won't be the case everytime.

To update the routes inside ConfirmedRoute we also need to call the GetRoute command there as well.

    useEffect(() => {
        // If the network is offline or we have the coordinate we don't need to call the api again.
        if (isOffline || !_.isEmpty(coordinates)) return;
        Transaction.getRoute(transaction.transactionID, waypoints);
    }, [coordinates, isOffline, transaction.transactionID, waypoints]);

What alternative solutions did you explore? (Optional)

NA

@neil-marcellini Please review the proposal. Sorry for the ping if you're not going to review :)

[Ollyws]

Will review this asap.

[Ollyws]

Thanks for your proposals.
@b4s36t4 What's the benefit to adding these changes to distanceRequest and ConfirmedRoute rather than just calling Transaction.getRoute() in a useEffect in MoneyRequestConfirmationList as @paultsimura suggested?

[b4s36t4]

@Ollyws I think the changes I mentioned for DistanceRequest is already being implemented/being reviewed for proposals in another issue. At this moment we don't re-fetch route when we do move to online from offline. At this moment we use the map at 2 places. DistanceRequest and ConfirmedRoute.

[b4s36t4]

If we go online at ConfirmedRoute component (I think it's confirmation page) we don't fetch route again, which requires the user to navigate back again to Distance Route tab and them move again to the confirmation page to see the updated route and the amount to be replaced with TBD.

[b4s36t4]

This is the page we want to show TBD and the values, if we don't fetch the route here when the user become online this page doesn't we update values instantly and show?? But with the first proposal we only solve it at the distance screen. But with my solution we fix it in both places. And results are instantly updated.

Let me know if you need more context on this @Ollyws . Thanks!!

[paultsimura]

I actually found a better solution for the issue without overcomplicating things with _.isEmpty(waypoints) and adding more conditionals:

App/src/components/DistanceRequest.js

Line 92 in 447d216

const doesRouteExist = lodashHas(transaction, 'routes.route0.geometry.coordinates');

Replace this line with:

const doesRouteExist = !lodashIsNil(lodashGet(transaction, 'routes.route0.geometry.coordinates', null));

We already do almost all we need to trigger re-fetch, it's just the logic for checking the doesRouteExist is incorrect, because lodashHas(transaction, 'routes.route0.geometry.coordinates') returns true even if routes.route0.geometry.coordinates == null

[b4s36t4]

@Ollyws Please watch this video to understand the context of my proposal.

Kapture.2023-09-05.at.00.05.44.mp4

And fetch route when we're back online is being raised in other PR.

[neil-marcellini]

I like the proposal from @paultsimura. It's clear and simple. We can iron out any details in the PR. I'm going to assign him now for urgency @Ollyws.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @paultsimura got assigned: 2023-09-05 19:27:37 Z
• when the PR got merged: 2023-09-18 23:26:11 UTC
• days elapsed: 9

On to the next one 🚀

[paultsimura]

Please also take into consideration the translations and copies waiting times + this comment.

[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.

[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.3.72-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: BUG: [distance] request confirmation offline doesn't show TBD #26836

If no regressions arise, payment will be issued on 2023-09-29. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @Ollyws requires payment offer (Reviewer)
• @paultsimura requires payment offer (Contributor)
• @situchan requires payment offer (Reporter)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@Ollyws] The PR that introduced the bug has been identified. Link to the PR:
• [@Ollyws] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@Ollyws] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@Ollyws] Determine if we should create a regression test for this bug.
• [@Ollyws] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

@garrettmknight, @dannymcclain, @Ollyws, @paultsimura, @neil-marcellini Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Ollyws]

This was a new feature so no PR caused this issue. I do think a regression test would be helpful here to ensure this functionality continues to work properly in the future, as it's for one of the main flows in the app and it's easy to test for.

[Ollyws]

Regression Test Proposal

1. Navigate to https://staging.new.expensify.com
2. Go offline
3. Click FAB menu > Request money > Distance
4. Enter start and finish addresses
5. Click Next
6. Select a recipent
7. Verify that fields 'Amount' and 'Distance' are filled as "TBD"

Do we agree 👍 or 👎

[paultsimura]

Hey @Ollyws @neil-marcellini, should the payment for this issue happen after the Regression Test is approved and added?
Otherwise it's a friendly bump 🙂 ☝️

[mountiny]

cc @garrettmknight bump for the payments

[neil-marcellini]

Not overdue, waiting for payment

[garrettmknight]

My bad, y'all.

Summary of payments for this issue:

• urgency bonus: no
• Reporter: @situchan - $50 paid via Upwork
• Contributor: @paultsimura - $500 paid via Upwork
• Reviewer: @Ollyws - $500 paid via Upwork

[garrettmknight]

Closing!