feat: add detailed logging and error handling to notification API view

[Rammohan-dev1]

Context: Why these changes were needed (e.g., improve observability of notification API behavior)

Changes Made: Added logging, enhanced error handling, improved API responses, etc.

Testing: Describe how you verified (manual/automated tests, API response checks)

Impact: Mention if it affects any production settings or downstream tasks (if none, say “No production impact”)

[Copilot]

Pull request overview

This PR aims to improve observability and resiliency of the Notifications API by adding structured logging and additional error handling across several DRF views.

Changes:

• Added module-level logger and additional info/warning/error logs across notification list/count/read/seen endpoints.
• Wrapped several view handlers in try/except blocks and introduced more explicit error responses (including some new 500 responses).
• Refactored imports and tightened/updated multiple docstrings and response messages.

Comments suppressed due to low confidence (7)

openedx/core/djangoapps/notifications/views.py:209

• QuerySet.filter()/aggregation here will not raise Notification.DoesNotExist, so including it in this exception handler is misleading. If you want to handle unexpected failures, either remove the try/except and let DRF handle errors, or catch the specific exceptions that can actually be raised in this block (and avoid turning programming errors into a generic 500).

        except (Notification.DoesNotExist, AttributeError) as exc:
            logger.error(
                'Failed to retrieve notification count for user %s: %s',
                request.user.id,
                str(exc),
            )
            return Response(
                {'error': 'Failed to retrieve notification count'},
                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
            )

openedx/core/djangoapps/notifications/views.py:266

• Notification.objects.filter(...).update(...) will not raise Notification.DoesNotExist, so this exception list is misleading/dead. Consider removing Notification.DoesNotExist here and only catching exceptions you can meaningfully handle (or let DRF propagate unexpected errors).

        except (Notification.DoesNotExist, AttributeError, TypeError) as exc:
            logger.error(
                'Failed to mark notifications seen for user %s: %s',
                request.user.id,
                str(exc),
            )
            return Response(
                {'error': _('Failed to mark notifications as seen.')},
                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
            )

openedx/core/djangoapps/notifications/views.py:379

• This except block includes Notification.DoesNotExist, but the operations in the try (get_object_or_404, filter().update()) won’t raise that exception (404s come from Http404). Consider removing Notification.DoesNotExist here and only handling the exceptions you expect, to avoid masking real bugs behind a generic 500.

        except (Notification.DoesNotExist, AttributeError, TypeError) as exc:
            logger.error(
                'Failed to mark notification as read for user %s: %s',
                request.user.id,
                str(exc),
            )
            return Response(
                {'error': _('Failed to mark notification as read.')},
                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
            )

openedx/core/djangoapps/notifications/views.py:133

• Notification.objects.filter(...).order_by(...) will never raise Notification.DoesNotExist, so this except Notification.DoesNotExist block is effectively dead code. Consider removing the try/except (or catching and handling the specific exceptions that can actually occur here).

        except Notification.DoesNotExist as exc:
            logger.error(
                'Failed to retrieve notifications for user %s: %s',
                self.request.user.id,
                str(exc)
            )
            raise

openedx/core/djangoapps/notifications/views.py:235

• This warning log uses an f-string, while the rest of the file uses parameterized logging (logger.warning('...', arg)). Using parameterized logging avoids eager string formatting when the log level is disabled and keeps formatting consistent.

                logger.warning(
                    f'Invalid app_name provided by user {request.user.id}'
                )

openedx/core/djangoapps/notifications/views.py:540

• NotificationPreference.objects.filter(...) will not raise NotificationPreference.DoesNotExist, so including it in this exception tuple is misleading. Consider removing ...DoesNotExist here and only catching exceptions you intend to handle (or let DRF surface unexpected errors for easier debugging).

        except (NotificationPreference.DoesNotExist, KeyError, AttributeError, TypeError) as exc:
            logger.error(
                'Failed to retrieve notification preferences for user %s: %s',
                request.user.id,
                str(exc),
            )
            return Response({
                'status': 'error',
                'message': 'Failed to retrieve notification preferences.'
            }, status=status.HTTP_500_INTERNAL_SERVER_ERROR)

