From 32a0a2d29dbf60713d355fc51f9a6e296af878de Mon Sep 17 00:00:00 2001 From: Jean Manuel Nater Date: Mon, 24 Jun 2013 11:13:59 -0400 Subject: [PATCH] In the middle of addressing pull request comments. This is a safety commit in case I have to revert some changes I'm about to make. --- .../xmodule/modulestore/tests/django_utils.py | 8 ++- .../xmodule/modulestore/tests/factories.py | 8 +-- .../courseware/tests/check_request_code.py | 24 ------- lms/djangoapps/courseware/tests/helpers.py | 33 ++++----- .../courseware/tests/modulestore_config.py | 16 +++-- .../courseware/tests/test_navigation.py | 16 +++-- .../tests/test_view_authentication.py | 72 +++++++++++++------ 7 files changed, 98 insertions(+), 79 deletions(-) delete mode 100644 lms/djangoapps/courseware/tests/check_request_code.py diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 944b9e5bd4..a2306a5c6b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -14,10 +14,16 @@ class ModuleStoreTestCase(TestCase): collection with templates before running the TestCase and drops it they are finished. """ - def update_course(self, course, data): + @staticmethod + def update_course(course, data): """ Updates the version of course in the mongo modulestore with the metadata in data and returns the updated version. + + 'course' is an instance of CourseDescriptor for which we want + to update metadata. + + 'data' is a dictionary with an entry for each CourseField we want to update. """ store = xmodule.modulestore.django.modulestore() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 4f63fbc1d2..82ff61204a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -60,8 +60,8 @@ class XModuleCourseFactory(Factory): if data is not None: store.update_item(new_course.location, data) - '''update_item updates the the course as it exists in the modulestore, but doesn't - update the instance we are working with, so have to refetch the course after updating it.''' + #update_item updates the the course as it exists in the modulestore, but doesn't + #update the instance we are working with, so have to refetch the course after updating it. new_course = store.get_instance(new_course.id, new_course.location) return new_course @@ -152,8 +152,8 @@ class XModuleItemFactory(Factory): if new_item.location.category not in DETACHED_CATEGORIES: store.update_children(parent_location, parent.children + [new_item.location.url()]) - '''update_children updates the the item as it exists in the modulestore, but doesn't - update the instance we are working with, so have to refetch the item after updating it.''' + #update_children updates the the item as it exists in the modulestore, but doesn't + #update the instance we are working with, so have to refetch the item after updating it. new_item = store.get_item(new_item.location) return new_item diff --git a/lms/djangoapps/courseware/tests/check_request_code.py b/lms/djangoapps/courseware/tests/check_request_code.py deleted file mode 100644 index 1393d2fe17..0000000000 --- a/lms/djangoapps/courseware/tests/check_request_code.py +++ /dev/null @@ -1,24 +0,0 @@ - - -def check_for_get_code(code, url): - """ - Check that we got the expected code when accessing url via GET. - Returns the response. - """ - resp = self.client.get(url) - self.assertEqual(resp.status_code, code, - "got code %d for url '%s'. Expected code %d" - % (resp.status_code, url, code)) - return resp - - -def check_for_post_code(code, url, data={}): - """ - Check that we got the expected code when accessing url via POST. - Returns the response. - """ - resp = self.client.post(url, data) - self.assertEqual(resp.status_code, code, - "got code %d for url '%s'. Expected code %d" - % (resp.status_code, url, code)) - return resp diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index 99da5e9061..ce0603990b 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -12,7 +12,12 @@ def check_for_get_code(self, code, url): """ Check that we got the expected code when accessing url via GET. Returns the HTTP response. - 'self' is a class that subclasses TestCase. + + `self` is a class that subclasses TestCase. + + `code` is a status code for HTTP responses. + + `url` is a url pattern for which we have to test the response. """ resp = self.client.get(url) self.assertEqual(resp.status_code, code, @@ -25,7 +30,11 @@ def check_for_post_code(self, code, url, data={}): """ Check that we got the expected code when accessing url via POST. Returns the HTTP response. - 'self' is a class that subclasses TestCase. + `self` is a class that subclasses TestCase. + + `code` is a status code for HTTP responses. + + `url` is a url pattern for which we want to test the response. """ resp = self.client.post(url, data) self.assertEqual(resp.status_code, code, @@ -62,8 +71,8 @@ class LoginEnrollmentTestCase(TestCase): def logout(self): """ - Logout, check that it worked. - Returns an HTTP response which e + Logout; check that the HTTP response code indicates redirection + as expected. """ resp = self.client.get(reverse('logout'), {}) # should redirect @@ -106,8 +115,8 @@ class LoginEnrollmentTestCase(TestCase): def enroll(self, course, verify=False): """ Try to enroll and return boolean indicating result. - 'course' is an instance of CourseDescriptor. - 'verify' is an optional parameter specifying whether we + `course` is an instance of CourseDescriptor. + `verify` is an optional parameter specifying whether we want to verify that the student was successfully enrolled in the course. """ @@ -122,20 +131,12 @@ class LoginEnrollmentTestCase(TestCase): self.assertTrue(result) return result - # def enroll(self, course): - # """ - # Enroll the currently logged-in user, and check that it worked. - # """ - - # result = self.try_enroll(course) - # self.assertTrue(result) - def unenroll(self, course): """ Unenroll the currently logged-in user, and check that it worked. - 'course' is an instance of CourseDescriptor. + `course` is an instance of CourseDescriptor. """ - resp = self.client.post('/change_enrollment', { + resp = self.client.post(reverse('change_enrollment'), { 'enrollment_action': 'unenroll', 'course_id': course.id, }) diff --git a/lms/djangoapps/courseware/tests/modulestore_config.py b/lms/djangoapps/courseware/tests/modulestore_config.py index 81d0f4f911..c3c4ce4e5b 100644 --- a/lms/djangoapps/courseware/tests/modulestore_config.py +++ b/lms/djangoapps/courseware/tests/modulestore_config.py @@ -4,11 +4,11 @@ from django.conf import settings def mongo_store_config(data_dir): - ''' - Defines default module store using MongoModuleStore + """ + Defines default module store using MongoModuleStore. - Use of this config requires mongo to be running - ''' + Use of this config requires mongo to be running. + """ store = { 'default': { 'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore', @@ -27,7 +27,9 @@ def mongo_store_config(data_dir): def draft_mongo_store_config(data_dir): - '''Defines default module store using DraftMongoModuleStore''' + """ + Defines default module store using DraftMongoModuleStore. + """ return { 'default': { 'ENGINE': 'xmodule.modulestore.mongo.DraftMongoModuleStore', @@ -55,7 +57,9 @@ def draft_mongo_store_config(data_dir): def xml_store_config(data_dir): - '''Defines default module store using XMLModuleStore''' + """ + Defines default module store using XMLModuleStore. + """ return { 'default': { 'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore', diff --git a/lms/djangoapps/courseware/tests/test_navigation.py b/lms/djangoapps/courseware/tests/test_navigation.py index 9f9bf7ba92..f4662f2ef5 100644 --- a/lms/djangoapps/courseware/tests/test_navigation.py +++ b/lms/djangoapps/courseware/tests/test_navigation.py @@ -36,15 +36,18 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase): # Create student accounts and activate them. for i in range(len(self.STUDENT_INFO)): - self.create_account('u{0}'.format(i), self.STUDENT_INFO[i][0], self.STUDENT_INFO[i][1]) - self.activate_user(self.STUDENT_INFO[i][0]) + email, password = self.STUDENT_INFO[i] + username = 'u{0}'.format(i) + self.create_account(username, email, password) + self.activate_user(email) def test_redirects_first_time(self): """ Verify that the first time we click on the courseware tab we are redirected to the 'Welcome' section. """ - self.login(self.STUDENT_INFO[0][0], self.STUDENT_INFO[0][1]) + email, password = self.STUDENT_INFO[0] + self.login(email, password) self.enroll(self.course, True) self.enroll(self.full, True) @@ -61,7 +64,8 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase): Verify the accordion remembers we've already visited the Welcome section and redirects correpondingly. """ - self.login(self.STUDENT_INFO[0][0], self.STUDENT_INFO[0][1]) + email, password = self.STUDENT_INFO[0] + self.login(email, password) self.enroll(self.course, True) self.enroll(self.full, True) @@ -80,8 +84,8 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase): """ Verify the accordion remembers which chapter you were last viewing. """ - - self.login(self.STUDENT_INFO[0][0], self.STUDENT_INFO[0][1]) + email, password = self.STUDENT_INFO[0] + self.login(email, password) self.enroll(self.course, True) self.enroll(self.full, True) diff --git a/lms/djangoapps/courseware/tests/test_view_authentication.py b/lms/djangoapps/courseware/tests/test_view_authentication.py index ffae4688bf..5db9847d45 100644 --- a/lms/djangoapps/courseware/tests/test_view_authentication.py +++ b/lms/djangoapps/courseware/tests/test_view_authentication.py @@ -32,28 +32,40 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): @classmethod def _instructor_urls(self, course): """ - List of urls that only instructors/staff should be able to see. + `course` is an instance of CourseDescriptor whose section URLs are to be returned. + + Returns a list of URLs corresponding to sections in the passed in course. """ + urls = [reverse(name, kwargs={'course_id': course.id}) for name in ( 'instructor_dashboard', 'gradebook', 'grade_summary',)] + email, _ = self.ACCOUNT_INFO[0] + student_id = User.objects.get(email=email).id + urls.append(reverse('student_progress', kwargs={'course_id': course.id, - 'student_id': User.objects.get(email=self.ACCOUNT_INFO[0][0]).id})) + 'student_id': student_id})) return urls @staticmethod def _reverse_urls(names, course): """ Reverse a list of course urls. + + `names` is a list of URL names that correspond to sections in a course. + + `course` is the instance of CourseDescriptor whose section URLs are to be returned. + + Returns a list URLs corresponding to section in the passed in course. + """ return [reverse(name, kwargs={'course_id': course.id}) for name in names] def setUp(self): - xmodule.modulestore.django._MODULESTORES = {} self.full = CourseFactory.create(number='666', display_name='Robot_Sub_Course') self.course = CourseFactory.create() @@ -68,8 +80,9 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): # Create two accounts and activate them. for i in range(len(self.ACCOUNT_INFO)): - self.create_account('u{0}'.format(i), self.ACCOUNT_INFO[i][0], self.ACCOUNT_INFO[i][1]) - self.activate_user(self.ACCOUNT_INFO[i][0]) + username, email, password = 'u{0}'.format(i), self.ACCOUNT_INFO[i][0], self.ACCOUNT_INFO[i][1] + self.create_account(username, email, password) + self.activate_user(email) def test_redirection_unenrolled(self): """ @@ -77,7 +90,8 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): instead of the 'Welcome' section after clicking on the courseware tab. """ - self.login(self.ACCOUNT_INFO[0][0], self.ACCOUNT_INFO[0][1]) + email, password = self.ACCOUNT_INFO[0] + self.login(email, password) response = self.client.get(reverse('courseware', kwargs={'course_id': self.course.id})) self.assertRedirects(response, @@ -90,7 +104,8 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): the chapter after clicking on the courseware tab. """ - self.login(self.ACCOUNT_INFO[0][0], self.ACCOUNT_INFO[0][1]) + email, password = self.ACCOUNT_INFO[0] + self.login(email, password) self.enroll(self.course) response = self.client.get(reverse('courseware', @@ -108,7 +123,8 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): dashboard, the grade views, and student profile pages. """ - self.login(self.ACCOUNT_INFO[0][0], self.ACCOUNT_INFO[0][1]) + email, password = self.ACCOUNT_INFO[0] + self.login(email, password) self.enroll(self.course) self.enroll(self.full) @@ -127,12 +143,14 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): and student profile pages for their course. """ + email, password = self.ACCOUNT_INFO[1] + # Make the instructor staff in self.course group_name = _course_staff_group_name(self.course.location) group = Group.objects.create(name=group_name) - group.user_set.add(User.objects.get(email=self.ACCOUNT_INFO[1][0])) + group.user_set.add(User.objects.get(email=email)) - self.login(self.ACCOUNT_INFO[1][0], self.ACCOUNT_INFO[1][1]) + self.login(email, password) # Now should be able to get to self.course, but not self.full url = random.choice(self._instructor_urls(self.course)) @@ -149,10 +167,11 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): staff permissions. """ - self.login(self.ACCOUNT_INFO[1][0], self.ACCOUNT_INFO[1][1]) + email, password = self.ACCOUNT_INFO[1] + self.login(email, password) # now make the instructor also staff - instructor = User.objects.get(email=self.ACCOUNT_INFO[1][0]) + instructor = User.objects.get(email=email) instructor.is_staff = True instructor.save() @@ -202,6 +221,9 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): Actually do the test, relying on settings to be right. """ + student_email, student_password = self.ACCOUNT_INFO[0] + instructor_email, instructor_password = self.ACCOUNT_INFO[1] + # Make courses start in the future now = datetime.datetime.now(pytz.UTC) tomorrow = now + datetime.timedelta(days=1) @@ -300,7 +322,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): # First, try with an enrolled student print '=== Testing student access....' - self.login(self.ACCOUNT_INFO[0][0], self.ACCOUNT_INFO[0][1]) + self.login(student_email, student_password) self.enroll(self.course, True) self.enroll(self.full, True) @@ -314,10 +336,10 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): # Make the instructor staff in self.course group_name = _course_staff_group_name(self.course.location) group = Group.objects.create(name=group_name) - group.user_set.add(User.objects.get(email=self.ACCOUNT_INFO[1][0])) + group.user_set.add(User.objects.get(email=instructor_email)) self.logout() - self.login(self.ACCOUNT_INFO[1][0], self.ACCOUNT_INFO[1][1]) + self.login(instructor_email, instructor_password) # Enroll in the classes---can't see courseware otherwise. self.enroll(self.course, True) self.enroll(self.full, True) @@ -329,7 +351,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): print '=== Testing staff access....' # now also make the instructor staff - instructor = User.objects.get(email=self.ACCOUNT_INFO[1][0]) + instructor = User.objects.get(email=instructor_email) instructor.is_staff = True instructor.save() @@ -342,6 +364,9 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): Actually do the test, relying on settings to be right. """ + student_email, student_password = self.ACCOUNT_INFO[0] + instructor_email, instructor_password = self.ACCOUNT_INFO[1] + # Make courses start in the future now = datetime.datetime.now(pytz.UTC) tomorrow = now + datetime.timedelta(days=1) @@ -360,7 +385,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): print "login" # First, try with an enrolled student print '=== Testing student access....' - self.login(self.ACCOUNT_INFO[0][0], self.ACCOUNT_INFO[0][1]) + self.login(student_email, student_password) self.assertFalse(self.enroll(self.course)) self.assertTrue(self.enroll(self.full)) @@ -368,18 +393,18 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): # Make the instructor staff in the self.course group_name = _course_staff_group_name(self.course.location) group = Group.objects.create(name=group_name) - group.user_set.add(User.objects.get(email=self.ACCOUNT_INFO[1][0])) + group.user_set.add(User.objects.get(email=instructor_email)) print "logout/login" self.logout() - self.login(self.ACCOUNT_INFO[1][0], self.ACCOUNT_INFO[1][1]) + self.login(instructor_email, instructor_password) print "Instructor should be able to enroll in self.course" self.assertTrue(self.enroll(self.course)) print '=== Testing staff access....' # now make the instructor global staff, but not in the instructor group - group.user_set.remove(User.objects.get(email=self.ACCOUNT_INFO[1][0])) - instructor = User.objects.get(email=self.ACCOUNT_INFO[1][0]) + group.user_set.remove(User.objects.get(email=instructor_email)) + instructor = User.objects.get(email=instructor_email) instructor.is_staff = True instructor.save() @@ -392,6 +417,9 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): Actually test beta periods, relying on settings to be right. """ + student_email, student_password = self.ACCOUNT_INFO[0] + instructor_email, instructor_password = self.ACCOUNT_INFO[1] + # trust, but verify :) self.assertFalse(settings.MITX_FEATURES['DISABLE_START_DATES']) @@ -408,7 +436,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): self.course.lms.days_early_for_beta = 2 # student user shouldn't see it - student_user = User.objects.get(email=self.ACCOUNT_INFO[0][0]) + student_user = User.objects.get(email=student_email) self.assertFalse(has_access(student_user, self.course, 'load')) # now add the student to the beta test group