Skip to content

chore: enable notification feature by default#38073

Draft
AhtishamShahid wants to merge 1 commit intomasterfrom
ahtisham/enable_notifications_by_default
Draft

chore: enable notification feature by default#38073
AhtishamShahid wants to merge 1 commit intomasterfrom
ahtisham/enable_notifications_by_default

Conversation

@AhtishamShahid
Copy link
Contributor

This pull request refactors the feature flag logic for notifications and email notifications throughout the codebase. The main change is switching from "enable" waffle flags (e.g., ENABLE_NOTIFICATIONS, ENABLE_EMAIL_NOTIFICATIONS) to "disable" waffle flags (e.g., DISABLE_NOTIFICATIONS, DISABLE_EMAIL_NOTIFICATIONS). This means that the features are now enabled by default unless explicitly disabled by the corresponding flag. The update includes changes in both application logic and test cases to use the new flags and invert the logic where necessary.

Key changes include:

Feature flag implementation and usage updates:

  • Replaced ENABLE_NOTIFICATIONS and ENABLE_EMAIL_NOTIFICATIONS with DISABLE_NOTIFICATIONS and DISABLE_EMAIL_NOTIFICATIONS in openedx/core/djangoapps/notifications/config/waffle.py, including updating flag names, documentation, and logic. The new flags now disable the respective features when enabled, instead of enabling them.

  • Updated all application logic in notification and discussion-related tasks to check for the "disable" flags (DISABLE_NOTIFICATIONS, DISABLE_EMAIL_NOTIFICATIONS) and invert the logic accordingly, so features are active unless the flag is enabled.
    Test updates:

  • Refactored all test cases to use the new "disable" flags, removing or inverting any @override_waffle_flag(ENABLE_NOTIFICATIONS, ...) and @override_waffle_flag(ENABLE_EMAIL_NOTIFICATIONS, ...) decorators and logic. Updated assertions to match the new default-enabled behavior.

These changes simplify feature management by making notifications and email notifications enabled by default, and only disabled when the respective "disable" flag is set. This also makes the codebase more consistent and easier to reason about.

Copilot AI review requested due to automatic review settings March 3, 2026 09:33
@AhtishamShahid AhtishamShahid requested a review from a team as a code owner March 3, 2026 09:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the notification feature flag strategy from opt-in ("enable" flags) to opt-out ("disable" flags). Both ENABLE_NOTIFICATIONS and ENABLE_EMAIL_NOTIFICATIONS waffle flags are replaced by DISABLE_NOTIFICATIONS and DISABLE_EMAIL_NOTIFICATIONS respectively. This makes notifications and email notifications enabled by default, and they are only suppressed when the corresponding disable flag is explicitly set.

Changes:

  • Introduces DISABLE_NOTIFICATIONS and DISABLE_EMAIL_NOTIFICATIONS waffle flags, inverts all flag checks throughout the application, and removes the now-obsolete is_email_notification_flag_enabled() helper.
  • Updates all test cases to remove @override_waffle_flag(ENABLE_NOTIFICATIONS, active=True) decorators/context managers (since the feature is now on by default), and inverts assertions in tests that explicitly test flag behavior.
  • Adjusts expected DB query counts upward by 1 in batch notification tests, reflecting the new waffle flag check behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
openedx/core/djangoapps/notifications/config/waffle.py Defines DISABLE_NOTIFICATIONS and DISABLE_EMAIL_NOTIFICATIONS flags replacing old enable flags
openedx/core/djangoapps/notifications/utils.py Inverts flag check to use DISABLE_NOTIFICATIONS
openedx/core/djangoapps/notifications/tasks.py Inverts flag check for send_notifications task
openedx/core/djangoapps/notifications/email/utils.py Removes is_email_notification_flag_enabled() helper; imports new disable flag
openedx/core/djangoapps/notifications/email/tasks.py Inverts flag checks to use DISABLE_EMAIL_NOTIFICATIONS in all email task functions
openedx/core/djangoapps/notifications/tests/test_tasks_with_account_level_pref.py Removes enable-flag decorators; updates query counts and flag override logic
openedx/core/djangoapps/notifications/tests/test_views.py Removes enable-flag decorator; updates expected show_preferences value to True
openedx/core/djangoapps/notifications/email/tests/test_utils.py Removes TestWaffleFlag class and cleans up imports
openedx/core/djangoapps/notifications/email/tests/test_tasks.py Removes enable-flag overrides from most tests; inverts flag logic in flag-specific tests
lms/djangoapps/discussion/rest_api/tests/test_tasks_v2.py Removes enable-flag class decorators from discussion task tests
cms/djangoapps/contentstore/tests/test_utils.py Removes enable-flag class decorator from course update notification tests
openedx/core/djangoapps/enrollments/tests/test_views.py Removes enable-flag class decorator from enrollment tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AhtishamShahid AhtishamShahid force-pushed the ahtisham/enable_notifications_by_default branch from 158c60a to d40d945 Compare March 3, 2026 15:06
@AhtishamShahid AhtishamShahid requested a review from Copilot March 3, 2026 15:07
@AhtishamShahid AhtishamShahid marked this pull request as draft March 3, 2026 15:07
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fix: Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix: Update comment for test

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix: updated unit test

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix: resolved linter errors
@AhtishamShahid AhtishamShahid force-pushed the ahtisham/enable_notifications_by_default branch from d40d945 to 8c90930 Compare March 3, 2026 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants