[HOLD for payment 2024-03-06] [$500] [MEDIUM] Distance - Map doesn't load and TBD remains when creating request and editing amount offline

[izarutskaya]

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: v1.4.5-7
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: Applause-Internal Tean
Slack conversation: @

Action Performed:

1. Navigate to staging.new.expensify.com
2. Go offline.
3. Go to workspace chat > + > Request money.
4. Create a distance request.
5. Open distance request details page > Amount.
6. Enter amount and save it.
7. Go online.

Expected Result:

The map will load and the fields with TBD will load data when the network connection is regained.

Actual Result:

The map never loads and all the details remain TBD.

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

Bug6295871_1701345082560.bandicam_2023-11-30_09-31-22-433.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a40cd0507a4b7824
• Upwork Job ID: 1730201171282763776
• Last Price Increase: 2024-02-16
• Automatic offers:
• Ollyws | Reviewer | 0
• paultsimura | Contributor | 0

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @greg-schroeder (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]

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

[dukenv0307]

@izarutskaya Can you please update the video of this issue.

[paultsimura]

Proposal

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

When creating & modifying a Distance request offline, the TBD state is stuck.

What is the root cause of that problem?

There is a race condition of onSuccess update of the transaction's pendingFields.

When creating the request, the successData contains the following update for the transaction:

"pendingFields": null

App/src/libs/actions/IOU.js

Line 259 in a56c5cb

pendingFields: null,

And when modifying the amount:

"pendingFields": {
    "amount": null,
    "currency": null
},

App/src/libs/actions/IOU.js

Line 799 in a56c5cb

pendingFields: clearedPendingFields,

As a result, when coming back online, both operations execute, and the "pendingFields": null is ignored (I believe, this is an in-Onyx bug, probably because the operations with null get stacked incorrectly), and the optimistically added "waypoints": "add" is not removed on successful response.

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

We should reset the pendingFields while creating a money request not fully: pendingFields: null, but only the pending fields which were set optimistically, similar to the editing scenario – in our case, it would look like this:

"pendingFields": {
    "waypoints": null
},

To do this, make the following changes:

        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`,
            value: {
                pendingAction: null,
                pendingFields: _.mapObject(transaction.pendingFields, () => null),
            },
        },

App/src/libs/actions/IOU.js

Lines 254 to 261 in a56c5cb

	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`,
	value: {
	pendingAction: null,
	pendingFields: null,
	},
	},

What alternative solutions did you explore? (Optional)

We set the transaction.isLoading when modifying the waypoints of the distance request here:

App/src/libs/actions/IOU.js

Lines 757 to 766 in cd9cd57

	optimisticData.push({
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
	value: {
	...updatedTransaction,
	pendingFields,
	isLoading: _.has(transactionChanges, 'waypoints'),
	errorFields: null,
	},
	});

And we handle this state in other places, like here:

App/src/components/DistanceRequest/index.js

Line 84 in 19c8e25

const isLoading = lodashGet(transaction, 'isLoading', false);

We should set this isLoading state not only while editing, but when creating the Distance request as well.

We shouldn't rely on the hasPendingWaypoints, but rather the transaction.isLoading as well in MoneyRequestView to display TBD:

const isLoading = lodashGet(transaction, 'isLoading', false);

App/src/components/ReportActionItem/MoneyRequestView.js

Line 102 in 12eb47f

const hasPendingWaypoints = lodashGet(transaction, 'pendingFields.waypoints', null);

We should also use if (!transaction.isLoading) instead of if (!Object.hasOwn(transaction?.pendingFields ?? {}, 'waypoints')) here, otherwise the map will be replaced with the pending receipt while the waypoints are being added.

App/src/libs/ReceiptUtils.ts

Line 40 in 61c8b11

