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] Self DM - Self DM is not bolded in LHN after it is marked as unread #51362

Closed
4 of 8 tasks
lanitochka17 opened this issue Oct 23, 2024 · 15 comments
Closed
4 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 23, 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!


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: N/A
Email or phone of affected tester (no customers): javascript:
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Log in with a new account
  3. Go to self DM
  4. Send a few messages
  5. Right click on the self DM in LHN
  6. Click Mark as unread

Expected Result:

Self DM will be bolded in LHN after it is marked as unread

Actual Result:

Self DM is not bolded in LHN after it is marked as unread

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6643712_1729715829826.self_dm_unread.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021849224049457734741
  • Upwork Job ID: 1849224049457734741
  • Last Price Increase: 2024-10-23
Issue OwnerCurrent Issue Owner: @mkhutornyi
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Oct 23, 2024
Copy link

melvin-bot bot commented Oct 23, 2024

Triggered auto assignment to @blimpich (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Oct 23, 2024

💬 A slack conversation has been started in #expensify-open-source

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@blimpich
Copy link
Contributor

I have a meeting right now but will look into this right after the meeting

@blimpich
Copy link
Contributor

blimpich commented Oct 23, 2024

Caused by this: #51212. Tested reverting it locally. Actually probably not caused by it.

@blimpich
Copy link
Contributor

blimpich commented Oct 23, 2024

Demoting because I don't think this is deploy blocker worthy

@NJ-2020
Copy link
Contributor

NJ-2020 commented Oct 23, 2024

Proposal

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

Self DM - Self DM is not bolded in LHN after it is marked as unread

What is the root cause of that problem?

When marking the self dm as unread, we will return bold text if the notificationPreference is not equal to mute and also not equal to hidden and the report.isUnread is true

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

But for selfDM the notificationPreference will always return mute, see here

App/src/libs/ReportUtils.ts

Lines 1217 to 1219 in 5e975f5

if (isSelfDM(report)) {
return CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE;
}

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

Right here we can check if the report is self dm, then we will only check if the notificationPreference is not equal to hidden and the report.isUnread is true

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

What alternative solutions did you explore? (Optional)

@blimpich
Copy link
Contributor

Gonna mark this as external since I don't know what PR caused it. @NJ-2020 as part of your proposal can you state what PR caused this regression in the first place?

@blimpich blimpich added the External Added to denote the issue can be worked on by a contributor label Oct 23, 2024
@melvin-bot melvin-bot bot changed the title Self DM - Self DM is not bolded in LHN after it is marked as unread [$250] Self DM - Self DM is not bolded in LHN after it is marked as unread Oct 23, 2024
Copy link

melvin-bot bot commented Oct 23, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 23, 2024
@blimpich blimpich added Bug Something is broken. Auto assigns a BugZero manager. and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Oct 23, 2024
Copy link

melvin-bot bot commented Oct 23, 2024

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

Copy link

melvin-bot bot commented Oct 23, 2024

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

@blimpich blimpich added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 23, 2024
@NJ-2020
Copy link
Contributor

NJ-2020 commented Oct 24, 2024

@blimpich I think this could be NAB, because inside the production code we will show the bold text if the notificationPreference is not equal to mute

But inside the shouldUseBoldText function we only check for isUnread and notificationPreference is not equal to mute only inside the production code, while the notificationPreference returns HIDDEN value which will return true for the shouldUseBoldText function

function shouldUseBoldText(report: ReportUtils.OptionData): boolean {
return report.isUnread === true && ReportUtils.getReportNotificationPreference(report) !== CONST.REPORT.NOTIFICATION_PREFERENCE.MUTE;
}

This if statement will not run if the second param value is true because !shouldDefaltToHidden is false

App/src/libs/ReportUtils.ts

Lines 1226 to 1229 in 4d4179b

function getReportNotificationPreference(report: OnyxEntry<Report>, shouldDefaltToHidden = true): ValueOf<typeof CONST.REPORT.NOTIFICATION_PREFERENCE> {
if (!shouldDefaltToHidden) {
return report?.participants?.[currentUserAccountID ?? -1]?.notificationPreference ?? getDefaultNotificationPreferenceForReport(report);
}

We will return either the report?.participants?.[currentUserAccountID ?? -1]?.notificationPreference or HIDDEN value, but right now in the production code the report?.participants?.[currentUserAccountID ?? -1] returns null value which will return the HIDDEN value
return report?.participants?.[currentUserAccountID ?? -1]?.notificationPreference ?? CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;

Why the report?.participants?.[currentUserAccountID ?? -1] return null value, because for the report we pass the optionItem value here

const textUnreadStyle = OptionsListUtils.shouldUseBoldText(optionItem) ? [textStyle, styles.sidebarLinkTextBold] : [textStyle];

And we're using the SidebarUtils.getOptionData function to get the optionItem value
const optionItem = useMemo(() => {
// Note: ideally we'd have this as a dependent selector in onyx!
const item = SidebarUtils.getOptionData({
report: fullReport,

But inside the production code we don't set any participants value for the report, instead we use the participantsList
result.participantsList = participantPersonalDetailList;

So in this production code we're using the participants value instead of participantsList which will return the HIDDEN value

return report?.participants?.[currentUserAccountID ?? -1]?.notificationPreference ?? CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;

@bernhardoj
Copy link
Contributor

We have the same issue before here too

@blimpich
Copy link
Contributor

Ah, I see, this actually makes a lot of sense. Based off looking at this PR this isn't a bug, its the intended behavior. Thank you @NJ-2020 and @bernhardoj for helping me understand this.

I will try to get applause to not mark this as a bug in the future.

Copy link

melvin-bot bot commented Oct 24, 2024

@anmurali @blimpich Be sure to fill out the Contact List!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

6 participants