From 7e984342180ff0e0998a90157f386b39862e0e66 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Oct 2014 14:21:09 -0400 Subject: [PATCH 1/2] Make notifier API resilient to bad enrollments Previously, the user endpoint would would return 404 for any request involving a user having an enrollment for a course that no longer existed. --- lms/djangoapps/notifier_api/serializers.py | 25 +++++++++++++--------- lms/djangoapps/notifier_api/tests.py | 5 +++++ 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/notifier_api/serializers.py b/lms/djangoapps/notifier_api/serializers.py index db145768bf..5164ad812f 100644 --- a/lms/djangoapps/notifier_api/serializers.py +++ b/lms/djangoapps/notifier_api/serializers.py @@ -1,4 +1,5 @@ from django.contrib.auth.models import User +from django.http import Http404 from rest_framework import serializers from course_groups.cohorts import is_course_cohorted @@ -47,16 +48,20 @@ class NotifierUserSerializer(serializers.ModelSerializer): for role in user.roles.all() for perm in role.permissions.all() if perm.name == "see_all_cohorts" } - return { - unicode(enrollment.course_id): { - "cohort_id": cohort_id_map.get(enrollment.course_id), - "see_all_cohorts": ( - enrollment.course_id in see_all_cohorts_set or - not is_course_cohorted(enrollment.course_id) - ), - } - for enrollment in user.courseenrollment_set.all() if enrollment.is_active - } + ret = {} + for enrollment in user.courseenrollment_set.all(): + if enrollment.is_active: + try: + ret[unicode(enrollment.course_id)] = { + "cohort_id": cohort_id_map.get(enrollment.course_id), + "see_all_cohorts": ( + enrollment.course_id in see_all_cohorts_set or + not is_course_cohorted(enrollment.course_id) + ), + } + except Http404: # is_course_cohorted raises this if course does not exist + pass + return ret class Meta: model = User diff --git a/lms/djangoapps/notifier_api/tests.py b/lms/djangoapps/notifier_api/tests.py index d741a4f940..9bdf8990ae 100644 --- a/lms/djangoapps/notifier_api/tests.py +++ b/lms/djangoapps/notifier_api/tests.py @@ -120,6 +120,11 @@ class NotifierUsersViewSetTest(UrlResetMixin, ModuleStoreTestCase): result = self._get_detail() self.assertEqual(result["course_info"], {}) + def test_course_info_non_existent_course_enrollment(self): + CourseEnrollmentFactory(user=self.user) + result = self._get_detail() + self.assertEqual(result["course_info"], {}) + def test_preferences(self): lang_pref = UserPreferenceFactory( user=self.user, From cff8e16de380e73ccae6b003f7cd78cb35809972 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 3 Oct 2014 14:40:37 -0400 Subject: [PATCH 2/2] fixup! Make notifier API resilient to bad enrollments Add explicit course key to enrollment for non-existent course in test case --- lms/djangoapps/notifier_api/tests.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/notifier_api/tests.py b/lms/djangoapps/notifier_api/tests.py index 9bdf8990ae..69733ae7ad 100644 --- a/lms/djangoapps/notifier_api/tests.py +++ b/lms/djangoapps/notifier_api/tests.py @@ -10,6 +10,7 @@ from django_comment_common.models import Role, Permission from lang_pref import LANGUAGE_KEY from notification_prefs import NOTIFICATION_PREF_KEY from notifier_api.views import NotifierUsersViewSet +from opaque_keys.edx.locator import CourseLocator from student.models import CourseEnrollment from student.tests.factories import UserFactory, CourseEnrollmentFactory from user_api.models import UserPreference @@ -121,7 +122,10 @@ class NotifierUsersViewSetTest(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(result["course_info"], {}) def test_course_info_non_existent_course_enrollment(self): - CourseEnrollmentFactory(user=self.user) + CourseEnrollmentFactory( + user=self.user, + course_id=CourseLocator(org="dummy", course="dummy", run="non_existent") + ) result = self._get_detail() self.assertEqual(result["course_info"], {})