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

[$250] Web - Thread - Switching to Search and back after clicking "Join thread" opens the report #52472

Open
1 of 8 tasks
IuliiaHerets opened this issue Nov 13, 2024 · 34 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 13, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.61-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

  1. (User A) Send any message to 1:1 conversation for the account you have access to
  2. (User B) go to the new message from User A and open context menu
  3. (User B) Click "Join Thread"
  4. Navigating to Settings/Search and go to Inbox again

Expected Result:

The "Join Thread" option should not navigate to the report.

Actual Result:

"Join thread" option navigates to the report page after navigating to other bottom navigations like Search or Setting and returning back to inbox.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6663491_1731486430970.bandicam_2024-11-13_11-14-04-868.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021858763130278701100
  • Upwork Job ID: 1858763130278701100
  • Last Price Increase: 2024-12-03
  • Automatic offers:
    • mkzie2 | Contributor | 105221093
Issue OwnerCurrent Issue Owner: @parasharrajat
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 13, 2024
Copy link

melvin-bot bot commented Nov 13, 2024

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 13, 2024

Edited by proposal-police: This proposal was edited at 2024-11-13 13:06:19 UTC.

Proposal

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

Web - Thread - Noting happen when clicking "Join thread" button unless returning to inbox again

What is the root cause of that problem?

When we press the join button openReport is called with new child report id here

openReport(newChat.reportID, '', participantLogins, newChat, parentReportAction.reportActionID);

and that will set the visitedTime of the child report to the current time
lastVisitTime: DateUtils.getDBTime(),

so when we navigate back to the inbox the empty child report will be opened as it is the last accessed report according to its visitedTime

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

We should not set the lastVisitedTime for the child report on join as we are not visiting it only subscribing to it. So:

add optional shouldSetVisitedTime param for openReport

shouldSetVisitedTime = true,

lastVisitTime: DateUtils.getDBTime(),



                ...(shouldSetVisitedTime && {lastVisitTime: DateUtils.getDBTime()}),

and pass as false for open reports in join thread for both when the childReport exists or not

openReport(childReportID);

        openReport(childReportID, undefined, undefined, undefined, undefined, undefined, undefined, undefined, false);

openReport(newChat.reportID, '', participantLogins, newChat, parentReportAction.reportActionID);

        openReport(newChat.reportID, '', participantLogins, newChat, parentReportAction.reportActionID, false);

What alternative solutions did you explore? (Optional)

@mkzie2
Copy link
Contributor

mkzie2 commented Nov 13, 2024

Edited by proposal-police: This proposal was edited at 2024-11-19 08:56:27 UTC.

Proposal

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

"Join thread" option navigating to "reply in thread" page after navigating to other bottom navigations like Search or Setting and returning back to inbox

What is the root cause of that problem?

  • Firstly, we need to explain why the join thread option is only shown in non-thread actions of other user

The difference is here, if reportAction?.childReportNotificationPreference is undefined, getChildReportNotificationPreference function returns always if the user is the creator of the action, otherwise it returns hidden

const childReportNotificationPreference = ReportUtils.getChildReportNotificationPreference(reportAction);

App/src/libs/ReportUtils.ts

Lines 1762 to 1768 in b1c1a4b

function getChildReportNotificationPreference(reportAction: OnyxInputOrEntry<ReportAction> | Partial<ReportAction>): NotificationPreference {
const childReportNotificationPreference = reportAction?.childReportNotificationPreference ?? '';
if (childReportNotificationPreference) {
return childReportNotificationPreference;
}
return isActionCreator(reportAction) ? CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS : CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;

  • When we click on Join thread, a new report is created and when we go back from search it is displayed as the last accessed report

function toggleSubscribeToChildReport(childReportID = '-1', parentReportAction: Partial<ReportAction> = {}, parentReportID = '-1', prevNotificationPreference?: NotificationPreference) {

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

I think the way we update lastVisitTime here is incorrect. It was added here. According to my understanding
, lastVisitTime saves the last time we opened a report (the report screen is focused).

lastVisitTime: DateUtils.getDBTime(),

Call openReport doesn't mean we navigate to this report and we handled in ReportScreen here to update lastVisitTime whenever the ReportScreen is focused so we can remove the wrong logic here

lastVisitTime: DateUtils.getDBTime(),

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Nov 18, 2024
@stephanieelliott
Copy link
Contributor

Agree this seems buggy, adding labels.

@melvin-bot melvin-bot bot removed the Overdue label Nov 19, 2024
@stephanieelliott stephanieelliott added External Added to denote the issue can be worked on by a contributor Overdue labels Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

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

@melvin-bot melvin-bot bot changed the title Web - Thread - Noting happen when clicking "Join thread" button unless returning to inbox again [$250] Web - Thread - Noting happen when clicking "Join thread" button unless returning to inbox again Nov 19, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 19, 2024
Copy link

melvin-bot bot commented Nov 19, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 19, 2024
@parasharrajat
Copy link
Member

parasharrajat commented Nov 19, 2024

Both of the proposals lack clear explanations.

  1. @FitseTLT When you say non-parent actions, what does that mean? Please don't use purely technical terms in explanations. It will help make it more readable. There are non-technical persons also reading these proposals.
  2. @mkzie2 You didn't explain what you are trying to achieve in your alternative solution. It just mentions changes.

Apart from this, the expected behavior here should be that the user does not navigate to the thread in any way. Please update your proposals for it. Ref: #27994

@mkzie2
Copy link
Contributor

mkzie2 commented Nov 19, 2024

@parasharrajat I updated my alternative solution.

@FitseTLT
Copy link
Contributor

@parasharrajat I have tried to make it more readable based on your suggestion. Can you check it again? Thx

@parasharrajat
Copy link
Member

Thanks, both of you. But I think you missed the following part of the comment.

Apart from this, the expected behavior here should be that the user does not navigate to the thread in any way. Please update your proposals for it. Ref: #27994

@mkzie2
Copy link
Contributor

mkzie2 commented Nov 19, 2024

Apart from this, the expected behavior here should be that the user does not navigate to the thread in any way.

@parasharrajat What does that mean? My alternative solution will only hide the Join thread, not add any navigate to thread logic.

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 19, 2024

Thanks, both of you. But I think you missed the following part of the comment.

Apart from this, the expected behavior here should be that the user does not navigate to the thread in any way. Please update your proposals for it. Ref: #27994

@parasharrajat
I have not proposed to navigate to the thread on join thread but only make the join thread option to be available only if the thread linked to the report action exists. Is there anything you haven't understood?

@parasharrajat
Copy link
Member

I am telling the expected behaviour for this issue. It's not a feedback for any proposal.

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 19, 2024

I am telling the expected behaviour for this issue. It's not a feedback for any proposal.

@parasharrajat There is no navigation going on join thread even on current main, the current issue is making the impression of it happening. When you navigate back to Inbox from search we open the recently accessed report but on join thread we called openReport, so that the previously unsubscribed chat thread will be available in FE, but that openReport request will make the chat thread as the lastAccessedReport, hence, that's why it navigates to the joined new chat thread on going back to inbox as shown in the OP.

But the purpose of the join thread button is, as clearly indicated in the PR you linked, to allow a user join back a thread they have unsubscribed. Thus, we shouldn't show the join thread menu for report actions that are not yet have a linked child report/ chat thread.

@mkzie2
Copy link
Contributor

mkzie2 commented Nov 19, 2024

I am telling the expected behaviour for this issue. It's not a feedback for any proposal.

When we click on Join thread, a new report is created and when we go back from search it is displayed as the last accessed report

@parasharrajat As I explained in the RCA, the thread is opened after going back from the search page because it's a newly created report. With my alternative solution, no new report is created then this bug doesn't happen.

@parasharrajat
Copy link
Member

It is briefly mentioned here #27994 (comment). IT should work like get Notified about replied feature of Slack.

I agree that navigation is happening due to lastaccessed report and openReport call. But that is what I am saying that it should not happen here.

But the purpose of the join thread button is, as clearly indicated in the PR you linked, to allow a user join back a thread they have unsubscribed. Thus, we shouldn't show the join thread menu for report actions that are not yet have a linked child report/ chat thread.

This is absolutely correct but this feature also works for chats where no thread is created so far.

Subscribing to a thread without comments (Need 2 users, User A and User B):

  1. With User B, comment in a chat
  2. With User A, subscribe to the comment that was just created
  3. With User B, reply to the comment
  4. Ensure that the thread shows up in User As LHN and they receive a notification
  5. Verify that no errors appear in the JS console

Here are the test steps for this one from that PR.

@parasharrajat
Copy link
Member

@srikarparsi Can you please help us clarify the purpose of this feature?

@srikarparsi
Copy link
Contributor

Sorry, the purpose of Join thread? If so, yes it should mimic get Notified about replied in replies. It shouldn't open the thread report.

Copy link

melvin-bot bot commented Nov 26, 2024

@parasharrajat, @stephanieelliott Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Nov 26, 2024
Copy link

melvin-bot bot commented Nov 26, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@parasharrajat
Copy link
Member

parasharrajat commented Nov 26, 2024

I hope the purpose and expected behavior is clear now. Could you please update your proposals as per expected behavior?

@mkzie2 @FitseTLT

@melvin-bot melvin-bot bot removed the Overdue label Nov 26, 2024
@FitseTLT
Copy link
Contributor

I hope the purpose and expected behavior is clear now. Could you please update your proposals as per expected behavior?

@mkzie2 @FitseTLT

@parasharrajat Updated

Copy link

melvin-bot bot commented Nov 27, 2024

@parasharrajat @stephanieelliott 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!

@mkzie2
Copy link
Contributor

mkzie2 commented Nov 27, 2024

@parasharrajat Updated proposal.

@parasharrajat
Copy link
Member

Thanks, both of you. Doing some research...and reviewing your proposals.

Copy link

melvin-bot bot commented Dec 3, 2024

@parasharrajat, @stephanieelliott Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label Dec 3, 2024
Copy link

melvin-bot bot commented Dec 3, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@stephanieelliott
Copy link
Contributor

Hey @parasharrajat have you had a chance to review the proposals?

@parasharrajat
Copy link
Member

Looks like there is no answer so far to my question, I have asked in slack for that. Let's wait for sometime to get the answer first.

@melvin-bot melvin-bot bot removed the Overdue label Dec 4, 2024
@srikarparsi srikarparsi changed the title [$250] Web - Thread - Noting happen when clicking "Join thread" button unless returning to inbox again [$250] Web - Thread - Switching to Search and back after clicking "Join thread" opens the report Dec 4, 2024
@srikarparsi srikarparsi self-assigned this Dec 4, 2024
@stephanieelliott
Copy link
Contributor

This is being discussed in Slack here: https://expensify.slack.com/archives/C01GTK53T8Q/p1733293681138239

@parasharrajat
Copy link
Member

parasharrajat commented Dec 6, 2024

After a long wait, we have an answer. It seems that setting lastVisitTime is not correct in openAPI. But we need to still test this a few different times to make sure that we are not missing any edge cases. Given that @mkzie2's proposal suggests that solution, I like their proposal.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Dec 6, 2024

Current assignee @srikarparsi is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 6, 2024
Copy link

melvin-bot bot commented Dec 6, 2024

📣 @mkzie2 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Dec 7, 2024
@mkzie2
Copy link
Contributor

mkzie2 commented Dec 7, 2024

@parasharrajat The PR is here.

@garrettmknight garrettmknight moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Dec 10, 2024
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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

6 participants