[HOLD for payment 2024-10-29] [$250] LHN - RBR appears on the wrong workspace chat for an error occurring on another workspace

IuliiaHerets commented 2 months ago

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

Version Number: 9.0.23-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): nathanmulugetatesting+1097@gmail.com Issue reported by: Applause Internal Team

Action Performed:

Navigate to Old Dot and sign up with a new account
Go to settings > Account and validate the account
Navigate to staging.new.expensify.com and sign in with the same account signed in on Old Dot
Create a workspace and name it as "Workspace 1" to make things easier to notice
Go to workspace editor and enable workflows
Go to workflows and enable delayed submission and set it to manual and enable approvals also
Create another workspace and name it as "Workspace 2"
Navigate to Workspace 1 chat and create a manual expense
Click on submit but do not click on approve
Navigate to Old dot and refresh the page
Go to reports and click on the expense created on ND
Click on detail and change the workspace from workspace 1 to workspace 2
Go to New dot and wait for a few seconds for the change to take effect
After the changes took effect (Workspace 1 will not have RBR on the LHN and instead workspace 2 will have RBR and the expense will be moved to it)
Go to workspace chat 2 and create a scan expense with a normal picture so that it will throw error after it is done scanning
Observe the LHN of workspace chat 1 on the LHN

Expected Result:

No RBR will be present on the workspace chat 1

Actual Result:

RBR is present on workspace chat 1 even if the scan expense which has an error is on the workspace chat 2. When opening the workspace chat 1 with RBR there is nothing inside the workspace chat.

Workaround:

Unknown

Platforms:

[x] Android: Native
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/a55bb442-598c-44df-8fa9-7fab7a178439

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~015a69a9ddbd29631a
Upwork Job ID: 1828511929613408564
Last Price Increase: 2024-09-03

Issue Owner

Current Issue Owner: @garrettmknight

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 2 months ago

We think that this bug might be related to #wave-collect - Release 1

IuliiaHerets commented 2 months ago

@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 2 months ago

@garrettmknight Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~015a69a9ddbd29631a

garrettmknight commented 2 months ago

Reproduced, opening up. Not sure if this is BE or FE so starting with External.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@garrettmknight, @ntdiary Eep! 4 days overdue now. Issues have feelings too...

SIMalik commented 2 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue. The Left-Hand Navigation (LHN) - Red Brick Road (RBR) indicator is incorrectly appearing in a workspace chat due to an error originating in a different workspace.

