Hide Notifications behind a feature flag #3358

shubham1g5 · 2025-10-07T10:07:57Z

Product Description

https://dimagi.atlassian.net/browse/QA-8140

Labels and Review

Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

coderabbitai · 2025-10-07T10:10:37Z

📝 Walkthrough

Walkthrough

The change introduces a NOTIFICATIONS feature flag and integrates it into UI visibility logic. In BaseDrawerController, the notification view is now only visible when the user is signed in and the NOTIFICATIONS flag is enabled. The credential visibility check switches to using the imported isFeatureEnabled(WORK_HISTORY). In PersonalIdFeatureFlagChecker, a new public constant NOTIFICATIONS is added, and isFeatureEnabled is updated to handle it, returning false by default.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Drawer as BaseDrawerController
  participant FF as FeatureFlagChecker

  User->>Drawer: Open nav drawer
  Drawer->>FF: isFeatureEnabled(WORK_HISTORY)
  FF-->>Drawer: boolean
  Drawer->>FF: isFeatureEnabled(NOTIFICATIONS)
  FF-->>Drawer: false (default)
  alt Signed in AND NOTIFICATIONS enabled
    Drawer->>Drawer: Show notificationView
  else
    Drawer->>Drawer: Hide notificationView
  end
  Drawer->>Drawer: shouldShowCredential based on WORK_HISTORY
  note over Drawer: Update UI visibility accordingly

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

OrangeAndGreen
Jignesh-dimagi

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description includes only the Product Description section and the Labels and Review checklist while omitting required sections such as Technical Summary, Feature Flag, and Safety Assurance. It therefore does not follow the repository’s description template and lacks essential context about the rationale, design decisions, and testing strategy. Without these sections, reviewers cannot fully assess the change’s safety, automated test coverage, or QA plan.	Please update the PR description to include the Technical Summary section with the ticket link and design rationale, specify the Feature Flag section, and add Safety Assurance details covering your testing approach, automated test coverage, and QA plan as outlined in the template. Ensure each required heading is present and populated with relevant information to help reviewers understand the change’s motivation and risk mitigation measures.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly and accurately summarizes the primary change of hiding notifications behind a feature flag, directly reflecting the main code modifications. It is concise, specific, and free of unnecessary details. As a result, it aligns well with the repository guidelines for a clear and focused pull request title.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch hideNotification

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

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

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)

24-25: Remove unused PersonalIdFeatureFlagChecker import
The class import on line 23 isn’t referenced (only its companion members are used via static imports).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 938597b and ac72c38.

📒 Files selected for processing (2)

app/src/org/commcare/navdrawer/BaseDrawerController.kt (2 hunks)
app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)

app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (1)

isFeatureEnabled (21-28)

🔇 Additional comments (2)

app/src/org/commcare/personalId/PersonalIdFeatureFlagChecker.kt (1)

15-15: LGTM! Feature flag implementation follows existing pattern.

The NOTIFICATIONS constant and its handling in isFeatureEnabled are correctly implemented, consistent with the WORK_HISTORY flag pattern. The default value of false appropriately hides notifications until the flag is enabled.

Also applies to: 25-25

app/src/org/commcare/navdrawer/BaseDrawerController.kt (1)

238-238: Good refactor for consistency.

Switching to the imported isFeatureEnabled function improves consistency with the notification visibility check at line 233 and reduces verbosity.

coderabbitai · 2025-10-07T10:10:40Z

app/src/org/commcare/navdrawer/BaseDrawerController.kt

        binding.navDrawerRecycler.visibility = if (isSignedIn) View.VISIBLE else View.GONE
        binding.profileCard.visibility = if (isSignedIn) View.VISIBLE else View.GONE
-        binding.notificationView.visibility = if (isSignedIn) View.VISIBLE else View.GONE
+        binding.notificationView.visibility = if (isSignedIn && isFeatureEnabled(NOTIFICATIONS)) View.VISIBLE else View.GONE


🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

LGTM! Notification visibility correctly gated by feature flag.

The visibility logic properly requires both user sign-in and the NOTIFICATIONS feature flag to be enabled.

