Skip to content

refactor: deprecated v2 preferences api and its dependencies#38072

Open
AhtishamShahid wants to merge 3 commits intomasterfrom
ahtisham/remove-v2-pref-api
Open

refactor: deprecated v2 preferences api and its dependencies#38072
AhtishamShahid wants to merge 3 commits intomasterfrom
ahtisham/remove-v2-pref-api

Conversation

@AhtishamShahid
Copy link
Contributor

This pull request refactors the notification preferences system by removing the NotificationTypeManager and NotificationAppManager classes, simplifying how default notification preferences are accessed and managed. It also updates terminology for clarity and removes unused or redundant functions. The changes streamline the codebase and improve maintainability.

Refactoring and Simplification:

  • Removed the NotificationTypeManager and NotificationAppManager classes, along with their associated methods, in base_notification.py. Preference logic is now handled by simpler utility functions.
  • Removed the get_default_values_of_preference function and updated all usages to use the more general get_default_values_of_preferences. [1] [2] [3]

Terminology and Documentation Updates:

  • Updated terminology and comments in NotificationType and NotificationApp to use use_app_defaults instead of is_core, clarifying the intent and behavior of these fields. [1] [2] [3]

Code Cleanup:

  • Removed the unused get_non_editable_channels function from serializers.py and cleaned up related validation logic. [1] [2]
  • Cleaned up imports in test_views.py to remove references to deleted classes.

Functionality Adjustments:

  • Updated the retrieval of notification type objects in get_notification_content to use the new utility function for preferences.

These changes collectively make the notification preference system easier to understand, reduce code duplication, and improve maintainability.

Copilot AI review requested due to automatic review settings March 3, 2026 09:31
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 removes the deprecated V2 notification preferences API and the NotificationTypeManager/NotificationAppManager abstractions, consolidating “default preferences” logic into simpler utility functions and aligning terminology around use_app_defaults / grouped notifications.

Changes:

  • Removed the V2 aggregated preferences view and URL route, and deleted its associated test coverage.
  • Replaced per-app default lookup (get_default_values_of_preference) with a unified get_default_values_of_preferences helper and updated call sites.
  • Updated comments/terminology around “core” notifications to use_app_defaults / grouped notifications.

Reviewed changes

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

Show a summary per file
File Description
openedx/core/djangoapps/notifications/views.py Removes V2 API view and relies on V3 grouped-notification update flow.
openedx/core/djangoapps/notifications/urls.py Removes the V2 route and import wiring.
openedx/core/djangoapps/notifications/tests/test_views.py Removes V2 view tests and associated imports.
openedx/core/djangoapps/notifications/tasks.py Switches default web-config lookup to the new unified defaults helper.
openedx/core/djangoapps/notifications/serializers.py Removes unused helper and adjusts notification type validation logic.
openedx/core/djangoapps/notifications/base_notification.py Removes manager classes, introduces unified defaults helper usage, and updates terminology/comments.
Comments suppressed due to low confidence (1)

openedx/core/djangoapps/notifications/base_notification.py:401

  • get_notification_content calls get_default_values_of_preferences(), which rebuilds a merged dict for all notification types on every invocation. Since Notification.content uses this during listing/serialization, this can become a hot path. Consider caching the merged mapping (e.g., functools.lru_cache or a module-level computed constant) and using that here to avoid repeated per-notification recomputation.
    # Retrieve the notification type object from NotificationTypeManager.
    notification_type = get_default_values_of_preferences().get(notification_type, None)

    if notification_type:
        # Check if the notification is grouped.
        is_grouped = context.get('grouped', False)

        # Determine the correct template key based on whether it's grouped or not.
        template_key = "grouped_content_template" if is_grouped else "content_template"

        # Get the corresponding template from the notification type.
        template = notification_type.get(template_key, None)


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

Comment on lines 306 to 309
Each notification type defined in COURSE_NOTIFICATION_TYPES also references an app.

Each notification type can also be optionally defined as a core notification.
In this case, the delivery preferences for that notification are taken
from the `core_*` fields of the associated notification app.
"""
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The NotificationApp docstring is now incomplete (“In this case, the delivery preferences for that notification are taken …”) after removing the core terminology. Please finish or rewrite this docstring so it fully explains how app defaults apply to use_app_defaults / grouped notifications.

Copilot uses AI. Check for mistakes.
Comment on lines 141 to 143
def add_non_editable_in_preference(preference):
"""
Add non_editable preferences to the preference dict
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

add_non_editable_in_preference still special-cases a notification type named "core" to pull non-editable channels from the app config, but the V3 API uses "grouped_notification" for that synthetic grouped entry. This means app-level non_editable settings won’t be applied to grouped notifications. Update the special-case to handle "grouped_notification" (and consider whether "core" is still needed anywhere).

Copilot uses AI. Check for mistakes.
Comment on lines 411 to 413
def get_default_values_of_preferences() -> dict[str, dict[str, Any]]:
"""
Returns default preferences for all notification apps
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

get_default_values_of_preferences merges app defaults into notification type configs. For use_app_defaults types, the current merge order ({**app_defaults, **values}) lets any overlapping keys on the type override app defaults, which contradicts the comment above saying the app config should override type-level web/email/push/email_cadence/non_editable. Consider reversing precedence (or explicitly ensuring app defaults win for those keys) so misconfigured types can’t silently override app-level defaults.

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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