[$250] Implement tests for reports that should be displayed in LHN

[OlimpiaZurek]

Tracking issue: https://github.com/Expensify/App/issues/52031

Develop and implement unit tests to verify that the following report types are correctly displayed in the LHN:

1. Current report: Ensure the currently active report is highlighted appropriately.
2. Reports with violations (RBR): Confirm that reports with outstanding violations are displayed at the top of the list with a red dot indicator.
3. Reports requiring attention (GBR): Validate that reports needing user action display a green dot and are sorted correctly.
4. Report with draft comment: Check that reports with valid draft comments are indicated and sorted by name.
5. Pinned report: Verify that pinned reports are at the top of the list and sorted correctly.
6. Unread chat in Focus Mode: Ensure unread chats in focus mode are displayed with bold text.
7. Archived report in Default Mode: Confirm that archived reports are shown correctly in default mode, sorted by lastVisibleActionCreated.
8. Self-DM report: Verify that self-DM reports are included in the LHN when the corresponding setting is enabled.
   Upwork Automation - Do Not Edit

   • Upwork Job URL: https://www.upwork.com/jobs/~021857141648927609929
   • Upwork Job ID: 1857141648927609929
   • Last Price Increase: 2024-11-21
   • Automatic offers:
   • ishpaul777 | Reviewer | 105084341
   • mkzie2 | Contributor | 105084342

Issue Owner

Current Issue Owner: @ishpaul777

[melvin-bot[bot]]

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

[trjExpensify]

I take it I'm not needed here?

[melvin-bot[bot]]

@trjExpensify Huh... This is 4 days overdue. Who can take care of this?

[trjExpensify]

@OlimpiaZurek can you confirm what I'm doing with this? Should it be assigned to you? CC: @mountiny

[mountiny]

We are going to export this issue to the community once the tests framework is setup, that is what we are holding for

[trjExpensify]

Okay, so I'll drop it to Weekly for now.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

⚠️ This issue has had its price increased by 4x or more. Please review the issue and ensure the price is correct.

[melvin-bot[bot]]

Upwork job price has been updated to $250

[melvin-bot[bot]]

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

[mountiny]

Just like that, its not on hold anymore

[mountiny]

@OlimpiaZurek can you also please help with reviewing the proposals and code to ensure the test coverage is robust and we follow the guidelines? Here are the latest https://github.com/Expensify/App/pull/52439

[Tony-MK]

Edited by proposal-police: This proposal was edited at 2024-11-22 00:09:31 UTC.

Proposal

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

Implement tests for reports that should be displayed in LHN.

What is the root cause of that problem?

LHN items have a historical record of rendering inconsistencies & regressions.

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

Write a robust unit test to test the shouldReportBeInOptionList function in ReportTestUtils.

What specific scenarios should we cover in unit tests to prevent reintroducing this issue in the future?

Firstly, similar to the ones below, we should create two helper functions to help write clean code called createReport and getOptions to easily create reports and call OptionsListUtils.getOptions respectively.

function createReport(
      isPinned = false,
      isArchived = false,
      ownerAccountID = 1, // Defaulted to current user
      lastVisibleActionCreated = 0,
) {
    return {
        isPinned: isPinned,
        reportID: rand.randInt(),
        private_isArchived: isArchived,
        ownerAccountID: ownerAccountID,
        // ... Will other attributes especially default ones
    };
}

function getOptions(reports){
   return OptionsListUtils.getOptions({ reports: reports },
        {
            includeRecentReports: true,
            includeMultipleParticipantReports: true,
            maxRecentReportsToShow: 0,
            includeP2P: true,
            forcePolicyNamePreview: true,
            includeOwnedWorkspaceChats: true,
            includeThreads: true,
            includeMoneyRequests: true,
            includeTasks: true,
            includeSelfDM: true
         }
   )
}

Secondly, we need to create reports based on the characteristics below and test ReportUtils.getOptions with the reports to check the reports returned are expected correct order.

1. An expense report with outstanding violations and not reimbursed is displayed at the top of the list with a red dot indicator.
2. Reports needing user action display a green dot and are sorted correctly
3. A report that is currently being viewed and highlighted appropriately.
4. Archived reports, sorted by lastVisibleActionCreated, in default mode.
5. A self-DM report when the corresponding setting is enabled.
6. Unread reports in focus mode and displayed with bold text.
7. Reports with a valid draft comment sorted by name.
8. Marked reports as pinned.

// Unsorted array of archived reports
const archivedReports = {
     test: [
         createReport(isArchived = true, lastVisibleActionCreated = 1),
         createReport(isArchived = true, lastVisibleActionCreated = 0),
         createReport(isArchived = true, ownerAccountID = 2, lastVisibleActionCreated = 3),
         createReport(isArchived = true, ownerAccountID = 3, lastVisibleActionCreated = 2)
     ]
}

// Sorted...
archivedReports.expected = [
    archivedReports.test[1],
    archivedReports.test[0],
    archivedReports.test[3],
    archivedReports.test[2],
]

const pinnedReports = {
    test: [
        createReport(isPinned = true),
        createReport(isPinned = true),
        createReport(isPinned = true, ownerAccountID = 2),
        createReport(isPinned = true, ownerAccountID = 3)

    ]
}

pinnedReports.expected = [
    pinnedReports.test[0],
    pinnedReports.test[1],
    pinnedReports.test[2],
    pinnedReports.test[3],
]

// Test `OptionsListUtils.getOptions`

expect(getOptions(pinnedReports.test)).toEqual(pinnedReports.expected);

expect(getOptions(archivedReports.test)).toEqual(archivedReports.expected);

// We will create different types of reports and test them to test like previously 

const unreadReports = {
    .......
}

const commentedReports = {
    .......
}

expect(getOptions(unreadReports.test)).toEqual(unreadReports.expected);

expect(getOptions(commentedReports.test)).toEqual(commentedReports.expected);

Finally, just like the previous step, let's test ReportUtils.getOptions by combining all the reports and checking whether the returned reports are sorted appropriately.

// All reports combined
const reports = {
    test: [
        ...archivedReports.test
        ...pinnedReports.test,

        // ...other types of test reports 
    ],

    expected: [
        pinnedReports.test[0],
        pinnedReports.test[1],
        pinnedReports.test[2],
        pinnedReports.test[3],

        archivedReports.test[1],
        archivedReports.test[0],
        archivedReports.test[3],
        archivedReports.test[2],

        // ...other types of expected reports 
    ],

}

expect(getOptions(reports.test)).toEqual(reports.expected);

[melvin-bot[bot]]

@trjExpensify, @OlimpiaZurek, @mountiny, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ishpaul777]

@Tony-MK Please provide relevant technical details in your proposal for it to be considered, i currently found it vague and not enough to be able to review properly

[OlimpiaZurek]

I agree that we need a more detailed proposal here.

[melvin-bot[bot]]

@trjExpensify @OlimpiaZurek @mountiny @ishpaul777 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[bot]]

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

[ishpaul777]

no proposals to review

[ishpaul777]

I have advertised this on slack

[Tony-MK]

Proposal

Hi @ishpaul777, I updated my proposal and added more technical details based on your comment.

[mkzie2]

Edited by proposal-police: This proposal was edited at 2024-11-22 08:49:08 UTC.

Proposal

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

Implement tests for reports that should be displayed in LHN

What is the root cause of that problem?

This is a new feature

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

1. To verify each type of report in the requirement display in LHN, we need to create a test case with shouldReportBeInOptionList function. Create a mock data for this report type and the expected shouldReportBeInOptionList function will return true for each type of report above.

To verify that the order is correct we need to create a test case for getOrderedReportIDs function. For each type of report, create a list of fake report data of this type and expect that getOrderedReportIDs return the correct order with the requirement.

https://github.com/Expensify/App/blob/5f483dc37ccfbb967d8eeb49302d3dfa010f93e9/src/libs/SidebarUtils.ts#L91

Reports with violations (RBR): displayed at the top of the list with a red dot indicator.

