[HOLD for payment 2024-01-04] [$500] Distance - Description is not saved when created offline and description edit message becomes amount

[lanitochka17]

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.97-2
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 Team
Slack conversation:

Action Performed:

Precondition: Have some saved address offline

1. Navigate to staging.new.expensify.com
2. Go offline
3. Go to workspace chat > + > Distance
4. Create a Distance request that has a description
5. Go to Distance request details page
6. Click on Description and add a description again
7. Go online

Expected Result:

In Step 5, the description which is added in Step 4 will appear in the distance details page
In Step 7, when the request is created, the system message for 'set a description to' will not change

Actual Result:

In Step 5, the description which is added in Step 4 is not saved
In Step 7, when the request is created, the system message for 'set a description to' changes to 'changed the amount to 0.00'

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

Bug6270188_1699547144451.bandicam_2023-11-09_23-57-04-982.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018d38eec8fb70c3c2
• Upwork Job ID: 1722653765286424576
• Last Price Increase: 2023-11-09
• Automatic offers:
• 0xmiroslav | Reviewer | 27953114
• esh-g | Contributor | 27953116

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018d38eec8fb70c3c2

[melvin-bot]

Triggered auto assignment to @dylanexpensify (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 - @0xmiroslav (External)

[esh-g]

Proposal

Please re-state the problem

Distance - Description is not saved when created offline and description edit message becomes amount

Root cause

Basically the existingTransaction object has properties which are also objects for example transaction.comment.comment to get the actual comment text.
This is how a transaction object can look:

type Transaction = {
    amount: number;
    billable: boolean;
    category: string;
    comment: {
      comment?: string;
      waypoints?: WaypointCollection;
      isLoading?: boolean;
      type?: string;
      customUnit?: Record<string, unknown>;
      source?: string;
      originalTransactionID?: string;
    };
    created: string;
}

Now the optimisticTransaction has the property for the comment text that user has entered while the existingTransaction has the waypoints. This means that the comment property in existingTransaction overwrites the property of optimisticTransaction which in-turn preserves the waypoints but deletes the user comment.

This happens because we don't do a deep merge here:

App/src/libs/actions/IOU.js

Lines 511 to 522 in 7b78542

	// If there is an existing transaction (which is the case for distance requests), then the data from the existing transaction
	// needs to be manually merged into the optimistic transaction. This is because buildOnyxDataForMoneyRequest() uses `Onyx.set()` for the transaction
	// data. This is a big can of worms to change it to `Onyx.merge()` as explored in https://expensify.slack.com/archives/C05DWUDHVK7/p1692139468252109.
	// I want to clean this up at some point, but it's possible this will live in the code for a while so I've created https://github.com/Expensify/App/issues/25417
	// to remind me to do this.
	const existingTransaction = existingTransactionID && TransactionUtils.getTransaction(existingTransactionID);
	if (existingTransaction) {
	optimisticTransaction = {
	...optimisticTransaction,
	...existingTransaction,
	};
	}

What changes should be made to fix this?

We just do a deep merge at the fore-mentioned code:

if (existingTransaction) {
    optimisticTransaction = merge(optimisticTransaction, existingTransaction),
}

import merge function from lodash

[esh-g]

These are still all in the code, but it's not causing any problems, so I am just going to close this since I haven't had any chance to look into it.

@tgolen Seems like it is causing problems... 😅

[tgolen]

There is a first time for everything! Thanks for looking into it.

[0xmiros]

@esh-g why does it happen only on distance request? I am not able to reproduce on manual request.

[esh-g]

@0xmiroslav You can see the following line comment in the code I quoted in my proposal

If there is an existing transaction (which is the case for distance requests)

Basically it is because distanceRequest already have a transaction created with the waypoints set and we need to merge the optimistic transaction into it which has the description that user entered.
For manual requests, there is no existing transaction

[0xmiros]

Proposal looks good to me.
@tgolen you wanna take this as engineer?

[namhihi237]

Proposal

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

Description is not saved in data optimistic, and after turning on network, we see update amount message

What is the root cause of that problem?

Like expectation , we have 2 problems to solve here:

1. description is not saved when creating optimisticTransaction
   When we create the distance request, the transaction has already been created, but because the comment is an object we are using a copy object which does not work as expected.

App/src/libs/actions/IOU.js

Lines 517 to 523 in 9e02e96

	if (existingTransaction) {
	optimisticTransaction = {
	...optimisticTransaction,
	...existingTransaction,
	};
	}

2. The second problem is that after turning online network, we see the modify amount action appear, even though we updated the description before.
   The reason here is because when offline amount has no value, after turning online network, we have 2 rows of requests in the queue: create request and update request. After the create request is made the amount will be calculated assuming a value of A, but because of the update request it will have an amount of 0 so we will have both the amount and comment changes.

So it will show message changing amount because of this condition

App/src/libs/ReportUtils.js

Lines 1920 to 1925 in 5a94095

	const hasModifiedAmount =
	_.has(reportActionOriginalMessage, 'oldAmount') &&
	_.has(reportActionOriginalMessage, 'oldCurrency') &&
	_.has(reportActionOriginalMessage, 'amount') &&
	_.has(reportActionOriginalMessage, 'currency');

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

1. We should use deep copy
2. We can add a condition to check whether it is a real amount change or not:

&& reportActionOriginalMessage.amount !== 0;

What alternative solutions did you explore? (Optional)

N/A

[namhihi237]

@0xmiroslav can you review my proposal, this issue has 2 problém and my proposal fully addresses them, thanks

[melvin-bot]

@dylanexpensify, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[0xmiros]

As amount issue is also in the issue title, proposal should cover fixing that as well.
That being said, I recommend @namhihi237's proposal.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[esh-g]

@0xmiroslav Please also notice that @namhihi237 builds upon my proposal of using deep merge (moreover, 'deep merge' isn't even mentioned in their proposal, only 'deep copy') and as for the second issue, I think it might be better that we handle the amount issue on the backend... because currently, when we change any field of the transaction, we send the whole transaction as a parameter to be updated. This means that even if we implement the proposed solution, the backend still sends the message that amount is changed, we just don't display it.

(notice the amount and also the category)

I think it would be better if we:
Option 1: Change the api endpoint to only accept the changed fields in the transaction.
Option 2: Add a safeguard in the backend that makes sure the amount is not set to 0

[dylanexpensify]

@youssef-lr to confirm proposal! Though @youssef-lr I know you're about to head ooo, so let us know if we should reassign!

[DylanDylann]

@youssef-lr @0xmiroslav Relating to the second problem of this issue, while checking this issue I see that when creating a distance request offline and going to the transaction thread, the reportID of the transaction thread is only optimistic reportID. Then if we go online, the BE will return another reportID for the transaction thread. So that if we are in a transaction thread and go online, there are no updates, we need to back to the expense report and go to the transaction thread again to get new updates

newbug.mp4

[tgolen]

Ah, great! Sounds good.

[dylanexpensify]

Making progress!

[dylanexpensify]

@0xmiroslav let's get that PR reviewed today please! 🙇‍♂️

[dylanexpensify]

Any update @0xmiroslav

[0xmiros]

updated in PR

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

• Deep merge transaction for Distance request #32437

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

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:

• @esh-g requires payment automatic offer (Contributor)
• @0xmiroslav requires payment automatic offer (Reviewer)

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

• [@0xmiroslav] The PR that introduced the bug has been identified. Link to the PR:
• [@0xmiroslav] 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:
• [@0xmiroslav] 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:
• [@0xmiroslav] Determine if we should create a regression test for this bug.
• [@0xmiroslav] 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.
• [@dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[esh-g]

gentle bump @dylanexpensify

[dylanexpensify]

Payment Summary:

• Contributor: @esh-g $500
• Contributor+: @0xmiroslav $500

Please apply!

[0xmiros]

Automatic offers were already sent by Melvin

[dylanexpensify]

Just paid! Had to post summary for good measure!

[dylanexpensify]

@0xmiroslav mind completing checklist?

[0xmiros]

We were already aware of the root cause and it was temporary solution.
Now that this bug was caught, we fixed with permanent solution.

As this was caught by Applause, we can skip regression test.

[melvin-bot]

@arosiclair, @dylanexpensify, @esh-g, @0xmiroslav Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!