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

[$4000] LHN - The group disappears when you open the chat and appears when the page is refreshed (#focus mode) #9391

Closed
kbecciv opened this issue Jun 9, 2022 · 149 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Needs Reproduction Reproducible steps needed

Comments

@kbecciv
Copy link

kbecciv commented Jun 9, 2022

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. Open https://staging.new.expensify.com
  2. Log in to the account
  3. Go to Avatar > Preferences > enable #focus mode
  4. Then, navigate back to the main chats page.
  5. Open any chat
  6. Tap on Fab menu
  7. Create a Group Chat with 2 user (or more)
  8. Go back to LHN
  9. Refresh
  10. Tap on any other DM chat (not the Group chat you created in step 5)
  11. Go back to LHN
  12. Refresh the page
  13. See the Group Chat that you made in step 5 disappears from LHN list.

Expected Result:

Group chat conversation should remain in the LHN

Actual Result:

The group disappears when you open the chat and appears when the page is refreshed.

Platform:

Where is this issue occurring?

  • Mobile Web

Version Number: 1.1.75.1

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any mobile device

Issue reported by: Applause - Internal Team

Bug5603164_video_2022-06-09_22-17-47.mp4

Slack conversation:

View all open jobs on GitHub

Upwork job URL: https://www.upwork.com/jobs/~01e478e78066932f92

@melvin-bot
Copy link

melvin-bot bot commented Jun 9, 2022

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

@pecanoro
Copy link
Contributor

I was able to reproduce, unless I was using search, I couldn't select the newly created groups.

@pecanoro pecanoro removed their assignment Jun 10, 2022
@pecanoro pecanoro added External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. labels Jun 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 10, 2022

Triggered auto assignment to @jboniface (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@jboniface
Copy link

upwork post is here: https://www.upwork.com/jobs/~01b28296ea218398c8

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jun 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 10, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 10, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 10, 2022

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

@melvin-bot melvin-bot bot changed the title LHN - The group disappears when you open the chat and appears when the page is refreshed [$250] LHN - The group disappears when you open the chat and appears when the page is refreshed Jun 10, 2022
@mollfpr
Copy link
Contributor

mollfpr commented Jun 11, 2022

Proposal

So there's a condition on getOptions from OptionsListUtils whether we want to show the empty report or not with passing showReportsWithNoComments to true.

const shouldFilterReportIfEmpty = !showReportsWithNoComments && report.lastMessageTimestamp === 0
// We make exceptions for defaultRooms and policyExpenseChats so we can immediately
// highlight them in the LHN when they are created and have no messsages yet. We do
// not give archived rooms this exception since they do not need to be higlihted.
&& !(!ReportUtils.isArchivedRoom(report) && (isDefaultRoom || isPolicyExpenseChat));
const shouldFilterReportIfRead = hideReadReports && report.unreadActionCount === 0;
const shouldFilterReport = shouldFilterReportIfEmpty || shouldFilterReportIfRead;
if (report.reportID !== activeReportID
&& !report.isPinned
&& !hasDraftComment
&& shouldFilterReport
&& !reportContainsIOUDebt) {
return;
}

In our current implementation, we use getSidebarOptions to get the report list but we not pass showReportsWithNoComments (which by default is false).

In this example, I have the group chat with report ID 95514795 and regular chat with report ID 95514772 (this report has a chat so report.lastMessageTimestamp doesn't have 0 value, and makes the report listed in LHN).

On the Web, when we select another chat from LHN, the group report 95514795 disappear because is an empty chat. But we can open the group chats by passing the report ID to the URL, and it will be listed again in LHN.

Screen.Recording.2022-06-11.at.12.37.41.mov

This is on mobile.

screen-recording-2022-06-11-at-124207_HR7wE9jY.mp4

The solution might be passing showReportsWithNoComments to true on getSidebarOptions but I think is not ideal because it will impact the empty regular chat (I don't know if we want to show empty chat on LHN or not) or we can make an exception for group chat when determining the shouldFilterReportIfEmpty value.

+                && !(report.participants.length > 1)

const shouldFilterReportIfEmpty = !showReportsWithNoComments && report.lastMessageTimestamp === 0
// We make exceptions for defaultRooms and policyExpenseChats so we can immediately
// highlight them in the LHN when they are created and have no messsages yet. We do
// not give archived rooms this exception since they do not need to be higlihted.
&& !(!ReportUtils.isArchivedRoom(report) && (isDefaultRoom || isPolicyExpenseChat));

@jboniface
Copy link

@mananjadhav or @MonilBhavsar can we get a review here?

@melvin-bot melvin-bot bot removed the Overdue label Jun 14, 2022
@mananjadhav
Copy link
Collaborator

I agree with @mollfpr's analysis. @MonilBhavsar Do we know the expected behaviour here? Should we be showing Group chats but not the individual chats?

@kbecciv
Copy link
Author

kbecciv commented Jun 16, 2022

@mananjadhav We also seen this issue with individual chat. Please let us know if we need a separate issue.

Bug5611128_bug_chat.mp4

@mananjadhav
Copy link
Collaborator

but I think is not ideal because it will impact the empty regular chat (I don't know if we want to show empty chat on LHN or not)

@kbecciv I am aware. @mollfpr also mentioned the same thing. We might not need a separate issue. Based on the expected behaviour, we might be able to address it here. Thanks for the ping.

@mollfpr
Copy link
Contributor

mollfpr commented Nov 11, 2022

@michaelhaxhiu On the web should the group chat remain in the LHN after refresh? Because we still in the active chat and the report screen is still showing.

@trjExpensify
Copy link
Contributor

Yeah, it should remain after a refresh because it's still your active chat.

@mananjadhav
Copy link
Collaborator

mananjadhav commented Nov 11, 2022

After discussing more internally, we feel the expected results are not ideal:

Expected Result:
Group chat conversation should remain in the LHN

  • In focus mode, if you create a group chat and navigate away from it (or refresh) then it shouldn't be retained in the LHN.
  • It should only remain in the LHN while it’s the active chat (i.e. you didn't click away or refresh).

@trjExpensify @michaelhaxhiu But then in mobileWeb, even if we refresh with the active Chat, it'll show LHN only. In that case, what is the expected behaviour? To show the empty group chat? I am asking to clarify for both modes, focus mode and most recent.

@mollfpr
Copy link
Contributor

mollfpr commented Nov 11, 2022

In my understanding, it should be removed? But I'm curious about the above confirmation.

@mananjadhav
Copy link
Collaborator

In my understanding, it should be removed? But I'm curious about the above confirmation.

That's where most discussion happened when we start tracking the issue. So yeah I would like a formal confirmation on this wrt to mobile platforms.

@mollfpr
Copy link
Contributor

mollfpr commented Nov 12, 2022

Is this the expected result? @michaelhaxhiu @trjExpensify

Screen.Recording.2022-11-12.at.11.44.08.mov

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2022
@trjExpensify
Copy link
Contributor

trjExpensify commented Nov 14, 2022

That seems to be a different problem regardless of whether the chat is empty or not, or whether you're in #focus or recent mode, summarised as:

If you refresh inside the active chat on mWeb, you get redirected to the LHN instead of staying in the active chat.

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Nov 14, 2022

I've been DM'ing with Tom to make sure I follow 100%. Sorry for the earlier confusion on my part all!

Let's focus this GH on #focus-mode please. Since #recent-mode is working as expected and not core to the bug report. I think we should arrive here.

Expected Result:

  • In focus mode: if you create a brand new group chat (with no messages in it), and navigate away from it, then it shouldn't be retained in the LHN.
  • The brand new group chat should only remain in the LHN while it’s the active chat, including if you refresh the page while you are inside that active chat. (i.e. you didn't click away or refresh)

@melvin-bot melvin-bot bot removed the Overdue label Nov 14, 2022
@mananjadhav
Copy link
Collaborator

Okay got it. Thanks for clarifying. With this I think @mollfpr's video looks like an expected output for mWeb

@michaelhaxhiu @trjExpensify

@trjExpensify
Copy link
Contributor

What @mollfpr's video highlighted is that the below happens on mWeb (but not Web):

If you refresh the browser inside the active chat on mWeb, you get redirected to the LHN instead of staying in the view of the active chat.

Do you agree that's a bug @mananjadhav? Granted, It's a separate issue entirely, so should be reported in #expensify-bugs for visibility and QA reproduction, but I can repro it across both focus and most recent modes, empty chat or not on the latest staging v1.2.27-3

RPReplay_Final1668465722.MP4

@mollfpr
Copy link
Contributor

mollfpr commented Nov 15, 2022

Do you agree that's a bug @mananjadhav?

@trjExpensify Already reported #8126

@trjExpensify
Copy link
Contributor

Dope, so let's close this then and we're done here. 👍 @michaelhaxhiu I'll leave that to you to confirm.

@mananjadhav
Copy link
Collaborator

yeah we're good to close here I guess @michaelhaxhiu @trjExpensify

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Nov 15, 2022

Last step before closing is to discuss compensation for the C+ and @mollfpr for their efforts in trying to resolve this one, despite it being closed.

@mananjadhav
Copy link
Collaborator

mananjadhav commented Nov 15, 2022

I am aware we have a precedent on setting up 25% of the value generally, but for this one it would be less as this was actively tracked for a very long time.

@michaelhaxhiu
Copy link
Contributor

Sorry, when you say "it would be less" what do you mean? Are you saying more compensation feels fair?

I am starting a discussion internally by the way and will include any context you share here. Always helps to hear your thoughts as a c+ or contributor on the topic of compensation.

@mananjadhav
Copy link
Collaborator

Sorry, when you say "it would be less" what do you mean? Are you saying more compensation feels fair?

Yes. Definitely not 100% here, but a little more than 25% would be greatly appreciated. The reasoning being.

  1. The first proposal review started on mid June
  2. After clarifying few cases like individual chat, expected behavior, and different plaforms, formally accepted the proposal here,
  3. Had few comments to review here, here, here and here
  4. We then moved to the direction of handling the mWeb navigation here , after which we decided to put this on hold here
  5. After the linked PR was deployed, we couldn't reproduce this while testing, with the last discussions starting here
  6. Last review for proposal as done here after I was back from OOO, with further discussions here

So overall it was a very engaging issue which did take good amount of time to be involved, test, and review.

@michaelhaxhiu
Copy link
Contributor

Thanks for the context. I'll get back to you today.

@michaelhaxhiu
Copy link
Contributor

Hey @mollfpr and @mananjadhav, we discussed this internally and feel the 25% rule applies best here.

I know this issue had a few months of back and forth, but we think 25% of the total price is fair for the time invested and hope you agree (as well as continue to earn bigger payments in future jobs!)

@michaelhaxhiu
Copy link
Contributor

Can you please apply for the job here - https://www.upwork.com/jobs/~01e478e78066932f92?

@mollfpr
Copy link
Contributor

mollfpr commented Nov 16, 2022

@michaelhaxhiu I just apply, thanks!

@mananjadhav
Copy link
Collaborator

Thanks for the update @michaelhaxhiu. Applied

@michaelhaxhiu
Copy link
Contributor

Awaiting you both to final accept, then I can pay 👍

@mananjadhav
Copy link
Collaborator

Done @michaelhaxhiu

@mollfpr
Copy link
Contributor

mollfpr commented Nov 16, 2022

Accepted @michaelhaxhiu

@michaelhaxhiu
Copy link
Contributor

Both paid, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Needs Reproduction Reproducible steps needed
Projects
None yet
Development

No branches or pull requests