Addressed pull request feedback

common/djangoapps/student/views.py

@@ -357,7 +357,6 @@ def change_enrollment(request):
         return HttpResponseBadRequest("Course id not specified")
 
     if action == "enroll":
-
         # Make sure the course exists
         # We don't do this check on unenroll, or a bad course id can't be unenrolled from
         try:
@@ -366,6 +365,7 @@ def change_enrollment(request):
             log.warning("User {0} tried to enroll in non-existent course {1}"
                         .format(user.username, course_id))
             return HttpResponseBadRequest("Course id is invalid")
+
         if not has_access(user, course, 'enroll'):
             return HttpResponseBadRequest("Enrollment is closed")
 

common/lib/xmodule/xmodule/modulestore/tests/django_utils.py

@@ -17,30 +17,24 @@ class ModuleStoreTestCase(TestCase):
     @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.
+        Updates the version of course in the 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()
-
-        store.update_item(course.location, data)
-
         store.update_metadata(course.location, data)
-
         updated_course = store.get_instance(course.id, course.location)
-
         return updated_course
 
     @staticmethod
     def flush_mongo_except_templates():
-        '''
-        Delete everything in the module store except templates
-        '''
+        """
+        Delete everything in the module store except templates.
+        """
         modulestore = xmodule.modulestore.django.modulestore()
 
         # This query means: every item in the collection
@@ -52,11 +46,11 @@ class ModuleStoreTestCase(TestCase):
 
     @staticmethod
     def load_templates_if_necessary():
-        '''
+        """
         Load templates into the direct modulestore only if they do not already exist.
         We need the templates, because they are copied to create
-        XModules such as sections and problems
-        '''
+        XModules such as sections and problems.
+        """
         modulestore = xmodule.modulestore.django.modulestore('direct')
 
         # Count the number of templates
@@ -68,9 +62,9 @@ class ModuleStoreTestCase(TestCase):
 
     @classmethod
     def setUpClass(cls):
-        '''
-        Flush the mongo store and set up templates
-        '''
+        """
+        Flush the mongo store and set up templates.
+        """
 
         # Use a uuid to differentiate
         # the mongo collections on jenkins.
@@ -88,9 +82,9 @@ class ModuleStoreTestCase(TestCase):
 
     @classmethod
     def tearDownClass(cls):
-        '''
-        Revert to the old modulestore settings
-        '''
+        """
+        Revert to the old modulestore settings.
+        """
 
         # Clean up by dropping the collection
         modulestore = xmodule.modulestore.django.modulestore()
@@ -102,9 +96,9 @@ class ModuleStoreTestCase(TestCase):
         settings.MODULESTORE = cls.orig_modulestore
 
     def _pre_setup(self):
-        '''
-        Remove everything but the templates before each test
-        '''
+        """
+        Remove everything but the templates before each test.
+        """
 
         # Flush anything that is not a template
         ModuleStoreTestCase.flush_mongo_except_templates()
@@ -116,9 +110,9 @@ class ModuleStoreTestCase(TestCase):
         super(ModuleStoreTestCase, self)._pre_setup()
 
     def _post_teardown(self):
-        '''
-        Flush everything we created except the templates
-        '''
+        """
+        Flush everything we created except the templates.
+        """
         # Flush anything that is not a template
         ModuleStoreTestCase.flush_mongo_except_templates()
 

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

lms/djangoapps/courseware/access.py

@@ -526,7 +526,6 @@ def _adjust_start_date_for_beta_testers(user, descriptor):
         start_as_datetime = descriptor.lms.start
         delta = timedelta(descriptor.lms.days_early_for_beta)
         effective = start_as_datetime - delta
-
         # ...and back to time_struct
         return effective
 

lms/djangoapps/courseware/tests/helpers.py

@@ -48,7 +48,6 @@ class LoginEnrollmentTestCase(TestCase):
     Provides support for user creation,
     activation, login, and course enrollment.
     """
-
     def setup_user(self):
         """
         Create a user account, activate, and log in.
@@ -67,8 +66,8 @@ class LoginEnrollmentTestCase(TestCase):
         """
         Login, check that the corresponding view's response has a 200 status code.
         """
-        resp = resp = self.client.post(reverse('login'),
-                                       {'email': email, 'password': password})
+        resp = self.client.post(reverse('login'),
+                                {'email': email, 'password': password})
         self.assertEqual(resp.status_code, 200)
         data = json.loads(resp.content)
         self.assertTrue(data['success'])
@@ -78,15 +77,14 @@ class LoginEnrollmentTestCase(TestCase):
         Logout; check that the HTTP response code indicates redirection
         as expected.
         """
-        resp = self.client.get(reverse('logout'), {})
         # should redirect
-        self.assertEqual(resp.status_code, 302)
+        check_for_get_code(self, 302, reverse('logout'))
 
     def create_account(self, username, email, password):
         """
         Create the account and check that it worked.
         """
