Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.5k stars 2.85k forks source link

Search - Unread message isnot shown in bold in search preview #51410

Open lanitochka17 opened 2 days ago

lanitochka17 commented 2 days ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.53-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/runs/view/26723&group_by=cases:section_id&group_order=asc&group_id=229066 Issue reported by: Applause - Internal Team

Action Performed:

  1. Send a message from another account to this account and dont open it
  2. Open search
  3. Observe that the unread message is listed in recent preview but is not shown in bold

Expected Result:

Unread message should be shown in bold in chat preview

Actual Result:

Message is not shown in bold

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/73d961ba-5dc4-40df-a7d0-ddb3c0df328b

View all open jobs on GitHub

melvin-bot[bot] commented 2 days ago

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

lanitochka17 commented 2 days ago

@muttmuure FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

FitseTLT commented 2 days ago

Edited by proposal-police: This proposal was edited at 2024-10-24 14:52:05 UTC.

Proposal

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

Search - Unread message isnot shown in bold in search preview

What is the root cause of that problem?

When we build the option we are setting isBold here https://github.com/Expensify/App/blob/f46bce0b96c533b6aac0e2464ae27615101a7f7d/src/libs/OptionsListUtils.ts#L1954 but inside shouldUseBoldText we are trying to get notification preference from the option but it doesn't exist (the report data is included in reportOption.item) so it will set isBold false

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

This function is used in two cases inside getOption as mentioned above and here for LHN case but in LHN case option.item is undefined and the notification preference data is available in optionItem so here https://github.com/Expensify/App/blob/f46bce0b96c533b6aac0e2464ae27615101a7f7d/src/libs/OptionsListUtils.ts#L2552-L2553 for getReportNotificationPreference we should pass report.item (this will apply for getOptions case) and as a backup we will pass report (this will apply for LHN case)

function shouldUseBoldText(report: ReportUtils.OptionData | SearchOption): boolean {
    const notificationPreference = ReportUtils.getReportNotificationPreference(report.item ?? report);
    return report.isUnread === true && notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE && notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
}

What alternative solutions did you explore? (Optional)

bernhardoj commented 2 days ago

Edited by proposal-police: This proposal was edited at 2024-10-24 14:40:40 UTC.

Proposal

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

Unread report in search router modal isn't shown in bold text.

What is the root cause of that problem?

We already have a logic to decide whether to bold the text/title or not. https://github.com/Expensify/App/blob/642fd97e9a078495176e01b3ac17d01262bf1fa4/src/libs/OptionsListUtils.ts#L1963

Because search page wants to bold based on the unread status, shouldBoldTitleByDefault is false, so we rely on shouldUseBoldText which checks if the report is unread and the notification preference is not MUTE or HIDDEN. https://github.com/Expensify/App/blob/642fd97e9a078495176e01b3ac17d01262bf1fa4/src/libs/OptionsListUtils.ts#L2626-L2629

shouldUseBoldText checks the notification preference from the report participants and defaults to HIDDEN if undefined. In our case, it's undefined. https://github.com/Expensify/App/blob/642fd97e9a078495176e01b3ac17d01262bf1fa4/src/libs/ReportUtils.ts#L1227-L1232

That's because the report option doesn't have participants data and only participantsList. https://github.com/Expensify/App/blob/642fd97e9a078495176e01b3ac17d01262bf1fa4/src/libs/OptionsListUtils.ts#L679-L710

participantsList contains the personal detail data of each participant. https://github.com/Expensify/App/blob/642fd97e9a078495176e01b3ac17d01262bf1fa4/src/libs/OptionsListUtils.ts#L712-L718

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

We can add participants to the report option data.

result.participants = report.participants;

And we can also consider replacing participantsList with participants to avoid confusion. To get the personal detail data, we can get it from PERSONAL_DETAILS_LIST onyx, keyed with the accountID from participants.

OR

Inside shouldUseBoldText, we can get the notification preference from report.notificationPreference because we already build it here. https://github.com/Expensify/App/blob/642fd97e9a078495176e01b3ac17d01262bf1fa4/src/libs/OptionsListUtils.ts#L745

const notificationPreference = report.notificationPreference ?? ReportUtils.getReportNotificationPreference(report);

but I prefer the 1st solution and remove the notificationPreference when creating the option (createOption) so we will only have 1 source of truth, that is by using ReportUtils.getReportNotificationPreference(report).

FitseTLT commented 2 days ago

Only added a little bit of detailed explanation of the solution.