Pinned report: Verify that pinned reports are at the top of the list and sorted correctly.

Reports requiring attention (GBR): Validate that reports needing user action display a green dot and are sorted correctly.

Unread chat in Focus Mode: Ensure unread chats in focus mode are displayed with bold text.

2. For special cases, we can add a test in tests/ui/LHNItemsPresence.tsx.

• For example for reports with violations (RBR), we can follow this step when creating a test

  1. Create a mock report that we expect it have RBR and merge the data to Onyx
  2. Get getDefaultRenderedSidebarLinks with the report above
  3. Use screen.queryAllByTestId to verify that we have a red dot

3. We can create some utils function in LHNTestUtils to create mock data for each type of report easier

• For example for reports with violations (RBR), we can create a util that returns the necessary Onyx data to expect this report has violation

  What alternative solutions did you explore? (Optional)

  We can create all cases in requirement in the UI test in tests/ui/LHNItemsPresence.tsx following the step that I mentioned in point 2

[ishpaul777]

@Tony-MK @mkzie2 Thank you both for your proposal. i have noticed we already have tests/perf-test/SidebarUtils.perf-test.ts which has the basic setup up for the tests we'd want to write for getOrderedReportIDs, we might want to reuse some of the code from that.

@OlimpiaZurek What do you think about @mkzie2 proposal, while not perfect i think it provides a high level overview of what we want..

[OlimpiaZurek]

Both proposals are well thought out, but they kind of miss the main goal we’re trying to achieve here:

• Our top priority is to build a solid unit test suite around shouldReportBeInOptionList function to cover all the scenarios for report visibility. This is key to ensuring we catch regressions early.
• getOrderedReportIDs is is more about ordering reports, not determining their presence in the LHN. Plus, we’ve already got tests for it in SidebarOrderTests.ts.
• The method getOptions for building options for the OptionsList component—it doesn’t directly relate to LHN items.

Let’s shift the focus to address the core issue more directly. Also, there’s already a basic setup prepared for LHN tests, which could be a great starting point for refining your proposals. Happy to hear your thoughts!

[mkzie2]

@OlimpiaZurek We have some cases that will return early without checking shouldReportBeInOptionList. So I think getOrderedReportIDs is the best place that we can test. Additionally, we use this function to pass the data in SidebarLinks

https://github.com/Expensify/App/blob/51e25af3df2eef49c2ac87e0aef816099f9d3747/src/pages/home/sidebar/SidebarLinks.tsx#L90

https://github.com/Expensify/App/blob/51e25af3df2eef49c2ac87e0aef816099f9d3747/src/libs/SidebarUtils.ts#L139-L145

[OlimpiaZurek]

@mkzie2 I agree that there are some cases that need to be considered, but I wanted to clarify that the main focus should be on the presence of items in the LHN, rather than their ordering. While getOrderedReportIDs plays an important role, the key objective is to ensure we're testing whether a report should appear in the LHN based on shouldReportBeInOptionList.

Additionally, we want to add all new tests exclusively to the tests/ui/LHNItemsPresence.tsx file, so there won’t be a need to include them in tests/unit/SidebarOrderTest.ts or tests/unit/SidebarTest.ts.

Once these two adjustments are made, I think we can move forward with your proposal.

[ishpaul777]

not overdue, thanks for clarifications @OlimpiaZurek.

[mkzie2]

@OlimpiaZurek @ishpaul777 So what do I need to update in my proposal?

[OlimpiaZurek]

@mkzie2 Just update it taking into account this comment:

• In the section describing the problem to solve, make sure to emphasize that the goal is to verify the presence of reports in the LHN (using shouldReportBeInOptionList), not just their order.
• Update the part about where the tests should be added. All new tests should go into tests/ui/LHNItemsPresence.tsx (move it from optional to main solution)

[mkzie2]

@OlimpiaZurek Thanks, I updated my proposal.

[ishpaul777]

@mkzie2 Proposal LGTM!

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

Current assignee @mountiny is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot[bot]]

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @mkzie2 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