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

[$500] DMs with a hidden notification preference show up in search twice, once with the email and once without #34900

Closed
1 of 6 tasks
kavimuru opened this issue Jan 22, 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 Engineering 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

Comments

@kavimuru
Copy link

kavimuru commented Jan 22, 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: 1.4.29-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @srikarparsi
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1705731187838889

Action Performed:

  1. Create a DM with another user that you have not previously DMed
  2. Navigate to another chat so that the DM disappears from your LHN
  3. Click search and see the DM that you have just created appears twice, once with email and once without email

Expected Result:

should be hiding these DMs from search entirely

Actual Result:

Only profile pics are shown

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

Recording.1699.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019a3378b1028ed211
  • Upwork Job ID: 1749471517916659712
  • Last Price Increase: 2024-02-12
  • Automatic offers:
    • mollfpr | Reviewer | 0
    • dukenv0307 | Contributor | 0
@kavimuru kavimuru added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 22, 2024
@melvin-bot melvin-bot bot changed the title DMs with a hidden notification preference show up in search without a name [$500] DMs with a hidden notification preference show up in search without a name Jan 22, 2024
Copy link

melvin-bot bot commented Jan 22, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 22, 2024
Copy link

melvin-bot bot commented Jan 22, 2024

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Jan 22, 2024

Proposal

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

DMs with a hidden notification preference show up in search without a name

What is the root cause of that problem?

As we open the chat after searching it is included in reports list and b/c We are not filtering out personal details without proper accountID (Nan) in Search Page it appears when we open search page.

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

We should filter out those personal details with accountid of Nan from recent reports before display in search page
We can add this code before here ingetOptions

recentReportOptions.push(reportOption);

            if (!reportOption.login && !reportOption.accountID && !reportOption.ownerAccountID) {
                continue;
            }

or in here

if (!report) {
return;
}

if (!report.login && !report.accountID && !report.ownerAccountID) {
            return;
        }

or we can iterate and filter out from sections here

App/src/pages/SearchPage.js

Lines 135 to 136 in 52adf77

return sections;
};

or we should stop adding personal details without proper accountID in to recents list

What alternative solutions did you explore? (Optional)


If we want to filter out all empty chats we can add an additional condition of in here

if (!report) {
return;
}

        const lastVisibleMessage = ReportActionUtils.getLastVisibleMessage(report.reportID);
        const isEmptyChat = !report.lastMessageText && !report.lastMessageTranslationKey && !lastVisibleMessage.lastMessageText && !lastVisibleMessage.lastMessageTranslationKey;

        if (isEmptyChat && ReportUtils.isChatReport(report) && !ReportUtils.isChatRoom(report) && !ReportUtils.isPolicyExpenseChat(report)) {
            return;
        }

We can optionally use this logic too to decide if it is empty chat getting the report actions by ReportActionsUtils.getAllReportActions(report?.reportID)

const isEmptyChat = useMemo(() => _.size(reportActions) === 1, [reportActions]);

We can also narrow down our filter using ReportUtils.isChatReport only filter out empty chat reports or also isDM or !ReportActionsUtils.isTransactionThread


@namhihi237
Copy link
Contributor

namhihi237 commented Jan 22, 2024

Proposal

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

should be hiding the empty DMs from search entirely

What is the root cause of that problem?

When the chat just created we do not have the accountID and the any chat message in the report. And Even when we send a message the accountID still NaN
And when we get list recentReports, we don't filter recentReports that don't have an accountID and alternateText is empty.

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

I think with the show Hidden user and have a message we also can search for them
So we should filter recentReports with the condition accountID correct and alternateText is not empty

What alternative solutions did you explore? (Optional)

N/A

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 22, 2024

Proposal

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

Only profile pics are shown

What is the root cause of that problem?

For reports where there's no account ids that should be visible (as in this case), visibleChatMemberAccountIDs here will be empty. But we still use that empty account ids list to calculate the option, leading to invalid options like can be seen in this issue.

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

Early return here if the accountIDs list is empty (and optionally check the chat is a DM also).

What alternative solutions did you explore? (Optional)

This issue might happen in other chat cases like if the chat is a group chat as well, we can test for those scenarios and add relevant checks (instead of just checking the chat is a DM) along with the accountIDs list is empty check.

An alternative is to early return if accountIDs list is empty and the chat doesn't have multiple participants (with same condition as here), that means the participant in the chat are not visible and shouldn't be shown in search list.

Or in here we could fallback to using participantAccountIDs (as accountIDs) if the visibleChatMemberAccountIDs list is empty.

