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

HIGH: [Polish] [$500] Finalize order of LHN in #focus mode #33029

Closed
quinthar opened this issue Dec 13, 2023 · 108 comments
Closed

HIGH: [Polish] [$500] Finalize order of LHN in #focus mode #33029

quinthar opened this issue Dec 13, 2023 · 108 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Not a priority Reviewing Has a PR in review Task

Comments

@quinthar
Copy link
Contributor

quinthar commented Dec 13, 2023

Problem:
When the LHN is in "focus" mode, its sorting should be:

  1. All pinned reports, alphabetically
  2. Everything else, alphabetically

However, it doesn't seem to be. Here's from @iwiznia:

Image

Solution:
Review the LHN logic for sorting in focus mode to understand what it's currently doing, and verify it's correct. The correct behavior should be:

#focus mode:

  • All pinned chats (alphabetically)
  • Everything else (alphabetically)

most recent mode:

  • All pinned chats (alphabetically)
  • Everything else (most recent sorting)

Additionally:

  • When sorting alphabetically, it should only sort the alphanumeric characters -- other characters should be ignored.
  • "pinned chats" refers to chats that are actually pinned, as well as any chat with a "green/red brick road" (GBR/RBR), which are treated as if they are pinned.

So, for this issue please:

  1. Review the existing sorting code
  2. Update it to match the logic above
  3. Update the sort comparator to ignore non-alphanumeric characters when sorting
  4. Verify that GBR chats are showing as pinned
  5. Verify that RBR chats are showing as pinned

You up to the challenge? Let's see those proposals!!

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01432274cc2b58438c
  • Upwork Job ID: 1744194942807576576
  • Last Price Increase: 2024-01-08