What is the root cause of that problem? At Step-9, during the SubmitReport response, the following REPORTPREVIEW action was received for iouReportID: "5726784747188717" in Workspace 77-1: Complete SubmitReport Response.

        {
            "key": "reportActions_2380165600597457",
            "onyxMethod": "merge",
            "value": {
                "5238883410418754601": {
                    "actionName": "REPORTPREVIEW",
                    "actorAccountID": 18253981,
                    "avatar": "https:\/\/[d2k5nsl2zxldvw.cloudfront.net](http://d2k5nsl2zxldvw.cloudfront.net/)\/images\/avatars\/default-avatar_14.png",
                    "childCommenterCount": 0,
                    "childLastActorAccountID": 18253981,
                    "childLastMoneyRequestComment": "",
                    "childLastReceiptTransactionIDs": "",
                    "childLastVisibleActionCreated": "",
                    "childMoneyRequestCount": 1,
                    "childOldestFourAccountIDs": "",
                    "childRecentReceiptTransactionIDs": [],
                    "childReportNotificationPreference": "hidden",
                    "childType": "expense",
                    "childVisibleActionCount": 0,
                    "created": "2024-09-02 05:16:26.449",
                    "lastModified": "2024-09-02 05:16:26.449",
                    "message": [
                        {
                            "html": "Workspace 77-1 owes Rs\u00a05.00",
                            "text": "Workspace 77-1 owes Rs\u00a05.00",
                            "type": "COMMENT",
                            "whisperedTo": []
                        }
                    ],
                    "originalMessage": {
                        "lastModified": "2024-09-02 05:16:26.449",
                        "linkedReportID": "5726784747188717"
                    },
                    "person": [
                        {
                            "style": "strong",
                            "text": "O77F O77L",
                            "type": "TEXT"
                        }
                    ],
                    "reportActionID": "5238883410418754601",
                    "shouldShow": true
                }

At Step-14, the reportActionID: "5238883410418754601" was marked as deleted in Workspace 77-1: [Complete OpenReport Response]

                    "message": [
                        {
                            "type": "COMMENT",
                            "html": "",
                            "text": "",
                            "isEdited": false,
                            "whisperedTo": [],
                            "isDeletedParentAction": false,
                            "deleted": "2024-09-02 05:40:23.154"
                        }
                    ],
                    "originalMessage": {
                        "deleted": "2024-09-02 05:40:23.154",
                        "lastModified": "2024-09-02 05:16:26.449",
                        "linkedReportID": "5726784747188717"
                    },

Simultaneously, the iouReport (ID: "5726784747188717") was shifted to Workspace 77-2 with a new reportActionID: 7661044324410620555. [Complete OpenReport Response]

However, the deleted reportActionID: "5238883410418754601" remains linked to Workspace 77-1. Now when Workspace77-1 try process it, it follows the following steps to find the errors

https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/OptionsListUtils.ts#L744

https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/OptionsListUtils.ts#L492

https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/ReportUtils.ts#L7067

And at following line it has wrongly picked the linkedReportID: "5726784747188717" which is an ID of iouReport and has been shifted in second Workspace 77-2 https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/ReportActionsUtils.ts#L921

Due to this wrong IOUReportID 5726784747188717 of Workspace 77-2 following line return true and causing to show RBR

shouldShowRBRForMissingSmartscanFields(IOUReportID)

https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/ReportUtils.ts#L7068

What changes do you think we should make in order to solve the problem? Replace https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/ReportUtils.ts#L7062-L7063 With

function hasSmartscanError(reportActions: ReportAction[]) {
    const nonDeletedReportActions = reportActions.filter(action => !ReportActionsUtils.isDeletedAction(action));

    return nonDeletedReportActions.some((action) => {

What alternative solutions did you explore? If we expect getIOUReportIDFromReportActionPreview to always return a non-deleted reportAction ID, we should modify the function directly.

Replace https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/ReportActionsUtils.ts#L917-L922 With

 /**
  * Get the iouReportID for a given report action.
 */
function getIOUReportIDFromReportActionPreview(reportAction: OnyxEntry<ReportAction>): string {
    return isActionOfType(reportAction, CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW) && !isDeletedAction(reportAction)? getOriginalMessage(reportAction)?.linkedReportID ?? '-1' : '-1';
}

After Code Change: Local: 47874-withoutRBR

melvin-bot[bot] commented 2 months ago

📣 @SIMalik! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

github-actions[bot] commented 2 months ago

@SIMalik Your proposal will be dismissed because you did not follow the proposal template.

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

ntdiary commented 2 months ago

Under review. can provide an update in about 12 hours.

ntdiary commented 2 months ago

@SIMalik, curious how did you start analyzing from the code below? Which component in the OP called this createOption function? https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/OptionsListUtils.ts#L744

As for the solution, if we decide to modify hasSmartscanError, I would prefer to

function hasSmartscanError(reportActions: ReportAction[]) {
    return reportActions.some((action) => {
        if (!ReportActionsUtils.isDeletedAction(action)) {
            return false
        }

// or 
function hasSmartscanError(reportActions: ReportAction[]) {
    return reportActions.some((action) => {
        if (!ReportActionsUtils.shouldReportActionBeVisible(action, action.reportActionID)) {
            return false
        }

unlike when displaying the message list, we didn't filter out the invisible messages when collecting all errors for the report. https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/OptionsListUtils.ts#L467-L469

message list logic: https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/pages/home/report/ReportActionsList.tsx#L190-L200

Additionally, these two RBRs should also not be displayed when switching to workspace 1

cc @garrettmknight

melvin-bot[bot] commented 2 months ago

@garrettmknight @ntdiary this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

SIMalik commented 2 months ago

@garrettmknight @ntdiary

1.

@SIMalik, curious how did you start analyzing from the code below? Which component in the OP called this createOption function?

The getAllReportErrors function is called in multiple parts of the app. Specifically, when we click on a scanned expense that has or will have violations, it follows the following path to trigger the function.

https://github.com/Expensify/App/blob/156b8b0da67d1df00b2dbc0aa0f25ed2f03f9ef3/src/hooks/useReportIDs.tsx#L98-L114 https://github.com/Expensify/App/blob/156b8b0da67d1df00b2dbc0aa0f25ed2f03f9ef3/src/libs/SidebarUtils.ts#L108 https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/OptionsListUtils.ts#L492

https://github.com/user-attachments/assets/aed3e3cc-af00-463e-bcb0-c913ac0f2937

2.

As for the solution, if we decide to modify hasSmartscanError, I would prefer to function hasSmartscanError(reportActions: ReportAction[]) { return reportActions.some((action) => { if (!ReportActionsUtils.isDeletedAction(action)) { return false } // or function hasSmartscanError(reportActions: ReportAction[]) { return reportActions.some((action) => { if (!ReportActionsUtils.shouldReportActionBeVisible(action, action.reportActionID)) { return false }

Yes, we can modify the function
https://github.com/Expensify/App/blob/156b8b0da67d1df00b2dbc0aa0f25ed2f03f9ef3/src/libs/ReportUtils.ts#L7075-L7080

to:

function hasSmartscanError(reportActions: ReportAction[]) {
    return reportActions.some((action) => {
        if ((!ReportActionsUtils.isSplitBillAction(action) && !ReportActionsUtils.isReportPreviewAction(action)) || ReportActionsUtils.isDeletedAction(action)) {
            return false;
        }
        const IOUReportID = ReportActionsUtils.getIOUReportIDFromReportActionPreview(action);

Additionally, these two RBRs should also not be displayed when switching to workspace 1

The RBR for Workspace 1 is no longer displayed.
Left Bottom RBR: This seems to be a separate issue, unrelated to the workspace. The RBR appears in other cases as well, as shown in the video. However, if you'd prefer to address it here, I can investigate further. Otherwise, we can raise a separate issue for it.

https://github.com/user-attachments/assets/ad5167d5-b7db-4a5a-877b-f31abbbc7a34

ntdiary commented 2 months ago

OK, I think we can move forward with @SIMalik's proposal. To summarize briefly, the cause of this issue is that we didn't filter out the deleted reportActions when showing a workspace's RBR. https://github.com/Expensify/App/blob/2c041292660d1d3b04cb2e19a72b56d16e70ef52/src/libs/OptionsListUtils.ts#L467-L469

So, we can fix this specific scan issue by applying the filter condition in hasSmartscanError, So far, I haven’t found any potential regressions this might cause:

function hasSmartscanError(reportActions: ReportAction[]) {
    return reportActions.some((action) => {
        if ((!ReportActionsUtils.isSplitBillAction(action) && !ReportActionsUtils.isReportPreviewAction(action)) || ReportActionsUtils.isDeletedAction(action)) {
            return false;
        }
        const IOUReportID = ReportActionsUtils.getIOUReportIDFromReportActionPreview(action);

Furthermore, if we don’t want to collect any errors related to deleted reportActions, we can also filter reportActions directly in getAllReportErrors. Or since getAllReportErrors is a common function, if we don’t want to affect other callers, we can pass already filtered reportActions when LHN calls getAllReportErrors. :)

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

SIMalik commented 2 months ago

Contributor details Your Expensify account email: f18ba010@ibitpu.edu.pk Upwork Profile Link: https://www.upwork.com/freelancers/~01511dd712b8a399be?viewMode=1

melvin-bot[bot] commented 2 months ago

⚠️ Unable to store contributor details.

neil-marcellini commented 1 month ago

OK, I think we can move forward with @SIMalik's proposal.

Cool, I agree. They did a good job finding the root cause.

Furthermore, if we don’t want to collect any errors related to deleted reportActions, we can also filter reportActions directly in getAllReportErrors.

I think we should take this approach because it seems the most simple. If an action is deleted I don't think it should be causing an error.

melvin-bot[bot] commented 1 month ago

The BZ member will need to manually hire SIMalik for the Contributor role. Please store your Upwork details and apply to our Upwork job so this process is automatic in the future!

SIMalik commented 1 month ago

Contributor details Expensify account email: f18ba010@ibitpu.edu.pk Upwork Profile Link: https://www.upwork.com/freelancers/~01511dd712b8a399be?viewMode=1

SIMalik commented 1 month ago

Hi @neil-marcellini, @ntdiary,

The code has been updated as per your suggestions. I made the change according to the new code. Specifically, I replaced the following line: https://github.com/Expensify/App/blob/d389de2c9a6ae0d51d03fea9edcfe78d0dd88861/src/libs/OptionsListUtils.ts#L474

with

const reportActionsArray = Object.values(reportActions ?? {}).filter(action => !ReportActionUtils.isDeletedAction(action));

This ensures we are only processing non-deleted reportActions, as suggested. Let me know if this aligns with your expectations.

Thanks!

https://github.com/user-attachments/assets/06791eb9-2e6d-4cf2-9cd9-517ceefeb3fb

ntdiary commented 1 month ago

@SIMalik, LGTM. :)

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @garrettmknight, @ntdiary, @neil-marcellini, @SIMalik 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!

ntdiary commented 1 month ago

Not overdue, PR is still under testing. :)

neil-marcellini commented 1 month ago

How's this going? When you do you think the PR will be ready?

ntdiary commented 4 weeks ago

How's this going? When you do you think the PR will be ready?

@neil-marcellini, @SIMalik is trying to setup the iOS native app, they have already completed the Android-mWeb & MacOS-Chrome screenshots, four other platforms is still missing. Optimistically, the testing could be completed by next week.

Or perhaps we could approve and merge the PR this week, as I've already tested it on all platforms. As for the other errors @SIMalik is encountering in their environment setup, I can help him work through them gradually so that they will feel more confident handling other issues in the future. 😄

melvin-bot[bot] commented 2 weeks ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 2 weeks ago

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

https://github.com/Expensify/App/pull/49202

If no regressions arise, payment will be issued on 2024-10-29. :confetti_ball:

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

@ntdiary requires payment through NewDot Manual Requests
@SIMalik requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 2 weeks ago

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:

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

SIMalik commented 2 weeks ago

Hi @neil-marcellini, @ntdiary

I hope this message finds you well. Please see the following details regarding the job offer on Upwork, which I have yet to receive. Could you kindly assist with any necessary actions to help move this forward?

https://github.com/Expensify/App/issues/47874#issuecomment-2338315958 melvin-bot:

Unable to store contributor details.

https://github.com/Expensify/App/issues/47874#issuecomment-2342315624 melvin-bot:

The BZ member will need to manually hire SIMalik for the Contributor role. Please store your Upwork details and apply to our Upwork job so this process is automatic in the future!

https://github.com/Expensify/App/issues/47874#issuecomment-2342900925 SIMalik commented on Sep 11

Contributor details Expensify account email: f18ba010@ibitpu.edu.pk Upwork Profile Link: https://www.upwork.com/freelancers/~01511dd712b8a399be?viewMode=1

Thank you for your time and support in helping facilitate this process. Please let me know if you need further information from my end.

Best regards, SIMalik

SIMalik commented 1 week ago

https://github.com/Expensify/App/issues/47874#issuecomment-2428231172 melvin-bot:

@SIMalik requires payment (Needs manual offer from BZ)

ntdiary commented 1 week ago

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.51-4 and is now subject to a 7-day regression period If no regressions arise, payment will be issued on 2024-10-29. 🎊

@SIMalik, don't worry, I will fill the BugZero Checklist tomorrow, and BZ member may send you the offer after the regression period. :)

ntdiary commented 1 week ago

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:

[x] [@ntdiary] The PR that introduced the bug has been identified. Link to the PR: N/A

[x] [@ntdiary] 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: N/A

[x] [@ntdiary] 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: No need

[x] [@ntdiary] Determine if we should create a regression test for this bug. No need

[x] [@ntdiary] 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. No need

[x] [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

This issue should be an edge case that was overlooked during the original feature development. After excluding the deleted messages, this issue should no longer recur, so I don't have strong feelings on regression test. If you have different thoughts, please feel free to let me know. :)

melvin-bot[bot] commented 1 week ago

Payment Summary

Upwork Job

Reviewer: @ntdiary owed $250 via NewDot
ROLE: @SIMalik paid $250 via Upwork (LINK)

BugZero Checklist (@garrettmknight)

[x] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[x] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1828511929613408564/hired)
[x] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[x] I have verified the payment summary above is correct

garrettmknight commented 1 week ago

@SIMalik offer out to you in Upwork https://www.upwork.com/nx/wm/offer/104649625

garrettmknight commented 1 week ago

@ntdiary request when you're ready.

JmillsExpensify commented 1 week ago

$250 approved for @ntdiary

Expensify / App