Verify that all notification-related UI elements and functionality are consistently gated by this feature flag. For example, check if notification click handlers, notification data fetching, or other notification UI components also need feature flag checks.

🏁 Script executed:

#!/bin/bash # Description: Find other notification-related code that might need feature flag gating # Search for notification-related UI elements and methods rg -n -C3 'notificationView|NotificationRecord|ic_bell|ic_new_notification_bell' app/src/org/commcare/navdrawer/BaseDrawerController.kt # Search for notification navigation rg -n 'goToNotification' app/src/org/commcare/

Length of output: 2176

Wrap notification fetch and icon update in refreshDrawerContent with feature flag
In BaseDrawerController.refreshDrawerContent(), you still call getAllNotifications(...) and update binding.ivNotification whenever signed in, even if NOTIFICATIONS is disabled. Enclose that block in if (isFeatureEnabled(NOTIFICATIONS)) { … } to fully gate notification behavior.

🤖 Prompt for AI Agents

In app/src/org/commcare/navdrawer/BaseDrawerController.kt around line 233, the notification fetch and icon update inside refreshDrawerContent are executed whenever the user is signed in even if the NOTIFICATIONS feature flag is disabled; wrap the entire block that calls getAllNotifications(...) and updates binding.ivNotification in if (isFeatureEnabled(NOTIFICATIONS)) { ... } so both fetching and UI updates only run when the feature is enabled, and ensure the visibility update for binding.notificationView remains consistent with that gating.

Jignesh-dimagi · 2025-10-07T11:44:23Z

@shubham1g5 We might need to the control here also instead of just making visibility false?

shubham1g5 · 2025-10-07T11:53:03Z

@Jignesh-dimagi is this what you meant ?

Jignesh-dimagi · 2025-10-07T11:55:07Z

@Jignesh-dimagi is this what you meant ?

Yes over here also and sorry I forgot to put link previously. I meant here

shubham1g5 · 2025-10-07T12:04:13Z

@Jignesh-dimagi makes sense, added here

shubham1g5 · 2025-10-07T12:23:07Z

@damagatchi retest this please

Hide Notifications behind a feature flag

ac72c38

shubham1g5 requested review from Jignesh-dimagi, OrangeAndGreen and jaypanchal-13 October 7, 2025 10:07

shubham1g5 added the skip-integration-tests Skip android tests. label Oct 7, 2025

coderabbitai bot reviewed Oct 7, 2025

View reviewed changes

Gate more notifications functionality

525c7f5

hide notifications on connect screens as well

666f046

Jignesh-dimagi approved these changes Oct 7, 2025

View reviewed changes

shubham1g5 merged commit f945886 into commcare_2.60 Oct 7, 2025
2 checks passed

coderabbitai bot mentioned this pull request Oct 27, 2025

Enables Work History and hides pending tab #3380

Merged

4 tasks

coderabbitai bot mentioned this pull request Nov 4, 2025

Enable notifications #3396

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Hide Notifications behind a feature flag #3358

Hide Notifications behind a feature flag #3358

Uh oh!

shubham1g5 commented Oct 7, 2025

Uh oh!

coderabbitai bot commented Oct 7, 2025

Walkthrough

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Oct 7, 2025

Uh oh!

Jignesh-dimagi commented Oct 7, 2025

Uh oh!

shubham1g5 commented Oct 7, 2025

Uh oh!

Jignesh-dimagi commented Oct 7, 2025

Uh oh!

shubham1g5 commented Oct 7, 2025

Uh oh!

shubham1g5 commented Oct 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Hide Notifications behind a feature flag #3358

Hide Notifications behind a feature flag #3358

Uh oh!

Conversation

shubham1g5 commented Oct 7, 2025

Product Description

Labels and Review

Uh oh!

coderabbitai bot commented Oct 7, 2025

Walkthrough

Sequence Diagram(s)

Estimated code review effort

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Jignesh-dimagi commented Oct 7, 2025

Uh oh!

shubham1g5 commented Oct 7, 2025

Uh oh!

Jignesh-dimagi commented Oct 7, 2025

Uh oh!

shubham1g5 commented Oct 7, 2025

Uh oh!

shubham1g5 commented Oct 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants