chore: correctly return server default notification preference when value is false

[sahillllllllll-bit]

Problem

getUserNotificationPreference checks server defaults using:

if (serverValue)

This ignores valid falsy values such as false and 0.
As a result, when an admin sets Desktop Notifications default to Disabled, the API returns null/undefined instead of the configured default.

Reproduction

1. Administration → Accounts → Default User Preferences
2. Set Desktop Notifications = Disabled
3. Call /api/v1/users.getPreferences

Solution

Check for undefined instead of truthiness:

typeof serverValue !== 'undefined'

This preserves valid falsy configuration values.

Impact

Ensures server default notification settings are respected and correctly returned by the API.

COMM-144

Summary by CodeRabbit

• Bug Fixes
  • Fixed notification preference handling to properly recognize and apply certain preference settings that were previously being disregarded due to how values were evaluated.

[dionisio-bot]

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

[changeset-bot]

⚠️ No Changeset found

Latest commit: fb253d5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

Walkthrough

The change replaces a truthy check with an explicit undefined check in a notification preference utility function. This allows falsy server values (0, '', false) to be returned as valid server-derived preferences instead of being treated as absent.

Changes

Cohort / File(s)	Summary
Notification Preference Logic apps/meteor/app/utils/server/getUserNotificationPreference.ts	Changed conditional from truthy check to explicit undefined check for serverValue, allowing falsy values like 0, empty string, and false to be recognized as valid server-provided preferences.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

type: bug

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title accurately describes the main change: replacing a truthy check with an explicit undefined check to properly return false as a valid server default notification preference.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cubic-dev-ai]

No issues found across 1 file

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the actual file to see the complete implementation
cat apps/meteor/app/utils/server/getUserNotificationPreference.ts

Length of output: 1217

---

🏁 Script executed:

#!/bin/bash
# Look at the git diff to see exactly what changed
git diff develop..HEAD -- apps/meteor/app/utils/server/getUserNotificationPreference.ts

Length of output: 631

---

🏁 Script executed:

#!/bin/bash
# Find where this function is used to understand the context
rg -n "getUserNotificationPreference" --type=ts -C 2

Length of output: 1768

---

🏁 Script executed:

#!/bin/bash
# Check if there are tests for this function
fd -e ts -e js getUserNotificationPreference test

Length of output: 91

---

🏁 Script executed:

#!/bin/bash
# Search for test files more broadly
fd -e spec.ts -e test.ts getUserNotificationPreference

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check what values the settings can have for notification preferences
rg -n "Accounts_Default_User_Preferences" --type=ts -C 2

Length of output: 25266

---

🏁 Script executed:

#!/bin/bash
# Look for any related tests in the push-notifications area
fd -e spec.ts -e test.ts . apps/meteor/app/push-notifications

Length of output: 0

---

@sahillllllllll-bit Yes, the fix is valid and good! Here's my analysis:

✅ The Fix is Correct

What Changed:

// Before
if (serverValue)

// After  
if (typeof serverValue !== 'undefined')

Why It's Better:

1. Handles Falsy Values Correctly: The old truthy check (if (serverValue)) incorrectly rejected valid falsy values like:

   • false - which represents "Disabled" notifications
   • 0 - valid for numeric settings
   • '' - potentially valid for string settings

2. Consistent with Existing Pattern: Your fix matches how user preferences are already checked on line 35:

   typeof user.settings.preferences[preferenceKey] !== 'undefined'

3. Solves the Reported Bug: When an admin sets "Desktop Notifications = Disabled", the setting has a value (likely false or 'nothing'), but the old code treated it as absent, returning null instead of the configured default.

4. Proper Undefined Handling: The new check correctly distinguishes between:

   • No setting configured (undefined) → returns null ✓
   • Setting explicitly set to falsy value → returns the value ✓

Settings Context

From the codebase, notification preferences can be:

• desktopNotifications: 'all', 'mentions', 'nothing'
• pushNotifications: 'all', 'mentions', 'nothing'
• Boolean flags: true / false

All of these are now properly handled. Great catch on this bug! 🎯

[sahillllllllll-bit]

@KevLehman please take a look at this if you have time , thankyou :)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.72%. Comparing base (d8baf39) to head (fb253d5).
⚠️ Report is 51 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #39142      +/-   ##
===========================================
+ Coverage    70.71%   70.72%   +0.01%     
===========================================
  Files         3195     3195              
  Lines       113095   113095              
  Branches     20498    20489       -9     
===========================================
+ Hits         79970    79986      +16     
+ Misses       31082    31059      -23     
- Partials      2043     2050       +7     

Flag	Coverage Δ
e2e	60.35% <ø> (-0.03%)	⬇️
e2e-api	48.87% <ø> (+1.10%)	⬆️
unit	71.35% <ø> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.