From b073cfaece205b1a8397726115fc34d148af5ff9 Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Mon, 2 Jun 2014 17:11:07 -0400 Subject: [PATCH 01/22] Fixes BLD-1104 --- common/static/js/capa/drag_and_drop/draggable_logic.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/common/static/js/capa/drag_and_drop/draggable_logic.js b/common/static/js/capa/drag_and_drop/draggable_logic.js index e7db461a37..99a7bb325d 100644 --- a/common/static/js/capa/drag_and_drop/draggable_logic.js +++ b/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); }); }, From 2c6437a279ec1889db5cac47c94b55f8e39dc27f Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 3 Jun 2014 11:46:36 -0400 Subject: [PATCH 02/22] Fix performance regression Remove course_from_id don't fetch whole course when the root will do don't fetch even the root if the id will do check for definition.data == null --- common/djangoapps/course_modes/views.py | 10 ++++------ common/djangoapps/external_auth/views.py | 4 ++-- common/djangoapps/student/views.py | 14 ++++---------- common/lib/xmodule/xmodule/modulestore/__init__.py | 4 ++-- common/lib/xmodule/xmodule/modulestore/mixed.py | 2 +- .../lib/xmodule/xmodule/modulestore/mongo/base.py | 11 ++++++----- .../xmodule/modulestore/split_mongo/split.py | 2 +- lms/djangoapps/courseware/views.py | 4 ++-- lms/djangoapps/instructor/views/legacy.py | 3 +-- lms/djangoapps/shoppingcart/models.py | 11 ++++------- lms/djangoapps/verify_student/views.py | 14 +++++++------- .../shoppingcart/verified_cert_receipt.html | 1 - 12 files changed, 34 insertions(+), 46 deletions(-) diff --git a/common/djangoapps/course_modes/views.py b/common/djangoapps/course_modes/views.py index 018dba5e7b..68b44e61cb 100644 --- a/common/djangoapps/course_modes/views.py +++ b/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 xmodule.modulestore.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, @@ -80,7 +78,7 @@ class ChooseModeView(View): # This is a bit redundant with logic in student.views.change_enrollement, # 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) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 37c7623602..9df444a85e 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -589,7 +589,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) @@ -607,7 +607,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 diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 97c7fcbe5d..d1ad604530 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -49,11 +49,10 @@ 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 xmodule.modulestore.locations import SlashSeparatedCourseKey -from xmodule.modulestore import XML_MODULESTORE_TYPE, Location +from xmodule.modulestore import XML_MODULESTORE_TYPE from collections import namedtuple @@ -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 diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 10816823f8..ce7f754eb3 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 11f5bd88ab..b4949aa6e2 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -157,7 +157,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 diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index db12b4f4f3..f277aeab95 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -184,7 +184,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, @@ -350,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 @@ -411,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.') @@ -421,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 @@ -590,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) """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 2d43de04f4..53d405a93c 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -319,7 +319,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 ''' diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index c7df0ca3fc..87bfdc4e15 100644 --- a/lms/djangoapps/courseware/views.py +++ b/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 @@ -719,7 +719,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) diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index 933f3e4d67..b8865418dc 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/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 @@ -1620,7 +1619,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} diff --git a/lms/djangoapps/shoppingcart/models.py b/lms/djangoapps/shoppingcart/models.py index b257345cf9..8e54a977b6 100644 --- a/lms/djangoapps/shoppingcart/models.py +++ b/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, diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index e5b08ad318..17d1513667 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/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 xmodule.modulestore.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)) diff --git a/lms/templates/shoppingcart/verified_cert_receipt.html b/lms/templates/shoppingcart/verified_cert_receipt.html index aa9f8e220a..44239622b5 100644 --- a/lms/templates/shoppingcart/verified_cert_receipt.html +++ b/lms/templates/shoppingcart/verified_cert_receipt.html @@ -1,5 +1,4 @@ <%! 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 From 84ee2b19f27728d4008ff2e4fe6abd802aaf56d7 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 4 Jun 2014 11:11:09 -0400 Subject: [PATCH 03/22] OpaqueKey copy and deepcopy are identity ops because keys are immutable --- common/lib/opaque_keys/opaque_keys/__init__.py | 13 +++++++++---- .../opaque_keys/tests/test_opaque_keys.py | 3 --- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/common/lib/opaque_keys/opaque_keys/__init__.py b/common/lib/opaque_keys/opaque_keys/__init__.py index bd097638f5..2a8af4b52a 100644 --- a/common/lib/opaque_keys/opaque_keys/__init__.py +++ b/common/lib/opaque_keys/opaque_keys/__init__.py @@ -187,12 +187,17 @@ class OpaqueKey(object): return self.NAMESPACE_SEPARATOR.join([self.CANONICAL_NAMESPACE, self._to_string()]) # pylint: disable=no-member def __copy__(self): - return self.replace() + """ + Because it's immutable, return itself + """ + return self def __deepcopy__(self, memo): - return self.replace(**{ - key: deepcopy(getattr(self, key), memo) for key in self.KEY_FIELDS # pylint: disable=no-member - }) + """ + Because it's immutable, return itself + """ + memo[id(self)] = self + return self def __setstate__(self, state_dict): # used by pickle to set fields on an unpickled object diff --git a/common/lib/opaque_keys/opaque_keys/tests/test_opaque_keys.py b/common/lib/opaque_keys/opaque_keys/tests/test_opaque_keys.py index 4f9c8ef5a9..100dd08844 100644 --- a/common/lib/opaque_keys/opaque_keys/tests/test_opaque_keys.py +++ b/common/lib/opaque_keys/opaque_keys/tests/test_opaque_keys.py @@ -136,12 +136,9 @@ class KeyTests(TestCase): deep = copy.deepcopy(original) self.assertEquals(original, copied) - self.assertNotEquals(id(original), id(copied)) self.assertEquals(id(original.value), id(copied.value)) self.assertEquals(original, deep) - self.assertNotEquals(id(original), id(deep)) - self.assertNotEquals(id(original.value), id(deep.value)) self.assertEquals(copy.deepcopy([original]), [original]) From d66b480740cd6be75a30587a3d4e578c82c4e03f Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 4 Jun 2014 14:10:16 -0400 Subject: [PATCH 04/22] pymongo son optimizations --- requirements/edx/base.txt | 2 +- requirements/edx/github.txt | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 16854aad36..7955137333 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -56,7 +56,7 @@ polib==1.0.3 pycrypto>=2.6 pygments==1.6 pygraphviz==1.1 -pymongo==2.4.1 +# pymongo==2.4.1 pyparsing==2.0.1 python-memcached==1.48 python-openid==2.2.5 diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index ac135296f4..bc014261aa 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -16,6 +16,9 @@ -e git+https://github.com/un33k/django-ipware.git@42cb1bb1dc680a60c6452e8bb2b843c2a0382c90#egg=django-ipware -e git+https://github.com/appliedsec/pygeoip.git@95e69341cebf5a6a9fbf7c4f5439d458898bdc3b#egg=pygeoip +# Temporary update to 3rd party libs: +-e git+https://github.com/edx/mongo-python-driver.git@aa99f4dc470bfc0f16fec02ea3aafd4fa27416b2#egg=pymongo + # Our libraries: -e git+https://github.com/edx/XBlock.git@fc5fea25c973ec66d8db63cf69a817ce624f5ef5#egg=XBlock -e git+https://github.com/edx/codejail.git@71f5c5616e2a73ae8cecd1ff2362774a773d3665#egg=codejail From 36dacba63a2032c2bb249287a87e3058c1cae6fe Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 4 Jun 2014 15:24:39 -0400 Subject: [PATCH 05/22] Update to tagged version of pymongo --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index bc014261aa..427ebcf2b5 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -17,7 +17,7 @@ -e git+https://github.com/appliedsec/pygeoip.git@95e69341cebf5a6a9fbf7c4f5439d458898bdc3b#egg=pygeoip # Temporary update to 3rd party libs: --e git+https://github.com/edx/mongo-python-driver.git@aa99f4dc470bfc0f16fec02ea3aafd4fa27416b2#egg=pymongo +-e git+https://github.com/edx/mongo-python-driver.git@2.4.1a#egg=pymongo # Our libraries: -e git+https://github.com/edx/XBlock.git@fc5fea25c973ec66d8db63cf69a817ce624f5ef5#egg=XBlock From 6230e14d23ae3868e1adae73262c7a65771be7d5 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Wed, 4 Jun 2014 17:31:16 -0400 Subject: [PATCH 06/22] Use commit hash v tag for pymongo --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 427ebcf2b5..726e7c0692 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -17,7 +17,7 @@ -e git+https://github.com/appliedsec/pygeoip.git@95e69341cebf5a6a9fbf7c4f5439d458898bdc3b#egg=pygeoip # Temporary update to 3rd party libs: --e git+https://github.com/edx/mongo-python-driver.git@2.4.1a#egg=pymongo +-e git+https://github.com/edx/mongo-python-driver.git@dd01bf051dd5373146f668b9c78da9cb15c82331#egg=pymongo # Our libraries: -e git+https://github.com/edx/XBlock.git@fc5fea25c973ec66d8db63cf69a817ce624f5ef5#egg=XBlock From a8a77b34cd93386cd7db9ba4c916d60267789391 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Thu, 5 Jun 2014 10:41:10 -0400 Subject: [PATCH 07/22] Update hash to latest version. --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 726e7c0692..35b797b232 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -17,7 +17,7 @@ -e git+https://github.com/appliedsec/pygeoip.git@95e69341cebf5a6a9fbf7c4f5439d458898bdc3b#egg=pygeoip # Temporary update to 3rd party libs: --e git+https://github.com/edx/mongo-python-driver.git@dd01bf051dd5373146f668b9c78da9cb15c82331#egg=pymongo +-e git+https://github.com/edx/mongo-python-driver.git@1882623fcf1689fe4e7ca621a49dbb57f90f5a04#egg=pymongo # Our libraries: -e git+https://github.com/edx/XBlock.git@fc5fea25c973ec66d8db63cf69a817ce624f5ef5#egg=XBlock From 729f0eb2032b15e9d5f8f73ac49537474b59fef6 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Wed, 4 Jun 2014 15:17:40 -0400 Subject: [PATCH 08/22] Fix variable name. --- lms/djangoapps/instructor/views/legacy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index b8865418dc..ba5f522f48 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -351,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: From 29bb179afc0827c334a1cf7e1ff6f4f572a80e2f Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 5 Jun 2014 15:26:59 -0400 Subject: [PATCH 09/22] Revert back to mongo-python-driver based on the 2.4.1 tag. --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 35b797b232..726e7c0692 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -17,7 +17,7 @@ -e git+https://github.com/appliedsec/pygeoip.git@95e69341cebf5a6a9fbf7c4f5439d458898bdc3b#egg=pygeoip # Temporary update to 3rd party libs: --e git+https://github.com/edx/mongo-python-driver.git@1882623fcf1689fe4e7ca621a49dbb57f90f5a04#egg=pymongo +-e git+https://github.com/edx/mongo-python-driver.git@dd01bf051dd5373146f668b9c78da9cb15c82331#egg=pymongo # Our libraries: -e git+https://github.com/edx/XBlock.git@fc5fea25c973ec66d8db63cf69a817ce624f5ef5#egg=XBlock From 3f3e9724d036aae28f255a55c0f38a1aaae806b3 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Wed, 4 Jun 2014 18:27:02 +0000 Subject: [PATCH 10/22] Add fallback for org based roles --- .../contentstore/tests/test_course_listing.py | 34 +++++++++++++++++-- cms/djangoapps/contentstore/views/access.py | 8 ++++- cms/djangoapps/contentstore/views/course.py | 23 ++++++++++--- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 9bcdeb707b..6f73530ef4 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/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 xmodule.modulestore.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 @@ -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) diff --git a/cms/djangoapps/contentstore/views/access.py b/cms/djangoapps/contentstore/views/access.py index b77c026d39..5e81ce3091 100644 --- a/cms/djangoapps/contentstore/views/access.py +++ b/cms/djangoapps/contentstore/views/access.py @@ -1,4 +1,6 @@ -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)) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 6cb11376da..fbd75c7dae 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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,16 @@ 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: + raise ItemNotFoundError + 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 +226,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) From e3459cd3cb474fb8018848a5f4476df280559496 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Thu, 5 Jun 2014 17:33:07 +0000 Subject: [PATCH 11/22] Opaque-Keys: Response to code review --- .../contentstore/tests/test_permissions.py | 40 +++++++++++++------ cms/djangoapps/contentstore/views/course.py | 3 +- common/djangoapps/student/roles.py | 4 ++ .../lib/xmodule/xmodule/contentstore/mongo.py | 15 ++++--- requirements/edx/base.txt | 2 +- requirements/edx/github.txt | 3 -- 6 files changed, 44 insertions(+), 23 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_permissions.py b/cms/djangoapps/contentstore/tests/test_permissions.py index afa357a9cd..dd4903d74c 100644 --- a/cms/djangoapps/contentstore/tests/test_permissions.py +++ b/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 xmodule.modulestore.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,15 @@ 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): + # pylint: disable=protected-access + group = role(self.course_key.org) + else: + # pylint: disable=protected-access + 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 +114,27 @@ 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 +143,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)) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index fbd75c7dae..468c16e023 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -204,7 +204,8 @@ def _accessible_courses_list_from_groups(request): try: course = modulestore('direct').get_course(course_key) except ItemNotFoundError: - raise 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 diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 89043b2c3a..6c56b934b9 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -7,6 +7,7 @@ from abc import ABCMeta, abstractmethod from django.contrib.auth.models import User from student.models import CourseAccessRole +from xmodule_django.models import CourseKeyField class AccessRole(object): @@ -129,6 +130,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, diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index 9cad2f1014..6aac2a43b0 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -63,10 +63,11 @@ 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) @@ -282,4 +283,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) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 7955137333..16854aad36 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -56,7 +56,7 @@ polib==1.0.3 pycrypto>=2.6 pygments==1.6 pygraphviz==1.1 -# pymongo==2.4.1 +pymongo==2.4.1 pyparsing==2.0.1 python-memcached==1.48 python-openid==2.2.5 diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index 726e7c0692..ac135296f4 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -16,9 +16,6 @@ -e git+https://github.com/un33k/django-ipware.git@42cb1bb1dc680a60c6452e8bb2b843c2a0382c90#egg=django-ipware -e git+https://github.com/appliedsec/pygeoip.git@95e69341cebf5a6a9fbf7c4f5439d458898bdc3b#egg=pygeoip -# Temporary update to 3rd party libs: --e git+https://github.com/edx/mongo-python-driver.git@dd01bf051dd5373146f668b9c78da9cb15c82331#egg=pymongo - # Our libraries: -e git+https://github.com/edx/XBlock.git@fc5fea25c973ec66d8db63cf69a817ce624f5ef5#egg=XBlock -e git+https://github.com/edx/codejail.git@71f5c5616e2a73ae8cecd1ff2362774a773d3665#egg=codejail From ade82c2d0549e4aad41419957569df728431121e Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Thu, 5 Jun 2014 15:41:41 +0000 Subject: [PATCH 12/22] Notes convert course_id to course_key --- lms/djangoapps/notes/views.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/notes/views.py b/lms/djangoapps/notes/views.py index 88dc78f741..0cdc5a3660 100644 --- a/lms/djangoapps/notes/views.py +++ b/lms/djangoapps/notes/views.py @@ -11,11 +11,13 @@ from xmodule.annotator_token import retrieve_token def notes(request, course_id): ''' Displays the student's notes. ''' - course = get_course_with_access(request.user, 'load', course_id) + 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): raise Http404 - notes = Note.objects.filter(course_id=course_id, user=request.user).order_by('-created', 'uri') + notes = Note.objects.filter(course_id=course_key, user=request.user).order_by('-created', 'uri') student = request.user storage = course.annotation_storage_url From acc02b0e094892929a4090796091086b17c312cb Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 5 Jun 2014 12:17:00 -0400 Subject: [PATCH 13/22] Fix to excess asset query --- .../contentstore/features/transcripts.py | 2 +- .../tests/test_transcripts_utils.py | 4 ++-- .../xmodule/xmodule/contentstore/content.py | 2 +- .../lib/xmodule/xmodule/contentstore/mongo.py | 23 +++++++------------ .../courseware/tests/test_video_handlers.py | 3 +-- 5 files changed, 13 insertions(+), 21 deletions(-) diff --git a/cms/djangoapps/contentstore/features/transcripts.py b/cms/djangoapps/contentstore/features/transcripts.py index 4e70912c3d..0783d01fea 100644 --- a/cms/djangoapps/contentstore/features/transcripts.py +++ b/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.') diff --git a/cms/djangoapps/contentstore/tests/test_transcripts_utils.py b/cms/djangoapps/contentstore/tests/test_transcripts_utils.py index e00e3fdad1..f0272764b4 100644 --- a/cms/djangoapps/contentstore/tests/test_transcripts_utils.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index c6e8b58be4..8b9f6706af 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -59,7 +59,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() diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index 6aac2a43b0..d80819faea 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -12,7 +12,7 @@ from xmodule.exceptions import NotFoundError from fs.osfs import OSFS import os import json -import bson.son +from bson.son import SON from xmodule.modulestore.locations import AssetLocation @@ -29,7 +29,7 @@ class MongoContentStore(ContentStore): pymongo.MongoClient( host=host, port=port, - document_class=bson.son.SON, + document_class=SON, **kwargs ), db @@ -43,7 +43,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) @@ -71,13 +71,6 @@ class MongoContentStore(ContentStore): 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: @@ -104,13 +97,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']) @@ -246,10 +239,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): """ @@ -262,7 +255,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 diff --git a/lms/djangoapps/courseware/tests/test_video_handlers.py b/lms/djangoapps/courseware/tests/test_video_handlers.py index 4e332c91a8..570fa0e0bc 100644 --- a/lms/djangoapps/courseware/tests/test_video_handlers.py +++ b/lms/djangoapps/courseware/tests/test_video_handlers.py @@ -67,8 +67,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): From c8082770073da04e6fe9d1daa3784b742a0826b7 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 5 Jun 2014 14:48:17 -0400 Subject: [PATCH 14/22] Change document_class back to dict --- cms/djangoapps/contentstore/tests/test_course_listing.py | 2 +- common/djangoapps/student/tests/test_course_listing.py | 2 +- common/lib/xmodule/xmodule/contentstore/mongo.py | 2 +- common/lib/xmodule/xmodule/modulestore/mongo/base.py | 3 +-- .../xmodule/modulestore/tests/test_mixed_modulestore.py | 1 - common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py | 2 +- 6 files changed, 5 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 6f73530ef4..1ead15c37e 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -265,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'], }}, diff --git a/common/djangoapps/student/tests/test_course_listing.py b/common/djangoapps/student/tests/test_course_listing.py index 6decc8cbe2..f0bc2bb67e 100644 --- a/common/djangoapps/student/tests/test_course_listing.py +++ b/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'], }}, diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index d80819faea..8763f5233a 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -29,7 +29,7 @@ class MongoContentStore(ContentStore): pymongo.MongoClient( host=host, port=port, - document_class=SON, + document_class=dict, **kwargs ), db diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index f277aeab95..cffd73fe31 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -295,8 +295,7 @@ class MongoModuleStore(ModuleStoreWriteBase): host=host, port=port, tz_aware=tz_aware, - # deserialize dicts as SONs - document_class=SON, + document_class=dict, **kwargs ), db diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 084fb7d91d..8cdb614191 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index ad2f121aa7..ca07038831 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/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) From dc7b2f076201b17d0aac86ebcb30ce330ccc8b31 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 6 Jun 2014 12:30:29 -0400 Subject: [PATCH 15/22] Don't create model objects to check the roles cache --- common/djangoapps/student/roles.py | 37 ++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 6c56b934b9..640b7cd703 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -10,6 +10,27 @@ 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): """ Object representing a role with particular access to a resource @@ -94,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): """ @@ -232,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): """ From 3a9f0b3f74eb9f2b7c4c0b1410beabb078abb55b Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Mon, 9 Jun 2014 16:11:07 +0000 Subject: [PATCH 16/22] Fix import --- lms/djangoapps/notes/views.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/notes/views.py b/lms/djangoapps/notes/views.py index 0cdc5a3660..59820ceedf 100644 --- a/lms/djangoapps/notes/views.py +++ b/lms/djangoapps/notes/views.py @@ -5,6 +5,7 @@ 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 From 2757ac4fcb23bfa970691c515e6e351a06bcd7f7 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Mon, 9 Jun 2014 17:30:22 +0000 Subject: [PATCH 17/22] Opaque-keys: response to code review --- cms/djangoapps/contentstore/tests/test_permissions.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_permissions.py b/cms/djangoapps/contentstore/tests/test_permissions.py index dd4903d74c..2534cd8c86 100644 --- a/cms/djangoapps/contentstore/tests/test_permissions.py +++ b/cms/djangoapps/contentstore/tests/test_permissions.py @@ -99,10 +99,8 @@ class TestCourseAccess(ModuleStoreTestCase): user_by_role[role] = [] # Org-based roles are created via org name, rather than course_key if (role is OrgStaffRole) or (role is OrgInstructorRole): - # pylint: disable=protected-access group = role(self.course_key.org) else: - # pylint: disable=protected-access 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 @@ -125,7 +123,8 @@ class TestCourseAccess(ModuleStoreTestCase): auth.add_users( self.user, role(copy_course_key.org), - *role(self.course_key.org).users_with_role()) + *role(self.course_key.org).users_with_role() + ) else: auth.add_users( self.user, From 87557a1af81bccf2ac1878f4dc2252dcf7979b91 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 9 Jun 2014 14:40:59 -0400 Subject: [PATCH 18/22] Add tests of RoleCache --- common/djangoapps/student/tests/test_roles.py | 45 +++++++++++++++++-- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index c5d6acdabd..27dfa906d0 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -1,14 +1,16 @@ """ 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 xmodule.modulestore.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)) From a9cad6a5bb1f5d32b618b0ad4fb4cfa9ff403be9 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Mon, 9 Jun 2014 14:25:51 -0400 Subject: [PATCH 19/22] Fix logging bug in openended grading --- lms/djangoapps/open_ended_grading/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/open_ended_grading/views.py b/lms/djangoapps/open_ended_grading/views.py index 26c6bbe021..804c8ca27b 100644 --- a/lms/djangoapps/open_ended_grading/views.py +++ b/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 From baf0cee0be6a357f31ec29fb962bb6a419493884 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Tue, 27 May 2014 11:53:43 -0400 Subject: [PATCH 20/22] Fix gen_cert_report to use CourseKeys. --- .../management/commands/gen_cert_report.py | 127 ++++++++++-------- 1 file changed, 71 insertions(+), 56 deletions(-) diff --git a/lms/djangoapps/certificates/management/commands/gen_cert_report.py b/lms/djangoapps/certificates/management/commands/gen_cert_report.py index 018e5e592b..3c37069753 100644 --- a/lms/djangoapps/certificates/management/commands/gen_cert_report.py +++ b/lms/djangoapps/certificates/management/commands/gen_cert_report.py @@ -1,9 +1,16 @@ -from django.core.management.base import BaseCommand +""" +Generate a report of certificate statuses +""" + +from django.core.management.base import BaseCommand, CommandError from certificates.models import GeneratedCertificate from django.contrib.auth.models import User from optparse import make_option from django.conf import settings +from opaque_keys import InvalidKeyError from xmodule.course_module import CourseDescriptor +from xmodule.modulestore.keys import CourseKey +from xmodule.modulestore.locations import SlashSeparatedCourseKey from xmodule.modulestore.django import modulestore from django.db.models import Count @@ -12,7 +19,7 @@ class Command(BaseCommand): help = """ - Generate a certificate status report for all courses that have ended. + Generate a certificate status report for a given course. This command does not do anything other than report the current certificate status. @@ -34,81 +41,89 @@ class Command(BaseCommand): dest='course', default=None, help='Only generate for COURSE_ID'), - ) - - def _ended_courses(self): - for course_id in [course # all courses in COURSE_LISTINGS - for sub in settings.COURSE_LISTINGS - for course in settings.COURSE_LISTINGS[sub]]: - course = modulestore().get_course(course_id) - if course.has_ended(): - yield course_id + ) def handle(self, *args, **options): # Find all courses that have ended if options['course']: - ended_courses = [options['course']] + try: + course_id = CourseKey.from_string(options['course']) + except InvalidKeyError: + print("Course id {} could not be parsed as a CourseKey; falling back to SSCK.from_dep_str".format(options['course'])) + course_id = SlashSeparatedCourseKey.from_deprecated_string(options['course']) else: - ended_courses = self._ended_courses() + raise CommandError("You must specify a course") cert_data = {} - for course_id in ended_courses: + # find students who are active + # number of enrolled students = downloadable + notpassing + print "Looking up certificate states for {0}".format(options['course']) + enrolled_current = User.objects.filter( + courseenrollment__course_id=course_id, + courseenrollment__is_active=True + ) + enrolled_total = User.objects.filter( + courseenrollment__course_id=course_id + ) + verified_enrolled = GeneratedCertificate.objects.filter( + course_id__exact=course_id, mode__exact='verified' + ) + honor_enrolled = GeneratedCertificate.objects.filter( + course_id__exact=course_id, mode__exact='honor' + ) + audit_enrolled = GeneratedCertificate.objects.filter( + course_id__exact=course_id, mode__exact='audit' + ) - # find students who are active - # enrolled students are always downloable + notpassing - print "Looking up certificate states for {0}".format(course_id) - enrolled_current = User.objects.filter( - courseenrollment__course_id=course_id, - courseenrollment__is_active=True) - enrolled_total = User.objects.filter( - courseenrollment__course_id=course_id) - verified_enrolled = GeneratedCertificate.objects.filter( - course_id__exact=course_id, mode__exact='verified') - honor_enrolled = GeneratedCertificate.objects.filter( - course_id__exact=course_id, mode__exact='honor') - audit_enrolled = GeneratedCertificate.objects.filter( - course_id__exact=course_id, mode__exact='audit') + cert_data[course_id] = { + 'enrolled_current': enrolled_current.count(), + 'enrolled_total': enrolled_total.count(), + 'verified_enrolled': verified_enrolled.count(), + 'honor_enrolled': honor_enrolled.count(), + 'audit_enrolled': audit_enrolled.count() + } - cert_data[course_id] = {'enrolled_current': enrolled_current.count(), - 'enrolled_total': enrolled_total.count(), - 'verified_enrolled': verified_enrolled.count(), - 'honor_enrolled': honor_enrolled.count(), - 'audit_enrolled': audit_enrolled.count()} + status_tally = GeneratedCertificate.objects.filter( + course_id__exact=course_id + ).values('status').annotate( + dcount=Count('status') + ) - status_tally = GeneratedCertificate.objects.filter( - course_id__exact=course_id).values('status').annotate( - dcount=Count('status')) - cert_data[course_id].update( - {status['status']: status['dcount'] - for status in status_tally}) + cert_data[course_id].update( + {status['status']: status['dcount'] + for status in status_tally}) - mode_tally = GeneratedCertificate.objects.filter( - course_id__exact=course_id, - status__exact='downloadable').values('mode').annotate( - dcount=Count('mode')) - cert_data[course_id].update( - {mode['mode']: mode['dcount'] - for mode in mode_tally}) + mode_tally = GeneratedCertificate.objects.filter( + course_id__exact=course_id, + status__exact='downloadable' + ).values('mode').annotate( + dcount=Count('mode') + ) + cert_data[course_id].update( + {mode['mode']: mode['dcount'] + for mode in mode_tally} + ) # all states we have seen far all courses status_headings = sorted(set( [status for course in cert_data - for status in cert_data[course]])) + for status in cert_data[course]]) + ) # print the heading for the report print "{:>26}".format("course ID"), print ' '.join(["{:>16}".format(heading) - for heading in status_headings]) + for heading in status_headings] + ) # print the report - for course_id in cert_data: - print "{0:>26}".format(course_id[0:24]), - for heading in status_headings: - if heading in cert_data[course_id]: - print "{:>16}".format(cert_data[course_id][heading]), - else: - print " " * 16, - print + print "{0:>26}".format(course_id.to_deprecated_string()), + for heading in status_headings: + if heading in cert_data[course_id]: + print "{:>16}".format(cert_data[course_id][heading]), + else: + print " " * 16, + print From bf6c342dcfcf3bda7c82bca85e5108210b4da71d Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Thu, 29 May 2014 14:58:22 -0400 Subject: [PATCH 21/22] Fix location.url [LMS-2786] --- common/djangoapps/terrain/ui_helpers.py | 21 ++++++++++++++++++- .../courseware/features/conditional.py | 7 +++++-- lms/templates/conditional_module.html | 4 ++-- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/terrain/ui_helpers.py b/common/djangoapps/terrain/ui_helpers.py index 38e984e41a..7260f70314 100644 --- a/common/djangoapps/terrain/ui_helpers.py +++ b/common/djangoapps/terrain/ui_helpers.py @@ -22,7 +22,7 @@ from selenium.common.exceptions import ( from selenium.webdriver.support import expected_conditions as EC from selenium.webdriver.common.by import By from selenium.webdriver.support.ui import WebDriverWait -from nose.tools import assert_true # pylint: disable=E0611 +from nose.tools import assert_true # pylint: disable=no-name-in-module REQUIREJS_WAIT = { @@ -305,6 +305,25 @@ def css_has_text(css_selector, text, index=0, strip=False): return actual_text == text +@world.absorb +def css_contains_text(css_selector, partial_text, index=0): + """ + Return a boolean indicating whether the element with `css_selector` + contains `partial_text`. + + If there are multiple elements matching the css selector, + use `index` to indicate which one. + """ + # If we're expecting a non-empty string, give the page + # a chance to fill in text fields. + if partial_text: + wait_for(lambda _: css_text(css_selector, index=index)) + + actual_text = css_text(css_selector, index=index) + + return partial_text in actual_text + + @world.absorb def css_has_value(css_selector, value, index=0): """ diff --git a/lms/djangoapps/courseware/features/conditional.py b/lms/djangoapps/courseware/features/conditional.py index db6b0436cd..4ec593feb8 100644 --- a/lms/djangoapps/courseware/features/conditional.py +++ b/lms/djangoapps/courseware/features/conditional.py @@ -1,6 +1,8 @@ +# pylint: disable=C0111 +# pylint: disable=W0621 from lettuce import world, steps -from nose.tools import assert_in, assert_equals, assert_true +from nose.tools import assert_in, assert_true # pylint: disable=no-name-in-module from common import i_am_registered_for_the_course, visit_scenario_item from problems_setup import add_problem_to_course, answer_problem @@ -90,6 +92,7 @@ class ConditionalSteps(object): assert_true(world.css_visible('.hidden-contents')) else: assert_true(world.is_css_not_present('.hidden-contents')) + assert_true(world.css_contains_text('.conditional-message', 'must be attempted before this will become visible.')) # sarina def answer_poll(self, step, answer): r' I answer the conditioned poll "([^"]*)"$' @@ -116,4 +119,4 @@ class ConditionalSteps(object): return -ConditionalSteps() \ No newline at end of file +ConditionalSteps() diff --git a/lms/templates/conditional_module.html b/lms/templates/conditional_module.html index ff37a3521b..fa4b7f3a68 100644 --- a/lms/templates/conditional_module.html +++ b/lms/templates/conditional_module.html @@ -5,11 +5,11 @@ from django.core.urlresolvers import reverse def _message(reqm, message): return message.format(link="{url_name}".format( url = reverse('jump_to', kwargs=dict(course_id=reqm.course_id.to_deprecated_string(), - location=reqm.location.url())), + location=reqm.location.to_deprecated_string())), url_name = reqm.display_name_with_default)) %> % if message: % for reqm in module.required_modules: -

${_message(reqm, message)}

+

${_message(reqm, message)}

% endfor % endif From 6e702c31312c6c80fc1a678a7527f3fc28732704 Mon Sep 17 00:00:00 2001 From: Julia Hansbrough Date: Mon, 9 Jun 2014 21:55:27 +0000 Subject: [PATCH 22/22] Fix conditional module rendering --- .../lib/xmodule/xmodule/conditional_module.py | 5 ++- .../xmodule/xmodule/tests/test_conditional.py | 42 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/conditional_module.py b/common/lib/xmodule/xmodule/conditional_module.py index 809c6ef54d..d68dd00741 100644 --- a/common/lib/xmodule/xmodule/conditional_module.py +++ b/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): diff --git a/common/lib/xmodule/xmodule/tests/test_conditional.py b/common/lib/xmodule/xmodule/tests/test_conditional.py index d985ff7ef4..4667bfbaa5 100644 --- a/common/lib/xmodule/xmodule/tests/test_conditional.py +++ b/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': '', + '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': '', + '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'] + )