Merge pull request #5498 from edx/jeskew/split_fix_auto_auth

Optimize LMS views for Split courses.

cms/djangoapps/models/settings/course_grading.py

@@ -205,10 +205,11 @@ class CourseGradingModel(object):
 
     @staticmethod
     def jsonize_grader(i, grader):
-        grader['id'] = i
-        if grader['weight']:
-            grader['weight'] *= 100
-        if not 'short_label' in grader:
-            grader['short_label'] = ""
-
-        return grader
+        return {
+            "id": i,
+            "type": grader["type"],
+            "min_count": grader.get('min_count', 0),
+            "drop_count": grader.get('drop_count', 0),
+            "short_label": grader.get('short_label', ""),
+            "weight": grader.get('weight', 0) * 100,
+        }

common/djangoapps/student/tests/test_auto_auth.py

@@ -7,14 +7,25 @@ from django_comment_common.utils import seed_permissions_roles
 from student.models import CourseEnrollment, UserProfile
 from util.testing import UrlResetMixin
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from opaque_keys.edx.locator import CourseLocator
 from mock import patch
+import ddt
 
-
+@ddt.ddt
 class AutoAuthEnabledTestCase(UrlResetMixin, TestCase):
     """
     Tests for the Auto auth view that we have for load testing.
     """
 
+    COURSE_ID_MONGO = 'edX/Test101/2014_Spring'
+    COURSE_ID_SPLIT = 'course-v1:edX+Test101+2014_Spring'
+    COURSE_IDS_DDT = (
+        (COURSE_ID_MONGO, SlashSeparatedCourseKey.from_deprecated_string(COURSE_ID_MONGO)),
+        (COURSE_ID_SPLIT, SlashSeparatedCourseKey.from_deprecated_string(COURSE_ID_SPLIT)),
+        (COURSE_ID_MONGO, CourseLocator.from_string(COURSE_ID_MONGO)),
+        (COURSE_ID_SPLIT, CourseLocator.from_string(COURSE_ID_SPLIT)),
+        )
+
     @patch.dict("django.conf.settings.FEATURES", {"AUTOMATIC_AUTH_FOR_TESTING": True})
     def setUp(self):
         # Patching the settings.FEATURES['AUTOMATIC_AUTH_FOR_TESTING']
@@ -24,8 +35,6 @@ class AutoAuthEnabledTestCase(UrlResetMixin, TestCase):
         super(AutoAuthEnabledTestCase, self).setUp()
         self.url = '/auto_auth'
         self.client = Client()
