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] [HOLD for payment 2024-06-13] Handle showing selfDM as the first item in the list when users search for their own email or display name #42627

Closed
techievivek opened this issue May 27, 2024 · 27 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 FirstPick Engineering only, please! Only add when there is an identified code solution.

Comments

@techievivek
Copy link
Contributor

techievivek commented May 27, 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!


Problem:

We recently merged a backend PR that returns selfDM reports in search results when users search for their email or display name. However, the backend response does not guarantee that the selfDM report will appear as the first result. This can make it difficult for users to quickly locate their selfDM in the search results even when they are returned by the backend.

Solution:

Update the frontend logic to ensure that the selfDM report appears first in the search results in UI when users specifically search for their own email or display name. Implement post-processing logic to reorder the search results, ensuring the selfDM report is at the top of the list. This will make it easier for users to find their selfDM when searching for their own email or display name.

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01401c18bd7e84e155
  • Upwork Job ID: 1801202714715048228
  • Last Price Increase: 2024-06-13
@techievivek techievivek added External Added to denote the issue can be worked on by a contributor Daily KSv2 FirstPick Engineering only, please! Only add when there is an identified code solution. Bug Something is broken. Auto assigns a BugZero manager. labels May 27, 2024
@techievivek techievivek self-assigned this May 27, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 27, 2024
Copy link

melvin-bot bot commented May 27, 2024

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

Copy link

melvin-bot bot commented May 27, 2024

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.

@dragnoir
Copy link
Contributor

dragnoir commented May 27, 2024

Proposal

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

Handle showing selfDM as the first item in the list when users search for their own email or display name

What is the root cause of that problem?

New feature

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

On ChatFinderPage we have the filteredOptions that turns the new list when we search.

const newOptions = OptionsListUtils.filterOptions(searchOptions, debouncedSearchValue, betas);

To implement the feature where items with isSelfDM: true in newOptions.recentReports are moved to the top of the list, you can use JavaScript's sort method within your useMemo hook. Here's how we can modify the function to achieve this:

const filteredOptions = useMemo(() => {
    if (debouncedSearchValue.trim() === '') {
        return {
            recentReports: [],
            personalDetails: [],
            userToInvite: null,
            headerMessage: '',
        };
    }

    const newOptions = OptionsListUtils.filterOptions(searchOptions, debouncedSearchValue, betas);
    console.log('newOptions: ', newOptions);

    // Sort recentReports to move items with isSelfDM: true to the top
    const sortedRecentReports = newOptions.recentReports.sort((a, b) => {
        if (a.isSelfDM && !b.isSelfDM) return -1;
        if (!a.isSelfDM && b.isSelfDM) return 1;
        return 0;
    });

    const header = OptionsListUtils.getHeaderMessage(sortedRecentReports.length + Number(!!newOptions.userToInvite) > 0, false, debouncedSearchValue);
    return {
        recentReports: sortedRecentReports,
        personalDetails: newOptions.personalDetails,
        userToInvite: newOptions.userToInvite,
        headerMessage: header,
    };
}, [debouncedSearchValue, searchOptions, betas]);

Explanation:

  1. Check if debouncedSearchValue is empty: If it is, return the initial empty state.
  2. Filter Options: Use OptionsListUtils.filterOptions to get the newOptions.
  3. Sort recentReports: Use the sort method to reorder newOptions.recentReports, moving items with isSelfDM: true to the top.
  4. Return Sorted Options: Return the sorted recentReports along with other newOptions properties.

This ensures that any item with the attribute isSelfDM: true appears at the top of the recentReports array in the returned object.

POC video:

my name: Rachid L and my email is mosaixel.org@gmail.com

20240527_095213.mp4

We can also integrate this feature inside OptionsListUtils.filterOptions

@nkdengineer
Copy link
Contributor

Proposal

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

What is the root cause of that problem?

In here, we're ordering the report by lastVisibleActionCreated and archived report will be remain at the bottom

