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

[quinthar]

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:

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 Owner

Current Issue Owner: @youssef-lr

[quinthar]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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]

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]

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: connect@sakshamtiwari.co.in
  Upwork Profile Link: https://www.upwork.com/freelancers/~018f9e03301b35bff0

[melvin-bot]

📣 @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.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[shubham1206agra]

Added some minor comment in the proposal.

[melvin-bot]

@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]

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

[quinthar]

What are the next steps? Who are we waiting on, and what are waiting on them to do?

[fedirjh]

What are the next steps?

Nex steps should be reviewing the proposals.

[melvin-bot]

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

• Fixed LHN Ordering #35907

If no regressions arise, payment will be issued on 2024-03-13. 🎊

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

• @fedirjh requires payment (Needs manual offer from BZ)
• @shubham1206agra requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

• [@fedirjh / @shubham1206agra] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh / @shubham1206agra] 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:
• [@fedirjh / @shubham1206agra] 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:
• [@fedirjh / @shubham1206agra] Determine if we should create a regression test for this bug.
• [@fedirjh / @shubham1206agra] 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.
• [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[shubham1206agra]

@youssef-lr The PR was reverted here #38081 due to performance issues. We again need to raise a PR to fix the same.

[youssef-lr]

Bummer. How about we make pinning RBR reports only rely on the transaction violations coming from the violations onyx key? I think the performance hit is coming from the fact that we iterate over all report actions to find out if they have an error. The errors stored in the report actions are just API errors, which I think are rare.

[shubham1206agra]

Maybe make some BE changes to get the error from these actions reflected in the report itself. This will help us to get RBR without going through reportActions.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @fedirjh paid $(AMOUNT) via Upwork (LINK)
• ROLE: @shubham1206agra paid $(AMOUNT) via Upwork (LINK)

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]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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!

[shubham1206agra]

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

[youssef-lr]

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

[luacmartins]

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

[youssef-lr]

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

[shubham1206agra]

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

[youssef-lr]

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