-        self.course_id = 'edX/Test101/2014_Spring'
-        self.course_key = SlashSeparatedCourseKey.from_deprecated_string(self.course_id)
 
     def test_create_user(self):
         """
@@ -83,42 +92,48 @@ class AutoAuthEnabledTestCase(UrlResetMixin, TestCase):
         user = User.objects.get(username='test')
         self.assertFalse(user.is_staff)
 
-    def test_course_enrollment(self):
+    @ddt.data(*COURSE_IDS_DDT)
+    @ddt.unpack
+    def test_course_enrollment(self, course_id, course_key):
 
         # Create a user and enroll in a course
-        self._auto_auth(username='test', course_id=self.course_id)
+        self._auto_auth(username='test', course_id=course_id)
 
         # Check that a course enrollment was created for the user
         self.assertEqual(CourseEnrollment.objects.count(), 1)
-        enrollment = CourseEnrollment.objects.get(course_id=self.course_key)
+        enrollment = CourseEnrollment.objects.get(course_id=course_key)
         self.assertEqual(enrollment.user.username, "test")
 
-    def test_double_enrollment(self):
+    @ddt.data(*COURSE_IDS_DDT)
+    @ddt.unpack
+    def test_double_enrollment(self, course_id, course_key):
 
         # Create a user and enroll in a course
-        self._auto_auth(username='test', course_id=self.course_id)
+        self._auto_auth(username='test', course_id=course_id)
 
         # Make the same call again, re-enrolling the student in the same course
-        self._auto_auth(username='test', course_id=self.course_id)
+        self._auto_auth(username='test', course_id=course_id)
 
         # Check that only one course enrollment was created for the user
         self.assertEqual(CourseEnrollment.objects.count(), 1)
-        enrollment = CourseEnrollment.objects.get(course_id=self.course_key)
+        enrollment = CourseEnrollment.objects.get(course_id=course_key)
         self.assertEqual(enrollment.user.username, "test")
 
-    def test_set_roles(self):
-        seed_permissions_roles(self.course_key)
-        course_roles = dict((r.name, r) for r in Role.objects.filter(course_id=self.course_key))
+    @ddt.data(*COURSE_IDS_DDT)
+    @ddt.unpack
+    def test_set_roles(self, course_id, course_key):
+        seed_permissions_roles(course_key)
+        course_roles = dict((r.name, r) for r in Role.objects.filter(course_id=course_key))
         self.assertEqual(len(course_roles), 4)  # sanity check
 
         # Student role is assigned by default on course enrollment.
-        self._auto_auth(username='a_student', course_id=self.course_id)
+        self._auto_auth(username='a_student', course_id=course_id)
         user = User.objects.get(username='a_student')
         user_roles = user.roles.all()
         self.assertEqual(len(user_roles), 1)
         self.assertEqual(user_roles[0], course_roles[FORUM_ROLE_STUDENT])
 
-        self._auto_auth(username='a_moderator', course_id=self.course_id, roles='Moderator')
+        self._auto_auth(username='a_moderator', course_id=course_id, roles='Moderator')
         user = User.objects.get(username='a_moderator')
         user_roles = user.roles.all()
         self.assertEqual(
@@ -127,7 +142,7 @@ class AutoAuthEnabledTestCase(UrlResetMixin, TestCase):
                 course_roles[FORUM_ROLE_MODERATOR]]))
 
         # check multiple roles work.
-        self._auto_auth(username='an_admin', course_id=self.course_id,
+        self._auto_auth(username='an_admin', course_id=course_id,
                         roles='{},{}'.format(FORUM_ROLE_MODERATOR, FORUM_ROLE_ADMINISTRATOR))
         user = User.objects.get(username='an_admin')
         user_roles = user.roles.all()

common/djangoapps/student/views.py

@@ -57,6 +57,7 @@ from dark_lang.models import DarkLangConfig
 from xmodule.modulestore.django import modulestore
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from opaque_keys.edx.locator import CourseLocator
 from xmodule.modulestore import ModuleStoreEnum
 
 from collections import namedtuple
@@ -245,7 +246,9 @@ def get_course_enrollment_pairs(user, course_org_filter, org_filter_out_set):
     a student's dashboard.
     """
     for enrollment in CourseEnrollment.enrollments_for_user(user):
-        course = modulestore().get_course(enrollment.course_id)
+        store = modulestore()
+        with store.bulk_operations(enrollment.course_id):
+            course = store.get_course(enrollment.course_id)
             if course and not isinstance(course, ErrorDescriptor):
 
                 # if we are in a Microsite, then filter out anything that is not
@@ -1677,7 +1680,7 @@ def auto_auth(request):
     course_id = request.GET.get('course_id', None)
     course_key = None
     if course_id:
-        course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+        course_key = CourseLocator.from_string(course_id)
     role_names = [v.strip() for v in request.GET.get('roles', '').split(',') if v.strip()]
 
     # Get or create the user object

common/lib/capa/capa/capa_problem.py

@@ -646,14 +646,14 @@ class LoncapaProblem(object):
             code = unescape(script.text, XMLESC)
             all_code += code
 
-        # An asset named python_lib.zip can be imported by Python code.
         extra_files = []
+        if all_code:
+            # An asset named python_lib.zip can be imported by Python code.
             zip_lib = self.capa_system.get_python_lib_zip()
             if zip_lib is not None:
                 extra_files.append(("python_lib.zip", zip_lib))
                 python_path.append("python_lib.zip")
 
-        if all_code:
             try:
                 safe_exec(
                     all_code,

common/lib/xmodule/xmodule/combined_open_ended_module.py

@@ -240,7 +240,7 @@ class CombinedOpenEndedFields(object):
         help=_("The number of times the student can try to answer this problem."),
         default=1,
         scope=Scope.settings,
-        values={"min": 1 }
+        values={"min": 1}
     )
     accept_file_upload = Boolean(
         display_name=_("Allow File Uploads"),

common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py

@@ -76,6 +76,11 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
 
     @contract(usage_key="BlockUsageLocator | BlockKey", course_entry_override="CourseEnvelope | None")
     def _load_item(self, usage_key, course_entry_override=None, **kwargs):
+        """
+        Instantiate the xblock fetching it either from the cache or from the structure
+
+        :param course_entry_override: the course_info with the course_key to use (defaults to cached)
+        """
         # usage_key is either a UsageKey or just the block_key. if a usage_key,
         if isinstance(usage_key, BlockUsageLocator):
 
@@ -90,21 +95,25 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
                     raise ItemNotFoundError
             else:
                 block_key = BlockKey.from_usage_key(usage_key)
+                version_guid = self.course_entry.course_key.version_guid
         else:
             block_key = usage_key
 
             course_info = course_entry_override or self.course_entry
             course_key = course_info.course_key
+            version_guid = course_key.version_guid
 
-        if course_entry_override:
-            structure_id = course_entry_override.structure.get('_id')
-        else:
-            structure_id = self.course_entry.structure.get('_id')
+        # look in cache
+        cached_module = self.modulestore.get_cached_block(course_key, version_guid, block_key)
+        if cached_module:
+            return cached_module
 
         json_data = self.get_module_data(block_key, course_key)
 
         class_ = self.load_block_type(json_data.get('block_type'))
-        return self.xblock_from_json(class_, course_key, block_key, json_data, course_entry_override, **kwargs)
+        block = self.xblock_from_json(class_, course_key, block_key, json_data, course_entry_override, **kwargs)
+        self.modulestore.cache_block(course_key, version_guid, block_key, block)
+        return block
 
     @contract(block_key=BlockKey, course_key=CourseLocator)
     def get_module_data(self, block_key, course_key):

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -117,6 +117,8 @@ class SplitBulkWriteRecord(BulkOpsRecord):
         self.index = None
         self.structures = {}
         self.structures_in_db = set()
+        # dict(version_guid, dict(BlockKey, module))
+        self.modules = defaultdict(dict)
         self.definitions = {}
         self.definitions_in_db = set()
 
@@ -309,6 +311,38 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
         else:
             self.db_connection.insert_structure(structure)
 
+    def get_cached_block(self, course_key, version_guid, block_id):
+        """
+        If there's an active bulk_operation, see if it's cached this module and just return it
+        Don't do any extra work to get the ones which are not cached. Make the caller do the work & cache them.
+        """
+        bulk_write_record = self._get_bulk_ops_record(course_key)
+        if bulk_write_record.active:
+            return bulk_write_record.modules[version_guid].get(block_id, None)
+        else:
+            return None
+
+    def cache_block(self, course_key, version_guid, block_key, block):
+        """
+        The counterpart to :method `get_cached_block` which caches a block.
+        Returns nothing.
+        """
+        bulk_write_record = self._get_bulk_ops_record(course_key)
+        if bulk_write_record.active:
+            bulk_write_record.modules[version_guid][block_key] = block
+
+    def decache_block(self, course_key, version_guid, block_key):
+        """
+        Write operations which don't write from blocks must remove the target blocks from the cache.
+        Returns nothing.
+        """
+        bulk_write_record = self._get_bulk_ops_record(course_key)
+        if bulk_write_record.active:
+            try:
+                del bulk_write_record.modules[version_guid][block_key]
+            except KeyError:
+                pass
+
     def get_definition(self, course_key, definition_guid):
         """
         Retrieve a single definition by id, respecting the active bulk operation
@@ -637,8 +671,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
     @contract(course_entry=CourseEnvelope, block_keys="list(BlockKey)", depth="int | None")
     def _load_items(self, course_entry, block_keys, depth=0, lazy=True, **kwargs):
         '''
-        Load & cache the given blocks from the course. Prefetch down to the
-        given depth. Load the definitions into each block if lazy is False;
+        Load & cache the given blocks from the course. May return the blocks in any order.
+
+        Load the definitions into each block if lazy is False;
         otherwise, use the lazy definition placeholder.
         '''
         runtime = self._get_cache(course_entry.structure['_id'])
@@ -646,6 +681,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             runtime = self.create_runtime(course_entry, lazy)
             self._add_cache(course_entry.structure['_id'], runtime)
             self.cache_items(runtime, block_keys, course_entry.course_key, depth, lazy)
+
         return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]
 
     def _get_cache(self, course_version_guid):
@@ -1364,6 +1400,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 # if the parent hadn't been previously changed in this bulk transaction, indicate that it's
                 # part of the bulk transaction
                 self.version_block(parent, user_id, new_structure['_id'])
+            self.decache_block(parent_usage_key.course_key, new_structure['_id'], block_id)
 
             # db update
             self.update_structure(parent_usage_key.course_key, new_structure)
@@ -1957,6 +1994,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 parent_block['edit_info']['edited_by'] = user_id
                 parent_block['edit_info']['previous_version'] = parent_block['edit_info']['update_version']
                 parent_block['edit_info']['update_version'] = new_id
+                self.decache_block(usage_locator.course_key, new_id, parent_block_key)
 
             self._remove_subtree(BlockKey.from_usage_key(usage_locator), new_blocks)
 

common/lib/xmodule/xmodule/open_ended_grading_classes/combined_open_ended_modulev1.py

@@ -1229,7 +1229,6 @@ class CombinedOpenEndedV1Descriptor():
 
         return {'task_xml': parse_task('task'), 'prompt': parse('prompt'), 'rubric': parse('rubric')}
 
-
     def definition_to_xml(self, resource_fs):
         '''Return an xml element representing this definition.'''
         elt = etree.Element('combinedopenended')

lms/djangoapps/courseware/courses.py

@@ -66,6 +66,7 @@ def get_course_by_id(course_key, depth=0):
 
     depth: The number of levels of children for the modulestore to cache. None means infinite depth
     """
+    with modulestore().bulk_operations(course_key):
         course = modulestore().get_course(course_key, depth=depth)
     if course:
         return course

lms/djangoapps/courseware/views.py

@@ -285,6 +285,11 @@ def index(request, course_id, chapter=None, section=None,
             return redirect(reverse('dashboard'))
 
     request.user = user  # keep just one instance of User
+    with modulestore().bulk_operations(course_key):
+        return _index_bulk_op(request, user, course_key, chapter, section, position)
+
+
+def _index_bulk_op(request, user, course_key, chapter, section, position):
     course = get_course_with_access(user, 'load', course_key, depth=2)
     staff_access = has_access(user, 'staff', course)
     registered = registered_for_course(course, user)
@@ -554,6 +559,7 @@ def course_info(request, course_id):
 
     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
 
+    with modulestore().bulk_operations(course_key):
         course = get_course_with_access(request.user, 'load', course_key)
         staff_access = has_access(request.user, 'staff', course)
         masq = setup_masquerade(request, staff_access)    # allow staff to toggle masquerade on info page
@@ -805,6 +811,7 @@ def progress(request, course_id, student_id=None):
 
     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
 
+    with modulestore().bulk_operations(course_key):
         with grades.manual_transaction():
             return _progress(request, course_key, student_id)