Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-10-30] [$250] Actions showing up in the workspace chat before the Created action #47895

Closed
m-natarajan opened this issue Aug 23, 2024 · 55 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Aug 23, 2024

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: n/a
Reproducible in staging?: n/a
Reproducible in production?: n/a
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: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1724257201383519

Action Performed:

  1. Have few open reports on policy A for user A
  2. Add user A to a policy B and add to a domain group which makes policy B as primary policy
  3. As a result, all user A's open expense reports were changed to policy B

Expected Result:

Actions should not show in the workspace chat before the created action

Actual Result:

All of those expense reports were created before your workspace chat for policy B so they are showing up above the created action

  • It looks like these actions came from some erroneous pusher update, but in any case, we shouldn't ever show any actions in a report prior to the CREATED report action.
    "6252776588141512376": {
        "actionName": "REPORTPREVIEW",
        "actorAccountID": 12371319,
        "avatar": "https://d1wpcgnaa73g0y.cloudfront.net/740b0eb3e57ee8843c93c563633e5c3161654aad_128.jpeg",
        "childCommenterCount": 0,
        "childLastActorAccountID": 0,
        "childLastMoneyRequestComment": "",
        "childLastReceiptTransactionIDs": "",
        "childLastVisibleActionCreated": "",
        "childMoneyRequestCount": 0,
        "childOldestFourAccountIDs": "",
        "childRecentReceiptTransactionIDs": [],
        "childReportNotificationPreference": "hidden",
        "childType": "expense",
        "childVisibleActionCount": 0,
        "created": "2024-08-17 10:07:25.927",
        "lastModified": "2024-08-17 10:07:25.927",
        "message": [
            {
                "html": "NewDot Test - Expensify US owes £0.00",
                "text": "NewDot Test - Expensify US owes £0.00",
                "type": "COMMENT",
                "whisperedTo": []
            }
        ],
        "originalMessage": {
            "lastModified": "2024-08-17 10:07:25.927",
            "linkedReportID": "1215551294263381"
        },
        "person": [
            {
                "style": "strong",
                "text": "Georgia Monahan",
                "type": "TEXT"
            }
        ],
        "reportActionID": "6252776588141512376",
        "shouldShow": true
    },
    "5084862352615282380": {
        "actionName": "REPORTPREVIEW",
        "actorAccountID": 12371319,
        "avatar": "https://d1wpcgnaa73g0y.cloudfront.net/740b0eb3e57ee8843c93c563633e5c3161654aad_128.jpeg",
        "childCommenterCount": 0,
        "childLastActorAccountID": 0,
        "childLastMoneyRequestComment": "",
        "childLastReceiptTransactionIDs": "",
        "childLastVisibleActionCreated": "",
        "childMoneyRequestCount": 0,
        "childOldestFourAccountIDs": "",
        "childRecentReceiptTransactionIDs": [],
        "childReportNotificationPreference": "hidden",
        "childType": "expense",
        "childVisibleActionCount": 0,
        "created": "2024-08-17 10:06:26.527",
        "lastModified": "2024-08-17 10:06:26.527",
        "message": [
            {
                "html": "NewDot Test - Expensify US owes £0.00",
                "text": "NewDot Test - Expensify US owes £0.00",
                "type": "COMMENT",
                "whisperedTo": []
            }
        ],
        "originalMessage": {
            "lastModified": "2024-08-17 10:06:26.527",
            "linkedReportID": "3360153746267419"
        },
        "person": [
            {
                "style": "strong",
                "text": "Georgia Monahan",
                "type": "TEXT"
            }
        ],
        "reportActionID": "5084862352615282380",
        "shouldShow": true
    },
    "5957720805168818909": {
        "actionName": "REPORTPREVIEW",
        "actorAccountID": 9645353,
        "avatar": "https://d1wpcgnaa73g0y.cloudfront.net/d77919198004a3d382b30ccc2edf037612ca2416_128.jpeg",
        "childCommenterCount": 0,
        "childLastActorAccountID": 0,
        "childLastMoneyRequestComment": "",
        "childLastReceiptTransactionIDs": "",
        "childLastVisibleActionCreated": "",
        "childMoneyRequestCount": 0,
        "childOldestFourAccountIDs": "",
        "childRecentReceiptTransactionIDs": [],
        "childReportNotificationPreference": "hidden",
        "childType": "expense",
        "childVisibleActionCount": 0,
        "created": "2024-04-05 14:37:28.035",
        "lastModified": "2024-04-05 14:37:28.035",
        "message": [
            {
                "html": "NewDot Test - Expensify US owes £0.00",
                "text": "NewDot Test - Expensify US owes £0.00",
                "type": "COMMENT",
                "whisperedTo": []
            }
        ],
        "originalMessage": {
            "lastModified": "2024-04-05 14:37:28.035",
            "linkedReportID": "3849616819060670",
            "originalActionAccountID": 0
        },
        "person": [
            {
                "style": "strong",
                "text": "EXFY Finance",
                "type": "TEXT"
            }
        ],
        "reportActionID": "5957720805168818909",
        "shouldShow": true
    },
    "5275740842263550920": {
        "actionName": "REPORTPREVIEW",
        "actorAccountID": 12371319,
        "avatar": "https://d1wpcgnaa73g0y.cloudfront.net/740b0eb3e57ee8843c93c563633e5c3161654aad_128.jpeg",
        "childCommenterCount": 0,
        "childLastActorAccountID": 0,
        "childLastMoneyRequestComment": "",
        "childLastReceiptTransactionIDs": "",
        "childLastVisibleActionCreated": "",
        "childMoneyRequestCount": 0,
        "childOldestFourAccountIDs": "",
        "childRecentReceiptTransactionIDs": [],
        "childReportNotificationPreference": "hidden",
        "childType": "expense",
        "childVisibleActionCount": 0,
        "created": "2024-08-17 07:41:57.180",
        "lastModified": "2024-08-17 07:41:57.180",
        "message": [
            {
                "html": "NewDot Test - Expensify US owes £0.00",
                "text": "NewDot Test - Expensify US owes £0.00",
                "type": "COMMENT",
                "whisperedTo": []
            }
        ],
        "originalMessage": {
            "lastModified": "2024-08-17 07:41:57.180",
            "linkedReportID": "4840725136389079"
        },
        "person": [
            {
                "style": "strong",
                "text": "Georgia Monahan",
                "type": "TEXT"
            }
        ],
        "reportActionID": "5275740842263550920",
        "shouldShow": true
    },
    "6771333556565437795": {
        "actionName": "REPORTPREVIEW",
        "actorAccountID": 12371319,
        "avatar": "https://d1wpcgnaa73g0y.cloudfront.net/740b0eb3e57ee8843c93c563633e5c3161654aad_128.jpeg",
        "childCommenterCount": 0,
        "childLastActorAccountID": 12371319,
        "childLastMoneyRequestComment": "",
        "childLastReceiptTransactionIDs": "",
        "childLastVisibleActionCreated": "",
        "childMoneyRequestCount": 3,
        "childOldestFourAccountIDs": "",
        "childRecentReceiptTransactionIDs": [],
        "childReportNotificationPreference": "always",
        "childType": "expense",
        "childVisibleActionCount": 0,
        "created": "2024-08-21 14:21:01.239",
        "lastModified": "2024-08-21 14:21:01.239",
        "message": [
            {
                "type": "COMMENT",
                "html": "",
                "text": "",
                "isEdited": false,
                "whisperedTo": [],
                "isDeletedParentAction": false,
                "deleted": "2024-08-21 16:08:33.117"
            }
        ],
        "originalMessage": {
            "lastModified": "2024-08-21 14:21:01.239",
            "linkedReportID": "8764984834814485",
            "deleted": "2024-08-21 16:08:33.117"
        },
        "person": [
            {
                "type": "TEXT",
                "style": "strong",
                "text": "Georgia Monahan"
            }
        ],
        "reportActionID": "6771333556565437795",
        "shouldShow": true,
        "timestamp": 1724250061,
        "reportActionTimestamp": 1724250061239,
        "automatic": false,
        "childReportID": "8764984834814485",
        "childReportName": "[NewDot Test - Expensify US] Georgia's Expenses 2024-08-21 #R00e8uTLDVx7",
        "childOwnerAccountID": 12371319,
        "whisperedToAccountIDs": []
    },
    "3597186314041479192": {
        "reportActionID": "3597186314041479192",
        "actionName": "CREATED",
        "created": "2024-08-21 14:21:01.003",
        "reportActionTimestamp": 1724250061003,
        "avatar": "https://d1wpcgnaa73g0y.cloudfront.net/740b0eb3e57ee8843c93c563633e5c3161654aad_128.jpeg",
        "message": [
            {
                "type": "TEXT",
                "style": "strong",
                "text": "Georgia Monahan (georgia@expensify.com)"
            },
            {
                "type": "TEXT",
                "style": "normal",
                "text": " created this report"
            }
        ],
        "person": [
            {
                "type": "TEXT",
                "style": "strong",
                "text": "georgia@expensify.com"
            }
        ],
        "automatic": false,
        "sequenceNumber": 0,
        "shouldShow": true,
        "lastModified": "2024-08-21 14:21:01.003"
    }
}```



## Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

## Platforms:
<!---
Check off any platforms that are affected by this issue
--->
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

</details>

[View all open jobs on GitHub](https://github.com/Expensify/App/issues?q=is%3Aopen+is%3Aissue+label%3A%22Help+Wanted%22)

<details><summary>Upwork Automation - Do Not Edit</summary>
    <ul>
        <li>Upwork Job URL: https://www.upwork.com/jobs/~01e262549903248d36</li>
        <li>Upwork Job ID: 1827939799718735549</li>
        <li>Last Price Increase: 2024-10-14</li>
<li>Automatic offers: </li>
<ul>
<li>dukenv0307 | Reviewer | 104455568</li>
<li>c3024 | Contributor | 104455570</li>
</ul></li>
    </ul>
</details>

<details><summary>Issue Owner</summary>Current Issue Owner: @dukenv0307

<details><summary>Issue Owner</summary>Current Issue Owner: @kadiealexander</details>
@m-natarajan m-natarajan added Daily KSv2 Needs Reproduction Reproducible steps needed Bug Something is broken. Auto assigns a BugZero manager. labels Aug 23, 2024
Copy link

melvin-bot bot commented Aug 23, 2024

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

@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@daledah
Copy link
Contributor

daledah commented Aug 23, 2024

Edited by proposal-police: This proposal was edited at 2024-08-23 03:56:38 UTC.

Proposal

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

All of those expense reports were created before your workspace chat for policy B so they are showing up above the created action

What is the root cause of that problem?

We're sorting the report actions based on timestamp and ID:

* The report actions need to be sorted by created timestamp first, and reportActionID second

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

Need to somehow shift the CREATED action up to the first item.

const createdActionIndex = sortedActions.findIndex((action) => action.actionName === 'CREATED');
if (createdActionIndex !== -1) {
    sortedActions.unshift(sortedActions.splice(createdActionIndex, 1)[0]);
}

@daledah
Copy link
Contributor

daledah commented Aug 23, 2024

Proposal Update

  • Add code changes

@kadiealexander kadiealexander added the External Added to denote the issue can be worked on by a contributor label Aug 26, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

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

@kadiealexander kadiealexander removed the Needs Reproduction Reproducible steps needed label Aug 26, 2024
@melvin-bot melvin-bot bot changed the title Actions showing up in the workspace chat before the Created action [$250] Actions showing up in the workspace chat before the Created action Aug 26, 2024
@melvin-bot melvin-bot bot added Overdue Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 26, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

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

@dukenv0307
Copy link
Contributor

@m-natarajan Can you share the video/screenshots? I don't know how to reproduce this issue. Thanks

@melvin-bot melvin-bot bot added the Overdue label Aug 29, 2024
@puneetlath
Copy link
Contributor

I think you can re-create it by doing the following.

  1. Create workspaceA
  2. Turn on delayed submissions
  3. Create an expense report, but don't submit it
  4. Create workspaceB
  5. In OldDot, move the workspace of the expense report from workspaceA to workspaceB
  6. This will cause the expense report to be moved onto the workspace chat for workspaceB, but with a created action that is prior to the workspaceB chat's creation

@daledah
Copy link
Contributor

daledah commented Aug 30, 2024

@puneetlath I could not reproduce following the steps. Maybe it only happens when expenses are moved to domain group's policy, which is a BE bug. However, based on OP:

It looks like these actions came from some erroneous pusher update, but in any case, we shouldn't ever show any actions in a report prior to the CREATED report action.

We should modify the FE to always show CREATED action first to prevent such issues in the future.

Copy link

melvin-bot bot commented Aug 30, 2024

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

@c3024
Copy link
Contributor

c3024 commented Aug 31, 2024

Proposal

In view of this comment, I am changing my suggestion into a proposal if this is how we want to fix it.

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

Other report actions show up before created action if they have time earlier than the created action of the report.

What is the root cause of that problem?

We sort the reports based on report action created time first and only after this we prioritize the CREATED action. So, an action with earlier created time can show up before CREATED action of the report.

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

We can swap the order of these two conditions:

if (first.created !== second.created) {
return (first.created < second.created ? -1 : 1) * invertedMultiplier;
}
// Then by action type, ensuring that `CREATED` actions always come first if they have the same timestamp as another action type
if ((first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED || second.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) && first.actionName !== second.actionName) {
return (first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED ? -1 : 1) * invertedMultiplier;
}

Then the CREATED action always comes first.

What alternative solutions did you explore? (Optional)

Earlier comment

If we swap the order of these two conditions:

if (first.created !== second.created) {
return (first.created < second.created ? -1 : 1) * invertedMultiplier;
}
// Then by action type, ensuring that `CREATED` actions always come first if they have the same timestamp as another action type
if ((first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED || second.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) && first.actionName !== second.actionName) {
return (first.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED ? -1 : 1) * invertedMultiplier;
}

the CREATED action always comes first.

This used to be the case until it was changed here by @roryabraham.

The issue for this PR also mentions that it was intended to fix the same issue of ensuring the CREATED action is shown first, but I could not understand how the change in the PR helps with that.

Copy link

melvin-bot bot commented Sep 2, 2024

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

Copy link

melvin-bot bot commented Sep 3, 2024

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

@puneetlath
Copy link
Contributor

We should modify the FE to always show CREATED action first to prevent such issues in the future.

Yes, I agree with this. This makes sense to me.

@dukenv0307
Copy link
Contributor

@roryabraham Can you check this comment

@melvin-bot melvin-bot bot removed the Overdue label Sep 4, 2024
Copy link

melvin-bot bot commented Sep 6, 2024

@kadiealexander @dukenv0307 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!

@melvin-bot melvin-bot bot changed the title [$250] Actions showing up in the workspace chat before the Created action [HOLD for payment 2024-10-30] [$250] Actions showing up in the workspace chat before the Created action Oct 23, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 23, 2024
Copy link

melvin-bot bot commented Oct 23, 2024

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

Copy link

melvin-bot bot commented Oct 23, 2024

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

If no regressions arise, payment will be issued on 2024-10-30. 🎊

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

Copy link

melvin-bot bot commented Oct 23, 2024

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:

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

@kadiealexander
Copy link
Contributor

Reassigning for someone to handle payment as I'm OOO for the next two weeks.

@kadiealexander kadiealexander removed their assignment Oct 25, 2024
@kadiealexander kadiealexander added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Oct 25, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 25, 2024
@kadiealexander kadiealexander added Weekly KSv2 and removed Daily KSv2 labels Oct 25, 2024
@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Oct 29, 2024
Copy link

melvin-bot bot commented Nov 1, 2024

@puneetlath, @stitesExpensify, @c3024, @dukenv0307 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@puneetlath
Copy link
Contributor

@c3024 has been paid.

@dukenv0307 waiting on the checklist for you.

@melvin-bot melvin-bot bot removed the Overdue label Nov 1, 2024
@dukenv0307
Copy link
Contributor

BugZero Checklist:

  • The PR that introduced the bug has been identified. Link to the PR: [No QA] Ensure CREATED actions come first #14213
  • 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: https://github.com/Expensify/App/pull/14213/files#r1826473040
  • 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: N/A
  • Determine if we should create a regression test for this bug. Yes
  • 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.

Regression test:

  1. Have few open reports on policy A for user A
  2. Add user A to a policy B and add to a domain group which makes policy B as primary policy
  3. As a result, all user A's open expense reports were changed to policy B
  4. Verify that there are no messages shown above the top most action (that is the action with Avatar and welcome message) in the workspace chat

Do we 👍 or 👎

@dukenv0307
Copy link
Contributor

@puneetlath added the checklist

@melvin-bot melvin-bot bot added the Overdue label Nov 4, 2024
Copy link

melvin-bot bot commented Nov 5, 2024

@puneetlath, @stitesExpensify, @c3024, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Nov 11, 2024

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

Copy link

melvin-bot bot commented Nov 13, 2024

@puneetlath, @stitesExpensify, @c3024, @dukenv0307 10 days overdue. I'm getting more depressed than Marvin.

@puneetlath
Copy link
Contributor

Whoops, this one fell through the cracks.

Regression test here: https://github.com/Expensify/Expensify/issues/444024

All paid. Thanks y'all!

@melvin-bot melvin-bot bot removed the Overdue label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests