[HOLD for payment 2024-01-09] [$500] IOU - Edited amount for a paid request does not revert to initial amount after closing error message

[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.1-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 Team
Slack conversation: @

Action Performed:

1. Navigate to staging.new.expensify.com.
2. [User A] Request money from User B.
3. [User A] Open IOU details page > Click Amount.
4. [User B] Pay the request.
5. [User A] Enter a new amount and save it.
6. [User A] Close the error message.

Expected Result:

The amount in IOU details page should revert to the original amount after closing the error message.

Actual Result:

The amount in IOU details page does not revert to the original amount after closing the error message. It only reverts after refreshing the app.

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

Bug6285393_1700568035744.20231121_133319.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01674cdc2bbfceb3e4
• Upwork Job ID: 1726944311277539328
• Last Price Increase: 2023-12-05
• Automatic offers:
• bernhardoj | Contributor | 28036430

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01674cdc2bbfceb3e4

[melvin-bot]

Triggered auto assignment to @slafortune (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 - @sobitneupane (External)

[bernhardoj]

Proposal

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

The amount is not reverted when the update is failed.

What is the root cause of that problem?

When we update the amount and get an error, we expect it to revert immediately to the previous value, not actually after pressing the error close button.

In the IOU edit code, we already have a failureData to revert the failed update to the previous amount (or any field) here

App/src/libs/actions/IOU.js

Lines 1933 to 1947 in 0cca90a

	const failureData = [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThread.reportID}`,
	value: {
	[updatedReportAction.reportActionID]: {
	errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericEditFailureMessage'),
	},
	},
	},
	{
	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
	value: transaction,
	},

However, I can see that we always get an error, that is, we can't use Onyx.set after Onyx.merge.

This issue happens after #27609 where we change the (3) onyx method from merge to set.

App/src/libs/actions/IOU.js

Lines 1944 to 1957 in 0cca90a

	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
	value: transaction,
	},
	{
	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
	value: iouReport,
	},
	{
	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.chatReportID}`,
	value: chatReport,
	},

The reason behind it is this. In short, when we update a transaction, either its amount, description, or other fields, we add a new field/property to the transaction object called modifiedAmount, modifiedCreated, etc. and if we merge the old transaction object, these new properties won't be removed from the object, so the new test added will fail.

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

There are 3 onyx keys that the method is changed to set (transaction, iou report, and chat report). Change all 3 back to merge.

Then, we will handle the revert when fails correctly by merging the new property with null.

Let's start with transaction. To know which property is new when we update the transaction, compare the new property from getUpdatedTransaction and buildOptimisticTransaction.

Those new properties are:

1. modifiedCreated
2. modifiedAmount
3. modifiedCurrency
4. modifiedMerchant
5. modifiedWaypoints
6. pendingFields

And in the failure data, we will do it like this:

onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
value: {
    ...transaction,
    modifiedCreated: transaction.modifiedCreated ?? null,
    modifiedAmount: transaction.modifiedAmount ?? null,
    modifiedCurrency: transaction.modifiedCurrency ?? null,
    modifiedMerchant: transaction.modifiedMerchant ?? null,
    modifiedWaypoints: transaction.modifiedWaypoints ?? null,
    pendingFields: null,
},

Next, iou report and chat report.

App/src/libs/actions/IOU.js

Lines 1789 to 1822 in 0cca90a

	let updatedMoneyRequestReport = {...iouReport};
	const updatedChatReport = {...chatReport};
	const diff = TransactionUtils.getAmount(transaction, true) - TransactionUtils.getAmount(updatedTransaction, true);
	if (updatedTransaction.currency === iouReport.currency && updatedTransaction.modifiedAmount && diff !== 0) {
	if (ReportUtils.isExpenseReport(iouReport)) {
	updatedMoneyRequestReport.total += diff;
	} else {
	updatedMoneyRequestReport = IOUUtils.updateIOUOwnerAndTotal(iouReport, updatedReportAction.actorAccountID, diff, TransactionUtils.getCurrency(transaction), false);
	}
	updatedMoneyRequestReport.cachedTotal = CurrencyUtils.convertToDisplayString(updatedMoneyRequestReport.total, updatedTransaction.currency);
	// Update the last message of the IOU report
	const lastMessage = ReportUtils.getIOUReportActionMessage(
	iouReport.reportID,
	CONST.IOU.REPORT_ACTION_TYPE.CREATE,
	updatedMoneyRequestReport.total,
	'',
	updatedTransaction.currency,
	'',
	false,
	);
	updatedMoneyRequestReport.lastMessageText = lastMessage[0].text;
	updatedMoneyRequestReport.lastMessageHtml = lastMessage[0].html;
	// Update the last message of the chat report
	const hasNonReimbursableTransactions = ReportUtils.hasNonReimbursableTransactions(iouReport);
	const messageText = Localize.translateLocal(hasNonReimbursableTransactions ? 'iou.payerSpentAmount' : 'iou.payerOwesAmount', {
	payer: ReportUtils.getPersonalDetailsForAccountID(updatedMoneyRequestReport.managerID).login || '',
	amount: CurrencyUtils.convertToDisplayString(updatedMoneyRequestReport.total, updatedMoneyRequestReport.currency),
	});
	updatedChatReport.lastMessageText = messageText;
	updatedChatReport.lastMessageHtml = messageText;
	}

If you see the above code, you can notice that for chat report, we only update lastMessageText and lastMessageHtml property and both are existing property that is available optimistically, so nothing to change for chat report. For the iou report though, we update

1. total
2. cachedTotal
3. ownerAccountID (in IOUUtils.updateIOUOwnerAndTotal)
4. managerID (in IOUUtils.updateIOUOwnerAndTotal)
5. hasOutstandingIOU (in IOUUtils.updateIOUOwnerAndTotal)
6. lastMessageText
7. lastMessageHtml

Both lastMessageText and lastMessageHtml is not created optimistically which I think a bit inconsistent with how we create a normal chat report. So, the solution for iou report is to create lastMessageText and lastMessageHtml value optimistically.

(taken from ReportUtils.buildOptimisticChatReport)

lastMessageHtml: '',
lastMessageText: null,

The solution is pretty long, but I think it's what we should do earlier

[melvin-bot]

@slafortune, @sobitneupane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@slafortune, @sobitneupane 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

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

[sobitneupane]

Thanks for the proposal @bernhardoj.

Can you please help me understand later part of your proposal. After we revert set to merge, what issue will we face and how your proposal will solve the issue?

[bernhardoj]

Sure.

Let's use an example. Let say we have this transaction object:

current transaction
{amount: 100, currency: 'USD', ...}

When we edit a transaction, there are a few new properties added to the transaction object,

new transaction
{amount: 100, modifiedAmount: 200, currency: 'USD', modifiedCurrency: 'EUR', ...}

and we optimistically merge this new transaction.

If fails, we want to revert to the current transaction. But if we use MERGE, the new properties are also merged into the object.

To solve this, we should clear the new properties by setting it as null if those properties doesn't exist in previous transaction object.

modifiedCreated: transaction.modifiedCreated ?? null,

without the solution
{amount: 100, modifiedAmount: 200, currency: 'USD', modifiedCurrency: 'EUR', ...} merge with {amount: 100, currency: 'USD', ...}
-> {amount: 100, modifiedAmount: 200, currency: 'USD', modifiedCurrency: 'EUR', ...}

with the solution
{amount: 100, modifiedAmount: 200, currency: 'USD', modifiedCurrency: 'EUR', ...} merge with {amount: 100, modifiedAmount: null, currency: 'USD', modifiedCurrency: null, ...}
-> {amount: 100, currency: 'USD' ...}

[melvin-bot]

@slafortune, @sobitneupane Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

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

[melvin-bot]

@slafortune @sobitneupane 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!

[melvin-bot]

@slafortune, @sobitneupane 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[slafortune]

@sobitneupane can you please update this?

[sobitneupane]

Will review the proposal by EOD.

[melvin-bot]

@slafortune, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @greg-schroeder you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[slafortune]

Whoops! I got it Greg!

[slafortune]

@sobitneupane can you complete the checklist?

[slafortune]

Offer sent @sobitneupane can you please accept that as well as complete the above checklist?

[slafortune]

Shoot - sorry about that @sobitneupane I canceled the offer

Once you've complete the checklist, I'll create the regression test if needed and we can close this as you'll be paid via NewDot.

[sobitneupane]

Sorry for the delay @slafortune. I am running quite busy. I will try to get to it before weekend.

[slafortune]

Thanks for the update @sobitneupane , I'll make sure to come back to this on Monday.

[slafortune]

coming back to this Monday.

[slafortune]

@sobitneupane will you be able to get this off your plate in the next day or two?

[melvin-bot]

@tylerkaraszewski, @slafortune, @sobitneupane, @bernhardoj 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

@tylerkaraszewski, @slafortune, @sobitneupane, @bernhardoj Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[sobitneupane]

I will complete the checklist by EOD.

[sobitneupane]

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#27609

• [@sobitneupane] 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:

https://github.com/Expensify/App/pull/27609/files#r1475723305

• [@sobitneupane] 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:

• [@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

• [@sobitneupane] 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.

#31622 (comment)

[sobitneupane]

Regression Test Proposal

1. Request money from user B
2. Press the IOU preview twice to open the IOU detail
3. Press the Amount field to open the edit amount page
4. From user B, pay the request
5. Back to user A, edit the amount to any amount
6. Verify you will get an error message
7. Verify the amount is reverted to the paid amount

Do we agree 👍 or 👎

[slafortune]

• @bernhardoj paid $500 via Upworks
• @sobitneupane is due $500 via NewDot

[JmillsExpensify]

$500 approved for @sobitneupane based on summary above.