if (!Object.hasOwn(transaction?.pendingFields ?? {}, 'waypoints')) {

[DylanDylann]

Proposal

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

Map doesn't load and TBD remains when creating request and editing amount offline

This is bug video

bug.mp4

I see we have 2 bugs:

1. When creating distance request offline the amount field will be TBD. If we click the amount field and edit another number, the amount field will be TBD (can't edit amount field when offline and there is pending waypoint)
2. After editing amount field and go online, the map doesn't load and the amount field still be TBD

What is the root cause of that problem?

For the bug 1

App/src/components/ReportActionItem/MoneyRequestView.js

Lines 103 to 105 in dad6391

	if (isDistanceRequest && (!formattedTransactionAmount || hasPendingWaypoints)) {
	formattedTransactionAmount = translate('common.tbd');
	}

We are setting amount field to TBD if pendingFields.waypoints is valuable

For the bug 2, when creating distance request in successData we set pendingFields: null to reset pendingFields.waypoints. When editing distance request in success data we set

pendingFields {
   amount:  null
   currency: null

But after 2 APIs success, the pending field of creating distance request isn't merged to ONYX so pendingFields.waypoints still be valuable. It seems It is ONYX problem.

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

For bug 1, I suggest 2 solution:

1. Because we always display TBD if there is any pending waypoint, we also need to disable amount field and don't allow user editing this field if there is any pending waypoint
2. We still allow user edit amount and display modifiedAmount if !formattedTransactionAmount || hasPendingWaypoints. If modifiedAmount is empty we still display TBD as old logic

For the bug 2, I suggest 2 solution:

1. When creating new distance request we only need set pendingAction and shouldn't set pendingField.waypoints It is redundant

App/src/libs/actions/Transaction.ts

Lines 61 to 63 in 05ae4c4

	pendingFields: {
	comment: isEditingWaypoint ? CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE : CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
	},

I am not sure why we add pendingField in saveWaypoint function (this function only merge data to ONYX, not send API). Noe Note that, pendingField only be added in optimisticaData (apply before sending API) and It is re-set in successData and FailureData

2. In successData of create distance request we should reset pending field like

pendingFields {
   waypoints:  null

[Ollyws]

Will get to this one asap.

[melvin-bot]

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

[greg-schroeder]

Friendly bump @Ollyws :)

[kbecciv]

@dukenv0307 Uploaded a video

[greg-schroeder]

Let's get to this one ASAP this week!

[Ollyws]

I can no longer reproduce this one.

[melvin-bot]

@Ollyws @greg-schroeder 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!

[tgolen]

LGTM!

[melvin-bot]

📣 @Ollyws 🎉 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]

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

[kmbcook]

The root cause is that the optimistic data is incorrect. If that root cause is eliminated, there is no concern with success data. As soon as the user confirms the distance request waypoints:add can be removed from pendingFields of the optimistic transaction. There is no need to wait for the API call to return successfully.

[paultsimura]

The root cause is that the optimistic data is incorrect. If that root cause is eliminated, there is no concern with success data. As soon as the user confirms the distance request waypoints:add can be removed from pendingFields of the optimistic transaction. There is no need to wait for the API call to return successfully.

This is incorrect. The waypoints are marked as pending intentionally, and they should remain pending until the CreateDistanceRequest API response comes from the server.

[kmbcook]

The root cause is that the optimistic data is incorrect. If that root cause is eliminated, there is no concern with success data. As soon as the user confirms the distance request waypoints:add can be removed from pendingFields of the optimistic transaction. There is no need to wait for the API call to return successfully.

This is incorrect. The waypoints are marked as pending intentionally, and they should remain pending until the CreateDistanceRequest API response comes from the server.

Actually, I believe that as soon as the user confirms the choice of waypoints, they are no longer "pending" in the user's mind. Pending waypoints are useful up until the time that the user confirms them to be correct and complete, which happens prior to the CreateDistanceRequest API call.

[paultsimura]

@kmbcook if you're interested - here's a quite informative thread explaining, in particular, why it was chosen to set the pending waypoints when creating a Distance request offline.

[paultsimura]

Thanks. The PR is ready for review: #37009

[greg-schroeder]

This was deployed to prod 2/28, so payment date will be 3/6

[greg-schroeder]

Hmm. The upwork job was closed for some reason, even though there was recent activity. I don't know. I'll probably have to create a new one...

[paultsimura]

@greg-schroeder it's weird because the offer is still active for me on Upwork 🤔

[greg-schroeder]

Apologies, I was out yesterday, but I'll generate a new job today and get this done

[greg-schroeder]

Doing

[greg-schroeder]

@paultsimura paid! @Ollyws can you accept the offer we sent you? I'll pay you as soon as you accept

[Ollyws]

Accepted, thanks!

[greg-schroeder]

cool, paid