[C+ Checklist Needs Completion] [$500] Money Request - Deleted IOU in offline mode appears when User switch to online

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Disable the internet connection on the device
2. Create a money request in a 1:1 conversation
3. Open the IOU details and DELETE the IOU request
4. Enable the internet connection on the device
5. Refresh the app

Expected Result:

The request should be deleted and the message should be still grayed out in offline mode.
When user switch to online mode the request should remain deleted

Actual Result:

When user delete the request in offline mode the request should be grayed out, in this case the request disappear
Deleted IOU in offline mode appears when User switch to online

Workaround:

Unknown

Platforms:

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

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

Version Number: 1.3.61-1

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6185284_Recording__837.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01bb43036fdffd3721
• Upwork Job ID: 1698671002276990976
• Last Price Increase: 2023-12-04
• Automatic offers:
• fedirjh | Reviewer | 27991248
• rojiphil | Contributor | 27991249

[melvin-bot]

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

Issue 1:

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

Problem: When Offline, deleting the last Money Request (without any comments) in IOU Report removes Report Preview from parent Chat Report
Steps:
1 Go Offline
2 Add Money Request in Chat Report.
3 Go to IOU Report for the Money Request
4 Delete Money Request

Actual Result: When offline, Report Preview is removed from parent Chat Report

Expected Result: When offline, Report Preview should be shown. Further, Money Request preview in IOU Report should also be shown with striked-out amount.

Issue-1 Video

1-mr-offline-removed.mp4

What is the root cause of that problem?

Deletion of the money request happens in deleteMoneyRequest function here. Here, we are setting IOUReport and Report Preview action to null as we can see here and here because of which Report Preview action and IOU Report are removed. This is the root cause.

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

The following changes can be done to solve this problem.

1. First thing to do is not to set IOUReport and reportPreviewAction to null. Instead, let us reuse the existing object until API response comes back. For this, we can set these variables to the existing objects.
   Secondly, for cases where the IOU report will be deleted, we can set the pendingAction to DELETE and UPDATE for other cases. We can update the ReportPreviewAction with these values so that Report Preview action will remain shown in offline mode. We can make the changes like this here.

    let updatedIOUReport = {...iouReport};
    let updatedReportPreviewAction = {...reportPreviewAction};
    updatedReportPreviewAction.pendingAction = shouldDeleteIOUReport ? CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE : CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE;

2. Further, for success API response, we need to reset the IOU Report and Report Preview Action accordingly like shown below. We can make the changes here
   a) For reportPreviewAction, we can set it to null if IOU report is deleted. Otherwise, we can reset the pendingAction to indicate that action is completed.
   b) For IOU Report, we can set it to null if it has been deleted.

        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${chatReport.reportID}`,
            value: {
                [reportPreviewAction.reportActionID]: shouldDeleteIOUReport ? null : {
                    pendingAction: null,
                    errors: null,
                },
            },
        },
        ...(shouldDeleteIOUReport
            ? [
                {
                    onyxMethod: Onyx.METHOD.SET,
                    key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
                    value: null,
                },       
            ]
            : []),

3. To support offline support, we also need to update the IOU Report and Report Preview action. So, we can update them by removing the following line here

if (!shouldDeleteIOUReport) {

4. Also, to keep showing money request preview in IOU Report, we should prevent resetting of the money request action. We can make the changes to the code here like this

    {
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${iouReport.reportID}`,
        value: updatedReportAction,
    },
   

5. Further, we can remove the code here as that would reset the iouReportID. This would fail the offline scenario. This is anyways going to be reset by API response.

What alternative solutions did you explore? (Optional)

---

Issue 2:

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

Problem: When Offline, Report Preview shows TBD on deletion of last Money Request in IOU Report with comments
Steps:
1 Go Offline
2 Add Money Request in Chat Report
3 Go to IOU Report for Money Request
4 Add a message in IOU Report
5 Delete the only Money Request in IOU Report (created in Step 2)
6 Go to parent Chat Report
Actual Result: TBD is shown in Report Preview in Chat Report

Expected Result: Amount 0 should be shown in Report Preview in Chat Report

Issue-2 Video

2-mr-tbd-with-msg.mp4

What is the root cause of that problem?

TBD is shown because hasOnlyDistanceRequestTransactions here returns true even for cases where there are no transactions. In our case here, since all money requests are deleted, there will be no transactions. So, the root cause is that hasOnlyDistanceRequestTransactions is faulty for cases with no transactions.

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

To solve the problem, we need to additionally handle the case when there are no transactions in the IOU Report. We can check for the size of the transactions’ list and return false if it is empty. If not empty, we go ahead and iterate through each to find out if it is a distance request.

Solution 1:
We can solve like this here:

return _.size(allTransactions) > 0 ? _.all(allTransactions, (transaction) => TransactionUtils.isDistanceRequest(transaction)) : false;