const orderedReportOptions = lodashSortBy(filteredReportOptions, (option) => {
const report = option.item;
if (option.isArchivedRoom) {
return CONST.DATE.UNIX_EPOCH;
}
return report?.lastVisibleActionCreated;

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

We should order the selfDM at the top of the report list by return new Date().toString() if the report is selfDM. So this report will be always at the top of the list

const orderedReportOptions = lodashSortBy(filteredReportOptions, (option) => {
    const report = option.item;
    if (option.isArchivedRoom) {
        return CONST.DATE.UNIX_EPOCH;
    }
    if (option.isSelfDM) {
        return new Date().toString();
    }

    return report?.lastVisibleActionCreated;
});

const orderedReportOptions = lodashSortBy(filteredReportOptions, (option) => {
const report = option.item;
if (option.isArchivedRoom) {
return CONST.DATE.UNIX_EPOCH;
}
return report?.lastVisibleActionCreated;

What alternative solutions did you explore? (Optional)

NA

@bernhardoj
Copy link
Contributor

Proposal

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

We want to show self DM as the first result when the user searches for their own login/display name.

What is the root cause of that problem?

A feature request.

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

We currently able to search from 2 pages, ChatFinderPage and NewChatPage. They have different logic to sort.

For ChatFinderPage, we use this logic to sort the results.

function orderOptions(options: ReportUtils.OptionData[], searchValue: string | undefined) {
return lodashOrderBy(
options,
[
(option) => {
if (!!option.isChatRoom || option.isArchivedRoom) {
return 3;
}
if (!option.login) {
return 2;
}
if (option.login.toLowerCase() !== searchValue?.toLowerCase()) {
return 1;
}
// When option.login is an exact match with the search value, returning 0 puts it at the top of the option list
return 0;
},
],
['asc'],
);

It's executed only when we type something in the search field. To put the self-DM at the top of the list, we can return 0 if the option is self DM.

if (option.isSelfDM) {
    return 0;
}

For NewChatPage, the result is sorted here.

const orderedReportOptions = lodashSortBy(filteredReportOptions, (option) => {
const report = option.item;
if (option.isArchivedRoom) {
return CONST.DATE.UNIX_EPOCH;
}
return report?.lastVisibleActionCreated;
});

To put the self-DM at the top of the list and only when we search for our self, we can add this code:

if (searchValue) {
    return [option.isSelfDM, report?.lastVisibleActionCreated];
}

This will sort it first by isSelfDM.

@getusha
Copy link
Contributor

getusha commented May 28, 2024

@bernhardoj's proposal looks good to me. the solution is straight forward and works perfectly.
🎀 👀 🎀 C+ Reviewed

Copy link

melvin-bot bot commented May 28, 2024

Current assignee @techievivek 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 May 28, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels May 28, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @getusha

@Julesssss
Copy link
Contributor

Hey all, it sounds like the PR didn't fully fix the issue. @kbecciv could you please point to the specific case that is failing?

@bernhardoj
Copy link
Contributor

Looking at the video, looks like the search request doesn't return the self DM. I can repro it in one of my accounts where the self DM is not loaded yet and when I try to search, SearchForReports always returns with an error.

image

@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 6, 2024
@melvin-bot melvin-bot bot changed the title Handle showing selfDM as the first item in the list when users search for their own email or display name [HOLD for payment 2024-06-13] Handle showing selfDM as the first item in the list when users search for their own email or display name Jun 6, 2024
Copy link

melvin-bot bot commented Jun 6, 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 6, 2024
Copy link

melvin-bot bot commented Jun 6, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.79-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-13. 🎊

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

  • @bernhardoj requires payment (Needs manual offer from BZ)
  • @getusha requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Jun 6, 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:

  • [@getusha] The PR that introduced the bug has been identified. Link to the PR:
  • [@getusha] 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:
  • [@getusha] 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:
  • [@getusha] Determine if we should create a regression test for this bug.
  • [@getusha] 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.
  • [@jliexpensify] 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 12, 2024
@jliexpensify
Copy link
Contributor

Bump @getusha for the checklist

@jliexpensify
Copy link
Contributor

jliexpensify commented Jun 13, 2024

Can I confirm there's no regressions or issues that affect payment @techievivek and @Julesssss (based off this comment)?

Payment Summary:

https://www.upwork.com/jobs/~01401c18bd7e84e155

@jliexpensify jliexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Jun 13, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-13] Handle showing selfDM as the first item in the list when users search for their own email or display name [$250] [HOLD for payment 2024-06-13] Handle showing selfDM as the first item in the list when users search for their own email or display name Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

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

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

melvin-bot bot commented Jun 13, 2024

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

@jliexpensify
Copy link
Contributor

Re-applied label as no Upworks job was initially created

@jliexpensify jliexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 13, 2024
@jliexpensify
Copy link
Contributor

@getusha I don't know who you are - what's your Upworks profile? Please add your full name (as per Upworks) in your GH profile, thanks!

@getusha
Copy link
Contributor

getusha commented Jun 13, 2024

@jliexpensify could we hold on payments for a few days please? i am working on moving payments to ND. thanks

@jliexpensify
Copy link
Contributor

jliexpensify commented Jun 13, 2024

No worries, I'll just spin up a new Summary.

Can I also confirm there's no regressions or issues that affect payment @techievivek and @Julesssss (based off this comment)?

UPDATED Payment Summary:

https://www.upwork.com/jobs/~01401c18bd7e84e155

@jliexpensify
Copy link
Contributor

Hi, sorry another bump for @techievivek and @Julesssss"

Can I also confirm there's no regressions or issues that affect payment @techievivek and @Julesssss (based off this #42627 (comment))?

@Julesssss
Copy link
Contributor

Yeah I don't believe there are any regressions. I think it's fine to pay out

@jliexpensify
Copy link
Contributor

Paid @bernhardoj, thanks for your patience! @getusha please refer to this summary for ND payments.

@jliexpensify
Copy link
Contributor

@getusha also, does a checklist need to be completed here?

@getusha
Copy link
Contributor

getusha commented Jun 17, 2024

[@getusha] The PR that introduced the bug has been identified. Link to the PR: N/a it is a feature request
[@getusha] 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: N/a
[@getusha] 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: N/a
[@getusha] Determine if we should create a regression test for this bug. Yes
[@getusha] 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.

Regression Test Proposal

  1. Open search page
  2. Search for the account you currently signed in with (email or name)
  3. Verify that self DM appears at the top of the result

Do we agree 👍 or 👎

@JmillsExpensify
Copy link

$250 approved for @getusha

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 FirstPick Engineering only, please! Only add when there is an identified code solution.
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants