Merge branch 'release'

Conflicts:
	common/djangoapps/course_modes/views.py
	common/djangoapps/student/tests/test_roles.py
	common/djangoapps/student/views.py
	common/lib/opaque_keys/opaque_keys/__init__.py
	common/lib/opaque_keys/opaque_keys/tests/test_opaque_keys.py
	common/lib/xmodule/xmodule/contentstore/mongo.py
	lms/djangoapps/certificates/management/commands/gen_cert_report.py
	lms/djangoapps/notes/views.py

cms/djangoapps/contentstore/features/transcripts.py

@@ -171,7 +171,7 @@ def remove_transcripts_from_store(_step, subs_id):
     )
     try:
         content = contentstore().find(content_location)
-        contentstore().delete(content.get_id())
+        contentstore().delete(content.location)
         print('Transcript file was removed from store.')
     except NotFoundError:
         print('Transcript file was NOT found and not removed.')

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -6,14 +6,15 @@ import random
 
 from chrono import Timer
 from mock import patch, Mock
+import ddt
 
 from django.test import RequestFactory
 
-from contentstore.views.course import _accessible_courses_list, _accessible_courses_list_from_groups
+from contentstore.views.course import _accessible_courses_list, _accessible_courses_list_from_groups, AccessListFallback
 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 student.roles import CourseInstructorRole, CourseStaffRole, GlobalStaff, OrgStaffRole, OrgInstructorRole
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
@@ -24,6 +25,7 @@ TOTAL_COURSES_COUNT = 500
 USER_COURSES_COUNT = 50
 
 
+@ddt.ddt
 class TestCourseListing(ModuleStoreTestCase):
     """
     Unit tests for getting the list of courses for a logged in user
@@ -263,7 +265,7 @@ class TestCourseListing(ModuleStoreTestCase):
         course_db_record = modulestore()._find_one(course.location)
         course_db_record.setdefault('metadata', {}).get('tabs', []).append({"type": "wiko", "name": "Wiki" })
         modulestore().collection.update(
-            {'_id': course_db_record['_id']},
+            {'_id': course.location.to_deprecated_son()},
             {'$set': {
                 'metadata.tabs': course_db_record['metadata']['tabs'],
             }},
@@ -271,3 +273,31 @@ class TestCourseListing(ModuleStoreTestCase):
 
         courses_list = _accessible_courses_list_from_groups(self.request)
         self.assertEqual(len(courses_list), 1, courses_list)
+
+    @ddt.data(OrgStaffRole('AwesomeOrg'), OrgInstructorRole('AwesomeOrg'))
+    def test_course_listing_org_permissions(self, role):
+        """
+        Create multiple courses within the same org.  Verify that someone with org-wide permissions can access
+        all of them.
+        """
+        org_course_one = SlashSeparatedCourseKey('AwesomeOrg', 'Course1', 'RunBabyRun')
+        CourseFactory.create(
+            org=org_course_one.org,
+            number=org_course_one.course,
+            run=org_course_one.run
+        )
+
+        org_course_two = SlashSeparatedCourseKey('AwesomeOrg', 'Course2', 'RunRunRun')
+        CourseFactory.create(
+            org=org_course_two.org,
+            number=org_course_two.course,
+            run=org_course_two.run
+        )
+
+        # Two types of org-wide roles have edit permissions: staff and instructor.  We test both
+        role.add_users(self.user)
+
+        with self.assertRaises(AccessListFallback):
+            _accessible_courses_list_from_groups(self.request)
+        courses_list = _accessible_courses_list(self.request)
+        self.assertEqual(len(courses_list), 2)

cms/djangoapps/contentstore/tests/test_permissions.py

@@ -11,7 +11,7 @@ from contentstore.tests.modulestore_config import TEST_MODULESTORE
 from contentstore.tests.utils import AjaxEnabledTestClient
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from contentstore.utils import reverse_url, reverse_course_url
-from student.roles import CourseInstructorRole, CourseStaffRole
+from student.roles import CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole
 from contentstore.views.access import has_course_access
 from student import auth
 
@@ -95,10 +95,13 @@ class TestCourseAccess(ModuleStoreTestCase):
         # doesn't use role.users_with_role b/c it's verifying the roles.py behavior
         user_by_role = {}
         # add the misc users to the course in different groups
-        for role in [CourseInstructorRole, CourseStaffRole]:
+        for role in [CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole]:
             user_by_role[role] = []
-            # pylint: disable=protected-access
-            group = role(self.course_key)
+            # Org-based roles are created via org name, rather than course_key
+            if (role is OrgStaffRole) or (role is OrgInstructorRole):
+                group = role(self.course_key.org)
+            else:
+                group = role(self.course_key)
             # NOTE: this loop breaks the roles.py abstraction by purposely assigning
             # users to one of each possible groupname in order to test that has_course_access
             # and remove_user work
@@ -109,21 +112,28 @@ class TestCourseAccess(ModuleStoreTestCase):
 
         course_team_url = reverse_course_url('course_team_handler', self.course_key)
         response = self.client.get_html(course_team_url)
-        for role in [CourseInstructorRole, CourseStaffRole]:
+        for role in [CourseInstructorRole, CourseStaffRole]:  # Global and org-based roles don't appear on this page
             for user in user_by_role[role]:
                 self.assertContains(response, user.email)
 
         # test copying course permissions
         copy_course_key = SlashSeparatedCourseKey('copyu', 'copydept.mycourse', 'myrun')
-        for role in [CourseInstructorRole, CourseStaffRole]:
-            auth.add_users(
-                self.user,
-                role(copy_course_key),
-                *role(self.course_key).users_with_role()
-            )
+        for role in [CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole]:
+            if (role is OrgStaffRole) or (role is OrgInstructorRole):
+                auth.add_users(
+                    self.user,
+                    role(copy_course_key.org),
+                    *role(self.course_key.org).users_with_role()
+                )
+            else:
+                auth.add_users(
+                    self.user,
+                    role(copy_course_key),
+                    *role(self.course_key).users_with_role()
+                )
         # verify access in copy course and verify that removal from source course w/ the various
         # groupnames works
-        for role in [CourseInstructorRole, CourseStaffRole]:
+        for role in [CourseInstructorRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole]:
             for user in user_by_role[role]:
                 # forcefully decache the groups: premise is that any real request will not have
                 # multiple objects repr the same user but this test somehow uses different instance
@@ -132,5 +142,8 @@ class TestCourseAccess(ModuleStoreTestCase):
                     del user._roles
 
                 self.assertTrue(has_course_access(user, copy_course_key), "{} no copy access".format(user))
-                auth.remove_users(self.user, role(self.course_key), user)
+                if (role is OrgStaffRole) or (role is OrgInstructorRole):
+                    auth.remove_users(self.user, role(self.course_key.org), user)
+                else:
+                    auth.remove_users(self.user, role(self.course_key), user)
                 self.assertFalse(has_course_access(user, self.course_key), "{} remove didn't work".format(user))

cms/djangoapps/contentstore/tests/test_transcripts_utils.py

@@ -88,7 +88,7 @@ class TestSaveSubsToStore(ModuleStoreTestCase):
         """Remove, if subtitles content exists."""
         try:
             content = contentstore().find(self.content_location)
-            contentstore().delete(content.get_id())
+            contentstore().delete(content.location)
         except NotFoundError:
             pass
 
@@ -171,7 +171,7 @@ class TestDownloadYoutubeSubs(ModuleStoreTestCase):
             content_location = StaticContent.compute_location(self.course.id, filename)
             try:
                 content = contentstore().find(content_location)
-                contentstore().delete(content.get_id())
+                contentstore().delete(content.location)
             except NotFoundError:
                 pass
 

cms/djangoapps/contentstore/views/access.py

-from student.roles import CourseStaffRole, GlobalStaff, CourseInstructorRole
+""" Helper methods for determining user access permissions in Studio """
+
+from student.roles import CourseStaffRole, GlobalStaff, CourseInstructorRole, OrgStaffRole, OrgInstructorRole
 from student import auth
 
 
@@ -14,6 +16,10 @@ def has_course_access(user, course_key, role=CourseStaffRole):
     """
     if GlobalStaff().has_user(user):
         return True
