Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

Unread items in LHN and recipient name not displayed #48599

Closed m-natarajan closed 1 month ago

m-natarajan 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: Reproducible in staging?: Needs reproduction Reproducible in production?: Needs reproduction 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: @twisterdotcom Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1725028457379559

Action Performed:

  1. Create a new account via NewDot and set the display name. Sign out.
  2. Sign into NewDot as an account manager.
  3. Sign into OldDot with an account on the managed domain. Create a new workspace.
  4. Invite the account from [1] to the workspace as an admin. This should trigger the automated message.

Expected Result:

The account manager should see the complete LHN item for the automated message we send on their behalf to the new invitee.

Actual Result:

The account manager sees the incomplete LHN item for the automated message we send on their behalf to the new invitee.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

image (5)

Onyx data for messages not clicked:

{
    "lastReadTime": "2024-08-30 14:08:35.518",
    "chatType": "",
    "lastActorAccountID": 5356024,
    "lastMessageText": "Hey there! Dropping by to say hello as I see that you're a new Expensify admin …",
    "lastVisibleActionCreated": "2024-08-30 14:08:35.632",
    "ownerAccountID": 0,
    "participants": {
        "5356024": {
            "hidden": false,
            "notificationPreference": "always"
        },
        "18246674": {
            "hidden": false,
            "notificationPreference": "always"
        }
    },
    "policyID": "_FAKE_",
    "reportID": "1067844102881951",
    "reportName": "Chat Report",
    "stateNum": 0,
    "statusNum": 0,
    "type": "chat"
}

Onyx Data for messages clicked:

    "lastReadTime": "2024-08-30 14:33:29.880",
    "chatType": "",
    "lastActorAccountID": "5356024",
    "lastMessageText": "Hey there! Dropping by to say hello as I see that you're a new Expensify admin …",
    "lastVisibleActionCreated": "2024-08-30 14:08:24.341",
    "ownerAccountID": 0,
    "participants": {
        "5356024": {
            "hidden": false,
            "notificationPreference": "always"
        },
        "9074303": {
            "hidden": false,
            "notificationPreference": "always"
        }
    },
    "policyID": "_FAKE_",
    "reportID": "5568459257246026",
    "reportName": "Chat Report",
    "stateNum": 0,
    "statusNum": 0,
    "type": "chat",
    "currency": "USD",
    "description": "",
    "errorFields": {},
    "hasOutstandingChildRequest": false,
    "hasOutstandingChildTask": false,
    "isCancelledIOU": false,
    "isOwnPolicyExpenseChat": false,
    "isPinned": false,
    "isWaitingOnBankAccount": false,
    "lastActionType": "ADDCOMMENT",
    "lastMessageHtml": "Hey there! Dropping by to say hello as I see that you're a new Expensify admin at your company 👋 Click this <a href=\"https://calendar.app.google/D65fYfraepNdE4GeA\">link</a> to book a call with me so that I can help you get up to speed with Expensify. I'll walk you through everything you need to know, address any concerns you have, and ensure you are getting the most out of Expensify. I'm also available via chat to answer any questions you have.",
    "lastReadSequenceNumber": 0,
    "lastVisibleActionLastModified": "2024-08-30 14:08:24.341",
    "managerID": 0,
    "nonReimbursableTotal": 0,
    "notificationPreference": "always",
    "oldPolicyName": "",
    "permissions": [
        "read",
        "write"
    ],
    "private_isArchived": "",
    "total": 0,
    "unheldTotal": 0,
    "welcomeMessage": "",
    "writeCapability": "all"
}

Add any screenshot/video evidence

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @jasperhuangg
melvin-bot[bot] commented 2 months ago

Current assignee @twisterdotcom is eligible for the Bug assigner, not assigning anyone new.

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @jasperhuangg (AutoAssignerNewDotQuality)

MelvinBot commented 2 months ago

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

jasperhuangg commented 2 months ago

So to summarize the linked Slack convo it seems like the unread items that aren't displaying correctly are for automated messages we send (namely this one).

It seems we're not sending the personal details of the new invitee to the account manager when we send the message on their behalf which is preventing the message from displaying correctly in the LHN.

This is a back-end issue.

jasperhuangg commented 2 months ago

Based on the code it seems the reproduction steps for this are:

  1. Create a new account via NewDot and set the display name. Sign out.
  2. Sign into NewDot as an account manager.
  3. Sign into OldDot with an account on the managed domain. Create a new workspace.
  4. Invite the account from [1] to the workspace as an admin. This should trigger the automated message.
  5. The account manager should see the incomplete LHN item for the automated message we send on their behalf to the new invitee.

Updating the OP

jasperhuangg commented 2 months ago

Hmm I'm unable to reproduce the issue locally based on the reproduction steps I provided above. It appears the personal details for the new invitee are always sent to the account manager, and the LHN item always appears as expected.

@twisterdotcom Do you mind looking over my reproduction steps to see if they make sense? There's a chance that this was just a transient Pusher issue.

We can try to reproduce this again with your account on staging too.

Screenshot 2024-09-05 at 11 04 38 AM Screenshot 2024-09-05 at 11 02 36 AM
jasperhuangg commented 2 months ago

Took a look again, here are the logs from when the automated message should have been sent. They don't really say much in terms of Onyx updates.

We should send the personalDetails from Auth here, gonna add a log to see if that's being hit locally. There's a chance something was wrong when I tried to reproduce it yesterday.

jasperhuangg commented 2 months ago
Screenshot 2024-09-06 at 9 32 39 PM

Hmm this time I wasn't able to reproduce exactly the same result as in the OP but the chat does show up as Hidden which probably isn't what we want.

jasperhuangg commented 2 months ago

Strange we're not passing shouldPushOnyxUpdates here which means we don't push the personal details of the participants when someone creates a chat report via this flow.

Passing true to that parameter causes the personal details to be pushed out correctly.

I'm guessing we don't call CreateChatReport normally when creating DMs for NewDot (we use OpenReport which handles pushing those updates separately) which explains why this happens. Since this is all automated in the back-end we don't call OpenReport.

I think a safer bet to just call OpenReport instead of CreateChatReport since that's what we call when we create a DM in the front-end anyways.

jasperhuangg commented 2 months ago

Opened https://github.com/Expensify/Web-Expensify/pull/43404 which I believe fixes the issue, even though the problem I reproduced wasn't exactly the same as the one in the OP. Regardless it definitely still fixes a bug that I was able to consistently reproduce locally.

@twisterdotcom Do you mind working together on the staging QA of that PR to verify it fixes your problem?

twisterdotcom commented 2 months ago

Happy to test on staging. One Q here though is that I wonder whether this also solves the underlying extra problem that this message shouldn't ever be unread for the AM anyway - it's a message from them, so it should always be read by them. It should only become unread once the other participant(s) send a message right?

jasperhuangg commented 2 months ago

@twisterdotcom True, that's definitely also an issue. The solution I provided doesn't address that issue, but I can handle that here too.

I wonder if there was some reason we left it this way though? Perhaps if an AM is on #focus mode then we want them to be able to see any new assignments they have, which is why we make it unread for them so it pops up in their LHN?

twisterdotcom commented 2 months ago

This isn't a new assignment, it's just a new admin for an existing assignment. We have enough triggers and reports for them to be aware of new assignments, so I don't think this is intended at all. They should not distract from the workload they already have on, so they really should be marked as read by the AM when this message is sent.

muttmuure commented 1 month ago

Whenever we designed the payload of Onyx data for these AM messages, we must have set the unread flag to true.

muttmuure commented 1 month ago

But it should be unread for the recipient, not unread for both the sender and recipient

muttmuure commented 1 month ago

I wonder if there's a comment in the code that will explain if/why we did it this way

muttmuure commented 1 month ago

I see Rachel Chang in the blame of the notification in the web repo. But the original issue is here:

https://github.com/Expensify/Expensify/issues/120062

melvin-bot[bot] commented 1 month ago

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

twisterdotcom commented 1 month ago

@jasperhuangg what do you need to get started on this?

jasperhuangg commented 1 month ago

Ah I missed this in my K2 because of the reviewing label, sorry for the delay. It seems I have everything I need to get started, I just wanted to confirm that I wasn't overriding something done on purpose to make these LHN rows unread.

melvin-bot[bot] commented 1 month ago

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

jasperhuangg commented 1 month ago

Got caught up addressing another daily issue today, I should be able to tackle this tomorrow.

muttmuure commented 1 month ago

Thanks!

jasperhuangg commented 1 month ago

A lot has happened in this issue so I just wanted to provide an update on what I'll be working on next.

The original reason this issue was created has been addressed in https://github.com/Expensify/Web-Expensify/pull/43404 (steps 1-6 below). However, there were concerns that the admin invite notification should not show as unread for the AMs since they are the ones sending the message (step 7 below)

So we basically want the following expected behavior:

  1. Create an account (accountA) and generate a domain via ./script/clitools.sh generator:domain for that account.
  2. Sign into your Expensify account (accountB) locally and navigate to https://www.expensify.com.dev/concierge/#/tools/UserAssignment and assign your Expensify account to the newly generated domain as an account manager.
  3. Sign into accountA on OldDot. Create a workspace.
  4. In a separate window, sign into accountB on NewDot.
  5. Invite a non-existent user (accountC) as an admin to the workspace in OldDot with accountA.
  6. Verify in accountB's LHN you see a well-formed LHN row that correctly displays accountC's email with Hey there! Dropping by to .....
  7. Verify that this LHN row does not have the unread styles applied.
melvin-bot[bot] commented 1 month ago

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

jasperhuangg commented 1 month ago

https://github.com/Expensify/Web-Expensify/pull/43659 is on prod, I think it should be safe to close this out!