@abzokhattab
Copy link
Contributor

abzokhattab commented Jan 22, 2024

I think this might be a backend issue because the backend returns the visibleChatMemberAccountIDs as an empty list when opening report with a new user, where it should be the same as the participantsLists

image

@FitseTLT
Copy link
Contributor

Updated to add more detail and alternative approaches

@mollfpr
Copy link
Contributor

mollfpr commented Jan 23, 2024

Screenshot 2024-01-23 at 22 16 45

@kavimuru I might need clarification on the expected result. Should we exclude all the empty chat reports on the recent list or those that only show the profile pic?

From the above screenshot, I see a bug showing the report with only the profile picture.

@srikarparsi srikarparsi self-assigned this Jan 24, 2024
@mollfpr
Copy link
Contributor

mollfpr commented Jan 24, 2024

@srikarparsi Could you help clarify the above question? Thank you!

@jeremy-croff
Copy link
Contributor

Proposal

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

When we search for a user without internet, the recent search results do not show the users display name

What is the root cause of that problem?

The accountId never got resolved while offline. This results in a null accountId for the record.
Snapshot of the entity is here:
Screenshot 2024-01-23 at 10 44 28 PM

We return an empty displayName under this condition:
https://github.com/Expensify/App/blob/0efabc2b40fe845b13faa94ec508a6693167ede9/src/libs/ReportUtils.ts#L1608C1-L1611C6
Hence why we render null.

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

We can update the displayName logic to fallback to option.text if it doesn't resolve the displayname here:

const displayName = getDisplayNameForParticipant(accountID, isMultipleParticipantReport, shouldFallbackToHidden) || user?.login || '';

It already short-circuits to login, but in this error case it's also not present.

Note:
After the user refreshes the page or interacts with the User - it fixes the database and will show the correct displayName.

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
Copy link

melvin-bot bot commented Jan 29, 2024

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

@mollfpr
Copy link
Contributor

mollfpr commented Jan 29, 2024

Need clarification #34900 (comment)

cc @kavimuru @srikarparsi

@melvin-bot melvin-bot bot removed the Overdue label Jan 29, 2024
Copy link

melvin-bot bot commented Jan 29, 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 Jan 31, 2024
@srikarparsi
Copy link
Contributor

In regard to this question, I believe we should exclude all empty chats. I think the search function should be optimized for existing chats, for empty chats it would be just as simple to "re-create" them.

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

mollfpr commented Feb 2, 2024

Does anyone want to update their proposal according to the above expectations?

@FitseTLT
Copy link
Contributor

FitseTLT commented Feb 2, 2024

Updated proposal in alternative solution considering the above expectation

@melvin-bot melvin-bot bot added the Overdue label Feb 5, 2024
Copy link

melvin-bot bot commented Feb 5, 2024

@mollfpr @srikarparsi 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 Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels Feb 19, 2024
Copy link

melvin-bot bot commented Feb 19, 2024

Current assignee @mollfpr is eligible for the Internal assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 19, 2024
@dukenv0307
Copy link
Contributor

@mollfpr PR #36816 is ready to review

@mollfpr
Copy link
Contributor

mollfpr commented Mar 1, 2024

@srikarparsi the issue missing label Awaiting Payment and the payment date, could you update it? Thank you!

@srikarparsi srikarparsi added Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. and removed Reviewing Has a PR in review Internal Requires API changes or must be handled by Expensify staff Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. labels Mar 5, 2024
Copy link

melvin-bot bot commented Mar 5, 2024

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 5, 2024
@srikarparsi
Copy link
Contributor

Yup! Seems to be the case. Hey @muttmuure do you think you could help with payment for this PR after the regression period. Also, I don't believe the automation worked, do you know if there's a way to get around that?

@melvin-bot melvin-bot bot added the Overdue label Mar 7, 2024
@muttmuure muttmuure added the External Added to denote the issue can be worked on by a contributor label Mar 8, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 8, 2024
Copy link

melvin-bot bot commented Mar 8, 2024

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

@muttmuure muttmuure added the Awaiting Payment Auto-added when associated PR is deployed to production label Mar 8, 2024
@muttmuure
Copy link
Contributor

@mollfpr - $500 for C+
@dukenv0307 - $500 for C

@melvin-bot melvin-bot bot removed the Overdue label Mar 8, 2024
@muttmuure
Copy link
Contributor

All paid up, @mollfpr needs to request in NewDot using the summary above

@JmillsExpensify
Copy link

$500 approved for @mollfpr based on summary.

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 Engineering 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
Projects
None yet
Development

No branches or pull requests