fix for offline money request issues #32698

rojiphil · 2023-12-07T20:19:38Z

Details

This PR covers various offline scenarios during the deletion of Money Request in IOU Report. The issues are listed in detail under the Tests section.

Fixed Issues

$ #26511
PROPOSAL: #26511 (comment)

Tests

Issue 1:

Problem: When Offline, deleting the last Money Request (without any comments) in IOU Report removes Report Preview from parent Chat Report
Steps:

Go Offline
Add Money Request in Chat Report.
Go to IOU Report for the Money Request
Delete Money Request
Verify the following: Report Preview should be shown with amount 0. Further, Money Request preview in IOU Report should also be shown with striked-out amount

Issue 2:

Problem: When Offline, Report Preview shows TBD on deletion of last Money Request in IOU Report with comments
Steps:

Go Offline
Add Money Request in Chat Report
Go to IOU Report for Money Request
Add a message in IOU Report
Delete the only Money Request in IOU Report (created in Step 2)
Go to parent Chat Report
Verify the following: Amount 0 should be shown in Report Preview in Chat Report

Issue 3:

Problem : When Offline, Report Preview does not update amount when a new Money Request is added
Steps:

Go Offline
Add Money Request in 1:1 DM Chat Report (Note: Chat Report is not a Workspace Chat)
Go to IOU Report for Money Request
Add a message in IOU Report
Delete the only Money Request in IOU Report (created in Step 2)
Go to parent Chat Report.
Go to IOU Report again by clicking on the Report Preview and add a new Money Request
Go to parent Chat Report and review Report Preview amount.
Verify the following: Report Preview amount should be updated with money request amount of step 7

Issue 4:

Problem : When Offline, IOU Report page shows blank on navigating back from parent Chat Report via click on Reply link
Steps:

Go Offline
Add Money Request in Chat Report
Go to IOU Report for Money Request
Add a message in IOU Report
Go to parent Chat Report. Verify that the Report Preview shows with Reply Count.
Click on Reply link to go to IOU Report
IOU Report Screen should be shown instead of showing blank.

Verify that no errors appear in the JS console

Offline tests

Same as the Steps for Tests Section.

QA Steps

Same as the Steps for Tests Section.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web-Safari

Issue-1

26511-web-safari-01_r.mp4

Issue-2

26511-web-safari-02_r.mp4

Issue-3

26511-web-safari-03-.mp4

Issue-4

26511-web-safari-04-.mp4

Desktop

Issue-1

26511-desktop-01_r.mp4

Issue-2

26511-desktop-02_r.mp4

Issue-3

26511-6-desktop-03.mp4

Issue-4

26511-6-desktop-04.mp4

Mobile Web - Chrome

Issue-1

26511-mweb-chrome-01_r.mp4

Issue-2

26511-mweb-chrome-02_r.mp4

Issue-3

26511-5-mweb-chrome-03.mp4

Issue-4

26511-5-mweb-chrome-04.mp4

Mobile Web - Safari

Issue-1

26511-mweb-safari-01_r.mp4

Issue-2

26511-mweb-safari-02_r.mp4

Issue-3

26511-3-mweb-safari-03.mp4

Issue-4

26511-3-mweb-safari-04.mp4

iOS

Issue-1

26511-ios-native-01_r.mp4

Issue-2

26511-ios-native-02_r.mp4

Issue-3

26511-ios-native-03-.mp4

Issue-4

26511-ios-native-04-.mp4

Android

Issue-1

26511-android-native-01_r.mp4

Issue-2

26511-android-native-02.mp4

Issue-3

26511-4-android-native-03.mp4

Issue-4

26511-4-android-native-04.mp4

…fline-money-request-issues

melvin-bot · 2023-12-08T15:16:19Z

@fedirjh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

rojiphil · 2023-12-09T17:38:00Z

@fedirjh

The PR is ready for review for the proposed issues.
I have tested across all platforms and it seems to work well.
Over to you for review.

rojiphil · 2023-12-11T11:04:30Z

@fedirjh

While testing, I found one more issue which is as below.
Can you please check this and let me know if we can add this to the scope?

Issue 5:

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

Problem : When Offline, deletion of a Money Request which has visible comments does not show greyed Money Request review in IOU Report
Steps:
1 Go Offline
2 Add Money Request (A) in Chat Report
3 Go to IOU Report for Money Request
4 Add a message to the IOU Report.
5 Click on the Money Request Preview for (A) to show the Transaction Thread.
6 Add a message to Transaction Thread
7 Delete the Money Request using 3dots menu. Verify that the focus comes back to IOU Report.
Actual Result: Deleted request is displayed for the deleted Money Request in offline scenario.
Expected Result: When offline, greyed Money Request preview should still be shown instead of Deleted request

Issue-5 Video

26511-Issue-5.mp4

What is the root cause of that problem?

Here, we check if we need to show the preview or the deleted/reversed transaction message. During deletion of Money Request, we set the pending action as delete or update here. However, when offline, we do not use this information to decide on showing the preview even when there is a delete or update pending action. This is the root cause.

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

We can check for pending action and show the preview instead of the deleted/reversed transaction text here like this.

    const isOfflinePendingForDeleteOrUpdate =
        network.isOffline &&
        (lodashGet(action, 'pendingAction', null) === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || lodashGet(action, 'pendingAction', null) === CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE);

    return (isDeletedParentAction || isReversedTransaction) && !isOfflinePendingForDeleteOrUpdate ? (

Also, pass the isOfflinePendingForDeleteOrUpdate to MoneyRequestPreview here like this:
```
     isOfflinePendingForDeleteOrUpdate={isOfflinePendingForDeleteOrUpdate}
```
Further, in MoneyRequestPreview, we must add a boolean property for isOfflinePendingForDeleteOrUpdate.

Then, use isOfflinePendingForDeleteOrUpdate to ensure that a) the original amount text is displayed instead of the updated amount and b) Line through style is applied to the amount.

That is, use isOfflinePendingForDeleteOrUpdate for display amount here with const displayAmount = isOfflinePendingForDeleteOrUpdate ? getDisplayDeleteAmountText() : getDisplayAmountText();

and line through style here with isOfflinePendingForDeleteOrUpdate && styles.lineThrough,

What alternative solutions did you explore? (Optional)

fedirjh

@rojiphil Thank you the work here , I just updated my xcode and got some issues with my simulator, I couldn’t test it in IOS. I will try to run the checklist by tomorrow.

fedirjh · 2023-12-11T22:10:39Z

src/pages/home/report/ReportActionItem.js

@@ -524,7 +524,7 @@ function ReportActionItem(props) {
                {shouldDisplayThreadReplies && (
                    <View style={draftMessageRightAlign}>
                        <ReportActionItemThread
-                            childReportID={`${props.action.childReportID}`}
+                            childReportID={`${props.action.childReportID || ReportActionsUtils.getIOUReportIDFromReportActionPreview(props.action)}`}


@rojiphil As you have already updated the optimistic data , there is no further need for this code.

Got it. I have removed the redundant code.

fedirjh · 2023-12-14T00:05:09Z

src/libs/ReportActionsUtils.ts

+    // Ignore Report Preview as last action for LHN display if the action is deleted and is also pending for deletion.
+    if (isReportPreviewAction(reportAction) && isMessageDeleted(reportAction) && reportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
+        return false;
+    }


Could you please explain a little bit this one, what to ignore ? and why we have to do it ?

Could you please explain a little bit this one, what to ignore ? and why we have to do it ?

Sure. Here we use shouldReportActionBeVisibleAsLastAction to check if an action (in our case Report Preview action) can be considered as last Report Action for display of LHN subtitle display text for the Chat Report.

Before our fix, the Report Preview action was deleted in offline scenarios because of which LHN would display the subtitle text of the action that was immediately previous to the deleted Report Preview. Now, since we do not remove the Report Preview in offline scenario until app comes online, the last action would turn up to be the Report Preview thereby resulting in the text message of Report Preview as subtitle in LHN. So, we need to ignore the Report Preview action as last action for LHN if the IOU Report is deleted offline. This is the reason why we added the condition here.

So, here, we ignore the Report Preview if the message was deleted as set here and if the pending action is delete which we set here. In all other cases, we will not ignore and show the Report Preview text in LHN.

Hope this will suffice. Please let me know otherwise.

@rojiphil Why we can't just dispaly the action as it is in offline ? when user goes online , it will be deleted and removed from Onyx. why not keeping it visible in offline ?

Have responded here.
Let me know what you think.

…fline-money-request-issues

rojiphil · 2023-12-30T02:45:13Z

I encountered this weird bug, go offline and create some requests and delete them, then send new comments inside the IOU report, then send a new request and go online.

This PR allows me to consistently reproduce the bug on all platforms.

I suspect that the changes made in this PR are related to the bug.

I believe that both the frontend and backend are responsible for reproducing this bug.

I was able to reproduce without adding any comments inside the IOU report.

@fedirjh
Issue 1:
I will investigate further today and get back.
Also, if you are following some simple steps to reproduce the issue, can you please share?
This will help me in my investigation too.

fedirjh · 2023-12-30T02:50:04Z

Also, if you are following some simple steps to reproduce the issue, can you please share?

@rojiphil Just test with a new report with another user:

Go offline
Add new money request inside the report
Delete the money request
Add other money requests inside the same report
Go to the main report
Go online
check the IOU preview disappear for a moment
once the preview is back, navigate to the IOU report
Check the IOU report

rojiphil · 2023-12-30T02:50:47Z

I also noticed another bug , the IOU report preview disappears for some moments when you go back online. Here is a video on dev demonstrating the bug:

Issue 2:
Thanks for the test video that demonstrates the bug.
I will investigate this too.

rojiphil · 2023-12-30T12:58:36Z

Just test with a new report with another user:

Go offline

Add new money request inside the report

Delete the money request

Add other money requests inside the same report

Go to the main report

Go online

check the IOU preview disappear for a moment

once the preview is back, navigate to the IOU report

Check the IOU report

@fedirjh

Thanks for the steps. That was a good catch. And the steps were also helpful in investigating this.

The root cause is that after deleting the last Money Request in offline scenarios (Step 3), FE continues to add any further Money Requests (Step 4) to the same IOU Report. The BE is expecting no activity in the IOU Report once the last Money Request is deleted. The BE even creates a new IOU Report to honor subsequent money requests.

So, looks like the fix from the FE is to ensure that all subsequent Money Requests are considered part of a new IOU Report. We can do so by resetting the iouReportID to null as it was already done here. This will make sure that it will create a new IOU Report as shown here. This change seems to avoid both Issue 1 (i.e. unwanted actions in deleted IOU Report) and Issue 2 (i.e. Report Preview disappearing for few seconds) in the tests.

I have committed the changes. Let me know if this is fine.

rojiphil · 2023-12-30T13:05:18Z

@fedirjh

But there is one more related issue I see. When offline, the user can go to the deleted IOU Report and attempt further actions using composer FAB which does not seem right.
Do you think hiding the composer makes sense here?

We can add a property hideComposer to ReportFooter and use the condition like this here.

hideComposer={report.type === CONST.REPORT.TYPE.IOU && parentReportAction.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE}

Let me know what you think.

fedirjh · 2024-01-02T08:19:25Z

But there is one more related issue I see. When offline, the user can go to the deleted IOU Report and attempt further actions using composer FAB which does not seem right.

@rojiphil I think we can do similar to settled reports , we can remove the money request option from the composer action list

rojiphil · 2024-01-02T12:14:26Z

I think we can do similar to settled reports , we can remove the money request option from the composer action list

@fedirjh
That is a better thing to do than hiding everything.
For this, we can add changes like this here to resolve this.

        // If the Money Request report is marked for deletion, let us prevent any further money requests from being generated.
        const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '', report?.parentReportActionID ?? '');
        if (parentReportAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
            return false;
        }

This tests well too. I have committed these changes.
Let me know if this is fine.

rojiphil · 2024-01-05T10:59:09Z

@fedirjh
Gentle bump on reviewing the latest commits.

fedirjh · 2024-01-05T16:27:08Z

@rojiphil I just retested it and it looks good now. However, I found another case that we should cover. When offline, if you create a money request and then delete it, you will still be able to submit other comments inside the IOU report. However, when the user goes online, the IOU will be deleted and the comments will be lost. I think the problem arises from the backend; when the report is empty, at some point the server removes the IOU report and any further comments sent to that report will be lost. I think we should disable the composer when the last money request, inside an IOU report that doesn’t have any comments, is removed. With this fix, all aspects should be resolved, and we can proceed with the merge.

Steps to reproduce

Go offline
Request money
Open IOU
Verify IOU onlu has one request
Delete the money reuest
Send new comment
Go online
IOU report is removed and comment is lost

fedirjh · 2024-01-05T16:51:53Z

@rojiphil As for hiding the composer, I think we can make an update to this util function canUserPerformWriteAction

App/src/libs/ReportUtils.ts

Line 3903 in 393f2be

function canUserPerformWriteAction(report: OnyxEntry<Report>) {

rojiphil · 2024-01-05T17:48:27Z

As for hiding the composer, I think we can make an update to this util function canUserPerformWriteAction

@fedirjh
Yes. That's a good place to make changes.
So, the change can look like this in canUserPerformWriteAction

    // If the Money Request report is marked for deletion, let us prevent any further write action.
    if (isMoneyRequestReport(report)) {
        const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '', report?.parentReportActionID ?? '');
        if (parentReportAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
            return false;
        }
    }

Tested and works well for me. I have committed the changes as well.
Let me know if this is fine.

fedirjh

This looks good and tests well.

blimpich

Great work! Thank you @rojiphil and thank you @fedirjh for doing a thorough review 🙂

Approved ✅

OSBotify · 2024-01-09T18:07:11Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-01-10T06:20:38Z

🚀 Deployed to staging by https://github.com/blimpich in version: 1.4.24-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

blimpich · 2024-01-10T23:07:25Z

This PR appears to have caused this deploy blocker. @rojiphil @fedirjh can you take a look soon and see if we can get a fix out quickly for this? Looking at the PR its not immediately clear to me what is causing the issue. Without a fix we'll have to revert.

OSBotify · 2024-01-11T08:52:18Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.24-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

rojiphil added 11 commits December 8, 2023 00:03

1 - show report preview in offline on mr deletion

588ac68

2 - fix for TBD on last MR deletion with comments

84b6233

Merge branch 'main' of https://github.com/Expensify/App into 26511-of…

060f80a

…fline-money-request-issues

3 - display correct total amount when MR exist

337813b

4 - reply link in report preview opens iou report

0a83948

safe logic for issue-3 as per proposal

3edf97e

lint fixes

efbe915

Merge branch 'Expensify:main' into 26511-offline-money-request-issues

27b8400

jest fixes

92bdf81

added comments and minor fixes

6b954ec

prettier fix

43d403c

rojiphil marked this pull request as ready for review December 8, 2023 15:16

rojiphil requested a review from a team as a code owner December 8, 2023 15:16

melvin-bot bot requested review from fedirjh and removed request for a team December 8, 2023 15:16

Merge branch 'Expensify:main' into 26511-offline-money-request-issues

cd8191a

rojiphil marked this pull request as draft December 9, 2023 02:31

rojiphil added 5 commits December 9, 2023 10:21

merge fix and usage of nullish coalescence

18e0e5e

lint fix

59816df

prettier fix

2786593

lhn message fix for report preview

4bcd51d

console error fix on ios native device

127b84b

rojiphil marked this pull request as ready for review December 9, 2023 17:38

rojiphil mentioned this pull request Dec 9, 2023

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

Closed

6 tasks

fedirjh reviewed Dec 14, 2023

View reviewed changes

rojiphil added 2 commits December 14, 2023 16:25

Merge branch 'main' of https://github.com/Expensify/App into 26511-of…

97392e5

…fline-money-request-issues

redundant code removal and merge fix

acfb595

rojiphil added 2 commits December 30, 2023 12:24

Merge branch 'Expensify:main' into 26511-offline-money-request-issues

d996d1a

fix for unwanted actions in deleted iou report

96a3688

Merge branch 'Expensify:main' into 26511-offline-money-request-issues

86305f5

rojiphil added 2 commits January 2, 2024 17:36

remove money request from composer action list on pending deletion

00cf257

Merge branch 'Expensify:main' into 26511-offline-money-request-issues

4b731a9

rojiphil mentioned this pull request Jan 4, 2024

[$500] Web - IOU - TBD shows up for preview component after uploading picture as receipt and deleting it #33935

Closed

6 tasks

hide composer on offline deletion of last money request

538b191

fedirjh approved these changes Jan 8, 2024

View reviewed changes

melvin-bot bot requested a review from blimpich January 8, 2024 20:14

fedirjh approved these changes Jan 8, 2024

View reviewed changes

blimpich approved these changes Jan 9, 2024

View reviewed changes

blimpich merged commit 35a6916 into Expensify:main Jan 9, 2024
16 of 17 checks passed

github-actions bot mentioned this pull request Jan 10, 2024

Deploy Checklist: New Expensify 2024-01-10 #34203

Closed

54 tasks

kbecciv mentioned this pull request Jan 10, 2024

[$500] IOU - Double IOU are seen in IOU report screen #34260

Closed

6 tasks

blimpich mentioned this pull request Jan 10, 2024

[$500] [HOLD for payment 2024-01-26] Request Money - User is redirected to chat then IOU report after deleting request offline #34263

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for offline money request issues #32698

fix for offline money request issues #32698

rojiphil commented Dec 7, 2023 •

edited

Loading

melvin-bot bot commented Dec 8, 2023

rojiphil commented Dec 9, 2023

rojiphil commented Dec 11, 2023

fedirjh left a comment

fedirjh Dec 11, 2023

rojiphil Dec 14, 2023 •

edited

Loading

fedirjh Dec 14, 2023

rojiphil Dec 14, 2023 •

edited

Loading

fedirjh Dec 21, 2023 •

edited

Loading

rojiphil Dec 21, 2023

rojiphil commented Dec 30, 2023 •

edited

Loading

fedirjh commented Dec 30, 2023

rojiphil commented Dec 30, 2023

rojiphil commented Dec 30, 2023

rojiphil commented Dec 30, 2023

fedirjh commented Jan 2, 2024

rojiphil commented Jan 2, 2024

rojiphil commented Jan 5, 2024

fedirjh commented Jan 5, 2024 •

edited

Loading

fedirjh commented Jan 5, 2024

rojiphil commented Jan 5, 2024

fedirjh left a comment

blimpich left a comment

OSBotify commented Jan 9, 2024

OSBotify commented Jan 10, 2024

blimpich commented Jan 10, 2024

OSBotify commented Jan 11, 2024

fix for offline money request issues #32698

fix for offline money request issues #32698

Conversation

rojiphil commented Dec 7, 2023 • edited Loading

Details

Fixed Issues

Tests

Issue 1:

Issue 2:

Issue 3:

Issue 4:

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 8, 2023

rojiphil commented Dec 9, 2023

rojiphil commented Dec 11, 2023

Issue 5:

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

fedirjh left a comment

Choose a reason for hiding this comment

fedirjh Dec 11, 2023

Choose a reason for hiding this comment

rojiphil Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

fedirjh Dec 14, 2023

Choose a reason for hiding this comment

rojiphil Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

fedirjh Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

rojiphil Dec 21, 2023

Choose a reason for hiding this comment

rojiphil commented Dec 30, 2023 • edited Loading

fedirjh commented Dec 30, 2023

rojiphil commented Dec 30, 2023

rojiphil commented Dec 30, 2023

rojiphil commented Dec 30, 2023

fedirjh commented Jan 2, 2024

rojiphil commented Jan 2, 2024

rojiphil commented Jan 5, 2024

fedirjh commented Jan 5, 2024 • edited Loading

fedirjh commented Jan 5, 2024

rojiphil commented Jan 5, 2024

fedirjh left a comment

Choose a reason for hiding this comment

blimpich left a comment

Choose a reason for hiding this comment

OSBotify commented Jan 9, 2024

OSBotify commented Jan 10, 2024

blimpich commented Jan 10, 2024

OSBotify commented Jan 11, 2024

rojiphil commented Dec 7, 2023 •

edited

Loading

rojiphil Dec 14, 2023 •

edited

Loading

rojiphil Dec 14, 2023 •

edited

Loading

fedirjh Dec 21, 2023 •

edited

Loading

rojiphil commented Dec 30, 2023 •

edited

Loading

fedirjh commented Jan 5, 2024 •

edited

Loading