Issue OwnerCurrent Issue Owner: @youssef-lr
@quinthar quinthar converted this from a draft issue Dec 13, 2023
@melvin-bot melvin-bot bot added the Monthly KSv2 label Dec 18, 2023
@quinthar quinthar moved this from MEDIUM to HIGH in [#whatsnext] #vip-vsb Dec 19, 2023
@quinthar quinthar moved this from HIGH to MEDIUM in [#whatsnext] #vip-vsb Dec 19, 2023
@quinthar quinthar moved this from MEDIUM to HIGH in [#whatsnext] #vip-vsb Dec 28, 2023
@quinthar quinthar changed the title MEDIUM: Finalize order of LHN in #focus mode HIGH: Finalize order of LHN in #focus mode Dec 28, 2023
@quinthar
Copy link
Contributor Author

Upgrading to HIGH as it's fundamentally about making it work correctly.

@quinthar quinthar added External Added to denote the issue can be worked on by a contributor Daily KSv2 Task and removed Monthly KSv2 labels Jan 8, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 8, 2024
@melvin-bot melvin-bot bot changed the title HIGH: Finalize order of LHN in #focus mode [$500] HIGH: Finalize order of LHN in #focus mode Jan 8, 2024
Copy link

melvin-bot bot commented Jan 8, 2024

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

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

melvin-bot bot commented Jan 8, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jan 8, 2024
@quinthar quinthar added the Bug Something is broken. Auto assigns a BugZero manager. label Jan 8, 2024
Copy link

melvin-bot bot commented Jan 8, 2024

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

Copy link

melvin-bot bot commented Jan 8, 2024

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Jan 8, 2024

Proposal

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

Finalize order of LHN in #focus mode

What is the root cause of that problem?

This should be fixed with getOrderedReportIDs

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

We should modify the getOrderedReportIDs logic to the above set of rules to get the desired result.

"pinned chats" refers to chats that are actually pinned, as well as any chat with a "green/red brick road" (GBR/RBR), which are treated as if they are pinned.

result.allReportErrors = OptionsListUtils.getAllReportErrors(report, reportActions) as OnyxCommon.Errors;
result.brickRoadIndicator = Object.keys(result.allReportErrors ?? {}).length !== 0 ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';

These lines of code was used to determine the RBR indicator in SideBar, which will be only used during rendering options after getting sorted reports.

We need to use this inside getOrderedReportIDs to get the RBR reports to get them alongside pinned and GBR reports.

When sorting alphabetically, it should only sort the alphanumeric characters -- other characters should be ignored.

For this we need to use the replace replace(/[^0-9a-z]/gi, '') to remove non-alphanumeric characters.

Only caveat right now is that after replace string can get empty, and in js, this will get highest preference in sorting order.

@quinthar I need your opinion on this: should we push these reports down by temporarily replacing empty string with Z (as this has the lowest priority in localeCompare) or let it be up (this may be weird as emoji only threads might get highest priority)?

Edit: The chatReportSelector inside SidebarLinksData has some fields missing (isPolicyExpenseChat, isOwnPolicyExpenseChat, isCancelledIOU) due to which title of the report is determined incorrectly by getOrderedReportIDs. Adding these fixed the order issue with policy expense reports.

What alternative solutions did you explore? (Optional)

@thebigshotsam
Copy link

thebigshotsam commented Jan 8, 2024

LHN Sorting Logic Proposal

Problem Statement

When the LHN (Left Hand Navigation) is in "focus" mode, the existing sorting logic was not aligning with the expected behavior. The sorting criteria were not correctly implemented, specifically for pinned reports and those in "focus" mode.

Solution

The LHN sorting logic has been reviewed and updated to meet the following requirements:

Sorting Criteria

  1. Focus Mode:

    • All pinned chats should be sorted alphabetically.
    • Everything else should be sorted alphabetically.
  2. Most Recent Mode:

    • All pinned chats should be sorted alphabetically.
    • Everything else should be sorted based on the most recent activity.

Additional Considerations

  • When sorting alphabetically, non-alphanumeric characters are ignored.
  • "Pinned chats" include both actually pinned chats and those with a GBR (Green Brick Road) or RBR (Red Brick Road).
  • GBR chats and RBR chats should be treated as pinned.

Implementation Details

Sorting Function

The existing sorting function has been modified to achieve the desired behavior. Here are the key updates:

  1. Grouping:

    • Reports are grouped into four categories: Pinned/GBR, Drafts, Non-archived reports, and Archived reports.
  2. Sorting:

    • Pinned/GBR reports and Drafts are sorted alphabetically.
    • Non-archived reports are sorted based on the most recent activity in "Most Recent" mode and alphabetically in "Focus" mode.
    • Archived reports are sorted similarly to non-archived reports based on the mode.
  3. Combining Arrays:

    • Pinned/GBR reports, Drafts, Non-archived reports, and Archived reports are combined into a single array (allReportsSorted).
  4. Final Sorting:

    • The combined array is sorted based on the specified criteria.
  5. Extracting ReportIDs:

    • The final array is used to extract reportIDs, and the cache is updated accordingly.

Usage

The sortLHNReports function should be used to obtain the sorted list of reportIDs for the LHN.

const sortedLHNReports = sortLHNReports(reportsToDisplay, isInDefaultMode);
// Function to sort LHN reports
function sortLHNReports(reportsToDisplay: Report[], isInDefaultMode: boolean): string[] {
    const pinnedAndGBRReports: Report[] = [];
    const draftReports: Report[] = [];
    const nonArchivedReports: Report[] = [];
    const archivedReports: Report[] = [];

    // Calculate properties and categorize reports
    reportsToDisplay.forEach((report) => {
        report.displayName = ReportUtils.getReportName(report);
        report.iouReportAmount = ReportUtils.getMoneyRequestReimbursableTotal(report, allReports);

        const isPinned = report.isPinned ?? false;
        const reportAction = ReportActionsUtils.getReportAction(report.parentReportID ?? '', report.parentReportActionID ?? '');

        if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction)) {
            pinnedAndGBRReports.push(report);
        } else if (report.hasDraft) {
            draftReports.push(report);
        } else if (ReportUtils.isArchivedRoom(report)) {
            archivedReports.push(report);
        } else {
            nonArchivedReports.push(report);
        }
    });

    // Sort each group of reports accordingly
    pinnedAndGBRReports.sort((a, b) => (a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0));
    draftReports.sort((a, b) => (a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0));

    if (isInDefaultMode) {
        // Sort non-archived reports based on most recent activity and alphabetically
        nonArchivedReports.sort((a, b) => {
            const compareDates = a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0;
            const compareDisplayNames = a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0;
            return compareDates || compareDisplayNames;
        });

        // For archived reports, ensure most recent reports are at the top by reversing the order
        archivedReports.sort((a, b) => (a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0));
    } else {
        // Sort non-archived and archived reports alphabetically
        nonArchivedReports.sort((a, b) => (a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0));
        archivedReports.sort((a, b) => (a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0));
    }

    // Combine all groups into a single array for final sorting
    const allReportsSorted: Report[] = [...pinnedAndGBRReports, ...draftReports, ...nonArchivedReports, ...archivedReports];

    // Sort the combined array based on specified criteria
    allReportsSorted.sort((a, b) => {
        const comparePinned = (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0);
        const compareDisplayNames = a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0;
        const compareDates = a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0;

        // First, sort by pinned status, then by display names, and finally by dates in most recent mode
        return comparePinned || compareDisplayNames || compareDates;
    });

    // Extract reportIDs from the final sorted array
    const LHNReports = allReportsSorted.map((report) => report.reportID);

    // Update the cache with the sorted reportIDs
    setWithLimit(reportIDsCache, cachedReportsKey, LHNReports);

    return LHNReports;
}

// Function to compare string dates
function compareStringDates(dateStringA: string, dateStringB: string): number {
    const dateA = new Date(dateStringA);
    const dateB = new Date(dateStringB);
    return dateB.getTime() - dateA.getTime();
}
The current sorting logic in the Last Heard Navigation (LHN) feature doesn't handle non-alphanumeric characters appropriately when sorting alphabetically. The requirement is to ignore these characters during alphabetical sorting for both "Pinned/GBR Reports" and "Draft Reports."

Proposed Changes:

1. Handling Non-Alphanumeric Characters:

  • Update the sorting logic for pinnedAndGBRReports and draftReports in the sortLHNReports function.
  • Implement a replacement mechanism to remove non-alphanumeric characters before performing alphabetical sorting.
pinnedAndGBRReports.sort((a, b) => {
    const cleanA = a?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
    const cleanB = b?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
    return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0;
});

draftReports.sort((a, b) => {
    const cleanA = a?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
    const cleanB = b?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
    return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0;
});

2. Verification of GBR and RBR:

  • Verify that Green/Red Brick Road (GBR/RBR) chats are correctly identified as "pinned" during the sorting process.
  • Ensure that the sort comparator is updated to handle these special cases appropriately.
// ... (previous code)

// Sort each group of reports accordingly
pinnedAndGBRReports.sort((a, b) => {
    const cleanA = a.displayName?.replace(/[^0-9a-zA-Z]/g, '');
    const cleanB = b.displayName?.replace(/[^0-9a-zA-Z]/g, '');
    return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0;
});

draftReports.sort((a, b) => {
    const cleanA = a.displayName?.replace(/[^0-9a-zA-Z]/g, '');
    const cleanB = b.displayName?.replace(/[^0-9a-zA-Z]/g, '');
    return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0;
});

// ... (remaining code)

// Sort the combined array based on specified criteria
allReportsSorted.sort((a, b) => {
    const comparePinned = (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0);
    const compareDisplayNames = a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0;
    const compareDates = a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0;

    // First, sort by pinned status, then by display names, and finally by dates in most recent mode
    return comparePinned || compareDisplayNames || compareDates;
});

// ... (remaining code)

Benefits:

  • This modification ensures that non-alphanumeric characters are ignored during the sorting of pinnedAndGBRReports and draftReports, aligning with the specified requirements. Additionally, the existing logic for GBR and RBR verification in the sorting process remains intact.
  • The proposed changes will enhance the alphabetical sorting logic by ignoring non-alphanumeric characters, aligning with the specified requirements.
    Expensify account email: [email protected]
    Upwork Profile Link: https://www.upwork.com/freelancers/~018f9e03301b35bff0

Copy link

melvin-bot bot commented Jan 8, 2024

📣 @thebigshotsam! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@shubham1206agra
Copy link
Contributor

Added some minor comment in the proposal.

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

melvin-bot bot commented Jan 10, 2024

@JmillsExpensify @fedirjh this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

  • Decide whether any proposals currently meet our guidelines and can be approved as-is today
  • If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@melvin-bot melvin-bot bot removed the External Added to denote the issue can be worked on by a contributor label Jan 10, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 13, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

Payment Summary

Upwork Job

BugZero Checklist (@JmillsExpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1744194942807576576/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@JmillsExpensify
Copy link

Removing the payment from title as we still work on the implementation.

@JmillsExpensify JmillsExpensify changed the title [HOLD for payment 2024-03-13] [$500] HIGH: Finalize order of LHN in #focus mode [$500] HIGH: Finalize order of LHN in #focus mode Mar 19, 2024
@quinthar quinthar changed the title [$500] HIGH: Finalize order of LHN in #focus mode [$500] HIGH: [Polish] Finalize order of LHN in #focus mode Mar 25, 2024
@quinthar quinthar changed the title [$500] HIGH: [Polish] Finalize order of LHN in #focus mode HIGH: [$500] [Polish] Finalize order of LHN in #focus mode Mar 25, 2024
@quinthar quinthar changed the title HIGH: [$500] [Polish] Finalize order of LHN in #focus mode HIGH: [Polish] [$500] Finalize order of LHN in #focus mode Mar 25, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

@JmillsExpensify, @youssef-lr, @luacmartins, @fedirjh, @shubham1206agra 12 days overdue. Walking. Toward. The. Light...

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

This issue has not been updated in over 14 days. @JmillsExpensify, @youssef-lr, @luacmartins, @fedirjh, @shubham1206agra eroding to Weekly issue.

@melvin-bot melvin-bot bot removed the Weekly KSv2 label May 1, 2024
Copy link

melvin-bot bot commented May 1, 2024

This issue has not been updated in over 15 days. @JmillsExpensify, @youssef-lr, @luacmartins, @fedirjh, @shubham1206agra eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@melvin-bot melvin-bot bot added the Monthly KSv2 label May 1, 2024
@shubham1206agra
Copy link
Contributor

@youssef-lr Can we restart discussion on this issue?

@youssef-lr
Copy link
Contributor

Yes I think we should. I'll get back to you this week @shubham1206agra as I'm busying finalizing a new feature.

@luacmartins
Copy link
Contributor

What's the latest here? I think we merged a fix for this issue and it's done? Are we missing payments?

@youssef-lr
Copy link
Contributor

@luacmartins we need to redo the fix, it caused performance issues and we reverted it.

@shubham1206agra
Copy link
Contributor

@youssef-lr Should we redo the fix now? Or is something blocking us now?

@youssef-lr
Copy link
Contributor

I think let's redo it and see if reassure-perf complains

@melvin-bot melvin-bot bot closed this as completed Oct 10, 2024
Copy link

melvin-bot bot commented Oct 10, 2024

@JmillsExpensify, @youssef-lr, @luacmartins, @fedirjh, @shubham1206agra, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

@shubham1206agra
Copy link
Contributor

@youssef-lr Please repoen this.

@youssef-lr youssef-lr reopened this Oct 16, 2024
@JmillsExpensify
Copy link

Is this being actively worked on? If not, my preference is to close.

@luacmartins
Copy link
Contributor

I don't think so, but @youssef-lr would be able to confirm

@JmillsExpensify
Copy link

Youssef please open back up if it's being worked on. Otherwise, I think we have more important priorities to focus on.

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. Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Not a priority Reviewing Has a PR in review Task
Projects
No open projects
Development

No branches or pull requests