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.6k stars 2.93k forks source link

Web - LHN - Conversation that doesn't have a message doesn't disappear from LHN #11633

Closed kavimuru closed 1 year ago

kavimuru commented 2 years 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:

  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

Expected Result:

Conversation with no messages should disappear from LHN conversation list

Actual Result:

Conversation that doesn't have any messages doesn't disappear from from LHN

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: v1.2.12-0 Reproducible in staging?: Y Reproducible in production?: N Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Bug5765545_screenshot_29

https://user-images.githubusercontent.com/43996225/194372269-408e6e2b-6a3d-4073-ae99-dfa0dfeaa3d2.mp4

Expensify/Expensify Issue URL: Issue reported by: Applause internal team Slack conversation:

View all open jobs on GitHub

OSBotify commented 2 years ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @johnmlee101 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

johnmlee101 commented 2 years ago

Can reproduce, looking why

johnmlee101 commented 2 years ago

PR is up!

kbecciv commented 2 years ago

@johnmlee101 QA team is experienced the same issue on build 1.2.18.2

https://user-images.githubusercontent.com/93399543/196811127-eeb184b0-342f-4847-908e-13d2bfeef10f.mp4

sketchydroide commented 2 years ago

It seems this is already happening in Prod removing blocker

sketchydroide commented 2 years ago

Is this a regression @johnmlee101? I set it as daily, as it's not a blocker

johnmlee101 commented 2 years ago

@tgolen @ctkochan22 https://github.com/Expensify/App/pull/11725 As per this change, we're okay leaving no comment standard chats in the LHN? I just realized this might have slipped through if this was unintended πŸ˜“

tgolen commented 2 years ago

Let's have @JmillsExpensify or @trjExpensify chime in on exactly what's expected. I don't think we had a clear idea before.

trjExpensify commented 2 years ago

Sorry for the delay, I've been OoO until today! (I'm also a poet.. πŸ˜„).

As per this change, we're okay leaving no comment standard chats in the LHN?

To clarify, we're not talking about archived chats like we were here? We're looking at..

  1. CMD+F to a DM with someone you haven't messaged before
  2. Navigate away from the chat without writing a message
  3. The empty chat remains in the LHN

If that's correct, then I agree we shouldn't keep those empty DMs in the LHN.

JmillsExpensify commented 2 years ago

Ooh oops I missed this as well. That said, I agree with @trjExpensify that we couldn't keep empty DMs in the LHN>

JmillsExpensify commented 1 year ago

Huh, interesting. What's going on with this issue? Did we ever circle back and push an updated PR?

flodnv commented 1 year ago

+1 πŸ˜„ we should probably wait until https://github.com/Expensify/App/pull/13324 is done to revisit this.

However, I disagree with what is written in the issue body. This is not a bug, the actual behavior is the expected behavior, IMO. I vote we close this.

flodnv commented 1 year ago

I agree with @trjExpensify that we couldn't keep empty DMs in the LHN

I disagree with that, I don't really see a problem with this. What's the actual problem?

trjExpensify commented 1 year ago

My rationale personally is that your LHN is sorted by "most recent chats". There's nothing recent about an empty chat nobody has chatted in, so why does it continue to show with no activity?

flodnv commented 1 year ago

Maybe.. I feel like this is really a detail that does not matter much. In your thinking, when you create a new chat with someone you've never chatted to before and do nothing, you have the chat open waiting for you to write something -- does it show in the LHN or not? I think either answer to that question is both wrong and right πŸ˜„ If we want to mimic Whatsapp then it won't appear in the LHN until there's a message in it, which I think is just as fine as the other option.

trjExpensify commented 1 year ago

That could be another option, for sure. It would be a change to the concept of the "active chat" always being highlighted in the LHN though (which is "yes" to your question above). When you have a "full" LHN of chats below the fold, it doesn't matter so much as you rarely see it until you write a message and it moves up to the top (well, beneath pinned and outstandingIOUs) as you draft.

It might be a bit weird before you have a lot of chats and in #focus mode though, because you could get that feeling of being "no where" in the UI if the active chat isn't highlighted in the LHN until you navigate away from it. As in, conceivably the LHN is entirely empty.

JmillsExpensify commented 1 year ago

Huh, that's interesting. I see where @flodnv is coming from.

It might be a bit weird before you have a lot of chats and in #focus mode though, because you could get that feeling of being "no where" in the UI if the active chat isn't highlighted in the LHN until you navigate away from it. As in, conceivably the LHN is entirely empty.

Mind explaining this another way? I'm not sure I follow it.

trjExpensify commented 1 year ago

(Excuse the old branding scheme yada yada), but if we roll with Flo's suggestion and don't show the active chat row in the LHN until there's a message, it opens up a scenario where it could look like this on large screens in #focus mode:

image

versus:

image

JmillsExpensify commented 1 year ago

Interesting. I agree that makes #focus mode pretty confusing. That said, if you have some semblance of a message, so a draft, then the chat would show in the LHN. So yes there is an inconsistency, but it's a pretty ephemeral one because all you need to do in order to get it show is type something.

trjExpensify commented 1 year ago

Right, I don't think anyone is advocating for not showing the active chat when in draft in the LHN. The point is to highlight that if we go this route, we are breaking an existing consistency of always having the active chat in the LHN and this weirdness (albeit short lived in practice) is possible as a result. Sounds like we're okay with that? In which case, we're here:

Don't show the chat in the LHN until there's a draft message.

If you delete the draft, the chat row disappears again presumably?

@flodnv confirming we're aligned here?

flodnv commented 1 year ago

I think we are, yes πŸ˜„ Basically, do not show a chat for which the only reportAction is the CREATED one.

flodnv commented 1 year ago

Maybe let's open a new, clean issue?

trjExpensify commented 1 year ago

do not show a chat for which the only reportAction is the CREATED one.

Question on that though for my understanding.. if you're typing a draft message is there still only a CREATED reportAction though?

flodnv commented 1 year ago

Hm yes, but we do agree that if there is a draft message, it should show show in the LHN.

trjExpensify commented 1 year ago

Right, we do.. so "do not show a chat for which the only reportAction is the CREATED one." isn't wholly accurate for our intentions with this then? πŸ˜…

flodnv commented 1 year ago

Correct. I think we want "Do not show a chat in the LHN for which the only reportAction is the CREATED one and does not have a draft."

Said another (non french) way, in the LHN we should show chats that have at least 1 ADDCOMMENT action, OR 1 draft.

trjExpensify commented 1 year ago

Cool, so I think a new issue with the following is where we landed. Will you be taking it, John, or someone else?

Title: Show chats in the LHN that have at least 1 ADDComment action, or 1 draft message.

Action Performed:

  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, open the chat but don't send any message

Expected results: Chat should not show in the LHN as there is not at least 1 ADDComment action, or 1 draft message.

Actual results: Empty chat shows in the LHN and remains in the LHN after simply navigating to it.

flodnv commented 1 year ago

πŸ‘

JmillsExpensify commented 1 year ago

Missed the more recent discussion, though I agree with where we landed. Did we create this issue yet?

trjExpensify commented 1 year ago

I didn't get to this, so if someone wants to create it, go for it. Else, I can later next week when I'm back.

On Wed, Jan 18, 2023 at 4:42 PM Jason Mills @.***> wrote:

Missed the more recent discussion, though I agree with where we landed. Did we create this issue yet?

β€” Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11633#issuecomment-1387369848, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD3246K4WYTA7KIDB6X5BILWTAMQHANCNFSM6AAAAAAQ62D7TI . You are receiving this because you were mentioned.Message ID: @.***>

--

Tom Rhys Jones Expensify

JmillsExpensify commented 1 year ago

Cool, I can get to it tomorrow. Leaving a tab open for this.

johnmlee101 commented 1 year ago

Not overdue!

JmillsExpensify commented 1 year ago

Related issue created here: https://github.com/Expensify/App/issues/14523

JmillsExpensify commented 1 year ago

Is there anything else left to do here? I kind of lost where we're at for this issue.

flodnv commented 1 year ago

Thanks @JmillsExpensify , we can close this now

lanitochka17 commented 1 year ago

Hello Issue reproducible. Windows10/Chrome. Build 1.3.37.1 Thanks

https://github.com/Expensify/App/assets/78819774/f03c1309-5b6b-4905-b50b-f6024692d8d8

melvin-bot[bot] commented 1 year ago

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

johnmlee101 commented 1 year ago

I think this is a separate issue. Do we want welcome messages to show as unread on the LHN? Can you create a new issue to address this?

melvin-bot[bot] commented 1 year ago

@johnmlee101 Huh... This is 4 days overdue. Who can take care of this?

johnmlee101 commented 1 year ago

Closing to create a new issue

flodnv commented 1 year ago

Maybe we are solving this here https://github.com/Expensify/App/pull/19321 ?

johnmlee101 commented 1 year ago

Yeah it seems relevant to the original issue https://github.com/Expensify/App/issues/14523