Solution 2:
We can also revise the existing implementation and migrate to typescript as follows. We can add this function in TransactionUtils.ts and call this within hasOnlyDistanceRequestTransactions here

function areAllDistanceRequestTransactions(reportID?: string): boolean  {
    const reportTransactions: Transaction[] = getAllReportTransactions(reportID);
    return reportTransactions.length > 0 && reportTransactions.every((transaction) => isDistanceRequest(transaction));
}

What alternative solutions did you explore? (Optional)

---

Issue 3:

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

Problem : When Offline, Report Preview does not update amount when a new Money Request is added
Steps:
1 Go Offline
2 Add Money Request in 1:1 DM Chat Report (Note: Chat Report is not a Workspace Chat)
3 Go to IOU Report for Money Request
4 Add a message in IOU Report
5 Delete the only Money Request in IOU Report (created in Step 2)
6 Go to parent Chat Report.
7 Go to IOU Report again by clicking on the Report Preview and add a new Money Request
8 Go to parent Chat Report and review Report Preview amount.
Actual Result: Report Preview amount is not updated with money request amount of step 7

Expected Result: Report Preview amount should be updated with money request amount of step 7

Issue-3 Video

3-amount-not-updated.mp4

What is the root cause of that problem?

Here, we update the total amount for Report Preview of IOU Report and Expense Report. While the Expense Report total is updated properly, the IOU Report total is updated using updateIOUOwnerAndTotal here. However, the amount will only be updated if the current total amount is not 0 as seen here. This is faulty as there can be cases where total amount is 0 and the IOU Report still exists due to presence of normal comments(messages). So, checking if total amount is 0 is the root cause of this issue.

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

To solve the problem, we need to allow updation of total even when the all the money requests are deleted.
We can solve this by updating the following condition here which will keep the logic consistent and also prevent any other bugs.

if (typeof iouReportUpdate.total !== 'undefined') {

What alternative solutions did you explore? (Optional)

Alternatively, we can also remove the following condition and resolve the problem.

- if (iouReportUpdate.total) {

---

Issue 4:

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

Problem : When Offline, IOU Report page shows blank on navigating back from parent Chat Report via click on Reply link
Steps:
1 Go Offline
2 Add Money Request in Chat Report
3 Go to IOU Report for Money Request
4 Add a message in IOU Report
6 Go to parent Chat Report. Verify that the Report Preview shows with Reply Count.
7 Click on Reply link to go to IOU Report
Actual Result: IOU Report screen is shown as blank
Expected Result: IOU Report Screen should be correctly shown

Issue-4 Video

4-blank-iou-report-1.mp4

What is the root cause of that problem?

When we click on Report Preview, we use the information of iouReportID as shown here. However, iouReportID is obtained from linkedReportID within Report Preview action as mentioned here. Since linkedReportID is optimistically added to the Report Preview action here IOU Report shows up on click of Report Preview even in offline scenaio.

Now, when we click on Reply link in Report Preview, we use the information of childReportID in Report Preview’s action as shown here for showing the IOU Report screen. However, when we optimistically create the Report Preview’s action for the IOU Report here, we do not assign the childReportID. If this was online, the BE would update the childReportID due to which this this issue will not occur in online scenario. But, since the childReportID was not optimistically set, the problem occurs in offline scenario. This is the root cause of the problem.

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

When we optimistically create the Report Preview action, we should also set childReportID here like this:

childReportID: iouReport.reportID,

In addition, we can leverage the existing originalMessage.linkedReportID to pass IOU Report ID as follows

childReportID={${props.action.childReportID || props.action.originalMessage.linkedReportID}}

What alternative solutions did you explore? (Optional)

Alternatively, we can also pass the linkedReportID instead of childReportID to the ReportActionItemThread and make this work. But, since we anyway have to optimistically add the childReportID to the Report Preview action, we can set childReportID and solve the problem.

[greg-schroeder]

Reviewing

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01bb43036fdffd3721

[melvin-bot]

Triggered auto assignment to @sonialiap (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[fedirjh]

@rojiphil Can you elaborate more on the root cause?

[rojiphil]

Proposal

Updated with more clarity on root cause.
@fedirjh Hope the explanation is better now. Please let me know.

[melvin-bot]

@greg-schroeder, @fedirjh Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@greg-schroeder, @fedirjh Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[fedirjh]

@rojiphil Could you provide more details about problem 1? How does problem 1 relate to the current bug?

Can you show me how we hide the action preview offline when it's deleted?

2. there is a bug in Onyx as referred here because of which the Report Preview action is not set to null on success response from BE

Can you please explain this further? We have an open PR in Onyx that targets that bug Expensify/react-native-onyx#333

[fedirjh]

@greg-schroeder This issue seems to be a dupe of

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

I think we should hold/close until Onyx PR is merged :

• Fix: nullish keys not removed/merged in IDB-Keyval (web) react-native-onyx#333

[rojiphil]

@rojiphil Could you provide more details about problem 1? How does problem 1 relate to the current bug?

@fedirjh I do not think this is dupe as this issue specifically aims to handle the offline case. Please give me some time. I will upload a video to explain this.

[fedirjh]

I do not think this is dupe

I noticed you posted the same proposal in the other issue. Anyway, let's reassess this after the Onyx PR is merged.

I will upload a video to explain this.

Please make sure to include the code for the root cause as well.

Thank you

[rojiphil]

a)

I noticed you posted the same proposal in the other issue.

Yes, I posted in the other issue as well as both these issues were related although they do not have the same Root Cause. May be, that adds to the confusion too.
Regarding this issue, this is what I think,
Problem 1: Current issue
Problem 2: Partly handled by #26627 which is fixed via Onyx PR fix.

b)

Please make sure to include the code for the root cause as well.

Regarding this, I think, the proposal here covers the code aspect as well. Or am I missing something there? Or may be, I should have additionally mentioned that Offline support is currently not there for Report Preview in Chat Report. That is what we are resolving here.

c) Here is an attached video of solution in offline mode. When offline, we still need to show the Report Preview Action until it becomes online. This is what I understood by Problem 1. Are we on the same page on this?

fixoffline.mp4

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

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

• fix for offline money request issues #32698

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

For reference, here are some details about the assignees on this issue:

• @rojiphil requires payment automatic offer (Contributor)
• @fedirjh 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:

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

[rojiphil]

While the identified bugs don't share the same root cause, I recommend contemplating an increase in the bounty for this issue.

@greg-schroeder @blimpich @fedirjh

Since it’s payment time here, I think this may be the right time to bring up again the above discussion on compensation.

Here we have fixed 5 issues in total (4 issues in PR here and 1 issue in follow-up PR here)
As I understand, the primary issue (i.e. offline support for money requests) is kind of a new feature as it was not there before.
Whereas the remaining money request issues had different root causes.

What are your thoughts on this?

[greg-schroeder]

Hmm. Tricky, because this PR was linked to a deploy blocker, no? So if we did increase the bounty, it would be halved as a result.

[situchan]

The regression issue is not fixed so yet payment time.

[situchan]

Seems like they're going to get reward for fixing #34263 as separate issue.
So neither increasing bounty nor applying penalty makes sense here.

[rojiphil]

because this PR was linked to a deploy blocker, no?

@greg-schroeder

That issue was considered a separate issue. Also, we have not discussed any reward there yet.
So, I have included that too in my comment here.
If we want to consider that issue separately, then we have solved 4 issues as part of this issue.
I think @blimpich can take a call on how to consider this issue.

Regarding the issue itself, I am not sure if we can consider it as a regression because offline support for money requests was introduced only here. It is sort of a new feature.
Also, the fix for this issue involved code changes due to recent changes in a separate unrelated PR here whereas the code changes as part of this issue stayed the same.
@fedirjh , @blimpich, please correct me if I am wrong here.

[greg-schroeder]

Okay. I mean what is the proposed bounty increase in the first place? Someone suggested we raise it at some point in the past but I don't think we ever got to a number.

Can anyone articulate the amount of work that went into this compared to a "typical" issue?

[blimpich]

Lets do $1000 for the whole thing, including #34263, and not apply any penalty to this issue. @rojiphil I really appreciate the attention to detail and professionalism, and I agree this deserves more of a reward than a normal issue 🙂. And in the future we can make sure we increase the bounty before you put up your PR, so that you can know what you're going to get paid before working on an issue.

[greg-schroeder]

Sounds good to me, I'll adjust the offers.

[greg-schroeder]

Paid $1k to both C and C+

[greg-schroeder]

@fedirjh just need to complete the checklist above and we're good to go!

[greg-schroeder]

Bump @fedirjh :)

[fedirjh]

BugZero Checklist:

• Link to the PR: This is a feature, the offline feedback was not implemented for this use case.
• Link to comment: N/A
• Link to discussion: N/A
• Determine if we should create a regression test for this bug: Yes

1. Go Offline
2. Add Money Request in Chat Report (DM)
3. Navigate to IOU Report for that Money Request
4. Delete the money request
5. Verify that the Money Request preview in the IOU Report is displayed with striked-out style
6. Navigate to main report
7. Verify the IOU report Preview is displayed with amount 0
8. Add another Money Request in Chat Report
9. Verify that a new IOU report is created
10. Navifgate to the new IOU report
11. Add a new Money Request inside it
12. Verify that the total amount is updated to match the correct amount
13. Send some messages inside the IOU report
14. Navigate back to the main report
15. Verify that the new IOU report is displayed with the correct amount and has the correct reply count 
16. Press on the thread reply count 
17. Verify that It navigates you to the correct IOU report 

[greg-schroeder]

Thanks! Added regression test and closing.