+    if OrgInstructorRole(org=course_key.org).has_user(user):
+        return True
+    if OrgStaffRole(org=course_key.org).has_user(user):
+        return True
     return auth.has_access(user, role(course_key))
 
 

cms/djangoapps/contentstore/views/course.py

@@ -68,6 +68,14 @@ __all__ = ['course_info_handler', 'course_handler', 'course_info_update_handler'
            'textbooks_list_handler', 'textbooks_detail_handler']
 
 
+class AccessListFallback(Exception):
+    """
+    An exception that is raised whenever we need to `fall back` to fetching *all* courses
+    available to a user, rather than using a shorter method (i.e. fetching by group)
+    """
+    pass
+
+
 def _get_course_module(course_key, user, depth=0):
     """
     Internal method used to calculate and return the locator and course module
@@ -190,11 +198,17 @@ def _accessible_courses_list_from_groups(request):
 
     for course_access in all_courses:
         course_key = course_access.course_id
-        if course_key not in courses_list:
+        if course_key is None:
+            # If the course_access does not have a course_id, it's an org-based role, so we fall back
+            raise AccessListFallback
+        try:
             course = modulestore('direct').get_course(course_key)
-            if course is not None and not isinstance(course, ErrorDescriptor):
-                # ignore deleted or errored courses
-                courses_list[course_key] = course
+        except ItemNotFoundError:
+            # If a user has access to a course that doesn't exist, don't do anything with that course
+            pass
+        if course is not None and not isinstance(course, ErrorDescriptor):
+            # ignore deleted or errored courses
+            courses_list[course_key] = course
 
     return courses_list.values()
 
@@ -213,7 +227,7 @@ def course_listing(request):
     else:
         try:
             courses = _accessible_courses_list_from_groups(request)
-        except ItemNotFoundError:
+        except AccessListFallback:
             # user have some old groups or there was some error getting courses from django groups
             # so fallback to iterating through all courses
             courses = _accessible_courses_list(request)

common/djangoapps/course_modes/views.py

@@ -4,9 +4,7 @@ Views for the course_mode module
 
 import decimal
 from django.core.urlresolvers import reverse
-from django.http import (
-    HttpResponseBadRequest,  Http404
-)
+from django.http import HttpResponseBadRequest
 from django.shortcuts import redirect
 from django.views.generic.base import View
 from django.utils.translation import ugettext as _
@@ -18,9 +16,9 @@ from edxmako.shortcuts import render_to_response
 from course_modes.models import CourseMode
 from courseware.access import has_access
 from student.models import CourseEnrollment
-from student.views import course_from_id
 from verify_student.models import SoftwareSecurePhotoVerification
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from xmodule.modulestore.django import modulestore
 
 
 class ChooseModeView(View):
@@ -54,7 +52,7 @@ class ChooseModeView(View):
         donation_for_course = request.session.get("donation_for_course", {})
         chosen_price = donation_for_course.get(course_key, None)
 
-        course = course_from_id(course_key)
+        course = modulestore().get_course(course_key)
         context = {
             "course_modes_choose_url": reverse("course_modes_choose", kwargs={'course_id': course_key.to_deprecated_string()}),
             "modes": modes,
@@ -78,9 +76,9 @@ class ChooseModeView(View):
         course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
         user = request.user
 
-        # This is a bit redundant with logic in student.views.change_enrollement,
+        # This is a bit redundant with logic in student.views.change_enrollment,
         # but I don't really have the time to refactor it more nicely and test.
-        course = course_from_id(course_key)
+        course = modulestore().get_course(course_key)
         if not has_access(user, 'enroll', course):
             error_msg = _("Enrollment is closed")
             return self.get(request, course_id, error=error_msg)

common/djangoapps/external_auth/views.py

@@ -588,7 +588,7 @@ def course_specific_login(request, course_id):
        Dispatcher function for selecting the specific login method
        required by the course
     """
-    course = student.views.course_from_id(course_id)
+    course = modulestore().get_course(course_id)
     if not course:
         # couldn't find the course, will just return vanilla signin page
         return redirect_with_get('signin_user', request.GET)
@@ -606,7 +606,7 @@ def course_specific_register(request, course_id):
         Dispatcher function for selecting the specific registration method
         required by the course
     """
-    course = student.views.course_from_id(course_id)
+    course = modulestore().get_course(course_id)
 
     if not course:
         # couldn't find the course, will just return vanilla registration page

common/djangoapps/student/roles.py

@@ -7,6 +7,28 @@ from abc import ABCMeta, abstractmethod
 
 from django.contrib.auth.models import User
 from student.models import CourseAccessRole
+from xmodule_django.models import CourseKeyField
+
+
+class RoleCache(object):
+    """
+    A cache of the CourseAccessRoles held by a particular user
+    """
+    def __init__(self, user):
+        self._roles = set(
+            CourseAccessRole.objects.filter(user=user).all()
+        )
+
+    def has_role(self, role, course_id, org):
+        """
+        Return whether this RoleCache contains a role with the specified role, course_id, and org
+        """
+        return any(
+            access_role.role == role and
+            access_role.course_id == course_id and
+            access_role.org == org
+            for access_role in self._roles
+        )
 
 
 class AccessRole(object):
@@ -93,12 +115,11 @@ class RoleBase(AccessRole):
 
         # pylint: disable=protected-access
         if not hasattr(user, '_roles'):
-            user._roles = set(
-                CourseAccessRole.objects.filter(user=user).all()
-            )
+            # Cache a list of tuples identifying the particular roles that a user has
+            # Stored as tuples, rather than django models, to make it cheaper to construct objects for comparison
+            user._roles = RoleCache(user)
 
-        role = CourseAccessRole(user=user, role=self._role_name, course_id=self.course_key, org=self.org)
-        return role in user._roles
+        return user._roles.has_role(self._role_name, self.course_key, self.org)
 
     def add_users(self, *users):
         """
@@ -129,6 +150,9 @@ class RoleBase(AccessRole):
         """
         Return a django QuerySet for all of the users with this role
         """