-        resp = self.client.post(reverse('create_account'), {
+        resp = check_for_post_code(self, 200, reverse('create_account'), {
             'username': username,
             'email': email,
             'password': password,
@@ -94,10 +92,8 @@ class LoginEnrollmentTestCase(TestCase):
             'terms_of_service': 'true',
             'honor_code': 'true',
         })
-        self.assertEqual(resp.status_code, 200)
         data = json.loads(resp.content)
         self.assertEqual(data['success'], True)
-
         # Check both that the user is created, and inactive
         self.assertFalse(User.objects.get(email=email).is_active)
 
@@ -107,12 +103,8 @@ class LoginEnrollmentTestCase(TestCase):
         No error checking.
         """
         activation_key = Registration.objects.get(user__email=email).activation_key
-
         # and now we try to activate
-        url = reverse('activate', kwargs={'key': activation_key})
-
-        resp = self.client.get(url)
-        self.assertEqual(resp.status_code, 200)
+        check_for_get_code(self, 200, reverse('activate', kwargs={'key': activation_key}))
         # Now make sure that the user is now actually activated
         self.assertTrue(User.objects.get(email=email).is_active)
 
@@ -128,8 +120,6 @@ class LoginEnrollmentTestCase(TestCase):
             'enrollment_action': 'enroll',
             'course_id': course.id,
         })
-        print ('Enrollment in %s result status code: %s'
-               % (course.location.url(), str(resp.status_code)))
         result = resp.status_code == 200
         if verify:
             self.assertTrue(result)
@@ -140,8 +130,5 @@ class LoginEnrollmentTestCase(TestCase):
         Unenroll the currently logged-in user, and check that it worked.
         `course` is an instance of CourseDescriptor.
         """
-        resp = self.client.post(reverse('change_enrollment'), {
-            'enrollment_action': 'unenroll',
-            'course_id': course.id,
-        })
-        self.assertEqual(resp.status_code, 200)
+        check_for_post_code(self, 200, reverse('change_enrollment'), {'enrollment_action': 'unenroll',
+                                                                      'course_id': course.id})

lms/djangoapps/courseware/tests/modulestore_config.py

@@ -16,7 +16,7 @@ def mongo_store_config(data_dir):
                 'default_class': 'xmodule.raw_module.RawDescriptor',
                 'host': 'localhost',
                 'db': 'test_xmodule',
-                'collection': 'modulestore_%s' % uuid4().hex,
+                'collection': 'modulestore',
                 'fs_root': data_dir,
                 'render_template': 'mitxmako.shortcuts.render_to_string'
             }
@@ -30,28 +30,24 @@ def draft_mongo_store_config(data_dir):
     """
     Defines default module store using DraftMongoModuleStore.
     """
+
+    modulestore_options = {
+        'default_class': 'xmodule.raw_module.RawDescriptor',
+        'host': 'localhost',
+        'db': 'xmodule',
+        'collection': 'modulestore_%s' % uuid4().hex,
+        'fs_root': data_dir,
+        'render_template': 'mitxmako.shortcuts.render_to_string'
+    }
+
     return {
         'default': {
             'ENGINE': 'xmodule.modulestore.mongo.DraftMongoModuleStore',
-            'OPTIONS': {
-                'default_class': 'xmodule.raw_module.RawDescriptor',
-                'host': 'localhost',
-                'db': 'test_xmodule',
-                'collection': 'modulestore_%s' % uuid4().hex,
-                'fs_root': data_dir,
-                'render_template': 'mitxmako.shortcuts.render_to_string',
-            }
+            'OPTIONS': modulestore_options
         },
         'direct': {
             'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
-            'OPTIONS': {
-                'default_class': 'xmodule.raw_module.RawDescriptor',
-                'host': 'localhost',
-                'db': 'test_xmodule',
-                'collection': 'modulestore_%s' % uuid4().hex,
-                'fs_root': data_dir,
-                'render_template': 'mitxmako.shortcuts.render_to_string',
-            }
+            'OPTIONS': modulestore_options
         }
     }
 

lms/djangoapps/courseware/tests/test_navigation.py

@@ -20,8 +20,8 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase):
 
     def setUp(self):
 
-        self.course = CourseFactory.create()
-        self.full = CourseFactory.create(display_name='Robot_Sub_Course')
+        self.test_course = CourseFactory.create(display_name='Robot_Sub_Course')
+        self.course = CourseFactory.create(display_name='Robot_Super_Course')
         self.chapter0 = ItemFactory.create(parent_location=self.course.location,
                                            display_name='Overview')
         self.chapter9 = ItemFactory.create(parent_location=self.course.location,
@@ -46,7 +46,7 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase):
         email, password = self.STUDENT_INFO[0]
         self.login(email, password)
         self.enroll(self.course, True)
-        self.enroll(self.full, True)
+        self.enroll(self.test_course, True)
 
         resp = self.client.get(reverse('courseware',
                                kwargs={'course_id': self.course.id}))
@@ -64,7 +64,7 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase):
         email, password = self.STUDENT_INFO[0]
         self.login(email, password)
         self.enroll(self.course, True)
-        self.enroll(self.full, True)
+        self.enroll(self.test_course, True)
 
         self.client.get(reverse('courseware_section', kwargs={'course_id': self.course.id,
                                                               'chapter': 'Overview',
@@ -84,7 +84,7 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase):
         email, password = self.STUDENT_INFO[0]
         self.login(email, password)
         self.enroll(self.course, True)
-        self.enroll(self.full, True)
+        self.enroll(self.test_course, True)
 
         # Now we directly navigate to a section in a chapter other than 'Overview'.
         check_for_get_code(self, 200, reverse('courseware_section',