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.34k stars 2.77k forks source link

[HOLD Issue #309277][$1000] LHN - Chat that doesn't have a message does not disappears from the LHN #22806

Closed lanitochka17 closed 11 months ago

lanitochka17 commented 1 year 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!


Action Performed:

Scenario "empty DMs"

  1. Go to https://staging.new.expensify.com/
  2. Login with any account
  3. Search for a user that you don't have any messages with and open the chat. Don't send any message
  4. Navigate away from the conversation by clicking any other available conversation in the LHN list

Scenario "empty threads"

  1. Go to https://staging.new.expensify.com/
  2. Login with any account
  3. Select any user
  4. Select "Reply in Tread" option

Expected Result:

  1. Conversation with no messages should disappear from LHN conversation list
  2. Thread report is not displayed until the comment is posted

Actual Result:

  1. Conversation that doesn't have any messages doesn't disappear from from LHN
  2. Thread report is displayed when the comment is not posted

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.37.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

Scenario "empty DMs"

https://github.com/Expensify/App/assets/78819774/dacf6489-8f37-4f7f-99b7-dbadc42cb41f

Scenario "empty threads"

https://github.com/Expensify/App/assets/93399543/74a0a9f2-16d7-4194-a7bc-320ea8633dc7

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013fc034f8ebadf86c
  • Upwork Job ID: 1681415083049984000
  • Last Price Increase: 2023-08-01
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

dukenv0307 commented 1 year ago

Proposal

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

LHN - Chat that doesn't have a message does not disappear from the LHN

What is the root cause of that problem?

Currently, when clicking new chat or creating thread report, we will call OpenRpeort API. And then we only hide chat threads that haven't been commented on as comments here: https://github.com/Expensify/App/blob/754b6edf175a10deb6c6876bdba13c2a99e0ffec/src/libs/ReportUtils.js#L2152

We don't have any logic to hide the main report that hasn't been commented on

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

When clicking new chat or creating new thread, we only should create new optimistic chat and redirect to this chat (don't call any API)

And then, In here https://github.com/Expensify/App/blob/754b6edf175a10deb6c6876bdba13c2a99e0ffec/src/libs/ReportUtils.js#L2156 We should add one more condition to hide the main report that hasn't been commented on

if ( !report.lastMessageText && !report. lastMessageTranslationKey && !report. isLastMessageDeletedParentAction) {
        return false;
    }

isLastMessageDeletedParentAction field is introduced by this PR: https://github.com/Expensify/App/pull/22864

What alternative solutions did you explore? (Optional)

dukenv0307 commented 1 year ago

@alexpensify

One more thing, if the user creates a new room and doesn't send any message, could we should hide the room report in LHN? If not, we should add condition to check if this report is room or not?

As I mentioned in my proposal above, could you help to confirm? And then I can update my proposal as expected

alexpensify commented 1 year ago

I'll review soon and update.

alexpensify commented 1 year ago

This one is on my testing list for the week.

kbecciv commented 1 year ago

Hi @alexpensify We have Slack discussion about the opening "empty threads" and for starting "empty DMs" https://expensify.slack.com/archives/C049HHMV9SM/p1689466696046149, how would you like me to handle the opening "empty threads? Would you like me to create another GH ticket or combine? @quinthar would like to fully test that shared solution on both flows. Thank you in advance.

alexpensify commented 1 year ago

@kbecciv - let's add the additional flow to this GH. Please update the original GH to include the flow that David mentioned, Thanks!

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~013fc034f8ebadf86c

melvin-bot[bot] commented 1 year ago

Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

alexpensify commented 1 year ago

Adding the external label for now. @rushatgabhane - could you please review the proposal and identify if it will work to address both flows? Thanks!

kbecciv commented 1 year ago

@alexpensify I added two scenarios, please let me know if anything else I need to do. Thanks

dukenv0307 commented 1 year ago

@alexpensify @kbecciv I have a bit curious. Could you help me to confirm?

  1. Currently, When clicking reply in the thread, The App will redirect to the thread report, and the new report is created in LHN. But if the user clicks on another report, the thread report will disappear. We don't want to display the thread report in LHN when the user clicks on reply in the thread eventhough the report screen is on this thread chat, right? I want to confirm that because maybe it is intentional as mentioned here https://github.com/Expensify/App/blob/2d2008aae95588443adb0814b12151023ddb9792/src/libs/ReportUtils.js#L2098-L2103
  2. If the user creates a new room and doesn't send any message, could we should hide the room report in LHN?
alexpensify commented 1 year ago

@quinthar - you were part of the Slack thread that mentioned the other flow. Can you share more feedback for @dukenv0307's questions? Thanks!

alexpensify commented 1 year ago

I'll start a discussion tomorrow to confirm the flows.

alexpensify commented 1 year ago

I started a discussion here: https://expensify.slack.com/archives/C049HHMV9SM/p1690236131313079?thread_ts=1689466696.046149&cid=C049HHMV9SM

quinthar commented 1 year ago

Hi there, great questions. First, I'd like to expand the problem description. The problem consists of two parts:

Problem 1

When you create a DM/thread, it is shared with the other party immediately, before you even type. It appears as unread on their LHN, which reveals that you are thinking about writing something to them, but haven't yet. This is a confusing privacy violation.

Problem 2

When you a DM/thread, if you choose not to post anything, you and the party still see it in your LHNs forever.

Solution

Do not notify the server of the new DM/thread until after the user has actually posted the first message. If they do not post a message, then the server should never know that you even considered it.

alexpensify commented 1 year ago

@dukenv0307 are there other questions or are you good to prepare your updated proposal that includes both flows?

dukenv0307 commented 1 year ago

@quinthar Thanks for your clarification.

If the user creates a new room without a message, could we hide the room report in LHN?

Could you also help to confirm this point?

melvin-bot[bot] commented 1 year ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

rushatgabhane commented 1 year ago

If the user creates a new room without a message, could we hide the room report in LHN?

I think we shouldn't hide room even if there is no message.

A room is different from a DM because it has multiple participants. There are no privacy concerns. Also, it should stay in LHN forever because other people can start a conversation.

For similar reasons, I think we shouldn't hide new groups either.

dukenv0307 commented 1 year ago

Updated proposal

melvin-bot[bot] commented 1 year ago

@alexpensify @rushatgabhane 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!

alexpensify commented 1 year ago

Not overdue, the proposal will go through another round of reviews.

alexpensify commented 1 year ago

Update: waiting for the proposal review here.

alexpensify commented 1 year ago

@rushatgabhane - any update here? If no reply by tomorrow, I will be looking to reassign. Thank you!

melvin-bot[bot] commented 1 year ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

rushatgabhane commented 1 year ago

@dukenv0307 did you forget to update your proposal?

dukenv0307 commented 1 year ago

Solution Do not notify the server of the new DM/thread until after the user has actually posted the first message. If they do not post a message, then the server should never know that you even considered it.

@quinthar While implementing this solution, I see that we have a problem: Currently, When the user clicks the new chat, we will send OpenReport API to Back-end, and then the correct avatar, display name, and accountIDs of the new user will be updated in Front-End. With our solution, We don't notify the server about the new chat so the information of the new user is wrong in Front-end. See the below video:

https://github.com/Expensify/App/assets/129500732/18d3b87b-4403-4f67-863e-8e3fd350a4cf

cc @rushatgabhane

dukenv0307 commented 1 year ago

@rushatgabhane @quinthar In case User A opens a new chat with User B, in order not to refactor too much code, I suggest that:

  1. The main screen of user A will redirect to a new chat with user B and the new report will appear in LHN. User A still notifies to server but the server doesn't notify user B.
  2. If User A click to another chat, the main screen will redirect and the report with user B will disappear in LHN
  3. If User A send a message to user B, user A will notify to Server and Server will notify to user B.
rushatgabhane commented 1 year ago

. User A still notifies to server but the server doesn't notify user B

@dukenv0307 will that require backend changes? Maybe we can have an endpoint to get the correct avatar, display name, and accountID. It shouldn't write to the database.

dukenv0307 commented 1 year ago

Maybe we can have an endpoint to get the correct avatar, display name, and accountID

@rushatgabhane We just migrated to accoundID, there is no way to get the correct avatar, display name, and accountID based on email.

@dukenv0307 will that require backend changes?

yes

rushatgabhane commented 1 year ago

๐ŸŽ€ ๐Ÿ‘€ ๐ŸŽ€ Requesting an internal review on the feasibility of @dukenv0307's suggestion here. Or if you have a better suggestion

melvin-bot[bot] commented 1 year ago

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

cristipaval commented 1 year ago

I'll discuss this internally and come up with an update soon.

melvin-bot[bot] commented 1 year ago

@alexpensify @cristipaval @rushatgabhane this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

cristipaval commented 1 year ago

@rushatgabhane @quinthar In case User A opens a new chat with User B, in order not to refactor too much code, I suggest that:

  1. The main screen of user A will redirect to a new chat with user B and the new report will appear in LHN. User A still notifies to server but the server doesn't notify user B.
  2. If User A click to another chat, the main screen will redirect and the report with user B will disappear in LHN
  3. If User A send a message to user B, user A will notify to Server and Server will notify to user B.

@dukenv0307 I totally get your challenge in solving the problem in the front-end only. Thanks for your suggested solution. I still wouldn't create a new report in the backend before the user posts anything in the chat. @Beamanator has more knowledge about how user details work and he has a great idea. What if the front end fetches the avatar and display name from the backend when a new chat is opened and there's nothing posted in the chat yet? And we stick with the plan to notify the server about the new chat only when the user posts something in it. Does this solution work?

rushatgabhane commented 1 year ago

yeah precisely. i agree!

Maybe we can have an endpoint to get the correct avatar, display name, and accountID. It shouldn't write to the database.

i suggested the same thing here https://github.com/Expensify/App/issues/22806#issuecomment-1661839008

dukenv0307 commented 1 year ago

@rushatgabhane @cristipaval From the last discussion

I still wouldn't create a new report in the backend before the user posts anything in the chat

I see that we can't get the correct avatar, display name, and accountID. We can only create the default report from FE and after the user sends the first message, we will call API from the server and update the correct avatar, display name, and accountID. Please correct me if I am wrong

cristipaval commented 1 year ago

yeah precisely. i agree!

Maybe we can have an endpoint to get the correct avatar, display name, and accountID. It shouldn't write to the database.

i suggested the same thing here #22806 (comment)

aaaah, I missed your comment there. Good one ๐Ÿ‘

cristipaval commented 1 year ago

@rushatgabhane @cristipaval From the last discussion

I still wouldn't create a new report in the backend before the user posts anything in the chat

I see that we can't get the correct avatar, display name, and accountID. We can only create the default report from FE and after the user sends the first message, we will call API from the server and update the correct avatar, display name, and accountID. Please correct me if I am wrong

The idea is to create a new command that front-end hits when opens a new chat in front-end:

  1. Bob never texted Alice before
  2. Bob searches for alice@email.com
  3. Front-end creates an optimistic chat and fetches the avatar and display name for alice@email.com to show them in the optimistically created chat
  4. If Bob leaves the chat, it disappears from LHN
  5. If Bob posts anything in the chat, the front-end notifies the server about the chat, and the message

Does this make sense @dukenv0307 @rushatgabhane ?

alexpensify commented 1 year ago

Thank you @cristipaval for this context!

rushatgabhane commented 1 year ago

sounds good! @dukenv0307 @cristipaval maybe we can work on the frontend in parallel ? or should we hold on backend changes?

dukenv0307 commented 1 year ago
  1. Bob never texted Alice before
  2. Bob searches for alice@email.com
  3. Front-end creates an optimistic chat and fetches the avatar and display name for alice@email.com to show them in the optimistically created chat
  4. If Bob leaves the chat, it disappears from LHN
  5. If Bob posts anything in the chat, the front-end notifies the server about the chat, and the message

Make sense to me. With this approach, I have a bit curios:

  1. we need to create a new API to get an avatar and display name for alice@email.com (step 3).
    1. In step 5, we need to call 2 API OpenReport and AddComment. @rushatgabhane @cristipaval Please correct me If I am wrong
cristipaval commented 1 year ago

Make sense to me. With this approach, I have a bit curios:

  1. we need to create a new API to get an avatar and display name for alice@email.com (step 3).

That's correct.

    1. In step 5, we need to call 2 API OpenReport and AddComment. @rushatgabhane @cristipaval Please correct me If I am wrong

You're correct, but this violates our 1:1:1 pattern, we want to have only 1 API call for a user action. We'll update the OpenReport API command to pass the first message to that report.

dukenv0307 commented 1 year ago

@rushatgabhane I create a totally new proposal to follow new design

Proposal

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

LHN - Chat that doesn't have a message does not disappear from the LHN

What is the root cause of that problem?

We decide to change the logic when opening a new chat as mentioned here

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

Summarize new flow here

From the front-end side, we need

  1. When the user clicks new user on the search page, we only create a new optimistic report and fetch the DisplayName, accountID, and avatar based on login (It is a new API) instead of calling OpenReport API
  2. We should hide the chat only if you leave it without any message https://github.com/Expensify/App/blob/754b6edf175a10deb6c6876bdba13c2a99e0ffec/src/libs/ReportUtils.js#L2153-L2156 We should add one more condition to hide the main report that hasn't been commented on like this
    
    if ( !report.lastMessageText && !report. lastMessageTranslationKey && !report. isLastMessageDeletedParentAction) {
        return false;
    }


If it is the current report, It always displays on LHN by this logic
https://github.com/Expensify/App/blob/754b6edf175a10deb6c6876bdba13c2a99e0ffec/src/libs/ReportUtils.js#L2121-L2123
3. When the user sends the first message, we will call OpenReport instead of AddComment

From the back-end side, we need
1. Create a new API to get DisplayName, accountID, and avatar based on login
2. Update OpenReport API also to add the first comment

### What alternative solutions did you explore? (Optional)
alexpensify commented 1 year ago

Not overdue - @rushatgabhane when you get a chance can you please review the most recent proposal? Thanks!

rushatgabhane commented 1 year ago

and avatar based on email

to be pedantic - based on login* (it could be email or phone)

rushatgabhane commented 1 year ago

We should hide 1:1 chat if there is no message

no. we hide the chat only if you leave it without any message how will that be achieved?