From 42ea4f3271661cef95df928b78f06d0e5ed8beda Mon Sep 17 00:00:00 2001 From: kimth Date: Wed, 15 Aug 2012 10:13:35 -0400 Subject: [PATCH 01/16] Rename acquire_lock to select_for_update, add docstring --- lms/djangoapps/courseware/models.py | 10 ++++++---- lms/djangoapps/courseware/module_render.py | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index 5fae05c177..261140dec7 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -67,7 +67,7 @@ class StudentModuleCache(object): """ A cache of StudentModules for a specific student """ - def __init__(self, user, descriptors, acquire_lock=False): + def __init__(self, user, descriptors, select_for_update=False): ''' Find any StudentModule objects that are needed by any descriptor in descriptors. Avoids making multiple queries to the database. @@ -77,6 +77,7 @@ class StudentModuleCache(object): Arguments user: The user for which to fetch maching StudentModules descriptors: An array of XModuleDescriptors. + select_for_update: Flag indicating whether the row should be locked until end of transaction ''' if user.is_authenticated(): module_ids = self._get_module_state_keys(descriptors) @@ -86,7 +87,7 @@ class StudentModuleCache(object): self.cache = [] chunk_size = 500 for id_chunk in [module_ids[i:i + chunk_size] for i in xrange(0, len(module_ids), chunk_size)]: - if acquire_lock: + if select_for_update: self.cache.extend(StudentModule.objects.select_for_update().filter( student=user, module_state_key__in=id_chunk) @@ -102,13 +103,14 @@ class StudentModuleCache(object): @classmethod - def cache_for_descriptor_descendents(cls, user, descriptor, depth=None, descriptor_filter=lambda descriptor: True, acquire_lock=False): + def cache_for_descriptor_descendents(cls, user, descriptor, depth=None, descriptor_filter=lambda descriptor: True, select_for_update=False): """ descriptor: An XModuleDescriptor depth is the number of levels of descendent modules to load StudentModules for, in addition to the supplied descriptor. If depth is None, load all descendent StudentModules descriptor_filter is a function that accepts a descriptor and return wether the StudentModule should be cached + select_for_update: Flag indicating whether the row should be locked until end of transaction """ def get_child_descriptors(descriptor, depth, descriptor_filter): @@ -128,7 +130,7 @@ class StudentModuleCache(object): descriptors = get_child_descriptors(descriptor, depth, descriptor_filter) - return StudentModuleCache(user, descriptors, acquire_lock) + return StudentModuleCache(user, descriptors, select_for_update) def _get_module_state_keys(self, descriptors): ''' diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 000d1ca830..66db426434 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -302,7 +302,7 @@ def xqueue_callback(request, course_id, userid, id, dispatch): user = User.objects.get(id=userid) student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( - user, modulestore().get_item(id), depth=0, acquire_lock=True) + user, modulestore().get_item(id), depth=0, select_for_update=True) instance = get_module(user, request, id, student_module_cache) instance_module = get_instance_module(user, instance, student_module_cache) From 9a14af4ba1e6580674ee02febdfd77e959f17807 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 15 Aug 2012 10:27:07 -0400 Subject: [PATCH 02/16] address comment on #412 --- common/lib/xmodule/xmodule/abtest_module.py | 6 +++--- common/lib/xmodule/xmodule/x_module.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/abtest_module.py b/common/lib/xmodule/xmodule/abtest_module.py index ca00db4c9a..ceca6ff9ed 100644 --- a/common/lib/xmodule/xmodule/abtest_module.py +++ b/common/lib/xmodule/xmodule/abtest_module.py @@ -49,9 +49,9 @@ class ABTestModule(XModule): return json.dumps({'group': self.group}) def displayable_items(self): - return filter(None, [self.system.get_module(child) - for child - in self.definition['data']['group_content'][self.group]]) + child_locations = self.definition['data']['group_content'][self.group] + children = [self.system.get_module(loc) for loc in child_locations] + return [c for c in children if c is not None] # TODO (cpennington): Use Groups should be a first class object, rather than being diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 06449dc37f..1063c5428b 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -219,11 +219,11 @@ class XModule(HTMLSnippet): Return module instances for all the children of this module. ''' if self._loaded_children is None: + child_locations = self.definition.get('children', []) + children = [self.system.get_module(loc) for loc in child_locations] # get_module returns None if the current user doesn't have access # to the location. - self._loaded_children = filter(None, - [self.system.get_module(child) - for child in self.definition.get('children', [])]) + self._loaded_children = [c for c in children if c is not None] return self._loaded_children From 9425bbba609029c44d8633afd044aba8dd205ff0 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 15 Aug 2012 10:28:08 -0400 Subject: [PATCH 03/16] Address other comment on #413 - don't call has_access directly from template, pass a staff_access variable instead --- lms/djangoapps/courseware/access.py | 2 +- lms/djangoapps/courseware/views.py | 30 +++++++++++++++++++--------- lms/djangoapps/simplewiki/views.py | 5 +++++ lms/djangoapps/staticbook/views.py | 10 ++++++++-- lms/templates/course_navigation.html | 2 +- 5 files changed, 36 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 9605c827de..1494de6662 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -65,7 +65,7 @@ def has_access(user, obj, action): # Passing an unknown object here is a coding error, so rather than # returning a default, complain. - raise TypeError("Unknown object type in has_access(). Object type: '{}'" + raise TypeError("Unknown object type in has_access(): '{}'" .format(type(obj))) # ================ Implementation helpers ================================ diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index ab63872170..98de50c089 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -110,6 +110,7 @@ def index(request, course_id, chapter=None, section=None, - HTTPresponse """ course = get_course_with_access(request.user, course_id, 'load') + staff_access = has_access(request.user, course, 'staff') registered = registered_for_course(course, request.user) if not registered: # TODO (vshnayder): do course instructors need to be registered to see course? @@ -123,7 +124,8 @@ def index(request, course_id, chapter=None, section=None, 'COURSE_TITLE': course.title, 'course': course, 'init': '', - 'content': '' + 'content': '', + 'staff_access': staff_access, } look_for_module = chapter is not None and section is not None @@ -166,7 +168,8 @@ def index(request, course_id, chapter=None, section=None, position=position )) try: - result = render_to_response('courseware-error.html', {}) + result = render_to_response('courseware-error.html', + {'staff_access': staff_access}) except: result = HttpResponse("There was an unrecoverable error") @@ -208,8 +211,10 @@ def course_info(request, course_id): Assumes the course_id is in a valid format. """ course = get_course_with_access(request.user, course_id, 'load') + staff_access = has_access(request.user, course, 'staff') - return render_to_response('info.html', {'course': course}) + return render_to_response('info.html', {'course': course, + 'staff_access': staff_access,}) def registered_for_course(course, user): @@ -257,13 +262,14 @@ def profile(request, course_id, student_id=None): Course staff are allowed to see the profiles of students in their class. """ course = get_course_with_access(request.user, course_id, 'load') + staff_access = has_access(request.user, course, 'staff') if student_id is None or student_id == request.user.id: # always allowed to see your own profile student = request.user else: # Requesting access to a different student's profile - if not has_access(request.user, course, 'staff'): + if not staff_access: raise Http404 student = User.objects.get(id=int(student_id)) @@ -282,8 +288,9 @@ def profile(request, course_id, student_id=None): 'email': student.email, 'course': course, 'csrf': csrf(request)['csrf_token'], - 'courseware_summary' : courseware_summary, - 'grade_summary' : grade_summary + 'courseware_summary': courseware_summary, + 'grade_summary': grade_summary, + 'staff_access': staff_access, } context.update() @@ -316,7 +323,10 @@ def gradebook(request, course_id): for student in enrolled_students] return render_to_response('gradebook.html', {'students': student_info, - 'course': course, 'course_id': course_id}) + 'course': course, + 'course_id': course_id, + # Checked above + 'staff_access': True,}) @cache_control(no_cache=True, no_store=True, must_revalidate=True) @@ -325,7 +335,8 @@ def grade_summary(request, course_id): course = get_course_with_access(request.user, course_id, 'staff') # For now, just a static page - context = {'course': course } + context = {'course': course, + 'staff_access': True,} return render_to_response('grade_summary.html', context) @@ -335,6 +346,7 @@ def instructor_dashboard(request, course_id): course = get_course_with_access(request.user, course_id, 'staff') # For now, just a static page - context = {'course': course } + context = {'course': course, + 'staff_access': True,} return render_to_response('instructor_dashboard.html', context) diff --git a/lms/djangoapps/simplewiki/views.py b/lms/djangoapps/simplewiki/views.py index 2ee76a1868..192035fcde 100644 --- a/lms/djangoapps/simplewiki/views.py +++ b/lms/djangoapps/simplewiki/views.py @@ -10,6 +10,7 @@ from django.utils.translation import ugettext_lazy as _ from mitxmako.shortcuts import render_to_response from courseware.courses import get_opt_course_with_access +from courseware.access import has_access from xmodule.course_module import CourseDescriptor from xmodule.modulestore.django import modulestore @@ -49,6 +50,10 @@ def update_template_dictionary(dictionary, request=None, course=None, article=No if request: dictionary.update(csrf(request)) + if request and course: + dictionary['staff_access'] = has_access(request.user, course, 'load') + else: + dictionary['staff_access'] = False def view(request, article_path, course_id=None): course = get_opt_course_with_access(request.user, course_id, 'load') diff --git a/lms/djangoapps/staticbook/views.py b/lms/djangoapps/staticbook/views.py index aec3fb1448..2e19ab6425 100644 --- a/lms/djangoapps/staticbook/views.py +++ b/lms/djangoapps/staticbook/views.py @@ -1,17 +1,23 @@ from django.contrib.auth.decorators import login_required from mitxmako.shortcuts import render_to_response +from courseware.access import has_access from courseware.courses import get_course_with_access from lxml import etree @login_required def index(request, course_id, page=0): course = get_course_with_access(request.user, course_id, 'load') - raw_table_of_contents = open('lms/templates/book_toc.xml', 'r') # TODO: This will need to come from S3 + staff_access = has_access(request.user, course, 'staff') + + # TODO: This will need to come from S3 + raw_table_of_contents = open('lms/templates/book_toc.xml', 'r') table_of_contents = etree.parse(raw_table_of_contents).getroot() + return render_to_response('staticbook.html', {'page': int(page), 'course': course, - 'table_of_contents': table_of_contents}) + 'table_of_contents': table_of_contents, + 'staff_access': staff_access}) def index_shifted(request, course_id, page): diff --git a/lms/templates/course_navigation.html b/lms/templates/course_navigation.html index dd1c8d93b9..9e93b2fb14 100644 --- a/lms/templates/course_navigation.html +++ b/lms/templates/course_navigation.html @@ -28,7 +28,7 @@ def url_class(url): % if user.is_authenticated():
  • Profile
  • % endif -% if has_access(user, course, 'staff'): +% if staff_access:
  • Instructor
  • % endif From a2057f9ea42d5ded3e1e5f15057ee470932a387b Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 15 Aug 2012 11:47:01 -0400 Subject: [PATCH 04/16] fix docstring --- common/lib/xmodule/xmodule/xml_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 399d5d3f91..3c2f3269ca 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -166,7 +166,7 @@ class XmlDescriptor(XModuleDescriptor): Subclasses should not need to override this except in special cases (e.g. html module)''' - # VS[compat] -- the filename tag should go away once everything is + # VS[compat] -- the filename attr should go away once everything is # converted. (note: make sure html files still work once this goes away) filename = xml_object.get('filename') if filename is None: From 346d5b91a1c07aad0b51fca09fbac2aa1014d586 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 15 Aug 2012 10:58:20 -0400 Subject: [PATCH 05/16] implement subdomain-based course displays --- common/djangoapps/external_auth/views.py | 7 ++- common/djangoapps/student/views.py | 7 ++- lms/djangoapps/courseware/courses.py | 78 +++++++++++++++++++++++- lms/djangoapps/courseware/views.py | 6 +- lms/envs/common.py | 6 ++ 5 files changed, 95 insertions(+), 9 deletions(-) diff --git a/common/djangoapps/external_auth/views.py b/common/djangoapps/external_auth/views.py index 0425f3e158..35e59db0ca 100644 --- a/common/djangoapps/external_auth/views.py +++ b/common/djangoapps/external_auth/views.py @@ -1,3 +1,4 @@ +import functools import json import logging import random @@ -156,7 +157,7 @@ def edXauth_signup(request, eamap=None): log.debug('ExtAuth: doing signup for %s' % eamap.external_email) - return student_views.main_index(extra_context=context) + return student_views.main_index(request, extra_context=context) #----------------------------------------------------------------------------- # MIT SSL @@ -206,7 +207,7 @@ def edXauth_ssl_login(request): pass if not cert: # no certificate information - go onward to main index - return student_views.main_index() + return student_views.main_index(request) (user, email, fullname) = ssl_dn_extract_info(cert) @@ -216,4 +217,4 @@ def edXauth_ssl_login(request): credentials=cert, email=email, fullname=fullname, - retfun = student_views.main_index) + retfun = functools.partial(student_views.main_index, request)) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index b6aa62e03d..e02a5f4562 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -68,9 +68,9 @@ def index(request): from external_auth.views import edXauth_ssl_login return edXauth_ssl_login(request) - return main_index(user=request.user) + return main_index(request, user=request.user) -def main_index(extra_context = {}, user=None): +def main_index(request, extra_context={}, user=None): ''' Render the edX main page. @@ -93,7 +93,8 @@ def main_index(extra_context = {}, user=None): entry.summary = soup.getText() # The course selection work is done in courseware.courses. - universities = get_courses_by_university(None) + universities = get_courses_by_university(None, + domain=request.META['HTTP_HOST']) context = {'universities': universities, 'entries': entries} context.update(extra_context) return render_to_response('index.html', context) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 2e74853760..5818252cf1 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -2,8 +2,8 @@ from collections import defaultdict from fs.errors import ResourceNotFoundError from functools import wraps import logging -from path import path +from path import path from django.conf import settings from django.http import Http404 @@ -142,19 +142,95 @@ def get_course_info_section(course, section_key): raise KeyError("Invalid about key " + str(section_key)) +<<<<<<< HEAD def get_courses_by_university(user): +======= +def course_staff_group_name(course): + ''' + course should be either a CourseDescriptor instance, or a string (the + .course entry of a Location) + ''' + if isinstance(course, str) or isinstance(course, unicode): + coursename = course + else: + # should be a CourseDescriptor, so grab its location.course: + coursename = course.location.course + return 'staff_%s' % coursename + +def has_staff_access_to_course(user, course): + ''' + Returns True if the given user has staff access to the course. + This means that user is in the staff_* group, or is an overall admin. + TODO (vshnayder): this needs to be changed to allow per-course_id permissions, not per-course + (e.g. staff in 2012 is different from 2013, but maybe some people always have access) + + course is the course field of the location being accessed. + ''' + if user is None or (not user.is_authenticated()) or course is None: + return False + if user.is_staff: + return True + + # note this is the Auth group, not UserTestGroup + user_groups = [x[1] for x in user.groups.values_list()] + staff_group = course_staff_group_name(course) + if staff_group in user_groups: + return True + return False + +def has_staff_access_to_course_id(user, course_id): + """Helper method that takes a course_id instead of a course name""" + loc = CourseDescriptor.id_to_location(course_id) + return has_staff_access_to_course(user, loc.course) + + +def has_staff_access_to_location(user, location): + """Helper method that checks whether the user has staff access to + the course of the location. + + location: something that can be passed to Location + """ + return has_staff_access_to_course(user, Location(location).course) + +def has_access_to_course(user, course): + '''course is the .course element of a location''' + if course.metadata.get('ispublic'): + return True + return has_staff_access_to_course(user,course) + + +def get_courses_by_university(user, domain=None): +>>>>>>> implement subdomain-based course displays ''' Returns dict of lists of courses available, keyed by course.org (ie university). Courses are sorted by course.number. ''' + subdomain = domain.split(".")[0] + # TODO: Clean up how 'error' is done. # filter out any courses that errored. + if settings.MITX_FEATURES.get('SUBDOMAIN_COURSE_LISTINGS'): + if subdomain in settings.COURSE_LISTINGS: + visible_courses = settings.COURSE_LISTINGS[subdomain] + else: + visible_courses = frozenset(settings.COURSE_LISTINGS['default']) + courses = [c for c in modulestore().get_courses() if isinstance(c, CourseDescriptor)] courses = sorted(courses, key=lambda course: course.number) universities = defaultdict(list) for course in courses: +<<<<<<< HEAD if has_access(user, course, 'see_exists'): universities[course.org].append(course) +======= + if settings.MITX_FEATURES.get('ACCESS_REQUIRE_STAFF_FOR_COURSE'): + if not has_access_to_course(user,course): + continue + if settings.MITX_FEATURES.get('SUBDOMAIN_COURSE_LISTINGS'): + if course.id not in visible_courses: + continue + universities[course.org].append(course) +>>>>>>> implement subdomain-based course displays return universities diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index ab63872170..b6a56830e0 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -63,7 +63,8 @@ def courses(request): ''' Render "find courses" page. The course selection work is done in courseware.courses. ''' - universities = get_courses_by_university(request.user) + universities = get_courses_by_university(request.user, + domain=request.META['HTTP_HOST']) return render_to_response("courses.html", {'universities': universities}) @@ -241,7 +242,8 @@ def university_profile(request, org_id): raise Http404("University Profile not found for {0}".format(org_id)) # Only grab courses for this org... - courses = get_courses_by_university(request.user)[org_id] + courses = get_courses_by_university(request.user, + domain=request.META['HTTP_HOST'])[org_id] context = dict(courses=courses, org_id=org_id) template_file = "university_profile/{0}.html".format(org_id).lower() diff --git a/lms/envs/common.py b/lms/envs/common.py index 303e73ea81..45818c0ff2 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -49,6 +49,11 @@ MITX_FEATURES = { ## Doing so will cause all courses to be released on production 'DISABLE_START_DATES': False, # When True, all courses will be active, regardless of start date + # When True, will only publicly list courses by the subdomain. Expects you + # to define COURSE_LISTINGS, a dictionary mapping subdomains to lists of + # course_ids (see dev_int.py for an example) + 'SUBDOMAIN_COURSE_LISTINGS' : False, + 'ENABLE_TEXTBOOK' : True, 'ENABLE_DISCUSSION' : True, @@ -61,6 +66,7 @@ MITX_FEATURES = { 'ACCESS_REQUIRE_STAFF_FOR_COURSE': False, 'AUTH_USE_OPENID': False, 'AUTH_USE_MIT_CERTIFICATES' : False, + } # Used for A/B testing From 7386d66c0f354a089d36d473c5a3a0786d792f17 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 15 Aug 2012 11:11:54 -0400 Subject: [PATCH 06/16] Add dev_int.py config for testing course listings by subdomain --- lms/envs/dev_int.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 lms/envs/dev_int.py diff --git a/lms/envs/dev_int.py b/lms/envs/dev_int.py new file mode 100644 index 0000000000..83c7fb0aba --- /dev/null +++ b/lms/envs/dev_int.py @@ -0,0 +1,32 @@ +""" +This enables use of course listings by subdomain. To see it in action, point the +following domains to 127.0.0.1 in your /etc/hosts file: + + berkeley.dev + harvard.dev + mit.dev + +Note that OS X has a bug where using *.local domains is excruciatingly slow, so +use *.dev domains instead for local testing. +""" +from .dev import * + +MITX_FEATURES['SUBDOMAIN_COURSE_LISTINGS'] = True + +COURSE_LISTINGS = { + 'default' : ['BerkeleyX/CS169.1x/2012_Fall', + 'BerkeleyX/CS188.1x/2012_Fall', + 'HarvardX/CS50x/2012', + 'HarvardX/PH207x/2012_Fall' + 'MITx/3.091x/2012_Fall', + 'MITx/6.002x/2012_Fall', + 'MITx/6.00x/2012_Fall'], + + 'berkeley': ['BerkeleyX/CS169.1x/2012_Fall', + 'BerkeleyX/CS188.1x/2012_Fall'], + + 'harvard' : ['HarvardX/CS50x/2012'], + + 'mit' : ['MITx/3.091x/2012_Fall', + 'MITx/6.00x/2012_Fall'] +} From d0f2641890c835aa7263f125ec9fd71a4ff5fac9 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 15 Aug 2012 11:12:44 -0400 Subject: [PATCH 07/16] Account for the fact that sometimes we don't get HTTP_HOST (like for tests) --- common/djangoapps/student/views.py | 2 +- lms/djangoapps/courseware/courses.py | 20 +++++++++----------- lms/djangoapps/courseware/views.py | 4 ++-- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index e02a5f4562..a99b46fd13 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -94,7 +94,7 @@ def main_index(request, extra_context={}, user=None): # The course selection work is done in courseware.courses. universities = get_courses_by_university(None, - domain=request.META['HTTP_HOST']) + domain=request.META.get('HTTP_HOST')) context = {'universities': universities, 'entries': entries} context.update(extra_context) return render_to_response('index.html', context) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 5818252cf1..02144ac3e0 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -205,19 +205,18 @@ def get_courses_by_university(user, domain=None): Returns dict of lists of courses available, keyed by course.org (ie university). Courses are sorted by course.number. ''' - subdomain = domain.split(".")[0] - # TODO: Clean up how 'error' is done. # filter out any courses that errored. - if settings.MITX_FEATURES.get('SUBDOMAIN_COURSE_LISTINGS'): - if subdomain in settings.COURSE_LISTINGS: - visible_courses = settings.COURSE_LISTINGS[subdomain] - else: - visible_courses = frozenset(settings.COURSE_LISTINGS['default']) - courses = [c for c in modulestore().get_courses() if isinstance(c, CourseDescriptor)] courses = sorted(courses, key=lambda course: course.number) + + if domain and settings.MITX_FEATURES.get('SUBDOMAIN_COURSE_LISTINGS'): + subdomain = settings.COURSE_LISTINGS.get(domain.split(".")[0], 'default') + visible_courses = frozenset(settings.COURSE_LISTINGS[subdomain]) + else: + visible_courses = frozenset(c.id for c in courses) + universities = defaultdict(list) for course in courses: <<<<<<< HEAD @@ -227,9 +226,8 @@ def get_courses_by_university(user, domain=None): if settings.MITX_FEATURES.get('ACCESS_REQUIRE_STAFF_FOR_COURSE'): if not has_access_to_course(user,course): continue - if settings.MITX_FEATURES.get('SUBDOMAIN_COURSE_LISTINGS'): - if course.id not in visible_courses: - continue + if course.id not in visible_courses: + continue universities[course.org].append(course) >>>>>>> implement subdomain-based course displays return universities diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index b6a56830e0..eaeb667b3e 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -64,7 +64,7 @@ def courses(request): Render "find courses" page. The course selection work is done in courseware.courses. ''' universities = get_courses_by_university(request.user, - domain=request.META['HTTP_HOST']) + domain=request.META.get('HTTP_HOST')) return render_to_response("courses.html", {'universities': universities}) @@ -243,7 +243,7 @@ def university_profile(request, org_id): # Only grab courses for this org... courses = get_courses_by_university(request.user, - domain=request.META['HTTP_HOST'])[org_id] + domain=request.META.get('HTTP_HOST'))[org_id] context = dict(courses=courses, org_id=org_id) template_file = "university_profile/{0}.html".format(org_id).lower() From 7f8f70297179628250d72b3e4666d48f19700678 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 15 Aug 2012 11:21:15 -0400 Subject: [PATCH 08/16] Fix silly error that was pulling the wrong info from COURSE_LISTINGS --- lms/djangoapps/courseware/courses.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 02144ac3e0..3eaff318a5 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -212,7 +212,9 @@ def get_courses_by_university(user, domain=None): courses = sorted(courses, key=lambda course: course.number) if domain and settings.MITX_FEATURES.get('SUBDOMAIN_COURSE_LISTINGS'): - subdomain = settings.COURSE_LISTINGS.get(domain.split(".")[0], 'default') + subdomain = domain.split(".")[0] + if subdomain not in settings.COURSE_LISTINGS: + subdomain = 'default' visible_courses = frozenset(settings.COURSE_LISTINGS[subdomain]) else: visible_courses = frozenset(c.id for c in courses) From 7d90617c5e9fb898609334c9c67d3a2dea062792 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 15 Aug 2012 12:21:59 -0400 Subject: [PATCH 09/16] Merge with master --- lms/djangoapps/courseware/courses.py | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 3eaff318a5..e142614f46 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -142,9 +142,6 @@ def get_course_info_section(course, section_key): raise KeyError("Invalid about key " + str(section_key)) -<<<<<<< HEAD -def get_courses_by_university(user): -======= def course_staff_group_name(course): ''' course should be either a CourseDescriptor instance, or a string (the @@ -200,7 +197,6 @@ def has_access_to_course(user, course): def get_courses_by_university(user, domain=None): ->>>>>>> implement subdomain-based course displays ''' Returns dict of lists of courses available, keyed by course.org (ie university). Courses are sorted by course.number. @@ -221,16 +217,10 @@ def get_courses_by_university(user, domain=None): universities = defaultdict(list) for course in courses: -<<<<<<< HEAD - if has_access(user, course, 'see_exists'): - universities[course.org].append(course) -======= - if settings.MITX_FEATURES.get('ACCESS_REQUIRE_STAFF_FOR_COURSE'): - if not has_access_to_course(user,course): - continue + if not has_access(user, course, 'see_exists'): + continue if course.id not in visible_courses: continue universities[course.org].append(course) ->>>>>>> implement subdomain-based course displays return universities From 7c6b9eba1c1a6004f4d93c8b84b64702a2dd92ac Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 15 Aug 2012 12:55:13 -0400 Subject: [PATCH 10/16] Note that 'load' access does not check for enrollment --- lms/djangoapps/courseware/access.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 1494de6662..760ae55fb4 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -68,6 +68,7 @@ def has_access(user, obj, action): raise TypeError("Unknown object type in has_access(): '{}'" .format(type(obj))) + # ================ Implementation helpers ================================ def _has_access_course_desc(user, course, action): @@ -83,8 +84,11 @@ def _has_access_course_desc(user, course, action): 'staff' -- staff access to course. """ def can_load(): - "Can this user load this course?" - # delegate to generic descriptor check + "Can this user load this course? + + NOTE: this is not checking whether user is actually enrolled in the course. + " + # delegate to generic descriptor check to check start dates return _has_access_descriptor(user, course, action) def can_enroll(): @@ -169,6 +173,12 @@ def _has_access_descriptor(user, descriptor, action): has_access(), it will not do the right thing. """ def can_load(): + """ + NOTE: This does not check that the student is enrolled in the course + that contains this module. We may or may not want to allow non-enrolled + students to see modules. If not, views should check the course, so we + don't have to hit the enrollments table on every module load. + """ # If start dates are off, can always load if settings.MITX_FEATURES['DISABLE_START_DATES']: debug("Allow: DISABLE_START_DATES") @@ -196,8 +206,6 @@ def _has_access_descriptor(user, descriptor, action): return _dispatch(checkers, action, user, descriptor) - - def _has_access_xmodule(user, xmodule, action): """ Check if user has access to this xmodule. From c4afddfd384cd858779061174e40e237a8cb01ab Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 15 Aug 2012 12:56:01 -0400 Subject: [PATCH 11/16] Fix staff access check in simplewiki --- lms/djangoapps/simplewiki/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/simplewiki/views.py b/lms/djangoapps/simplewiki/views.py index 192035fcde..ac807b13ed 100644 --- a/lms/djangoapps/simplewiki/views.py +++ b/lms/djangoapps/simplewiki/views.py @@ -51,7 +51,7 @@ def update_template_dictionary(dictionary, request=None, course=None, article=No dictionary.update(csrf(request)) if request and course: - dictionary['staff_access'] = has_access(request.user, course, 'load') + dictionary['staff_access'] = has_access(request.user, course, 'staff') else: dictionary['staff_access'] = False From 573c58d02b6cb5894624a0298564efef1742de08 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Aug 2012 13:59:25 -0400 Subject: [PATCH 12/16] Fix broken docstring --- lms/djangoapps/courseware/access.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 760ae55fb4..e588f807da 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -84,10 +84,11 @@ def _has_access_course_desc(user, course, action): 'staff' -- staff access to course. """ def can_load(): - "Can this user load this course? + """ + Can this user load this course? NOTE: this is not checking whether user is actually enrolled in the course. - " + """ # delegate to generic descriptor check to check start dates return _has_access_descriptor(user, course, action) From e524f3bf6c930c2af7a980aa5ecbe9ca40a716e2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Aug 2012 14:12:32 -0400 Subject: [PATCH 13/16] Point at the new urls for the sandbox queue server LB --- lms/envs/dev.py | 2 +- lms/envs/test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 882a82b8f0..8457b50374 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -54,7 +54,7 @@ CACHES = { } XQUEUE_INTERFACE = { - "url": "http://xqueue.sandbox.edx.org", + "url": "http://sandbox-xqueue.edx.org", "django_auth": { "username": "lms", "password": "***REMOVED***" diff --git a/lms/envs/test.py b/lms/envs/test.py index 187cb5c68e..11534b3f4d 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -51,7 +51,7 @@ GITHUB_REPO_ROOT = ENV_ROOT / "data" XQUEUE_INTERFACE = { - "url": "http://xqueue.sandbox.edx.org", + "url": "http://sandbox-xqueue.edx.org", "django_auth": { "username": "lms", "password": "***REMOVED***" From f535f44e625663d21f26d8879f293206ec60cf39 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 15 Aug 2012 14:32:36 -0400 Subject: [PATCH 14/16] Fix typo that caused two classes to not get loaded --- lms/envs/dev_int.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/envs/dev_int.py b/lms/envs/dev_int.py index 83c7fb0aba..12123e12d4 100644 --- a/lms/envs/dev_int.py +++ b/lms/envs/dev_int.py @@ -17,7 +17,7 @@ COURSE_LISTINGS = { 'default' : ['BerkeleyX/CS169.1x/2012_Fall', 'BerkeleyX/CS188.1x/2012_Fall', 'HarvardX/CS50x/2012', - 'HarvardX/PH207x/2012_Fall' + 'HarvardX/PH207x/2012_Fall', 'MITx/3.091x/2012_Fall', 'MITx/6.002x/2012_Fall', 'MITx/6.00x/2012_Fall'], From a4d67bab33e23f5369b8e390e01b7c697b675ec8 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 15 Aug 2012 14:36:06 -0400 Subject: [PATCH 15/16] Add support metadata in policy.json * if there is a policy.json in the course dir, read it * file format is a dict with keys {category}/{url_name}, and values metadata dictionaries * apply the policy, overwriting keys that are in the xml * then do metadata inheritance, inheriting any overwritten keys. * also a management cmd to generate a policy.json from a course dir. --- common/lib/xmodule/xmodule/modulestore/xml.py | 30 +++++- common/lib/xmodule/xmodule/x_module.py | 26 +++++ .../management/commands/metadata_to_json.py | 98 +++++++++++++++++++ 3 files changed, 152 insertions(+), 2 deletions(-) create mode 100644 lms/djangoapps/courseware/management/commands/metadata_to_json.py diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 8c4c373d4f..0c0d750882 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -1,3 +1,4 @@ +import json import logging import os import re @@ -149,7 +150,7 @@ class XMLModuleStore(ModuleStoreBase): for course_dir in course_dirs: self.try_load_course(course_dir) - def try_load_course(self,course_dir): + def try_load_course(self, course_dir): ''' Load a course, keeping track of errors as we go along. ''' @@ -170,7 +171,27 @@ class XMLModuleStore(ModuleStoreBase): ''' String representation - for debugging ''' - return 'data_dir=%s, %d courses, %d modules' % (self.data_dir,len(self.courses),len(self.modules)) + return 'data_dir=%s, %d courses, %d modules' % ( + self.data_dir, len(self.courses), len(self.modules)) + + def load_policy(self, policy_path, tracker): + """ + Attempt to read a course policy from policy_path. If the file + exists, but is invalid, log an error and return {}. + + If the policy loads correctly, returns the deserialized version. + """ + if not os.path.exists(policy_path): + return {} + try: + with open(policy_path) as f: + return json.load(f) + except (IOError, ValueError) as err: + msg = "Error loading course policy from {}".format(policy_path) + tracker(msg) + log.warning(msg + " " + str(err)) + return {} + def load_course(self, course_dir, tracker): """ @@ -214,6 +235,11 @@ class XMLModuleStore(ModuleStoreBase): system = ImportSystem(self, org, course, course_dir, tracker) course_descriptor = system.process_xml(etree.tostring(course_data)) + policy_path = self.data_dir / course_dir / 'policy.json' + + policy = self.load_policy(policy_path, tracker) + XModuleDescriptor.apply_policy(course_descriptor, policy) + # NOTE: The descriptors end up loading somewhat bottom up, which # breaks metadata inheritance via get_children(). Instead # (actually, in addition to, for now), we do a final inheritance pass diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 06449dc37f..af3f04e366 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -298,6 +298,14 @@ class XModule(HTMLSnippet): return "" +def policy_key(location): + """ + Get the key for a location in a policy file. (Since the policy file is + specific to a course, it doesn't need the full location url). + """ + return '{cat}/{name}'.format(cat=location.category, name=location.name) + + class XModuleDescriptor(Plugin, HTMLSnippet): """ An XModuleDescriptor is a specification for an element of a course. This @@ -416,6 +424,24 @@ class XModuleDescriptor(Plugin, HTMLSnippet): return dict((k,v) for k,v in self.metadata.items() if k not in self._inherited_metadata) + + @staticmethod + def apply_policy(node, policy): + """ + Given a descriptor, traverse all its descendants and update its metadata + with the policy. + + Notes: + - this does not propagate inherited metadata. The caller should + call compute_inherited_metadata after applying the policy. + - metadata specified in the policy overrides metadata in the xml + """ + k = policy_key(node.location) + if k in policy: + node.metadata.update(policy[k]) + for c in node.get_children(): + XModuleDescriptor.apply_policy(c, policy) + @staticmethod def compute_inherited_metadata(node): """Given a descriptor, traverse all of its descendants and do metadata diff --git a/lms/djangoapps/courseware/management/commands/metadata_to_json.py b/lms/djangoapps/courseware/management/commands/metadata_to_json.py new file mode 100644 index 0000000000..0f48e93319 --- /dev/null +++ b/lms/djangoapps/courseware/management/commands/metadata_to_json.py @@ -0,0 +1,98 @@ +""" +A script to walk a course xml tree, generate a dictionary of all the metadata, +and print it out as a json dict. +""" +import os +import sys +import json + +from collections import OrderedDict +from path import path + +from django.core.management.base import BaseCommand + +from xmodule.modulestore.xml import XMLModuleStore +from xmodule.x_module import policy_key + +def import_course(course_dir, verbose=True): + course_dir = path(course_dir) + data_dir = course_dir.dirname() + course_dirs = [course_dir.basename()] + + # No default class--want to complain if it doesn't find plugins for any + # module. + modulestore = XMLModuleStore(data_dir, + default_class=None, + eager=True, + course_dirs=course_dirs) + + def str_of_err(tpl): + (msg, exc_str) = tpl + return '{msg}\n{exc}'.format(msg=msg, exc=exc_str) + + courses = modulestore.get_courses() + + n = len(courses) + if n != 1: + sys.stderr.write('ERROR: Expect exactly 1 course. Loaded {n}: {lst}\n'.format( + n=n, lst=courses)) + return None + + course = courses[0] + errors = modulestore.get_item_errors(course.location) + if len(errors) != 0: + sys.stderr.write('ERRORs during import: {}\n'.format('\n'.join(map(str_of_err, errors)))) + + return course + +def node_metadata(node): + # make a copy + to_export = ('format', 'display_name', + 'graceperiod', 'showanswer', 'rerandomize', + 'start', 'due', 'graded', 'hide_from_toc', + 'ispublic', 'xqa_key') + + orig = node.own_metadata + d = {k: orig[k] for k in to_export if k in orig} + return d + +def get_metadata(course): + d = OrderedDict({}) + queue = [course] + while len(queue) > 0: + node = queue.pop() + d[policy_key(node.location)] = node_metadata(node) + # want to print first children first, so put them at the end + # (we're popping from the end) + queue.extend(reversed(node.get_children())) + return d + + +def print_metadata(course_dir, output): + course = import_course(course_dir) + if course: + meta = get_metadata(course) + result = json.dumps(meta, indent=4) + if output: + with file(output, 'w') as f: + f.write(result) + else: + print result + + +class Command(BaseCommand): + help = """Imports specified course.xml and prints its +metadata as a json dict. + +Usage: metadata_to_json PATH-TO-COURSE-DIR OUTPUT-PATH + +if OUTPUT-PATH isn't given, print to stdout. +""" + def handle(self, *args, **options): + n = len(args) + if n < 1 or n > 2: + print Command.help + return + + output_path = args[1] if n > 1 else None + print_metadata(args[0], output_path) From 1c2b6e80885e200c96e770c1b54b24c71b536608 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 15 Aug 2012 14:39:56 -0400 Subject: [PATCH 16/16] Remove stuff that I should have deleted during the rebase --- lms/djangoapps/courseware/courses.py | 53 ---------------------------- 1 file changed, 53 deletions(-) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index e142614f46..f0b82a3c9c 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -142,59 +142,6 @@ def get_course_info_section(course, section_key): raise KeyError("Invalid about key " + str(section_key)) -def course_staff_group_name(course): - ''' - course should be either a CourseDescriptor instance, or a string (the - .course entry of a Location) - ''' - if isinstance(course, str) or isinstance(course, unicode): - coursename = course - else: - # should be a CourseDescriptor, so grab its location.course: - coursename = course.location.course - return 'staff_%s' % coursename - -def has_staff_access_to_course(user, course): - ''' - Returns True if the given user has staff access to the course. - This means that user is in the staff_* group, or is an overall admin. - TODO (vshnayder): this needs to be changed to allow per-course_id permissions, not per-course - (e.g. staff in 2012 is different from 2013, but maybe some people always have access) - - course is the course field of the location being accessed. - ''' - if user is None or (not user.is_authenticated()) or course is None: - return False - if user.is_staff: - return True - - # note this is the Auth group, not UserTestGroup - user_groups = [x[1] for x in user.groups.values_list()] - staff_group = course_staff_group_name(course) - if staff_group in user_groups: - return True - return False - -def has_staff_access_to_course_id(user, course_id): - """Helper method that takes a course_id instead of a course name""" - loc = CourseDescriptor.id_to_location(course_id) - return has_staff_access_to_course(user, loc.course) - - -def has_staff_access_to_location(user, location): - """Helper method that checks whether the user has staff access to - the course of the location. - - location: something that can be passed to Location - """ - return has_staff_access_to_course(user, Location(location).course) - -def has_access_to_course(user, course): - '''course is the .course element of a location''' - if course.metadata.get('ispublic'): - return True - return has_staff_access_to_course(user,course) - def get_courses_by_university(user, domain=None): '''