From 85565a3de8e2f6da02148a909578e854b1a803c4 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 29 May 2014 13:15:47 -0400 Subject: [PATCH] lms dashboard ignores errored courses --- .../contentstore/tests/test_course_listing.py | 1 - .../student/tests/test_course_listing.py | 118 ++++++++++++++++++ common/djangoapps/student/views.py | 8 +- 3 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 common/djangoapps/student/tests/test_course_listing.py diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index d65c94997d..9bcdeb707b 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -14,7 +14,6 @@ from contentstore.utils import delete_course_and_groups, reverse_course_url from contentstore.tests.utils import AjaxEnabledTestClient from student.tests.factories import UserFactory from student.roles import CourseInstructorRole, CourseStaffRole, GlobalStaff -from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.locations import SlashSeparatedCourseKey diff --git a/common/djangoapps/student/tests/test_course_listing.py b/common/djangoapps/student/tests/test_course_listing.py new file mode 100644 index 0000000000..6decc8cbe2 --- /dev/null +++ b/common/djangoapps/student/tests/test_course_listing.py @@ -0,0 +1,118 @@ +""" +Unit tests for getting the list of courses for a user through iterating all courses and +by reversing group name formats. +""" +from mock import patch, Mock + +from student.tests.factories import UserFactory +from student.roles import GlobalStaff +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, studio_store_config +from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.locations import SlashSeparatedCourseKey +from xmodule.modulestore.django import modulestore +from xmodule.error_module import ErrorDescriptor +from django.test.client import Client +from student.models import CourseEnrollment +from student.views import get_course_enrollment_pairs +from django.conf import settings +from django.test.utils import override_settings + +TEST_MODULESTORE = studio_store_config(settings.TEST_ROOT / "data") + + +@override_settings(MODULESTORE=TEST_MODULESTORE) +class TestCourseListing(ModuleStoreTestCase): + """ + Unit tests for getting the list of courses for a logged in user + """ + def setUp(self): + """ + Add a student & teacher + """ + super(TestCourseListing, self).setUp() + + self.student = UserFactory() + self.teacher = UserFactory() + GlobalStaff().add_users(self.teacher) + self.client = Client() + self.client.login(username=self.teacher.username, password='test') + + def _create_course_with_access_groups(self, course_location): + """ + Create dummy course with 'CourseFactory' and enroll the student + """ + course = CourseFactory.create( + org=course_location.org, + number=course_location.course, + run=course_location.run, + modulestore=modulestore('direct'), + ) + + CourseEnrollment.enroll(self.student, course.id) + + return course + + def tearDown(self): + """ + Reverse the setup + """ + self.client.logout() + super(TestCourseListing, self).tearDown() + + def test_get_course_list(self): + """ + Test getting courses + """ + course_location = SlashSeparatedCourseKey('Org1', 'Course1', 'Run1') + self._create_course_with_access_groups(course_location) + + # get dashboard + courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + self.assertEqual(len(courses_list), 1) + self.assertEqual(courses_list[0][0].id, course_location) + + CourseEnrollment.unenroll(self.student, course_location) + # get dashboard + courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + self.assertEqual(len(courses_list), 0) + + def test_errored_course_regular_access(self): + """ + Test the course list for regular staff when get_course returns an ErrorDescriptor + """ + course_key = SlashSeparatedCourseKey('Org1', 'Course1', 'Run1') + self._create_course_with_access_groups(course_key) + + with patch('xmodule.modulestore.mongo.base.MongoKeyValueStore', Mock(side_effect=Exception)): + self.assertIsInstance(modulestore('direct').get_course(course_key), ErrorDescriptor) + + # get courses through iterating all courses + courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + self.assertEqual(courses_list, []) + + def test_course_listing_errored_deleted_courses(self): + """ + Create good courses, courses that won't load, and deleted courses which still have + roles. Test course listing. + """ + good_location = SlashSeparatedCourseKey('testOrg', 'testCourse', 'RunBabyRun') + self._create_course_with_access_groups(good_location) + + course_location = SlashSeparatedCourseKey('testOrg', 'doomedCourse', 'RunBabyRun') + self._create_course_with_access_groups(course_location) + modulestore('direct').delete_course(course_location) + + course_location = SlashSeparatedCourseKey('testOrg', 'erroredCourse', 'RunBabyRun') + course = self._create_course_with_access_groups(course_location) + course_db_record = modulestore('direct')._find_one(course.location) + course_db_record.setdefault('metadata', {}).get('tabs', []).append({"type": "wiko", "name": "Wiki" }) + modulestore('direct').collection.update( + {'_id': course_db_record['_id']}, + {'$set': { + 'metadata.tabs': course_db_record['metadata']['tabs'], + }}, + ) + + courses_list = list(get_course_enrollment_pairs(self.student, None, [])) + self.assertEqual(len(courses_list), 1, courses_list) + self.assertEqual(courses_list[0][0].id, good_location) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 661bb9763e..97c7fcbe5d 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -85,6 +85,7 @@ from util.password_policy_validators import ( ) from third_party_auth import pipeline, provider +from xmodule.error_module import ErrorDescriptor log = logging.getLogger("edx.student") AUDIT_LOG = logging.getLogger("audit") @@ -242,7 +243,7 @@ def get_course_enrollment_pairs(user, course_org_filter, org_filter_out_set): """ for enrollment in CourseEnrollment.enrollments_for_user(user): course = course_from_id(enrollment.course_id) - if course: + if course and not isinstance(course, ErrorDescriptor): # if we are in a Microsite, then filter out anything that is not # attributed (by ORG) to that Microsite @@ -255,8 +256,9 @@ def get_course_enrollment_pairs(user, course_org_filter, org_filter_out_set): yield (course, enrollment) else: - log.error("User {0} enrolled in non-existent course {1}" - .format(user.username, enrollment.course_id)) + log.error("User {0} enrolled in {2} course {1}".format( + user.username, enrollment.course_id, "broken" if course else "non-existent" + )) def _cert_info(user, course, cert_status):