+        # Org roles don't query by CourseKey, so use CourseKeyField.Empty for that query
+        if self.course_key is None:
+            self.course_key = CourseKeyField.Empty
         entries = User.objects.filter(
             courseaccessrole__role=self._role_name,
             courseaccessrole__org=self.org,
@@ -228,12 +252,9 @@ class UserBasedRole(object):
 
         # pylint: disable=protected-access
         if not hasattr(self.user, '_roles'):
-            self.user._roles = list(
-                CourseAccessRole.objects.filter(user=self.user).all()
-            )
+            self.user._roles = RoleCache(self.user)
 
-        role = CourseAccessRole(user=self.user, role=self.role, course_id=course_key, org=course_key.org)
-        return role in self.user._roles
+        return self.user._roles.has_role(self.role, course_key, course_key.org)
 
     def add_course(self, *course_keys):
         """

common/djangoapps/student/tests/test_course_listing.py

@@ -107,7 +107,7 @@ class TestCourseListing(ModuleStoreTestCase):
         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']},
+            {'_id': course.location.to_deprecated_son()},
             {'$set': {
                 'metadata.tabs': course_db_record['metadata']['tabs'],
             }},

common/djangoapps/student/tests/test_roles.py

 """
 Tests of student.roles
 """
-
+import ddt
 from django.test import TestCase
 
 from courseware.tests.factories import UserFactory, StaffFactory, InstructorFactory
 from student.tests.factories import AnonymousUserFactory
 
-from student.roles import GlobalStaff, CourseRole, CourseStaffRole, OrgStaffRole, OrgInstructorRole, \
-    CourseInstructorRole
+from student.roles import (
+    GlobalStaff, CourseRole, CourseStaffRole, CourseInstructorRole,
+    OrgStaffRole, OrgInstructorRole, RoleCache, CourseBetaTesterRole
+)
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
 
@@ -152,3 +154,40 @@ class RolesTestCase(TestCase):
         role.add_users(self.student)
         role.remove_users(self.student)
         self.assertFalse(role.has_user(self.student))
+
+
+@ddt.ddt
+class RoleCacheTestCase(TestCase):
+
+    IN_KEY = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall')
+    NOT_IN_KEY = SlashSeparatedCourseKey('edX', 'toy', '2013_Fall')
+
+    ROLES = (
+        (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')),
+        (CourseInstructorRole(IN_KEY), ('instructor', IN_KEY, 'edX')),
+        (OrgStaffRole(IN_KEY.org), ('staff', None, 'edX')),
+        (OrgInstructorRole(IN_KEY.org), ('instructor', None, 'edX')),
+        (CourseBetaTesterRole(IN_KEY), ('beta_testers', IN_KEY, 'edX')),
+    )
+
+    def setUp(self):
+        self.user = UserFactory()
+
+    @ddt.data(*ROLES)
+    @ddt.unpack
+    def test_only_in_role(self, role, target):
+        role.add_users(self.user)
+        cache = RoleCache(self.user)
+        self.assertTrue(cache.has_role(*target))
+
+        for other_role, other_target in self.ROLES:
+            if other_role == role:
+                continue
+
+            self.assertFalse(cache.has_role(*other_target))
+
+    @ddt.data(*ROLES)
+    @ddt.unpack
+    def test_empty_cache(self, role, target):
+        cache = RoleCache(self.user)
+        self.assertFalse(cache.has_role(*target))

common/djangoapps/student/views.py

@@ -49,7 +49,6 @@ from verify_student.models import SoftwareSecurePhotoVerification, MidcourseReve
 from certificates.models import CertificateStatuses, certificate_status_for_student
 from dark_lang.models import DarkLangConfig
 
-from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.django import modulestore
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
@@ -128,11 +127,6 @@ def index(request, extra_context={}, user=AnonymousUser()):
     return render_to_response('index.html', context)
 
 
-def course_from_id(course_id):
-    """Return the CourseDescriptor corresponding to this course_id"""
-    return modulestore().get_course(course_id)
-
-
 def embargo(_request):
     """
     Render the embargo page.
@@ -242,7 +236,7 @@ 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 = course_from_id(enrollment.course_id)
+        course = modulestore().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
@@ -603,7 +597,7 @@ def change_enrollment(request):
         # Make sure the course exists
         # We don't do this check on unenroll, or a bad course id can't be unenrolled from
         try:
-            course = course_from_id(course_id)
+            course = modulestore().get_course(course_id)
         except ItemNotFoundError:
             log.warning("User {0} tried to enroll in non-existent course {1}"
                         .format(user.username, course_id))
@@ -671,7 +665,7 @@ def _get_course_enrollment_domain(course_id):
     @param course_id:
     @return:
     """
-    course = course_from_id(course_id)
+    course = modulestore().get_course(course_id)
     if course is None:
         return None
 

common/lib/xmodule/xmodule/conditional_module.py

@@ -197,7 +197,10 @@ class ConditionalDescriptor(ConditionalFields, SequenceDescriptor):
         # substitution can be done.
         if not self.sources_list:
             if 'sources' in self.xml_attributes and isinstance(self.xml_attributes['sources'], basestring):
-                self.sources_list = ConditionalDescriptor.parse_sources(self.xml_attributes)
+                self.sources_list = [
+                    self.location.course_key.make_usage_key_from_deprecated_string(item)
+                    for item in ConditionalDescriptor.parse_sources(self.xml_attributes)
+                ]
 
     @staticmethod
     def parse_sources(xml_element):

common/lib/xmodule/xmodule/contentstore/content.py

@@ -60,7 +60,7 @@ class StaticContent(object):
         )
 
     def get_id(self):
-        return self.location.to_deprecated_son(tag=XASSET_LOCATION_TAG)
+        return self.location
 
     def get_url_path(self):
         return self.location.to_deprecated_string()

common/lib/xmodule/xmodule/contentstore/mongo.py

@@ -13,6 +13,7 @@ from fs.osfs import OSFS
 import os
 import json
 import bson.son
+from bson.son import SON
 from opaque_keys.edx.locations import AssetLocation
 
 
@@ -29,7 +30,7 @@ class MongoContentStore(ContentStore):
             pymongo.MongoClient(
                 host=host,
                 port=port,
-                document_class=bson.son.SON,
+                document_class=dict,
                 **kwargs
             ),
             db
@@ -43,7 +44,7 @@ class MongoContentStore(ContentStore):
         self.fs_files = _db[bucket + ".files"]  # the underlying collection GridFS uses
 
     def save(self, content):
-        content_id = content.get_id()
+        content_id = self.asset_db_key(content.location)
 
         # Seems like with the GridFS we can't update existing ID's we have to do a delete/add pair
         self.delete(content_id)
@@ -63,20 +64,14 @@ class MongoContentStore(ContentStore):
 
         return content
 
-    def delete(self, content_id):
-        if self.fs.exists({"_id": content_id}):
-            self.fs.delete(content_id)
-            assert not self.fs.exists({"_id": content_id})
+    def delete(self, location_or_id):
+        if isinstance(location_or_id, AssetLocation):
+            location_or_id = self.asset_db_key(location_or_id)
+        if self.fs.exists({"_id": location_or_id}):
+            self.fs.delete(location_or_id)
 
     def find(self, location, throw_on_not_found=True, as_stream=False):
         content_id = self.asset_db_key(location)
-        fs_pointer = self.fs_files.find_one(content_id, fields={'_id': 1})
-        if fs_pointer is None:
-            if throw_on_not_found:
-                raise NotFoundError()
-            else:
-                return None
-        content_id = fs_pointer['_id']
 
         try:
             if as_stream:
@@ -103,13 +98,13 @@ class MongoContentStore(ContentStore):
                     )
         except NoFile:
             if throw_on_not_found:
-                raise NotFoundError()
+                raise NotFoundError(content_id)
             else:
                 return None
 
     def get_stream(self, location):
         content_id = self.asset_db_key(location)
-        fs_pointer = self.fs_files.find_one(content_id, fields={'_id': 1})
+        fs_pointer = self.fs_files.find_one({'_id': content_id}, fields={'_id': 1})
 
         try:
             handle = self.fs.get(fs_pointer['_id'])
@@ -245,10 +240,10 @@ class MongoContentStore(ContentStore):
                 raise AttributeError("{} is a protected attribute.".format(attr))
         asset_db_key = self.asset_db_key(location)
         # FIXME remove fetch and use a form of update which fails if doesn't exist
-        item = self.fs_files.find_one(asset_db_key)
+        item = self.fs_files.find_one({'_id': asset_db_key})
         if item is None:
             raise NotFoundError(asset_db_key)
-        self.fs_files.update(asset_db_key, {"$set": attr_dict})
+        self.fs_files.update({'_id': asset_db_key}, {"$set": attr_dict})
 
     def get_attrs(self, location):
         """
@@ -261,7 +256,7 @@ class MongoContentStore(ContentStore):
         :param location: a c4x asset location
         """
         asset_db_key = self.asset_db_key(location)
-        item = self.fs_files.find_one(asset_db_key)
+        item = self.fs_files.find_one({'_id': asset_db_key})
         if item is None:
             raise NotFoundError(asset_db_key)
         return item
@@ -282,4 +277,8 @@ class MongoContentStore(ContentStore):
         """
         Returns the database query to find the given asset location.
         """
-        return location.to_deprecated_son(tag=XASSET_LOCATION_TAG, prefix='_id.')
+        # codifying the original order which pymongo used for the dicts coming out of location_to_dict
+        # stability of order is more important than sanity of order as any changes to order make things
+        # unfindable
+        ordered_key_fields = ['category', 'name', 'course', 'tag', 'org', 'revision']
+        return SON((field_name, getattr(location, field_name)) for field_name in ordered_key_fields)

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -165,7 +165,7 @@ class ModuleStoreRead(object):
         pass
 
     @abstractmethod
-    def get_course(self, course_id, depth=None):
+    def get_course(self, course_id, depth=0):
         '''
         Look for a specific course by its id (:class:`CourseKey`).
         Returns the course descriptor, or None if not found.
@@ -332,7 +332,7 @@ class ModuleStoreReadBase(ModuleStoreRead):
         """
         return {}
 
-    def get_course(self, course_id, depth=None):
+    def get_course(self, course_id, depth=0):
         """
         See ModuleStoreRead.get_course
 

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -158,7 +158,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
         return courses.values()
 
-    def get_course(self, course_key, depth=None):
+    def get_course(self, course_key, depth=0):
         """
         returns the course module associated with the course_id. If no such course exists,
         it returns None

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -185,7 +185,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 if isinstance(data, basestring):
                     data = {'data': data}
                 mixed_class = self.mixologist.mix(class_)
-                data = self._convert_reference_fields_to_keys(mixed_class, location.course_key, data)
+                if data is not None:
+                    data = self._convert_reference_fields_to_keys(mixed_class, location.course_key, data)
                 metadata = self._convert_reference_fields_to_keys(mixed_class, location.course_key, metadata)
                 kvs = MongoKeyValueStore(
                     data,
@@ -295,8 +296,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
                     host=host,
                     port=port,
                     tz_aware=tz_aware,
-                    # deserialize dicts as SONs
-                    document_class=SON,
+                    document_class=dict,
                     **kwargs
                 ),
                 db
@@ -351,7 +351,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         # call out to the DB
         resultset = self.collection.find(query, record_filter)
 
-        # it's ok to keep these as urls b/c the overall cache is indexed by course_key and this
+        # it's ok to keep these as deprecated strings b/c the overall cache is indexed by course_key and this
         # is a dictionary relative to that course
         results_by_url = {}
         root = None
@@ -412,7 +412,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
 
             # then look in any caching subsystem (e.g. memcached)
             if self.metadata_inheritance_cache_subsystem is not None:
-                tree = self.metadata_inheritance_cache_subsystem.get(course_id, {})
+                tree = self.metadata_inheritance_cache_subsystem.get(unicode(course_id), {})
             else:
                 logging.warning('Running MongoModuleStore without a metadata_inheritance_cache_subsystem. This is OK in localdev and testing environment. Not OK in production.')
 
@@ -422,7 +422,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
 
             # now write out computed tree to caching subsystem (e.g. memcached), if available
             if self.metadata_inheritance_cache_subsystem is not None:
-                self.metadata_inheritance_cache_subsystem.set(course_id, tree)
+                self.metadata_inheritance_cache_subsystem.set(unicode(course_id), tree)
 
         # now populate a request_cache, if available. NOTE, we are outside of the
         # scope of the above if: statement so that after a memcache hit, it'll get
@@ -591,7 +591,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
             raise ItemNotFoundError(location)
         return item
 
-    def get_course(self, course_key, depth=None):
+    def get_course(self, course_key, depth=0):
         """
         Get the course with the given courseid (org/course/run)
         """

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

@@ -337,7 +337,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 result.append(course_list[0])
         return result
 
-    def get_course(self, course_id, depth=None):
+    def get_course(self, course_id, depth=0):
         '''
         Gets the course descriptor for the course identified by the locator
         '''

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

@@ -99,7 +99,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             host=self.HOST,
             port=self.PORT,
             tz_aware=True,
-            document_class=bson.son.SON,
         )
         self.connection.drop_database(self.DB)
         self.addCleanup(self.connection.drop_database, self.DB)

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

@@ -66,7 +66,7 @@ class TestMongoModuleStore(unittest.TestCase):
             host=HOST,
             port=PORT,
             tz_aware=True,
-            document_class=bson.son.SON,
+            document_class=dict,
         )
         cls.connection.drop_database(DB)
 

common/lib/xmodule/xmodule/tests/test_conditional.py

@@ -253,3 +253,45 @@ class ConditionalModuleXmlTest(unittest.TestCase):
         print "post-attempt ajax: ", ajax
         html = ajax['html']
         self.assertTrue(any(['This is a secret' in item for item in html]))
+
+    def test_conditional_module_with_empty_sources_list(self):
+        """
+        If a ConditionalDescriptor is initialized with an empty sources_list, we assert that the sources_list is set
+        via generating UsageKeys from the values in xml_attributes['sources']
+        """
+        dummy_system = Mock()
+        dummy_location = Location("edX", "conditional_test", "test_run", "conditional", "SampleConditional", None)
+        dummy_scope_ids = ScopeIds(None, None, dummy_location, dummy_location)
+        dummy_field_data = DictFieldData({
+            'data': '<conditional/>',
+            'xml_attributes': {'sources': 'i4x://HarvardX/ER22x/poll_question/T15_poll'},
+            'children': None,
+        })
+        conditional = ConditionalDescriptor(
+            dummy_system,
+            dummy_field_data,
+            dummy_scope_ids,
+        )
+        self.assertEqual(
+            conditional.sources_list[0],
+            conditional.location.course_key.make_usage_key_from_deprecated_string(conditional.xml_attributes['sources'])
+        )
+
+    def test_conditional_module_parse_sources(self):
+        dummy_system = Mock()
+        dummy_location = Location("edX", "conditional_test", "test_run", "conditional", "SampleConditional", None)
+        dummy_scope_ids = ScopeIds(None, None, dummy_location, dummy_location)
+        dummy_field_data = DictFieldData({
+            'data': '<conditional/>',
+            'xml_attributes': {'sources': 'i4x://HarvardX/ER22x/poll_question/T15_poll;i4x://HarvardX/ER22x/poll_question/T16_poll'},
+            'children': None,
+        })
+        conditional = ConditionalDescriptor(
+            dummy_system,
+            dummy_field_data,
+            dummy_scope_ids,
+        )
+        self.assertEqual(
+            conditional.parse_sources(conditional.xml_attributes),
+            ['i4x://HarvardX/ER22x/poll_question/T15_poll', 'i4x://HarvardX/ER22x/poll_question/T16_poll']
+        )

common/static/js/capa/drag_and_drop/draggable_logic.js

@@ -237,10 +237,10 @@ return {
     },
 
     'toggleTargets': function (isEnabled) {
-        var effect = (isEnabled ? 'move' : undefined);
-        this.state.baseImageEl.attr('aria-dropeffect', effect);
+        var effect = isEnabled ? 'move' : null;
 
-        $.each(this.state.targets, function (target) {
+        this.state.baseImageEl.attr('aria-dropeffect', effect);
+        $.each(this.state.targets, function (index, target) {
             target.targetEl.attr('aria-dropeffect', effect);
         });
     },

lms/djangoapps/courseware/tests/test_video_handlers.py

@@ -66,8 +66,7 @@ def _clear_assets(location):
     for asset in assets:
         asset_location = AssetLocation._from_deprecated_son(asset["_id"], location.course_key.run)
         del_cached_content(asset_location)
-        mongo_id = asset_location.to_deprecated_son()
-        store.delete(mongo_id)
+        store.delete(asset_location)
 
 
 def _get_subs_id(filename):

lms/djangoapps/courseware/views.py

@@ -35,7 +35,7 @@ from course_modes.models import CourseMode
 
 from open_ended_grading import open_ended_notifications
 from student.models import UserTestGroup, CourseEnrollment
-from student.views import course_from_id, single_course_reverification_info
+from student.views import single_course_reverification_info
 from util.cache import cache, cache_if_anonymous
 from xblock.fragment import Fragment
 from xmodule.modulestore.django import modulestore
@@ -746,7 +746,7 @@ def fetch_reverify_banner_info(request, course_key):
     if not user.id:
         return reverifications
     enrollment = CourseEnrollment.get_or_create_enrollment(request.user, course_key)
-    course = course_from_id(course_key)
+    course = modulestore().get_course(course_key)
     info = single_course_reverification_info(user, course, enrollment)
     if info:
         reverifications[info.status].append(info)

lms/djangoapps/instructor/views/legacy.py

@@ -72,7 +72,6 @@ from student.models import (
     unique_id_for_user,
     anonymous_id_for_user
 )
-from student.views import course_from_id
 import track.views
 from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
@@ -352,7 +351,7 @@ def instructor_dashboard(request, course_id):
                 msg += message
 
     elif "Show Background Task History" in action:
-        problem_location = strip_if_string(request.POST.get('problem_for_all_students', ''))
+        problem_location_str = strip_if_string(request.POST.get('problem_for_all_students', ''))
         try:
             problem_location = course_key.make_usage_key_from_deprecated_string(problem_location_str)
         except InvalidKeyError:
@@ -1591,7 +1590,7 @@ def _do_unenroll_students(course_key, students, email_students=False):
         settings.SITE_NAME
     )
     if email_students:
-        course = course_from_id(course_key)
+        course = modulestore().get_course(course_key)
         #Composition of email
         d = {'site_name': stripped_site_name,
              'course': course}

lms/djangoapps/notes/views.py

@@ -6,11 +6,13 @@ from courseware.courses import get_course_with_access
 from notes.models import Note
 from notes.utils import notes_enabled_for_course
 from xmodule.annotator_token import retrieve_token
+from xmodule.modulestore.locations import SlashSeparatedCourseKey
 
 
 @login_required
 def notes(request, course_id):
     ''' Displays the student's notes. '''
+
     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
     course = get_course_with_access(request.user, 'load', course_key)
     if not notes_enabled_for_course(course):

lms/djangoapps/open_ended_grading/views.py

@@ -103,7 +103,7 @@ def find_peer_grading_module(course):
         except NoPathToItem:
             # In the case of nopathtoitem, the peer grading module that was found is in an invalid state, and
             # can no longer be accessed.  Log an informational message, but this will not impact normal behavior.
-            log.info(u"Invalid peer grading module location {0} in course {1}.  This module may need to be removed.".format(item_location, course.id))
+            log.info(u"Invalid peer grading module location %s in course %s.  This module may need to be removed.", item.location, course.id)
             continue
         problem_url = generate_problem_url(problem_url_parts, base_course_url)
         found_module = True

lms/djangoapps/shoppingcart/models.py

@@ -21,12 +21,9 @@ from django.core.urlresolvers import reverse
 from model_utils.managers import InheritanceManager
 
 from xmodule.modulestore.django import modulestore
-from xmodule.course_module import CourseDescriptor
-from xmodule.modulestore.exceptions import ItemNotFoundError
 
 from course_modes.models import CourseMode
 from edxmako.shortcuts import render_to_string
-from student.views import course_from_id
 from student.models import CourseEnrollment, unenroll_done
 from util.query import use_read_replica_if_available
 from xmodule_django.models import CourseKeyField
@@ -332,7 +329,7 @@ class PaidCourseRegistration(OrderItem):
         Returns the order item
         """
         # First a bunch of sanity checks
-        course = course_from_id(course_id)  # actually fetch the course to make sure it exists, use this to
+        course = modulestore().get_course(course_id)  # actually fetch the course to make sure it exists, use this to
                                                 # throw errors if it doesn't
         if not course:
             log.error("User {} tried to add non-existent course {} to cart id {}"
@@ -528,7 +525,7 @@ class CertificateItem(OrderItem):
         item.status = order.status
         item.qty = 1
         item.unit_cost = cost
-        course_name = course_from_id(course_id).display_name
+        course_name = modulestore().get_course(course_id).display_name
         item.line_desc = _("Certificate of Achievement, {mode_name} for course {course}").format(mode_name=mode_info.name,
                                                                                                  course=course_name)
         item.currency = currency
@@ -544,7 +541,7 @@ class CertificateItem(OrderItem):
         try:
             verification_attempt = SoftwareSecurePhotoVerification.active_for_user(self.course_enrollment.user)
             verification_attempt.submit()
-        except Exception as e:
+        except Exception:
             log.exception(
                 "Could not submit verification attempt for enrollment {}".format(self.course_enrollment)
             )
@@ -560,7 +557,7 @@ class CertificateItem(OrderItem):
 
     @property
     def single_item_receipt_context(self):
-        course = course_from_id(self.course_id)
+        course = modulestore().get_course(self.course_id)
         return {
             "course_id": self.course_id,
             "course_name": course.display_name_with_default,

lms/djangoapps/verify_student/views.py

@@ -23,7 +23,7 @@ from django.contrib.auth.decorators import login_required
 
 from course_modes.models import CourseMode
 from student.models import CourseEnrollment
-from student.views import course_from_id, reverification_info
+from student.views import reverification_info
 from shoppingcart.models import Order, CertificateItem
 from shoppingcart.processors.CyberSource import (
     get_signed_purchase_params, get_purchase_endpoint
@@ -36,6 +36,7 @@ import ssencrypt
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from .exceptions import WindowExpiredException
+from xmodule.modulestore.django import modulestore
 
 log = logging.getLogger(__name__)
 
@@ -83,7 +84,7 @@ class VerifyView(View):
         else:
             chosen_price = verify_mode.min_price
 
-        course = course_from_id(course_id)
+        course = modulestore().get_course(course_id)
         context = {
             "progress_state": progress_state,
             "user_full_name": request.user.profile.name,
@@ -133,7 +134,7 @@ class VerifiedView(View):
             verify_mode.min_price
         )
 
-        course = course_from_id(course_id)
+        course = modulestore().get_course(course_id)
         context = {
             "course_id": course_id.to_deprecated_string(),
             "course_modes_choose_url": reverse('course_modes_choose', kwargs={'course_id': course_id.to_deprecated_string()}),
@@ -271,7 +272,6 @@ def results_callback(request):
     # If this is a reverification, log an event
     if attempt.window:
         course_id = attempt.window.course_id
-        course = course_from_id(course_id)
         course_enrollment = CourseEnrollment.get_or_create_enrollment(attempt.user, course_id)
         course_enrollment.emit_event(EVENT_NAME_USER_REVERIFICATION_REVIEWED_BY_SOFTWARESECURE)
 
@@ -288,7 +288,7 @@ def show_requirements(request, course_id):
         return redirect(reverse('dashboard'))
 
     upgrade = request.GET.get('upgrade', False)
-    course = course_from_id(course_id)
+    course = modulestore().get_course(course_id)
     context = {
         "course_id": course_id.to_deprecated_string(),
         "course_modes_choose_url": reverse("course_modes_choose", kwargs={'course_id': course_id.to_deprecated_string()}),
@@ -372,7 +372,7 @@ class MidCourseReverifyView(View):
         display this view
         """
         course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id)
-        course = course_from_id(course_id)
+        course = modulestore().get_course(course_id)
         course_enrollment = CourseEnrollment.get_or_create_enrollment(request.user, course_id)
         course_enrollment.update_enrollment(mode="verified")
         course_enrollment.emit_event(EVENT_NAME_USER_ENTERED_MIDCOURSE_REVERIFY_VIEW)
@@ -440,7 +440,7 @@ def midcourse_reverify_dash(request):
     course_enrollment_pairs = []
     for enrollment in CourseEnrollment.enrollments_for_user(user):
         try:
-            course_enrollment_pairs.append((course_from_id(enrollment.course_id), enrollment))
+            course_enrollment_pairs.append((modulestore().get_course(enrollment.course_id), enrollment))
         except ItemNotFoundError:
             log.error("User {0} enrolled in non-existent course {1}"
                       .format(user.username, enrollment.course_id))

lms/templates/shoppingcart/verified_cert_receipt.html

 <%! from django.utils.translation import ugettext as _ %>
-<%! from student.views import course_from_id %>
 
 <%inherit file="../main.html" />
 <%block name="bodyclass">register verification-process step-confirmation</%block>