[$500] Web - Report Preview in Report Screen shows up even after deletion of Money Request

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Use FAB on a ChatReport and generate money request
2. Click on IOUReport to show the IOUReport
3. Use menu to delete the Money Request
4. Come back to ChatReport of Step 1
5. Reload the page.

Expected Result:

Report Previous should not show up for a deleted Money Request

Actual Result:

Report Preview shows up for a deleted Money Request

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.62.0
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
Notes/Photos/Videos: Any additional supporting documentation

deletemrfails.mp4

Recording.4231.mp4

Expensify/Expensify Issue URL:
Issue reported by: @rojiphil
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693399731169779

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013c7ff8faa3f9e5a6
• Upwork Job ID: 1704099592925270016
• Last Price Increase: 2023-09-26

[melvin-bot]

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

[rojiphil]

Proposal

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

Web - Report Preview in Report Screen shows up even after deletion of Money Request

What is the root cause of that problem?

The problem occurs because we are not clearing the Report Preview on the deletion of the IOU Report. . Further, pendingAction is not set for ReportPreview for Offline support.

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

1. This can be solved by clearing up the Report Preview when IOU Report is deleted at this location within successData of deleteMoneyRequest method

The changes can be as follows:

        ...(shouldDeleteIOUReport
            ? [
                {
                    onyxMethod: Onyx.METHOD.MERGE,
                    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
                    value: {
                        [reportPreviewAction.reportActionID]: {
                            pendingAction: null,
                            message: [
                                {
                                    type: 'COMMENT',
                                    html: '',
                                    text: '',
                                    isEdited: true,
                                },
                            ],
                            errors: null,
                        },
                    },
                },
              ]
            : []),        

2. In addition, we need to support offline mode. For this, we can add the pendingAction to DELETE as follows here

    let updatedReportPreviewAction = {...reportPreviewAction};
    if(shouldDeleteIOUReport)
    {
        updatedReportPreviewAction.pendingAction = CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;
        updatedReportPreviewAction.errors = null;
    }

What alternative solutions did you explore? (Optional)

[s-alves10]

Proposal

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

Report preview in report page shows again even after money request is deleted

What is the root cause of that problem?

When we delete a money request, deleteMoneyRequest function is called.
As you can see, if shouldDeleteIOUReport is true, we optimistically merge the report preview action with null

App/src/libs/actions/IOU.js

Lines 1183 to 1189 in d971190

	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
	value: {
	[reportPreviewAction.reportActionID]: updatedReportPreviewAction,
	},
	},

And the deleteMoneyRequest API returns {[reportPreviewActionID]: null}, too.
The problem is in Onyx.merge

Please check the code here

We remove the keys with null value in modifiedData and call Storage.mergeItem. This means null values are not updated in storage though they are updated in memory. This bug looks like was introduced by this PR

This is the root cause of this issue

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

In Onyx.merge function, we merge the changes with existing data and remove all keys with null values, we have to call Storage.setItem instead of Storage.mergeItem

This works as expected

Result

26627.mp4

What alternative solutions did you explore? (Optional)

[chrispader]

Calling Storage.setItem instead of Storage.mergeItem is definitely not what we want to do here, because we added JSON_PATCH functionality for SQLite on native here.

I just discovered the potential problem. I think it’s related to the idb-keyval PR by @hannojg. The problem is, that before this PR, on web we just set the modified data with the nullish keys removed already. After this PR, on web we’re using yet another merge funcationality, which will not drop the nullish keys, because the modified data with the removed nullish keys will be merged.

I’m gonna open a PR, which fixes this.

[s-alves10]

I hope you open the PR asap. I think this bug may cause several issues

[melvin-bot]

@muttmuure Still overdue 6 days?! Let's take care of this!

[chrispader]

I hope you open the PR asap. I think this bug may cause several issues

@s-alves10 just created a PR in Onyx that fixes this issue: Expensify/react-native-onyx#333

[melvin-bot]

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

[melvin-bot]

@muttmuure 10 days overdue. I'm getting more depressed than Marvin.

[melvin-bot]

@muttmuure this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013c7ff8faa3f9e5a6

[melvin-bot]

Current assignee @muttmuure is eligible for the External assigner, not assigning anyone new.

[muttmuure]

I was out last week. Catching up this week

[melvin-bot]

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

[melvin-bot]

@allroundexperts @muttmuure this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[melvin-bot]

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

[muttmuure]

@allroundexperts can you verify if @chrispader's PR fixes 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]

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

[melvin-bot]

@allroundexperts @muttmuure this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot]

Current assignee @allroundexperts is eligible for the Internal assigner, not assigning anyone new.

[melvin-bot]

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

[muttmuure]

No proposals, going to come back in a week and see if this is reproducible

[rojiphil]

@muttmuure
As I understand, the PR in this comment was supposed to resolve this issue.
@chrispader @s-alves10 can confirm

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[rojiphil]

@allroundexperts @muttmuure
Can we not close this issue if the issue is fixed already?
Also, there are other issues dependent on the resolution of this one. Closing this would help us move further on the other issues.

[miljakljajic]

This hasn't been reproducible for two weeks [following this PR] (#26627 (comment)) - closing. Thank you @rojiphil !

[rojiphil]

@miljakljajic @muttmuure
Don't I qualify for a reporter bonus here as I had reported the bug?