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

[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] Room - Endless Loading of Header When Navigating to Thread of Mentioned Room #40928

Closed
4 of 6 tasks
kbecciv opened this issue Apr 24, 2024 · 48 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Apr 24, 2024

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: v1.4.65-0
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by: Applause - Internal Testing

Action Performed:

  1. Navigate to any room.
  2. Type "#" and select the suggested room, then send the message.
  3. Proceed to its thread.

Expected Result:

Upon entering the thread, the header enters an infinite loading state.

Actual Result:

The mentioned room should be displayed in the header as expected, without any endless loading.

Workaround:

n/a

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6460103_1713955694030.Screen_Recording_2024-04-24_at_3.45.34_AM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0137b18b289d467863
  • Upwork Job ID: 1783902532804722688
  • Last Price Increase: 2024-05-17
  • Automatic offers:
    • tsa321 | Contributor | 0
Issue OwnerCurrent Issue Owner: @isabelastisser
@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 24, 2024
Copy link

melvin-bot bot commented Apr 24, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Apr 24, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Apr 24, 2024

@isabelastisser 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.

@isabelastisser isabelastisser added External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Apr 26, 2024
@melvin-bot melvin-bot bot changed the title Room - Endless Loading of Header When Navigating to Thread of Mentioned Room [$250] Room - Endless Loading of Header When Navigating to Thread of Mentioned Room Apr 26, 2024
Copy link

melvin-bot bot commented Apr 26, 2024

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

Copy link

melvin-bot bot commented Apr 26, 2024

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

@shahinyan11
Copy link

shahinyan11 commented Apr 26, 2024

Proposal

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

Room - Endless Loading of Header When Navigating to Thread of Mentioned Room

What is the root cause of that problem?

Here we set isLoading to true if the title is not exists. We get title using ReportUtils.getReportName here in which we return empty string here if the report is policyRoom and do not have participants

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

Update this line like below. It is not specified exactly what should be displayed, so I suggested еру report?.reportName. We can update it to desired value

return getAdminRoomInvitedParticipants(parentReportAction, parentReportActionMessage) || report?.reportName;

What alternative solutions did you explore? (Optional)

@mollfpr
Copy link
Contributor

mollfpr commented Apr 29, 2024

@shahinyan11 I think the expected result should show the message in the header, but with your solution, it will show Chat Report which is the report name from the report.

Screenshot 2024-04-29 at 12 13 54

We get title using ReportUtils.getReportName here in which we return empty string here if the report is policyRoom and do not have participants

Your root cause might not be correct, the issue is only reproduced if the message just contains the mention tag.

Screen.Recording.2024-04-29.at.12.24.51.mp4

@mollfpr
Copy link
Contributor

mollfpr commented Apr 29, 2024

Screenshot 2024-04-29 at 12 44 18

If you try to edit the message with a message that only contains the report mentioned, it will display in the header. So to be consistent we should display the report mentioned in the header.

Screen.Recording.2024-04-29.at.12.35.55.mp4

We also show the mention tag in the header report.

@melvin-bot melvin-bot bot added the Overdue label May 1, 2024
@isabelastisser
Copy link
Contributor

@mollfpr, any updates? Are we still waiting for proposals?

@melvin-bot melvin-bot bot removed the Overdue label May 1, 2024
@mollfpr
Copy link
Contributor

mollfpr commented May 2, 2024

@isabelastisser Still waiting for @shahinyan11 and other proposals.

Copy link

melvin-bot bot commented May 3, 2024

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

@melvin-bot melvin-bot bot added the Overdue label May 6, 2024
@tsa321
Copy link
Contributor

tsa321 commented May 6, 2024

Proposal

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

Header of thread report displayed as skeleton view if the parent report action message is report mention.

What is the root cause of that problem?

In HeaderView the skeleton view will be displayed if isLoading:

const isLoading = !report.reportID || !title;

is true. It's value will be true in this issue because title variabletitle = ReportUtils.getReportName(reportHeaderData); is empty string.
This function will go to here and then in here and return empty string. This is because reportAction.message[0].text is empty string. The parent reportAction.message[0].text is empty but reportAction.message[0].html is something like <mention-report reportID=\"50451872397987297398\"/>

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

In this line:

return Str.removeSMSDomain(reportAction?.message?.[0]?.text ?? '');

We could use reportAction?.message?.[0]?.html and detect all occurrence of pattern <mention-report reportID=\"xxxxx\"/> by using regex and replace it by it's report name, and add the report names in reportAction?.message?.[0]?.text in that line.

The code could be:

Code:
function embedMentionedReportName(reportAction){
    const {html, text} = reportAction?.message?.[0];
    if (!html) {
        return text;
    }

    const mentionReportRegex = /(\<mention-report reportID\=\"(\d+)\"\/>)/gm;
    if (!mentionReportRegex.test(html)) {
        return text;
    } 

    const splittedText = text.split(" ");    
  
    // Remove tags except for mention-report
    const filteredHtml = html.replace(/(\<(?!mention-report).*?\>)/gm, "");

    // Embed report name into html enclosed by tag, for exampple: <#announce>
    const htmlWithReportName = filteredHtml.replace(/(\<mention-report reportID\=\"(\d+?)\"\/\>)/gm, 
        (match, p1, p2) => `<${getReportName(getReport(p2))}>`
    );
    const splittedHtmlWithReportName = htmlWithReportName.split(" ");

    const taggedReportNameRegex = /\<(.+)\>/gm;
    splittedHtmlWithReportName.forEach((str, index) => {
        const match = taggedReportNameRegex.exec(str ?? "");        
        if (match) {
            const cSpliitedText = splittedText[index] ?? "";
            splittedText[index] = match.index > 0 ? cSpliitedText + match[1] : match[1] + cSpliitedText;  
        }  
        taggedReportNameRegex.lastIndex = 0;
    })

    return splittedText.join(" ");
}

Then in here:

return Str.removeSMSDomain(reportAction?.message?.[0]?.text ?? '');

call it with:

    const embeddedMentionedReportName = embedMentionedReportName(reportAction);
    return Str.removeSMSDomain( embeddedMentionedReportName ?? '');

What alternative solutions did you explore?

We could use ExpensiMark htmlToText to parse the html string.

@isabelastisser
Copy link
Contributor

@mollfpr can you please review the proposal above? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label May 6, 2024
@mollfpr
Copy link
Contributor

mollfpr commented May 6, 2024

@tsa321 I'm incline with your approach, could you share the result of your proposal?

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@tsa321
Copy link
Contributor

tsa321 commented May 8, 2024

@mollfpr

The code could be:

Code:
function embedMentionedReportName(reportAction){
    const {html, text} = reportAction?.message?.[0];
    if (!html) {
        return text;
    }

    const mentionReportRegex = /(\<mention-report reportID\=\"(\d+)\"\/>)/gm;
    if (!mentionReportRegex.test(html)) {
        return text;
    } 

    const splittedText = text.split(" ");    
  
    // Remove tags except for mention-report
    const filteredHtml = html.replace(/(\<(?!mention-report).*?\>)/gm, "");

    // Embed report name into html enclosed by tag, for exampple: <#announce>
    const htmlWithReportName = filteredHtml.replace(/(\<mention-report reportID\=\"(\d+?)\"\/\>)/gm, 
        (match, p1, p2) => `<${getReportName(getReport(p2))}>`
    );
    const splittedHtmlWithReportName = htmlWithReportName.split(" ");

    const taggedReportNameRegex = /\<(.+)\>/gm;
    splittedHtmlWithReportName.forEach((str, index) => {
        const match = taggedReportNameRegex.exec(str ?? "");        
        if (match) {
            const cSpliitedText = splittedText[index] ?? "";
            splittedText[index] = match.index > 0 ? cSpliitedText + match[1] : match[1] + cSpliitedText;  
        }  
        taggedReportNameRegex.lastIndex = 0;
    })

    return splittedText.join(" ");
}

Then in here:

return Str.removeSMSDomain(reportAction?.message?.[0]?.text ?? '');

call it with:

    const embeddedMentionedReportName = embedMentionedReportName(reportAction);
    return Str.removeSMSDomain( embeddedMentionedReportName ?? '');
Result:
macos-web-d.mp4

Copy link

melvin-bot bot commented May 8, 2024

@mollfpr @isabelastisser 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!

@melvin-bot melvin-bot bot added the Overdue label May 9, 2024
@isabelastisser
Copy link
Contributor

not overdue.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels May 23, 2024
@isabelastisser
Copy link
Contributor

It looks like the PR was submitted here: #42504

@luacmartins
Copy link
Contributor

#40928 (comment) I could still reproduce the issue just now.

This comment was marked as resolved.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jun 11, 2024
@melvin-bot melvin-bot bot changed the title [$250] Room - Endless Loading of Header When Navigating to Thread of Mentioned Room [HOLD for payment 2024-06-18] [$250] Room - Endless Loading of Header When Navigating to Thread of Mentioned Room Jun 11, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 11, 2024
Copy link

melvin-bot bot commented Jun 11, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.81-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-18. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jun 11, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
  • [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@mollfpr] Determine if we should create a regression test for this bug.
  • [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 13, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-18] [$250] Room - Endless Loading of Header When Navigating to Thread of Mentioned Room [HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] Room - Endless Loading of Header When Navigating to Thread of Mentioned Room Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.82-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-06-20. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jun 13, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
  • [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@mollfpr] Determine if we should create a regression test for this bug.
  • [@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 18, 2024
@isabelastisser
Copy link
Contributor

@mollfpr, please complete the BZ list. Thanks!

@isabelastisser
Copy link
Contributor

isabelastisser commented Jun 20, 2024

Bump @mollfpr to complete the checklist.

@isabelastisser
Copy link
Contributor

Payment summary:

@mollfpr requires payment through NewDot Manual Requests Role C+ $250 PENDING
@tsa321 requires payment automatic offer (Contributor) PAID!

@isabelastisser
Copy link
Contributor

isabelastisser commented Jun 21, 2024

Bump @mollfpr. I DM'd you for visibility.

@mollfpr
Copy link
Contributor

mollfpr commented Jun 22, 2024

Sorry for the delay.

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

No offending PR. The issue seems a case that we missing to handle and how we should process the mentioned action message from BE.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

  1. Open admins or announce room
  2. Send a message containing only the mentioned room name (for example #admins or #announce)
  3. Select Reply in thread of that message
  4. Wait for at least 5 seconds
  5. Verify the thread report header looks like the common thread header and is not changed into endless loading
  6. 👍 or 👎

@JmillsExpensify
Copy link

$250 approved for @mollfpr

@isabelastisser
Copy link
Contributor

All set!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants