Web - Expense - Fields are blank and header shows $0.00 request when changing date

[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.4.13.0
Reproducible in staging?: Y
Reproducible in production?: N
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. Go to staging.new.expensify.com
2. Go to created manual expense details page
3. Click Date
4. Change the date

Expected Result:

Date is saved without having other areas of the app going blank

Actual Result:

When date is saved, the other fields go blank, the header turns $0.00 request and the date reverts to today's date briefly

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

Bug6313370_1702574868668.bandicam_2023-12-15_00-28-41-162.mp4

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @twisterdotcom (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

[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 @francoisl (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[abzokhattab]

Proposal

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

Fields are blank and when changing money request date

What is the root cause of that problem?

when chaning the money request date we set onlyIncludeChangedFields param as true here That is why it only returns the changed
param:

    const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, false);
    API.write('UpdateDistanceRequest', params, onyxData);
}

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

we should set it to false in order not to affect other fields

[paultsimura]

Proposal

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

When updating Money Request date, the transaction goes blank. It's easily noted if we update the date offline.

What is the root cause of that problem?

We are building the Onyx data that contains only updated fields, and use SET operation that fully replaces the transaction:

App/src/libs/actions/IOU.js

Lines 1016 to 1022 in 052c5cc

	function updateMoneyRequestDate(transactionID, transactionThreadReportID, val) {
	const transactionChanges = {
	created: val,
	};
	const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, true);
	API.write('UpdateMoneyRequestDate', params, onyxData);
	}

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

We should call getUpdateMoneyRequestParams with onlyIncludeChangedFields=false.

What alternative solutions did you explore? (Optional)

I see there is a bigger issue with the way we use the onlyIncludeChangedFields param in IOU.getUpdateMoneyRequestParams:

First, the Onyx SET operation:
The intention for using the onlyIncludeChangedFields param was to either build a partial transaction change (which should be used with the MERGE operation), or a full updated transaction (this will be used in the SET operation).

However, we only return the SET operation no matter what parameter was passed:

App/src/libs/actions/IOU.js

Lines 929 to 939 in 052c5cc

	optimisticData.push({
	// We need to use SET method to save updated waypoint instead MERGE method to avoid wrong update of waypoints. More detail: https://github.com/Expensify/App/issues/30290#issuecomment-1778957070
	onyxMethod: Onyx.METHOD.SET,
	key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
	value: {
	...optimisticTransaction,
	pendingFields,
	isLoading: _.has(transactionChanges, 'waypoints'),
	errorFields: null,
	},
	});

This will result in fully replacing the transaction with only the updated fields when the onlyIncludeChangedFields is set to true.

I would say we need to use MERGE if only updated fields were requested:

onyxMethod: onlyIncludeChangedFields ? Onyx.METHOD.MERGE : Onyx.METHOD.SET,

Second, the way we build the optimisticTransaction for the onlyIncludeChangedFields case:

App/src/libs/actions/IOU.js

Line 928 in 052c5cc

const optimisticTransaction = onlyIncludeChangedFields ? _.pick(updatedTransaction, _.keys(transactionChanges)) : updatedTransaction;

We plainly use the _.pick(updatedTransaction, _.keys(transactionChanges)), and here is the discrepancy:

We pass the transaction changes as { created: "..." } (as it should be), but the updatedTransaction contains 2 fields:

created: "original date",
modifiedCreated: "modified date"

We never get the updated value here, because using pick will lead to always getting the original date, which is under created.

So we should modify this approach as well, to match the way we build the updatedTransaction.

Having fixed that, we should keep calling IOU.getUpdateMoneyRequestParams with onlyIncludeChangedFields=true to use the MERGE operation, which is more safe and acceptable than SET.

---

If we're OK with using SET operation and just limit the data we send to the BE, the only change we should make is remove this line, and use updatedTransaction regardless of onlyIncludeChangedFields param:

App/src/libs/actions/IOU.js

Line 928 in 052c5cc

const optimisticTransaction = onlyIncludeChangedFields ? _.pick(updatedTransaction, _.keys(transactionChanges)) : updatedTransaction;

[francoisl]

So this is mostly a visual issue where the money request's fields temporarily show as blank, right? Going to remove the blocker label while we evaluate the proposals, it doesn't break updating the money request.

[paultsimura]

@tgolen I think you might want to look at the alternative solution section of my proposal #33103 (comment), since it might become quite a bad bug in the future, and you are probably most in the context of those changes

[francoisl]

Yes overall, I don't know if using onlyIncludeChangedFields=false is the best solution, we don't need to pass all the data that didn't change to the backend.

An alternative solution that works for me is to simply use the full transaction object in the optimistic data that we SET, i.e. replace this line with ...updatedTransaction.

App/src/libs/actions/IOU.js

Line 934 in bd25112

...optimisticTransaction,

Curious for Tim's thoughts as well since he indeed probably has the most context on this.

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

[tgolen]

Hi, the main proposal to change Onyx.set() to Onyx.merge() is the right thing to do here. I am not sure why I used set() and it could have just been a mistake or oversight. Let's give that a try. All the changes mentioned to turn this into merge() look correct.

[paultsimura]

I am not sure why I used set() and it could have just been a mistake or oversight

The merge was intentionally changed to set in #31340 in the middle of your big refactoring, and most likely was mishandled during the merge into the refactoring PR

[bondydaa]

hi coming from this other blocker #33098 (comment), looks like @situchan is suggesting we revert #31340 which sounds would fix this GH and #33098

We'd have to redo a PR for #30290 to fix that bug but def seems like #31340 is the main problem here.

Do others agree with reverting #31340?

[bondydaa]

took this to slack here to discuss https://expensify.slack.com/archives/C01GTK53T8Q/p1702656464396679

[paultsimura]

Do others agree with reverting #31340?

That should fix this issue only partially. But we could link this issue and a proper fix to #30290 together in one PR, as they are tightly connected code-wise.

[paultsimura]

This shouldn't be reproducible anymore after the fix was deployed: #33174

[melvin-bot]

This issue has not been updated in over 15 days. @francoisl, @twisterdotcom eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[francoisl]

Confirmed this no longer happens, closing.