[HOLD for payment 2023-12-28] [HOLD for payment 2023-12-26] Expense - App crashes when saving expense date twice in offline mode

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

Issue found when executing PR #30737

Action Performed:

1. Go to staging.new.expensify.com
2. Go offline
3. Navigate to the created expense details page
4. Click on the date
5. Change it once
6. Click on the date
7. Change it for the second time

Expected Result:

App does not crash

Actual Result:

App crashes

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

Bug6313351_1702573939733.bandicam_2023-12-15_00-35-49-447.mp4

logs.txt

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @Christinadobrzyn

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

[abzokhattab]

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 changing the money request date we set onlyIncludeChangedFields param as true here That is why it only returns the changed
param as the money transaction

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

[bondydaa]

interesting, i guess i don't see how that is causing the crash only on the 2nd time it's called though.

cc @tgolen I see your name in that blame for updateMoneyRequestDate and getUpdateMoneyRequestParams any tips here for how to proceed?

Looks like this PR might be the root cause? #30737

[paultsimura]

Here is an issue with the same RC, which was created earlier: #33103
We hope to have a more fundamental discussion with Tim there 🙂

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

[situchan]

I think we should revert #31340 (SET > MERGE) and find proper solution to fix #30290. That can be handled in #32511.

[bondydaa]

@situchan should we do anything for this GH then? Or should it be fixed by something on those PRs you've linked?

[situchan]

We should fix "crash" here. App is useless once user faced crash by updating date 2 times in offline mode.
Do you agree with my suggestion?
This will generate #30290 but that should not block deploy. Better than crash

[bondydaa]

It's very unclear to me what we should do.

Sounds like on #33103, #31340 is also discussed that added onyx.set

but not clear to me what the plan is there. Revert, CP fix to change .set to .merge 🤷

If we're going to revert #31340 then I think it would fix both this and #33103

[situchan]

If we're going to revert #31340 then I think it would fix both this and #33103

yes, correct. but the only concern is that it re-generates this bug due to revert

[paultsimura]

@situchan #31340 has just been waiting for your decision on causing a regression of #32511 as well🙂

As I described here and in the discussion of the selected proposal, using SET was a very risky decision.

If we are going to revert SET back to MERGE, I would like to ask for checking my proposal to the original issue first, and then continue in #32511.

[situchan]

@situchan #31340 has just been waiting for your decision on causing a regression of #32511 as well🙂

As I described here and in the discussion of the selected proposal, using SET was a very risky decision.

If we are going to revert SET back to MERGE, I would like to ask for checking my proposal to the original issue first, and then continue in #32511.

Sure, it's in my next TODO. For now, focusing on this deploy blocker as requested by @trjExpensify (please assign me here as this is same as #33099 🙂)

[bondydaa]

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

[bondydaa]

okay assigning @paultsimura to spin up the revert PR of #31340.

@situchan throwing you on as the c+ then.

[jasperhuangg]

Removing Deploy Blocker label since we CP'd a revert for this

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

• fix: Revert the use of SET operation when updating Money Requests #33174

If no regressions arise, payment will be issued on 2023-12-26. 🎊

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:

• @paultsimura requires payment (Needs manual offer from BZ)
• @situchan requires payment (Needs manual offer from BZ)

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.14-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Revert the use of SET operation when updating Money Requests #33174

If no regressions arise, payment will be issued on 2023-12-28. 🎊

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:

• @paultsimura requires payment (Needs manual offer from BZ)
• @situchan requires payment (Needs manual offer from BZ)

[melvin-bot]

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

[melvin-bot]

@bondydaa, @paultsimura, @situchan 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

@bondydaa, @paultsimura, @situchan Still overdue 6 days?! Let's take care of this!

[melvin-bot]

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

[bondydaa]

@Christinadobrzyn can you handle payment please?

@paultsimura wrote the PR #33174 and @situchan was c+.

[melvin-bot]

@bondydaa, @paultsimura, @situchan 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[Christinadobrzyn]

So sorry for the delay here!

Payouts due:

Issue Reporter: NA
Contributor: $500 @paultsimura (paid in Upwork)
Contributor+: $500 @situchan (paid in Upwork)

Eligible for 50% #urgency bonus? N

Upwork job is here. @paultsimura and @situchan can you accept the offer so I can pay you?

Do we need a regression test for this?

[situchan]

We already have regression test as bug was caught during staging QA

[Christinadobrzyn]

Awesome! I've paid out this job based on this payment summary - #33098 (comment)

I think we can close this?