openedx/core/djangoapps/notifications/views.py:612

• Similar to the GET handler: filter(...).update(...) won’t raise NotificationPreference.DoesNotExist, so this exception tuple is misleading. Consider removing ...DoesNotExist and narrowing exception handling to cases that can actually occur (or rely on DRF’s default error handling).

        except (NotificationPreference.DoesNotExist, KeyError, AttributeError, TypeError) as exc:
            logger.error(
                'Failed to update notification preferences for user %s: %s',
                request.user.id,
                str(exc),
            )
            return Response({
                'status': 'error',
                'message': 'Failed to update notification preferences.'
            }, status=status.HTTP_500_INTERNAL_SERVER_ERROR)

---

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

[asharma4-sonata]

The second commit should use chore: for example: "chore: added a trailing newline to resolve the quality check requirement" and not feat: since it doesn’t introduce any new functionality. I’d also recommend keeping the message shorter, such as chore: enabled quality check pipeline to succeed.

Also, please address the Copilot errors, as well as the special characters appearing in a few lines of code.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (5)

openedx/core/djangoapps/notifications/views.py:136

• Notification.objects.filter(...).order_by(...) will never raise Notification.DoesNotExist (that exception is only raised by .get()). The try/except Notification.DoesNotExist block is therefore dead code and can give a false sense of error handling. Consider removing this except or, if you want to handle unexpected DB issues, catch a more appropriate exception (e.g., DatabaseError) and return/log accordingly.

        try:
            expiry_date = datetime.now(UTC) - timedelta(
                days=settings.NOTIFICATIONS_EXPIRY
            )
            app_name = self.request.query_params.get('app_name')

            if self.request.query_params.get('tray_opened'):
                unseen_count = Notification.objects.filter(
                    user_id=self.request.user,
                    last_seen__isnull=True
                ).count()
                notification_tray_opened_event(
                    self.request.user,
                    unseen_count
                )
            params = {
                'user': self.request.user,
                'created__gte': expiry_date,
                'web': True
            }

            if app_name:
                params['app_name'] = app_name
            queryset = Notification.objects.filter(
                **params
            ).order_by('-created')
            logger.info(
                'Retrieved notifications for user %s with app_name=%s',
                self.request.user.id,
                app_name,
            )
            return queryset
        except Notification.DoesNotExist as exc:
            logger.error(
                'Failed to retrieve notifications for user %s: %s',
                self.request.user.id,
                str(exc)
            )
            raise

openedx/core/djangoapps/notifications/views.py:238

• Avoid f-strings in logger calls; they eagerly format the message even when the log level is disabled. Prefer parameterized logging (e.g., with %s) for consistency with the rest of this file and to avoid unnecessary string interpolation.

                logger.warning(
                    f'Invalid app_name provided by user {request.user.id}'
                )

openedx/core/djangoapps/notifications/views.py:324

• notification_id is taken directly from request.data and then passed as pk into get_object_or_404. If a client sends a non-integer value (e.g., 'abc'), Django will raise ValueError during PK coercion, which will currently bubble up as a 500 (and is not caught by the current except). Consider validating/casting notification_id (or using a serializer) and returning a 400 for invalid types; alternatively include ValueError in the handled exceptions with an appropriate 400 response.

        notification_id = request.data.get('notification_id')
        app_name = request.data.get('app_name')

        # Require at least one identifier.
        if not notification_id and not app_name:
            logger.warning(
                'Invalid app_name (%s) or notification_id (%s) from user %s',
                app_name,
                notification_id,
                request.user.id,
            )
            return Response(
                {'error': _('Invalid app_name or notification_id.')},
                status=status.HTTP_400_BAD_REQUEST,
            )

        read_at = datetime.now(UTC)

        try:
            # If notification_id is provided, it takes precedence
            # over app_name.
            if notification_id:
                notification = get_object_or_404(
                    Notification,
                    pk=notification_id,
                    user=request.user,
                )

openedx/core/djangoapps/notifications/views.py:685

• This except (KeyError, AttributeError, TypeError, ValueError) block logs user.id, but AttributeError is explicitly caught—if user lacks id, the logger call can raise another AttributeError and mask the original error. Log with a safe lookup (e.g., getattr(user, 'id', None)) in both the success and error paths, or avoid catching AttributeError here.

            logger.debug(
                'Logged preference update events for user %s',
                user.id
            )
        except (KeyError, AttributeError, TypeError, ValueError) as exc:
            logger.error(
                'Failed to log preference update events for user %s: %s',
                user.id,
                str(exc)
            )

openedx/core/djangoapps/notifications/views.py:437

• The new error branches here (invalid encrypted token -> 400, user not found -> 404, bad request -> 400) aren’t covered by the existing UpdatePreferenceFromEncryptedDataView tests, which currently only exercise success and rate limiting. Please add tests for these new response paths to prevent regressions and to lock in the intended status code mapping.

    try:
        update_user_preferences_from_patch(username)
        logger.info(
            "Updated preferences from one-click unsubscribe request"
        )
        return Response({"result": "success"}, status=status.HTTP_200_OK)
    except UsernameDecryptionException:
        logger.warning(
            "Invalid encrypted username token in one-click unsubscribe request"
        )
        return Response(
            {"error": "Invalid token"},
            status=status.HTTP_400_BAD_REQUEST,
        )
    except Http404:
        logger.warning(
            "User not found for one-click unsubscribe request"
        )
        return Response(
            {"error": "User not found"},
            status=status.HTTP_404_NOT_FOUND,
        )
    except BadRequest as exc:
        logger.warning(
            "Bad request in one-click unsubscribe: %s",
            str(exc),
        )
        return Response(
            {"error": "Bad request"},
            status=status.HTTP_400_BAD_REQUEST,
        )

---

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

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (8)

openedx/core/djangoapps/notifications/views.py:437

• There are existing view tests under openedx/core/djangoapps/notifications/tests/test_views.py, but they currently only cover the success and 429 paths for this one-click unsubscribe view. Since this PR adds new response behaviors for invalid tokens (400) and missing users (404), it would be good to add/extend tests to assert those status codes and payloads so regressions don’t slip in.

    try:
        update_user_preferences_from_patch(username)
        logger.info(
            "Updated preferences from one-click unsubscribe request"
        )
        return Response({"result": "success"}, status=status.HTTP_200_OK)
    except UsernameDecryptionException:
        logger.warning(
            "Invalid encrypted username token in one-click unsubscribe request"
        )
        return Response(
            {"error": "Invalid token"},
            status=status.HTTP_400_BAD_REQUEST,
        )
    except Http404:
        logger.warning(
            "User not found for one-click unsubscribe request"
        )
        return Response(
            {"error": "User not found"},
            status=status.HTTP_404_NOT_FOUND,
        )
    except BadRequest as exc:
        logger.warning(
            "Bad request in one-click unsubscribe: %s",
            str(exc),
        )
        return Response(
            {"error": "Bad request"},
            status=status.HTTP_400_BAD_REQUEST,
        )

openedx/core/djangoapps/notifications/views.py:548

• NotificationPreference.DoesNotExist won’t be raised by the operations in this try block (filter, bulk_create, dict access). Catching it here is misleading and makes it harder to reason about what failures are actually expected vs. truly exceptional. Consider removing NotificationPreference.DoesNotExist from the caught exceptions (or narrowing the try/except to only the statements that can raise the listed exceptions).

        except (NotificationPreference.DoesNotExist, KeyError, AttributeError, TypeError) as exc:
            logger.error(

openedx/core/djangoapps/notifications/views.py:173

• This query uses user_id=request.user (and similarly user_id=self.request.user above). Elsewhere in this module the code consistently filters with user=request.user. For clarity/consistency (and to avoid any ambiguity around _id lookups), prefer user=request.user or user_id=request.user.id here.

                Notification.objects
                .filter(
                    user_id=request.user,
                    last_seen__isnull=True,
                    web=True
                )

openedx/core/djangoapps/notifications/views.py:238

• This log line uses an f-string. The rest of the file uses parameterized logging (logger.info('...', arg1, arg2)), which avoids eager string formatting and is the standard pattern for structured logs. Consider switching this to parameterized logging for consistency.

                logger.warning(
                    f'Invalid app_name provided by user {request.user.id}'
                )

openedx/core/djangoapps/notifications/views.py:266

• Notification.DoesNotExist won’t be raised by filter(...).update(...), so including it in this except tuple is misleading. If the intent is to handle unexpected failures, consider catching Exception (or removing the try/except entirely) and using logger.exception(...) to capture a traceback before returning a 500.

        except (Notification.DoesNotExist, AttributeError, TypeError) as exc:
            logger.error(
                'Failed to mark notifications seen for user %s: %s',
                getattr(request.user, 'id', None),
                str(exc),
            )
            return Response(

openedx/core/djangoapps/notifications/views.py:490

• user_preferences_map is populated from the queryset and then immediately filled with any missing types (and bulk-created). After that, if not user_preferences_map: is effectively unreachable in normal operation and adds confusing/contradictory behavior (auto-create defaults vs. return 404 for “no preferences”). Consider removing this check, or moving it before the missing-types creation if you truly want a 404 when the user has no saved prefs.

            # Ensure all notification types present in user's preferences.
            diff = set(COURSE_NOTIFICATION_TYPES.keys()) - set(
                user_preferences_map.keys()
            )
            missing_types = []
            for missing_type in diff:
                new_pref = create_notification_preference(
                    user_id=request.user.id,
                    notification_type=missing_type,
                )
                missing_types.append(new_pref)
                user_preferences_map[missing_type] = new_pref
            if missing_types:
                NotificationPreference.objects.bulk_create(missing_types)
                logger.info(
                    'Created %d missing notification preferences for user %s',
                    len(missing_types),
                    request.user.id
                )

            # If no user preferences found, return error response.
            if not user_preferences_map:
                logger.warning(
                    'No active notification preferences for user %s',
                    request.user.id
                )
                return Response({
                    'status': 'error',
                    'message': 'No active notification preferences found.'
                }, status=status.HTTP_404_NOT_FOUND)

openedx/core/djangoapps/notifications/views.py:620

• NotificationPreference.DoesNotExist is unlikely to be raised here since this method uses filter(...).update(...) rather than get(). Removing that exception type (and narrowing the try/except scope) would make the error handling clearer and avoid implying a failure mode that can’t happen.

        except (NotificationPreference.DoesNotExist, KeyError, AttributeError, TypeError) as exc:
            logger.error(

openedx/core/djangoapps/notifications/views.py:136

• Notification.objects.filter(...) / .order_by(...) will never raise Notification.DoesNotExist, so this except Notification.DoesNotExist block is effectively dead code and won’t catch real failures. Consider removing it, or if you want to log unexpected failures here, catch Exception and use logger.exception(...) before re-raising so you get a traceback in logs.

        try:
            expiry_date = datetime.now(UTC) - timedelta(
                days=settings.NOTIFICATIONS_EXPIRY
            )
            app_name = self.request.query_params.get('app_name')

            if self.request.query_params.get('tray_opened'):
                unseen_count = Notification.objects.filter(
                    user_id=self.request.user,
                    last_seen__isnull=True
                ).count()
                notification_tray_opened_event(
                    self.request.user,
                    unseen_count
                )
            params = {
                'user': self.request.user,
                'created__gte': expiry_date,
                'web': True
            }

            if app_name:
                params['app_name'] = app_name
            queryset = Notification.objects.filter(
                **params
            ).order_by('-created')
            logger.info(
                'Retrieved notifications for user %s with app_name=%s',
                self.request.user.id,
                app_name,
            )
            return queryset
        except Notification.DoesNotExist as exc:
            logger.error(
                'Failed to retrieve notifications for user %s: %s',
                self.request.user.id,
                str(exc)
            )
            raise

---

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

[asharma4-sonata]

Please make minimal functional changes to the original code first so it can be reviewed. After that, we can apply auto-formatting. Otherwise, formatting changes make it difficult to see what was modified

[asharma4-sonata]

You can delete this PR, revert the formatting commits, make the functional changes first, and then open a new PR.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

---

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

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

---

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