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

[$250] Thread – Group chat opens when leave thread and reopen it #47586

Closed IuliiaHerets closed 2 months ago

IuliiaHerets commented 3 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: 9.0.21-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4865736 Email or phone of affected tester (no customers): applausetester+jp_e_category_2@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in
  3. Navigate to any group chat
  4. Send a message in the group chat
  5. Open context menu and click on Reply in thread
  6. Send a comment in the thread
  7. Click on the header and Leave the thread
  8. Verify you are redirected to group chat
  9. Click on Reply to reopen the thread

Expected Result:

Thread opens

Actual Result:

Thread appears for a moment and redirect to group chat

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/f9a3af3c-7c8c-4f4d-8c3b-0fe05bf3aa9a

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ce5b7018d6f6dd8e
  • Upwork Job ID: 1825335787602372137
  • Last Price Increase: 2024-08-19
melvin-bot[bot] commented 3 months ago

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

IuliiaHerets commented 3 months ago

We think that this bug might be related to #vip-vsb

IuliiaHerets commented 3 months ago

@jliexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

excile1 commented 3 months ago

Proposal

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

After leaving a thread, opening it again causes you to get thrown back to the group chat

What is the root cause of that problem?

The root cause of the problem is the isRemovalExpectedForReportType check in the useEffect in ReportScreen.tsx, which becomes true because of || ReportUtils.isGroupChat(prevReport) https://github.com/Expensify/App/blob/62722e8bc636f1be2e2c9e8e980aac93b76804c9/src/pages/home/ReportScreen.tsx#L583

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

The fix for this is altering the check to make it something like: || (ReportUtils.isGroupChat(prevReport) && !ReportUtils.isThread(prevReport)), which will still make it true if the user leaves the entire group chat, but will be false for threads. After that, the group chat opening doesn't happen anymore, but I noticed that the thread updates (the "left the chat" message) appears after a delay, which is caused by the following lines: https://github.com/Expensify/App/blob/62722e8bc636f1be2e2c9e8e980aac93b76804c9/src/libs/actions/Report.ts#L2811-L2825 https://github.com/Expensify/App/blob/62722e8bc636f1be2e2c9e8e980aac93b76804c9/src/libs/actions/Report.ts#L2834-L2839 which makes the thread "hidden", without actually forcing the thread to reload after the user leaves. If the loading delay is an issue worth fixing, then it can be done by removing the || IsChatThread part of the check.

Result (with both fixed applied): https://github.com/user-attachments/assets/dfac5144-7a01-45ab-b270-ff7f7855a268

What alternative solutions did you explore? (Optional)

jliexpensify commented 3 months ago

Still looks to be happening on v9.0.21-3. Going to try and get this fixed because it affects everyone.

melvin-bot[bot] commented 3 months ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

nkdengineer commented 3 months ago

Proposal

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

Thread appears for a moment and redirect to group chat

What is the root cause of that problem?

After we leave a group chat thread and open it again, GetMissingOnyxMessages is called and returns the data that sets the thread report as null (see the image below).

Screenshot 2024-08-19 at 11 36 36

Then because the report is empty and this thread is also a group chat, isRemovalExpectedForReportType is true and we will navigate back to parent report here https://github.com/Expensify/App/blob/62722e8bc636f1be2e2c9e8e980aac93b76804c9/src/pages/home/ReportScreen.tsx#L581-L583

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

We should use isRootGroupChat check here that also covers the isDeprecatedGroupDM case and exclude the case the thread is a group chat

const isRemovalExpectedForReportType = 
     isEmpty(report) && 
     (ReportUtils.isMoneyRequest(prevReport) || ReportUtils.isMoneyRequestReport(prevReport) || ReportUtils.isPolicyExpenseChat(prevReport) || ReportUtils.isRootGroupChat(prevReport)); 

https://github.com/Expensify/App/blob/62722e8bc636f1be2e2c9e8e980aac93b76804c9/src/pages/home/ReportScreen.tsx#L581-L583

https://github.com/Expensify/App/blob/62722e8bc636f1be2e2c9e8e980aac93b76804c9/src/libs/ReportUtils.ts#L6944-L6946

What alternative solutions did you explore? (Optional)

NA

wildan-m commented 3 months ago

Proposal

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

Opening a group chat occurs when leaving and reopening a thread.

What is the root cause of that problem?

There is inconsistencies between FE and BE, while FE only hide the notification preferences. But BE set the report to null.

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

If we want to handle it in BE then BE should sent the below code instead of setting report to null.

{
                          notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN,
                          participants: {
                              [currentUserAccountID]: {
                                  hidden: true,
                              },
                          },
                      }

if we want to resolve it in FE, we can remove || isChatThread here and here.

What alternative solutions did you explore? (Optional)

N/A

paultsimura commented 3 months ago

Commenting to get the engineer's eyes:

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 3 months ago

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

paultsimura commented 3 months ago

Hey @srikarparsi. As the Contributors have pointed out correctly, after leaving the thread and re-visiting it, the GetMissingOnyxMessages API call returns an Onyx operation that removes the thread:

image

Is it expected for some memory optimization reasons? Should we build a FE fix around it, or change the BE behavior as @wildan-m suggested?


The FE fix works but it requires a full chat reload as it gets removed and re-fetched:

https://github.com/user-attachments/assets/e923ec44-40f0-48ad-9414-18e1fded92f6

srikarparsi commented 3 months ago

Agreed, this should be fixed in the backend. Working on a PR to do this.

paultsimura commented 2 months ago

@srikarparsi could you please remove the "external" and "help wanted" labels?

melvin-bot[bot] commented 2 months ago

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

srikarparsi commented 2 months ago

The backend fix took care of this, closing this out.