From 7664f910b1428084d4e0eff52cb25103f6eaa565 Mon Sep 17 00:00:00 2001 From: ichuang Date: Sun, 2 Sep 2012 19:04:55 -0400 Subject: [PATCH 01/23] add is_released --- common/djangoapps/xmodule_modifiers.py | 12 +++++++++++- lms/templates/staff_problem_info.html | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 17380bff18..10cce1a95b 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -1,6 +1,7 @@ import re import json import logging +import time from django.conf import settings from functools import wraps @@ -117,6 +118,13 @@ def add_histogram(get_html, module, user): data_dir = "" source_file = module.metadata.get('source_file','') # source used to generate the problem XML, eg latex or word + # useful to indicate to staff if problem has been released or not + now = time.gmtime() + is_released = "unknown" + if hasattr(module,'start'): + if module.start is not None: + is_released = "Yes!" if (now > module.start) else "Not yet" + staff_context = {'definition': module.definition.get('data'), 'metadata': json.dumps(module.metadata, indent=4), 'location': module.location, @@ -130,7 +138,9 @@ def add_histogram(get_html, module, user): 'xqa_server' : settings.MITX_FEATURES.get('USE_XQA_SERVER','http://xqa:server@content-qa.mitx.mit.edu/xqa'), 'histogram': json.dumps(histogram), 'render_histogram': render_histogram, - 'module_content': get_html()} + 'module_content': get_html(), + 'is_released': is_released, + } return render_to_string("staff_problem_info.html", staff_context) return _get_html diff --git a/lms/templates/staff_problem_info.html b/lms/templates/staff_problem_info.html index 47194aa6fd..6d9d1a3a30 100644 --- a/lms/templates/staff_problem_info.html +++ b/lms/templates/staff_problem_info.html @@ -32,6 +32,7 @@ ${module_content}

Staff Debug

+is_released = ${is_released} location = ${location | h} github = ${edit_link | h} %if source_file: From 2efe481237d0f60c36be9fbba756447d83a3965d Mon Sep 17 00:00:00 2001 From: ichuang Date: Sun, 2 Sep 2012 19:05:34 -0400 Subject: [PATCH 02/23] move instructor dashboard into its own lms djangoapp; add new functionality - grade dump and download as csv, manage staff list, force reload of course from xml --- common/djangoapps/student/models.py | 3 + lms/djangoapps/courseware/access.py | 52 ++- lms/djangoapps/courseware/grades.py | 13 +- lms/djangoapps/courseware/views.py | 93 ----- lms/djangoapps/instructor/__init__.py | 0 lms/djangoapps/instructor/views.py | 330 ++++++++++++++++++ lms/envs/common.py | 1 + .../courseware/instructor_dashboard.html | 81 +++++ lms/urls.py | 8 +- 9 files changed, 474 insertions(+), 107 deletions(-) create mode 100644 lms/djangoapps/instructor/__init__.py create mode 100644 lms/djangoapps/instructor/views.py diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 9cde878d21..c494035104 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -184,6 +184,9 @@ class CourseEnrollment(models.Model): class Meta: unique_together = (('user', 'course_id'), ) + def __unicode__(self): + return "%s: %s (%s)" % (self.user,self.course_id,self.created) + @receiver(post_save, sender=CourseEnrollment) def assign_default_role(sender, instance, **kwargs): if instance.user.is_staff: diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 91c769f90a..4f35aa98fa 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -30,7 +30,7 @@ def has_access(user, obj, action): Things this module understands: - start dates for modules - DISABLE_START_DATES - - different access for staff, course staff, and students. + - different access for instructor, staff, course staff, and students. user: a Django user object. May be anonymous. @@ -70,6 +70,20 @@ def has_access(user, obj, action): raise TypeError("Unknown object type in has_access(): '{0}'" .format(type(obj))) +def get_access_group_name(obj,action): + ''' + Returns group name for user group which has "action" access to the given object. + + Used in managing access lists. + ''' + + if isinstance(obj, CourseDescriptor): + return _get_access_group_name_course_desc(obj, action) + + # Passing an unknown object here is a coding error, so rather than + # returning a default, complain. + raise TypeError("Unknown object type in get_access_group_name(): '{0}'" + .format(type(obj))) # ================ Implementation helpers ================================ @@ -138,11 +152,19 @@ def _has_access_course_desc(user, course, action): 'load': can_load, 'enroll': can_enroll, 'see_exists': see_exists, - 'staff': lambda: _has_staff_access_to_descriptor(user, course) + 'staff': lambda: _has_staff_access_to_descriptor(user, course), + 'instructor': lambda: _has_staff_access_to_descriptor(user, course, require_instructor=True), } return _dispatch(checkers, action, user, course) +def _get_access_group_name_course_desc(course, action): + ''' + Return name of group which gives staff access to course. Only understands action = 'staff' + ''' + if not action=='staff': + return [] + return _course_staff_group_name(course.location) def _has_access_error_desc(user, descriptor, action): """ @@ -292,6 +314,15 @@ def _course_staff_group_name(location): """ return 'staff_%s' % Location(location).course +def _course_instructor_group_name(location): + """ + Get the name of the instructor group for a location. Right now, that's instructor_COURSE. + A course instructor has all staff privileges, but also can manage list of course staff (add, remove, list). + + location: something that can passed to Location. + """ + return 'instructor_%s' % Location(location).course + def _has_global_staff_access(user): if user.is_staff: debug("Allow: user.is_staff") @@ -301,11 +332,13 @@ def _has_global_staff_access(user): return False -def _has_staff_access_to_location(user, location): +def _has_staff_access_to_location(user, location, require_instructor=False): ''' Returns True if the given user has staff access to a location. For now this is equivalent to having staff access to the course location.course. + If require_instructor=True, then user must be in instructor_* group. + 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 @@ -323,8 +356,13 @@ def _has_staff_access_to_location(user, location): # If not global staff, is the user in the Auth group for this class? user_groups = [g.name for g in user.groups.all()] staff_group = _course_staff_group_name(location) - if staff_group in user_groups: - debug("Allow: user in group %s", staff_group) + if not require_instructor: + if staff_group in user_groups: + debug("Allow: user in group %s", staff_group) + return True + instructor_group = _course_instructor_group_name(location) + if instructor_group in user_groups: + debug("Allow: user in group %s", instructor_group) return True debug("Deny: user not in group %s", staff_group) return False @@ -335,11 +373,11 @@ def _has_staff_access_to_course_id(user, course_id): return _has_staff_access_to_location(user, loc) -def _has_staff_access_to_descriptor(user, descriptor): +def _has_staff_access_to_descriptor(user, descriptor, require_instructor=False): """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_location(user, descriptor.location) + return _has_staff_access_to_location(user, descriptor.location, require_instructor=require_instructor) diff --git a/lms/djangoapps/courseware/grades.py b/lms/djangoapps/courseware/grades.py index 7f28f3ca5c..f32da532df 100644 --- a/lms/djangoapps/courseware/grades.py +++ b/lms/djangoapps/courseware/grades.py @@ -24,7 +24,7 @@ def yield_module_descendents(module): stack.extend( next_module.get_display_items() ) yield next_module -def grade(student, request, course, student_module_cache=None): +def grade(student, request, course, student_module_cache=None, keep_raw_scores=False): """ This grades a student as quickly as possible. It retuns the output from the course grader, augmented with the final letter @@ -38,11 +38,13 @@ def grade(student, request, course, student_module_cache=None): up the grade. (For display) - grade_breakdown : A breakdown of the major components that make up the final grade. (For display) + - keep_raw_scores : if True, then value for key 'raw_scores' contains scores for every graded module More information on the format is in the docstring for CourseGrader. """ grading_context = course.grading_context + raw_scores = [] if student_module_cache == None: student_module_cache = StudentModuleCache(course.id, student, grading_context['all_descriptors']) @@ -83,7 +85,7 @@ def grade(student, request, course, student_module_cache=None): if correct is None and total is None: continue - if settings.GENERATE_PROFILE_SCORES: + if settings.GENERATE_PROFILE_SCORES: # for debugging! if total > 1: correct = random.randrange(max(total - 2, 1), total + 1) else: @@ -97,6 +99,8 @@ def grade(student, request, course, student_module_cache=None): scores.append(Score(correct, total, graded, module.metadata.get('display_name'))) section_total, graded_total = graders.aggregate_scores(scores, section_name) + if keep_raw_scores: + raw_scores += scores else: section_total = Score(0.0, 1.0, False, section_name) graded_total = Score(0.0, 1.0, True, section_name) @@ -117,7 +121,10 @@ def grade(student, request, course, student_module_cache=None): letter_grade = grade_for_percentage(course.grade_cutoffs, grade_summary['percent']) grade_summary['grade'] = letter_grade - + grade_summary['totaled_scores'] = totaled_scores # make this available, eg for instructor download & debugging + if keep_raw_scores: + grade_summary['raw_scores'] = raw_scores # way to get all RAW scores out to instructor + # so grader can be double-checked return grade_summary def grade_for_percentage(grade_cutoffs, percentage): diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 71ec687cf6..aa3444b193 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -361,96 +361,3 @@ def progress(request, course_id, student_id=None): -# ======== Instructor views ============================================================================= - -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -def gradebook(request, course_id): - """ - Show the gradebook for this course: - - only displayed to course staff - - shows students who are enrolled. - """ - course = get_course_with_access(request.user, course_id, 'staff') - - enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username') - - # TODO (vshnayder): implement pagination. - enrolled_students = enrolled_students[:1000] # HACK! - - student_info = [{'username': student.username, - 'id': student.id, - 'email': student.email, - 'grade_summary': grades.grade(student, request, course), - 'realname': UserProfile.objects.get(user=student).name - } - for student in enrolled_students] - - return render_to_response('courseware/gradebook.html', {'students': student_info, - 'course': course, - 'course_id': course_id, - # Checked above - 'staff_access': True,}) - - -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -def grade_summary(request, course_id): - """Display the grade summary for a course.""" - course = get_course_with_access(request.user, course_id, 'staff') - - # For now, just a static page - context = {'course': course, - 'staff_access': True,} - return render_to_response('courseware/grade_summary.html', context) - - -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -def instructor_dashboard(request, course_id): - """Display the instructor dashboard for a course.""" - course = get_course_with_access(request.user, course_id, 'staff') - - # For now, just a static page - context = {'course': course, - 'staff_access': True,} - - return render_to_response('courseware/instructor_dashboard.html', context) - -@ensure_csrf_cookie -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -def enroll_students(request, course_id): - ''' Allows a staff member to enroll students in a course. - - This is a short-term hack for Berkeley courses launching fall - 2012. In the long term, we would like functionality like this, but - we would like both the instructor and the student to agree. Right - now, this allows any instructor to add students to their course, - which we do not want. - - It is poorly written and poorly tested, but it's designed to be - stripped out. - ''' - - course = get_course_with_access(request.user, course_id, 'staff') - existing_students = [ce.user.email for ce in CourseEnrollment.objects.filter(course_id = course_id)] - - if 'new_students' in request.POST: - new_students = request.POST['new_students'].split('\n') - else: - new_students = [] - new_students = [s.strip() for s in new_students] - - added_students = [] - rejected_students = [] - - for student in new_students: - try: - nce = CourseEnrollment(user=User.objects.get(email = student), course_id = course_id) - nce.save() - added_students.append(student) - except: - rejected_students.append(student) - - return render_to_response("enroll_students.html", {'course':course_id, - 'existing_students': existing_students, - 'added_students': added_students, - 'rejected_students': rejected_students, - 'debug':new_students}) diff --git a/lms/djangoapps/instructor/__init__.py b/lms/djangoapps/instructor/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py new file mode 100644 index 0000000000..ccf40cd5e5 --- /dev/null +++ b/lms/djangoapps/instructor/views.py @@ -0,0 +1,330 @@ +# ======== Instructor views ============================================================================= + +import csv +import json +import logging +import urllib +import itertools + +from functools import partial +from collections import defaultdict + +from django.conf import settings +from django.core.context_processors import csrf +from django.core.urlresolvers import reverse +from django.contrib.auth.models import User, Group +from django.contrib.auth.decorators import login_required +from django.http import Http404, HttpResponse +from django.shortcuts import redirect +from mitxmako.shortcuts import render_to_response, render_to_string +#from django.views.decorators.csrf import ensure_csrf_cookie +from django_future.csrf import ensure_csrf_cookie +from django.views.decorators.cache import cache_control + +from courseware import grades +from courseware.access import has_access, get_access_group_name +from courseware.courses import (get_course_with_access, get_courses_by_university) +from student.models import UserProfile + +from student.models import UserTestGroup, CourseEnrollment +from util.cache import cache, cache_if_anonymous +from xmodule.course_module import CourseDescriptor +from xmodule.modulestore import Location +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem +from xmodule.modulestore.search import path_to_location + +log = logging.getLogger("mitx.courseware") + +template_imports = {'urllib': urllib} + +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +def instructor_dashboard(request, course_id): + """Display the instructor dashboard for a course.""" + course = get_course_with_access(request.user, course_id, 'staff') + + instructor_access = has_access(request.user, course, 'instructor') # an instructor can manage staff lists + + msg = '' + # msg += ('POST=%s' % dict(request.POST)).replace('<','<') + + def escape(s): + """escape HTML special characters in string""" + return str(s).replace('<','<').replace('>','>') + + # assemble some course statistics for output to instructor + datatable = {'header': ['Statistic','Value'], + 'title': 'Course Statistics At A Glance', + } + data = [ ['# Enrolled' ,CourseEnrollment.objects.filter(course_id=course_id).count()] ] + data += compute_course_stats(course).items() + if request.user.is_staff: + data.append(['metadata', escape(str(course.metadata))]) + datatable['data'] = data + + def return_csv(fn,datatable): + response = HttpResponse(mimetype='text/csv') + response['Content-Disposition'] = 'attachment; filename=%s' % fn + writer = csv.writer(response,dialect='excel',quotechar='"', quoting=csv.QUOTE_ALL) + writer.writerow(datatable['header']) + for datarow in datatable['data']: + writer.writerow(datarow) + return response + + def get_staff_group(course): + staffgrp = get_access_group_name(course,'staff') + try: + group = Group.objects.get(name=staffgrp) + except Group.DoesNotExist: + group = Group(name=staffgrp) # create the group + group.save() + return group + + # process actions from form POST + action = request.POST.get('action','') + + if 'Reload' in action: + log.debug('reloading %s (%s)' % (course_id,course)) + try: + data_dir = course.metadata['data_dir'] + modulestore().try_load_course(data_dir) + msg += "

Course reloaded from %s

" % data_dir + except Exception as err: + msg += '

Error: %s

' % escape(err) + + elif action=='Dump list of enrolled students': + log.debug(action) + datatable = get_student_grade_summary_data(request, course, course_id, get_grades=False) + datatable['title'] = 'List of students enrolled in %s' % course_id + + elif 'Dump Grades' in action: + log.debug(action) + datatable = get_student_grade_summary_data(request, course, course_id, get_grades=True) + datatable['title'] = 'Summary Grades of students enrolled in %s' % course_id + + elif 'Dump all RAW grades' in action: + log.debug(action) + datatable = get_student_grade_summary_data(request, course, course_id, get_grades=True, + get_raw_scores=True) + datatable['title'] = 'Raw Grades of students enrolled in %s' % course_id + + elif 'Download CSV of all student grades' in action: + return return_csv('grades_%s.csv' % course_id, + get_student_grade_summary_data(request, course, course_id)) + + elif 'Download CSV of all RAW grades' in action: + return return_csv('grades_%s_raw.csv' % course_id, + get_student_grade_summary_data(request, course, course_id, get_raw_scores=True)) + + elif 'List course staff' in action: + group = get_staff_group(course) + msg += 'Staff group = %s' % group.name + log.debug('staffgrp=%s' % group.name) + uset = group.user_set.all() + datatable = {'header': ['Username','Full name']} + datatable['data'] = [[ x.username, x.profile.name ] for x in uset] + datatable['title'] = 'List of Staff in course %s' % course_id + + elif action=='Add course staff': + uname = request.POST['staffuser'] + try: + user = User.objects.get(username=uname) + except User.DoesNotExist: + msg += 'Error: unknown username "%s"' % uname + user = None + if user is not None: + group = get_staff_group(course) + msg += 'Added %s to staff group = %s' % (user,group.name) + log.debug('staffgrp=%s' % group.name) + user.groups.add(group) + + elif action=='Remove course staff': + uname = request.POST['staffuser'] + try: + user = User.objects.get(username=uname) + except User.DoesNotExist: + msg += 'Error: unknown username "%s"' % uname + user = None + if user is not None: + group = get_staff_group(course) + msg += 'Removed %s from staff group = %s' % (user,group.name) + log.debug('staffgrp=%s' % group.name) + user.groups.remove(group) + + # For now, mostly a static page + context = {'course': course, + 'staff_access': True, + 'admin_access' : request.user.is_staff, + 'instructor_access' : instructor_access, + 'datatable' : datatable, + 'msg' : msg, + } + + return render_to_response('courseware/instructor_dashboard.html', context) + +def get_student_grade_summary_data(request, course, course_id, get_grades=True, get_raw_scores=False): + ''' + Return data arrays with student identity and grades for specified course. + + course = CourseDescriptor + course_id = course ID + + Note: both are passed in, only because instructor_dashboard already has them already. + + returns datatable = dict(header=header, data=data) + where + + header = list of strings labeling the data fields + data = list (one per student) of lists of data corresponding to the fields + + If get_raw_scores=True, then instead of grade summaries, the raw grades for all graded modules are returned. + + ''' + enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username') + + header = ['ID', 'Username','Full Name','edX email','External email'] + if get_grades: + gradeset = grades.grade(enrolled_students[0], request, course, keep_raw_scores=get_raw_scores) # just to construct the header + # log.debug('student %s gradeset %s' % (enrolled_students[0], gradeset)) + if get_raw_scores: + header += [score.section for score in gradeset['raw_scores']] + else: + header += [x['label'] for x in gradeset['section_breakdown']] + + datatable = {'header': header} + data = [] + + for student in enrolled_students: + datarow = [ student.id, student.username, student.profile.name, student.email ] + try: + datarow.append(student.externalauthmap.external_email) + except: # ExternalAuthMap.DoesNotExist + datarow.append('') + + if get_grades: + gradeset = grades.grade(student, request, course, keep_raw_scores=get_raw_scores) + # log.debug('student=%s, gradeset=%s' % (student,gradeset)) + if get_raw_scores: + datarow += [score.earned for score in gradeset['raw_scores']] + else: + datarow += [x['percent'] for x in gradeset['section_breakdown']] + + data.append(datarow) + datatable['data'] = data + return datatable + +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +def gradebook(request, course_id): + """ + Show the gradebook for this course: + - only displayed to course staff + - shows students who are enrolled. + """ + course = get_course_with_access(request.user, course_id, 'staff') + + enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username') + + # TODO (vshnayder): implement pagination. + enrolled_students = enrolled_students[:1000] # HACK! + + student_info = [{'username': student.username, + 'id': student.id, + 'email': student.email, + 'grade_summary': grades.grade(student, request, course), + 'realname': student.profile.name, + } + for student in enrolled_students] + + return render_to_response('courseware/gradebook.html', {'students': student_info, + 'course': course, + 'course_id': course_id, + # Checked above + 'staff_access': True,}) + + +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +def grade_summary(request, course_id): + """Display the grade summary for a course.""" + course = get_course_with_access(request.user, course_id, 'staff') + + # For now, just a static page + context = {'course': course, + 'staff_access': True,} + return render_to_response('courseware/grade_summary.html', context) + +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +def enroll_students(request, course_id): + ''' Allows a staff member to enroll students in a course. + + This is a short-term hack for Berkeley courses launching fall + 2012. In the long term, we would like functionality like this, but + we would like both the instructor and the student to agree. Right + now, this allows any instructor to add students to their course, + which we do not want. + + It is poorly written and poorly tested, but it's designed to be + stripped out. + ''' + + course = get_course_with_access(request.user, course_id, 'staff') + existing_students = [ce.user.email for ce in CourseEnrollment.objects.filter(course_id = course_id)] + + if 'new_students' in request.POST: + new_students = request.POST['new_students'].split('\n') + else: + new_students = [] + new_students = [s.strip() for s in new_students] + + added_students = [] + rejected_students = [] + + for student in new_students: + try: + nce = CourseEnrollment(user=User.objects.get(email = student), course_id = course_id) + nce.save() + added_students.append(student) + except: + rejected_students.append(student) + + return render_to_response("enroll_students.html", {'course':course_id, + 'existing_students': existing_students, + 'added_students': added_students, + 'rejected_students': rejected_students, + 'debug':new_students}) + +#----------------------------------------------------------------------------- + +def compute_course_stats(course): + ''' + Compute course statistics, including number of problems, videos, html. + + course is a CourseDescriptor from the xmodule system. + ''' + + # walk the course by using get_children() until we come to the leaves; count the + # number of different leaf types + + counts = defaultdict(int) + + print "hello world" + + def walk(module): + children = module.get_children() + if not children: + category = module.__class__.__name__ # HtmlDescriptor, CapaDescriptor, ... + counts[category] += 1 + return + for c in children: + # print c.__class__.__name__ + walk(c) + + walk(course) + + print "course %s counts=%s" % (course.display_name,counts) + + stats = dict(counts) # number of each kind of module + + return stats + diff --git a/lms/envs/common.py b/lms/envs/common.py index 5cd28d24d9..cf9a767d9f 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -604,6 +604,7 @@ INSTALLED_APPS = ( 'track', 'util', 'certificates', + 'instructor', #For the wiki 'wiki', # The new django-wiki from benjaoming diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html index 9508624f9b..49b16cf122 100644 --- a/lms/templates/courseware/instructor_dashboard.html +++ b/lms/templates/courseware/instructor_dashboard.html @@ -8,17 +8,98 @@ <%include file="/courseware/course_navigation.html" args="active_page='instructor'" /> + +

Instructor Dashboard

+
+ +

Gradebook

Grade summary +

+ + +

+ + + +

+ + + +%if instructor_access: +

+

+ +

+ + +

+ %endif + +%if admin_access: +

+ +%endif + +

+ +
+
+

+

+

${datatable['title']}

+ + + %for hname in datatable['header']: + + %endfor + + %for row in datatable['data']: + + %for value in row: + + %endfor + + %endfor +
${hname}
${value}
+

+ +%if msg: +

${msg}

+%endif +
diff --git a/lms/urls.py b/lms/urls.py index 278239751b..26aa10a3f4 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -153,14 +153,14 @@ if settings.COURSEWARE_ENABLED: # For the instructor url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/instructor$', - 'courseware.views.instructor_dashboard', name="instructor_dashboard"), + 'instructor.views.instructor_dashboard', name="instructor_dashboard"), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/gradebook$', - 'courseware.views.gradebook', name='gradebook'), + 'instructor.views.gradebook', name='gradebook'), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/grade_summary$', - 'courseware.views.grade_summary', name='grade_summary'), + 'instructor.views.grade_summary', name='grade_summary'), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/enroll_students$', - 'courseware.views.enroll_students', name='enroll_students'), + 'instructor.views.enroll_students', name='enroll_students'), ) # discussion forums live within courseware, so courseware must be enabled first From 4d5ca284b2c798c6d3c0d1e71d05ae649e5dbc29 Mon Sep 17 00:00:00 2001 From: ichuang Date: Sun, 2 Sep 2012 19:10:47 -0400 Subject: [PATCH 03/23] add fix to histogram (so 6.00x codemirror is not broken) --- common/djangoapps/xmodule_modifiers.py | 2 +- rakefile | 229 ------------------------- 2 files changed, 1 insertion(+), 230 deletions(-) delete mode 100644 rakefile diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 10cce1a95b..6c3c237ea6 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -76,7 +76,7 @@ def grade_histogram(module_id): grades = list(cursor.fetchall()) grades.sort(key=lambda x: x[0]) # Add ORDER BY to sql query? - if len(grades) == 1 and grades[0][0] is None: + if len(grades) >= 1 and grades[0][0] is None: return [] return grades diff --git a/rakefile b/rakefile deleted file mode 100644 index 053abf56a8..0000000000 --- a/rakefile +++ /dev/null @@ -1,229 +0,0 @@ -require 'rake/clean' -require 'tempfile' - -# Build Constants -REPO_ROOT = File.dirname(__FILE__) -BUILD_DIR = File.join(REPO_ROOT, "build") -REPORT_DIR = File.join(REPO_ROOT, "reports") -LMS_REPORT_DIR = File.join(REPORT_DIR, "lms") - -# Packaging constants -DEPLOY_DIR = "/opt/wwc" -PACKAGE_NAME = "mitx" -LINK_PATH = "/opt/wwc/mitx" -PKG_VERSION = "0.1" -COMMIT = (ENV["GIT_COMMIT"] || `git rev-parse HEAD`).chomp()[0, 10] -BRANCH = (ENV["GIT_BRANCH"] || `git symbolic-ref -q HEAD`).chomp().gsub('refs/heads/', '').gsub('origin/', '') -BUILD_NUMBER = (ENV["BUILD_NUMBER"] || "dev").chomp() - -if BRANCH == "master" - DEPLOY_NAME = "#{PACKAGE_NAME}-#{BUILD_NUMBER}-#{COMMIT}" -else - DEPLOY_NAME = "#{PACKAGE_NAME}-#{BRANCH}-#{BUILD_NUMBER}-#{COMMIT}" -end -PACKAGE_REPO = "packages@gp.mitx.mit.edu:/opt/pkgrepo.incoming" - -NORMALIZED_DEPLOY_NAME = DEPLOY_NAME.downcase().gsub(/[_\/]/, '-') -INSTALL_DIR_PATH = File.join(DEPLOY_DIR, NORMALIZED_DEPLOY_NAME) -# Set up the clean and clobber tasks -CLOBBER.include(BUILD_DIR, REPORT_DIR, 'cover*', '.coverage', 'test_root/*_repo', 'test_root/staticfiles') -CLEAN.include("#{BUILD_DIR}/*.deb", "#{BUILD_DIR}/util") - -def select_executable(*cmds) - cmds.find_all{ |cmd| system("which #{cmd} > /dev/null 2>&1") }[0] || fail("No executables found from #{cmds.join(', ')}") -end - -def django_admin(system, env, command, *args) - django_admin = ENV['DJANGO_ADMIN_PATH'] || select_executable('django-admin.py', 'django-admin') - return "#{django_admin} #{command} --settings=#{system}.envs.#{env} --pythonpath=. #{args.join(' ')}" -end - -task :default => [:test, :pep8, :pylint] - -directory REPORT_DIR - -default_options = { - :lms => '8000', - :cms => '8001', -} - -task :predjango do - sh("find . -type f -name *.pyc -delete") - sh('pip install -e common/lib/xmodule') - sh('git submodule update --init') -end - -task :clean_test_files do - sh("git clean -fdx test_root") -end - -[:lms, :cms, :common].each do |system| - report_dir = File.join(REPORT_DIR, system.to_s) - directory report_dir - - desc "Run pep8 on all #{system} code" - task "pep8_#{system}" => report_dir do - sh("pep8 --ignore=E501 #{system}/djangoapps #{system}/lib | tee #{report_dir}/pep8.report") - end - task :pep8 => "pep8_#{system}" - - desc "Run pylint on all #{system} code" - task "pylint_#{system}" => report_dir do - Dir["#{system}/djangoapps/*", "#{system}/lib/*"].each do |app| - if File.exists? "#{app}/setup.py" - pythonpath_prefix = "PYTHONPATH=#{app}" - else - pythonpath_prefix = "PYTHONPATH=#{File.dirname(app)}" - end - app = File.basename(app) - sh("#{pythonpath_prefix} pylint --rcfile=.pylintrc -f parseable #{app} | tee #{report_dir}/#{app}.pylint.report") - end - end - task :pylint => "pylint_#{system}" -end - -$failed_tests = 0 - -def run_tests(system, report_dir, stop_on_failure=true) - ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml") - ENV['NOSE_COVER_HTML_DIR'] = File.join(report_dir, "cover") - dirs = Dir["common/djangoapps/*"] + Dir["#{system}/djangoapps/*"] - sh(django_admin(system, :test, 'test', *dirs.each)) do |ok, res| - if !ok and stop_on_failure - abort "Test failed!" - end - $failed_tests += 1 unless ok - end -end - -TEST_TASKS = [] - -[:lms, :cms].each do |system| - report_dir = File.join(REPORT_DIR, system.to_s) - directory report_dir - - # Per System tasks - desc "Run all django tests on our djangoapps for the #{system}" - task "test_#{system}", [:stop_on_failure] => ["clean_test_files", "#{system}:collectstatic:test", "fasttest_#{system}"] - - # Have a way to run the tests without running collectstatic -- useful when debugging without - # messing with static files. - task "fasttest_#{system}", [:stop_on_failure] => [report_dir, :predjango] do |t, args| - args.with_defaults(:stop_on_failure => 'true') - run_tests(system, report_dir, args.stop_on_failure) - end - - TEST_TASKS << "test_#{system}" - - desc <<-desc - Start the #{system} locally with the specified environment (defaults to dev). - Other useful environments are devplus (for dev testing with a real local database) - desc - task system, [:env, :options] => [:predjango] do |t, args| - args.with_defaults(:env => 'dev', :options => default_options[system]) - sh(django_admin(system, args.env, 'runserver', args.options)) - end - - # Per environment tasks - Dir["#{system}/envs/*.py"].each do |env_file| - env = File.basename(env_file).gsub(/\.py/, '') - desc "Attempt to import the settings file #{system}.envs.#{env} and report any errors" - task "#{system}:check_settings:#{env}" => :predjango do - sh("echo 'import #{system}.envs.#{env}' | #{django_admin(system, env, 'shell')}") - end - - desc "Run collectstatic in the specified environment" - task "#{system}:collectstatic:#{env}" => :predjango do - sh("#{django_admin(system, env, 'collectstatic', '--noinput')}") - end - end -end - -Dir["common/lib/*"].each do |lib| - task_name = "test_#{lib}" - - report_dir = File.join(REPORT_DIR, task_name.gsub('/', '_')) - directory report_dir - - desc "Run tests for common lib #{lib}" - task task_name => report_dir do - ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml") - sh("nosetests #{lib} --cover-erase --with-xunit --with-xcoverage --cover-html --cover-inclusive --cover-package #{File.basename(lib)} --cover-html-dir #{File.join(report_dir, "cover")}") - end - TEST_TASKS << task_name -end - -task :test do - TEST_TASKS.each do |task| - Rake::Task[task].invoke(false) - end - - if $failed_tests > 0 - abort "Tests failed!" - end -end - -task :runserver => :lms - -desc "Run django-admin against the specified system and environment" -task "django-admin", [:action, :system, :env, :options] => [:predjango] do |t, args| - args.with_defaults(:env => 'dev', :system => 'lms', :options => '') - sh(django_admin(args.system, args.env, args.action, args.options)) -end - -task :package do - FileUtils.mkdir_p(BUILD_DIR) - - Dir.chdir(BUILD_DIR) do - afterremove = Tempfile.new('afterremove') - afterremove.write <<-AFTERREMOVE.gsub(/^\s*/, '') - #! /bin/bash - set -e - set -x - - # to be a little safer this rm is executed - # as the makeitso user - - if [[ -d "#{INSTALL_DIR_PATH}" ]]; then - sudo rm -rf "#{INSTALL_DIR_PATH}" - fi - - AFTERREMOVE - afterremove.close() - FileUtils.chmod(0755, afterremove.path) - - args = ["fakeroot", "fpm", "-s", "dir", "-t", "deb", - "--after-remove=#{afterremove.path}", - "--prefix=#{INSTALL_DIR_PATH}", - "--exclude=**/build/**", - "--exclude=**/rakefile", - "--exclude=**/.git/**", - "--exclude=**/*.pyc", - "--exclude=**/reports/**", - "--exclude=**/test_root/**", - "--exclude=**/.coverage/**", - "-C", "#{REPO_ROOT}", - "--provides=#{PACKAGE_NAME}", - "--name=#{NORMALIZED_DEPLOY_NAME}", - "--version=#{PKG_VERSION}", - "-a", "all", - "."] - system(*args) || raise("fpm failed to build the .deb") - end -end - -task :publish => :package do - sh("scp #{BUILD_DIR}/#{NORMALIZED_DEPLOY_NAME}_#{PKG_VERSION}*.deb #{PACKAGE_REPO}") -end - -namespace :cms do - desc "Import course data within the given DATA_DIR variable" - task :import do - if ENV['DATA_DIR'] - sh(django_admin(:cms, :dev, :import, ENV['DATA_DIR'])) - else - raise "Please specify a DATA_DIR variable that point to your data directory.\n" + - "Example: \`rake cms:import DATA_DIR=../data\`" - end - end -end From 0ddd8949ba856973edffe17084b2d1336d87b29c Mon Sep 17 00:00:00 2001 From: ichuang Date: Sun, 2 Sep 2012 19:17:19 -0400 Subject: [PATCH 04/23] re-add rakefile which was deleted by mistake --- rakefile | 229 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 229 insertions(+) create mode 100644 rakefile diff --git a/rakefile b/rakefile new file mode 100644 index 0000000000..053abf56a8 --- /dev/null +++ b/rakefile @@ -0,0 +1,229 @@ +require 'rake/clean' +require 'tempfile' + +# Build Constants +REPO_ROOT = File.dirname(__FILE__) +BUILD_DIR = File.join(REPO_ROOT, "build") +REPORT_DIR = File.join(REPO_ROOT, "reports") +LMS_REPORT_DIR = File.join(REPORT_DIR, "lms") + +# Packaging constants +DEPLOY_DIR = "/opt/wwc" +PACKAGE_NAME = "mitx" +LINK_PATH = "/opt/wwc/mitx" +PKG_VERSION = "0.1" +COMMIT = (ENV["GIT_COMMIT"] || `git rev-parse HEAD`).chomp()[0, 10] +BRANCH = (ENV["GIT_BRANCH"] || `git symbolic-ref -q HEAD`).chomp().gsub('refs/heads/', '').gsub('origin/', '') +BUILD_NUMBER = (ENV["BUILD_NUMBER"] || "dev").chomp() + +if BRANCH == "master" + DEPLOY_NAME = "#{PACKAGE_NAME}-#{BUILD_NUMBER}-#{COMMIT}" +else + DEPLOY_NAME = "#{PACKAGE_NAME}-#{BRANCH}-#{BUILD_NUMBER}-#{COMMIT}" +end +PACKAGE_REPO = "packages@gp.mitx.mit.edu:/opt/pkgrepo.incoming" + +NORMALIZED_DEPLOY_NAME = DEPLOY_NAME.downcase().gsub(/[_\/]/, '-') +INSTALL_DIR_PATH = File.join(DEPLOY_DIR, NORMALIZED_DEPLOY_NAME) +# Set up the clean and clobber tasks +CLOBBER.include(BUILD_DIR, REPORT_DIR, 'cover*', '.coverage', 'test_root/*_repo', 'test_root/staticfiles') +CLEAN.include("#{BUILD_DIR}/*.deb", "#{BUILD_DIR}/util") + +def select_executable(*cmds) + cmds.find_all{ |cmd| system("which #{cmd} > /dev/null 2>&1") }[0] || fail("No executables found from #{cmds.join(', ')}") +end + +def django_admin(system, env, command, *args) + django_admin = ENV['DJANGO_ADMIN_PATH'] || select_executable('django-admin.py', 'django-admin') + return "#{django_admin} #{command} --settings=#{system}.envs.#{env} --pythonpath=. #{args.join(' ')}" +end + +task :default => [:test, :pep8, :pylint] + +directory REPORT_DIR + +default_options = { + :lms => '8000', + :cms => '8001', +} + +task :predjango do + sh("find . -type f -name *.pyc -delete") + sh('pip install -e common/lib/xmodule') + sh('git submodule update --init') +end + +task :clean_test_files do + sh("git clean -fdx test_root") +end + +[:lms, :cms, :common].each do |system| + report_dir = File.join(REPORT_DIR, system.to_s) + directory report_dir + + desc "Run pep8 on all #{system} code" + task "pep8_#{system}" => report_dir do + sh("pep8 --ignore=E501 #{system}/djangoapps #{system}/lib | tee #{report_dir}/pep8.report") + end + task :pep8 => "pep8_#{system}" + + desc "Run pylint on all #{system} code" + task "pylint_#{system}" => report_dir do + Dir["#{system}/djangoapps/*", "#{system}/lib/*"].each do |app| + if File.exists? "#{app}/setup.py" + pythonpath_prefix = "PYTHONPATH=#{app}" + else + pythonpath_prefix = "PYTHONPATH=#{File.dirname(app)}" + end + app = File.basename(app) + sh("#{pythonpath_prefix} pylint --rcfile=.pylintrc -f parseable #{app} | tee #{report_dir}/#{app}.pylint.report") + end + end + task :pylint => "pylint_#{system}" +end + +$failed_tests = 0 + +def run_tests(system, report_dir, stop_on_failure=true) + ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml") + ENV['NOSE_COVER_HTML_DIR'] = File.join(report_dir, "cover") + dirs = Dir["common/djangoapps/*"] + Dir["#{system}/djangoapps/*"] + sh(django_admin(system, :test, 'test', *dirs.each)) do |ok, res| + if !ok and stop_on_failure + abort "Test failed!" + end + $failed_tests += 1 unless ok + end +end + +TEST_TASKS = [] + +[:lms, :cms].each do |system| + report_dir = File.join(REPORT_DIR, system.to_s) + directory report_dir + + # Per System tasks + desc "Run all django tests on our djangoapps for the #{system}" + task "test_#{system}", [:stop_on_failure] => ["clean_test_files", "#{system}:collectstatic:test", "fasttest_#{system}"] + + # Have a way to run the tests without running collectstatic -- useful when debugging without + # messing with static files. + task "fasttest_#{system}", [:stop_on_failure] => [report_dir, :predjango] do |t, args| + args.with_defaults(:stop_on_failure => 'true') + run_tests(system, report_dir, args.stop_on_failure) + end + + TEST_TASKS << "test_#{system}" + + desc <<-desc + Start the #{system} locally with the specified environment (defaults to dev). + Other useful environments are devplus (for dev testing with a real local database) + desc + task system, [:env, :options] => [:predjango] do |t, args| + args.with_defaults(:env => 'dev', :options => default_options[system]) + sh(django_admin(system, args.env, 'runserver', args.options)) + end + + # Per environment tasks + Dir["#{system}/envs/*.py"].each do |env_file| + env = File.basename(env_file).gsub(/\.py/, '') + desc "Attempt to import the settings file #{system}.envs.#{env} and report any errors" + task "#{system}:check_settings:#{env}" => :predjango do + sh("echo 'import #{system}.envs.#{env}' | #{django_admin(system, env, 'shell')}") + end + + desc "Run collectstatic in the specified environment" + task "#{system}:collectstatic:#{env}" => :predjango do + sh("#{django_admin(system, env, 'collectstatic', '--noinput')}") + end + end +end + +Dir["common/lib/*"].each do |lib| + task_name = "test_#{lib}" + + report_dir = File.join(REPORT_DIR, task_name.gsub('/', '_')) + directory report_dir + + desc "Run tests for common lib #{lib}" + task task_name => report_dir do + ENV['NOSE_XUNIT_FILE'] = File.join(report_dir, "nosetests.xml") + sh("nosetests #{lib} --cover-erase --with-xunit --with-xcoverage --cover-html --cover-inclusive --cover-package #{File.basename(lib)} --cover-html-dir #{File.join(report_dir, "cover")}") + end + TEST_TASKS << task_name +end + +task :test do + TEST_TASKS.each do |task| + Rake::Task[task].invoke(false) + end + + if $failed_tests > 0 + abort "Tests failed!" + end +end + +task :runserver => :lms + +desc "Run django-admin against the specified system and environment" +task "django-admin", [:action, :system, :env, :options] => [:predjango] do |t, args| + args.with_defaults(:env => 'dev', :system => 'lms', :options => '') + sh(django_admin(args.system, args.env, args.action, args.options)) +end + +task :package do + FileUtils.mkdir_p(BUILD_DIR) + + Dir.chdir(BUILD_DIR) do + afterremove = Tempfile.new('afterremove') + afterremove.write <<-AFTERREMOVE.gsub(/^\s*/, '') + #! /bin/bash + set -e + set -x + + # to be a little safer this rm is executed + # as the makeitso user + + if [[ -d "#{INSTALL_DIR_PATH}" ]]; then + sudo rm -rf "#{INSTALL_DIR_PATH}" + fi + + AFTERREMOVE + afterremove.close() + FileUtils.chmod(0755, afterremove.path) + + args = ["fakeroot", "fpm", "-s", "dir", "-t", "deb", + "--after-remove=#{afterremove.path}", + "--prefix=#{INSTALL_DIR_PATH}", + "--exclude=**/build/**", + "--exclude=**/rakefile", + "--exclude=**/.git/**", + "--exclude=**/*.pyc", + "--exclude=**/reports/**", + "--exclude=**/test_root/**", + "--exclude=**/.coverage/**", + "-C", "#{REPO_ROOT}", + "--provides=#{PACKAGE_NAME}", + "--name=#{NORMALIZED_DEPLOY_NAME}", + "--version=#{PKG_VERSION}", + "-a", "all", + "."] + system(*args) || raise("fpm failed to build the .deb") + end +end + +task :publish => :package do + sh("scp #{BUILD_DIR}/#{NORMALIZED_DEPLOY_NAME}_#{PKG_VERSION}*.deb #{PACKAGE_REPO}") +end + +namespace :cms do + desc "Import course data within the given DATA_DIR variable" + task :import do + if ENV['DATA_DIR'] + sh(django_admin(:cms, :dev, :import, ENV['DATA_DIR'])) + else + raise "Please specify a DATA_DIR variable that point to your data directory.\n" + + "Example: \`rake cms:import DATA_DIR=../data\`" + end + end +end From 9e8ec09b4d9855c140dfa8b097d2906c48c25e27 Mon Sep 17 00:00:00 2001 From: ichuang Date: Sun, 2 Sep 2012 20:15:57 -0400 Subject: [PATCH 05/23] pep8 fixes --- common/djangoapps/student/models.py | 36 +++++++++---- lms/djangoapps/instructor/views.py | 78 ++++++++++++++--------------- 2 files changed, 65 insertions(+), 49 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index c494035104..529a6f1298 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -4,7 +4,7 @@ Models for Student Information Replication Notes In our live deployment, we intend to run in a scenario where there is a pool of -Portal servers that hold the canoncial user information and that user +Portal servers that hold the canoncial user information and that user information is replicated to slave Course server pools. Each Course has a set of servers that serves only its content and has users that are relevant only to it. @@ -61,6 +61,7 @@ from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) + class UserProfile(models.Model): """This is where we store all the user demographic fields. We have a separate table for this rather than extending the built-in Django auth_user. @@ -175,6 +176,7 @@ class PendingEmailChange(models.Model): new_email = models.CharField(blank=True, max_length=255, db_index=True) activation_key = models.CharField(('activation key'), max_length=32, unique=True, db_index=True) + class CourseEnrollment(models.Model): user = models.ForeignKey(User) course_id = models.CharField(max_length=255, db_index=True) @@ -185,7 +187,8 @@ class CourseEnrollment(models.Model): unique_together = (('user', 'course_id'), ) def __unicode__(self): - return "%s: %s (%s)" % (self.user,self.course_id,self.created) + return "%s: %s (%s)" % (self.user, self.course_id, self.created) + @receiver(post_save, sender=CourseEnrollment) def assign_default_role(sender, instance, **kwargs): @@ -276,6 +279,7 @@ def add_user_to_default_group(user, group): utg.users.add(User.objects.get(username=user)) utg.save() + @receiver(post_save, sender=User) def update_user_information(sender, instance, created, **kwargs): try: @@ -286,6 +290,7 @@ def update_user_information(sender, instance, created, **kwargs): log.error(unicode(e)) log.error("update user info to discussion failed for user with id: " + str(instance.id)) + ########################## REPLICATION SIGNALS ################################# # @receiver(post_save, sender=User) def replicate_user_save(sender, **kwargs): @@ -295,6 +300,7 @@ def replicate_user_save(sender, **kwargs): for course_db_name in db_names_to_replicate_to(user_obj.id): replicate_user(user_obj, course_db_name) + # @receiver(post_save, sender=CourseEnrollment) def replicate_enrollment_save(sender, **kwargs): """This is called when a Student enrolls in a course. It has to do the @@ -320,12 +326,14 @@ def replicate_enrollment_save(sender, **kwargs): log.debug("Replicating user profile because of new enrollment") user_profile = UserProfile.objects.get(user_id=enrollment_obj.user_id) replicate_model(UserProfile.save, user_profile, enrollment_obj.user_id) - + + # @receiver(post_delete, sender=CourseEnrollment) def replicate_enrollment_delete(sender, **kwargs): enrollment_obj = kwargs['instance'] return replicate_model(CourseEnrollment.delete, enrollment_obj, enrollment_obj.user_id) - + + # @receiver(post_save, sender=UserProfile) def replicate_userprofile_save(sender, **kwargs): """We just updated the UserProfile (say an update to the name), so push that @@ -333,12 +341,13 @@ def replicate_userprofile_save(sender, **kwargs): user_profile_obj = kwargs['instance'] return replicate_model(UserProfile.save, user_profile_obj, user_profile_obj.user_id) - + ######### Replication functions ######### USER_FIELDS_TO_COPY = ["id", "username", "first_name", "last_name", "email", "password", "is_staff", "is_active", "is_superuser", "last_login", "date_joined"] + def replicate_user(portal_user, course_db_name): """Replicate a User to the correct Course DB. This is more complicated than it should be because Askbot extends the auth_user table and adds its own @@ -362,9 +371,10 @@ def replicate_user(portal_user, course_db_name): course_user.save(using=course_db_name) unmark(course_user) + def replicate_model(model_method, instance, user_id): """ - model_method is the model action that we want replicated. For instance, + model_method is the model action that we want replicated. For instance, UserProfile.save """ if not should_replicate(instance): @@ -379,8 +389,10 @@ def replicate_model(model_method, instance, user_id): model_method(instance, using=db_name) unmark(instance) + ######### Replication Helpers ######### + def is_valid_course_id(course_id): """Right now, the only database that's not a course database is 'default'. I had nicer checking in here originally -- it would scan the courses that @@ -390,26 +402,30 @@ def is_valid_course_id(course_id): """ return course_id != 'default' + def is_portal(): """Are we in the portal pool? Only Portal servers are allowed to replicate their changes. For now, only Portal servers see multiple DBs, so we use that to decide.""" return len(settings.DATABASES) > 1 + def db_names_to_replicate_to(user_id): """Return a list of DB names that this user_id is enrolled in.""" return [c.course_id for c in CourseEnrollment.objects.filter(user_id=user_id) if is_valid_course_id(c.course_id)] + def marked_handled(instance): """Have we marked this instance as being handled to avoid infinite loops caused by saving models in post_save hooks for the same models?""" return hasattr(instance, '_do_not_copy_to_course_db') and instance._do_not_copy_to_course_db + def mark_handled(instance): """You have to mark your instance with this function or else we'll go into - an infinite loop since we're putting listeners on Model saves/deletes and + an infinite loop since we're putting listeners on Model saves/deletes and the act of replication requires us to call the same model method. We create a _replicated attribute to differentiate the first save of this @@ -418,16 +434,18 @@ def mark_handled(instance): """ instance._do_not_copy_to_course_db = True + def unmark(instance): - """If we don't unmark a model after we do replication, then consecutive + """If we don't unmark a model after we do replication, then consecutive save() calls won't be properly replicated.""" instance._do_not_copy_to_course_db = False + def should_replicate(instance): """Should this instance be replicated? We need to be a Portal server and the instance has to not have been marked_handled.""" if marked_handled(instance): - # Basically, avoid an infinite loop. You should + # Basically, avoid an infinite loop. You should log.debug("{0} should not be replicated because it's been marked" .format(instance)) return False diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index ccf40cd5e5..bfbfab6e25 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -38,54 +38,55 @@ log = logging.getLogger("mitx.courseware") template_imports = {'urllib': urllib} + @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) def instructor_dashboard(request, course_id): """Display the instructor dashboard for a course.""" course = get_course_with_access(request.user, course_id, 'staff') - instructor_access = has_access(request.user, course, 'instructor') # an instructor can manage staff lists + instructor_access = has_access(request.user, course, 'instructor') # an instructor can manage staff lists msg = '' # msg += ('POST=%s' % dict(request.POST)).replace('<','<') def escape(s): """escape HTML special characters in string""" - return str(s).replace('<','<').replace('>','>') + return str(s).replace('<', '<').replace('>', '>') # assemble some course statistics for output to instructor - datatable = {'header': ['Statistic','Value'], + datatable = {'header': ['Statistic', 'Value'], 'title': 'Course Statistics At A Glance', } - data = [ ['# Enrolled' ,CourseEnrollment.objects.filter(course_id=course_id).count()] ] + data = [['# Enrolled', CourseEnrollment.objects.filter(course_id=course_id).count()]] data += compute_course_stats(course).items() if request.user.is_staff: data.append(['metadata', escape(str(course.metadata))]) datatable['data'] = data - def return_csv(fn,datatable): + def return_csv(fn, datatable): response = HttpResponse(mimetype='text/csv') response['Content-Disposition'] = 'attachment; filename=%s' % fn - writer = csv.writer(response,dialect='excel',quotechar='"', quoting=csv.QUOTE_ALL) + writer = csv.writer(response, dialect='excel', quotechar='"', quoting=csv.QUOTE_ALL) writer.writerow(datatable['header']) for datarow in datatable['data']: writer.writerow(datarow) return response def get_staff_group(course): - staffgrp = get_access_group_name(course,'staff') + staffgrp = get_access_group_name(course, 'staff') try: group = Group.objects.get(name=staffgrp) except Group.DoesNotExist: - group = Group(name=staffgrp) # create the group + group = Group(name=staffgrp) # create the group group.save() return group # process actions from form POST - action = request.POST.get('action','') + action = request.POST.get('action', '') if 'Reload' in action: - log.debug('reloading %s (%s)' % (course_id,course)) + log.debug('reloading %s (%s)' % (course_id, course)) try: data_dir = course.metadata['data_dir'] modulestore().try_load_course(data_dir) @@ -93,7 +94,7 @@ def instructor_dashboard(request, course_id): except Exception as err: msg += '

Error: %s

' % escape(err) - elif action=='Dump list of enrolled students': + elif action == 'Dump list of enrolled students': log.debug(action) datatable = get_student_grade_summary_data(request, course, course_id, get_grades=False) datatable['title'] = 'List of students enrolled in %s' % course_id @@ -122,11 +123,11 @@ def instructor_dashboard(request, course_id): msg += 'Staff group = %s' % group.name log.debug('staffgrp=%s' % group.name) uset = group.user_set.all() - datatable = {'header': ['Username','Full name']} - datatable['data'] = [[ x.username, x.profile.name ] for x in uset] + datatable = {'header': ['Username', 'Full name']} + datatable['data'] = [[x.username, x.profile.name] for x in uset] datatable['title'] = 'List of Staff in course %s' % course_id - elif action=='Add course staff': + elif action == 'Add course staff': uname = request.POST['staffuser'] try: user = User.objects.get(username=uname) @@ -135,11 +136,11 @@ def instructor_dashboard(request, course_id): user = None if user is not None: group = get_staff_group(course) - msg += 'Added %s to staff group = %s' % (user,group.name) + msg += 'Added %s to staff group = %s' % (user, group.name) log.debug('staffgrp=%s' % group.name) user.groups.add(group) - elif action=='Remove course staff': + elif action == 'Remove course staff': uname = request.POST['staffuser'] try: user = User.objects.get(username=uname) @@ -148,21 +149,22 @@ def instructor_dashboard(request, course_id): user = None if user is not None: group = get_staff_group(course) - msg += 'Removed %s from staff group = %s' % (user,group.name) + msg += 'Removed %s from staff group = %s' % (user, group.name) log.debug('staffgrp=%s' % group.name) user.groups.remove(group) # For now, mostly a static page context = {'course': course, 'staff_access': True, - 'admin_access' : request.user.is_staff, - 'instructor_access' : instructor_access, - 'datatable' : datatable, - 'msg' : msg, + 'admin_access': request.user.is_staff, + 'instructor_access': instructor_access, + 'datatable': datatable, + 'msg': msg, } return render_to_response('courseware/instructor_dashboard.html', context) + def get_student_grade_summary_data(request, course, course_id, get_grades=True, get_raw_scores=False): ''' Return data arrays with student identity and grades for specified course. @@ -171,7 +173,7 @@ def get_student_grade_summary_data(request, course, course_id, get_grades=True, course_id = course ID Note: both are passed in, only because instructor_dashboard already has them already. - + returns datatable = dict(header=header, data=data) where @@ -183,9 +185,10 @@ def get_student_grade_summary_data(request, course, course_id, get_grades=True, ''' enrolled_students = User.objects.filter(courseenrollment__course_id=course_id).order_by('username') - header = ['ID', 'Username','Full Name','edX email','External email'] + header = ['ID', 'Username', 'Full Name', 'edX email', 'External email'] if get_grades: - gradeset = grades.grade(enrolled_students[0], request, course, keep_raw_scores=get_raw_scores) # just to construct the header + # just to construct the header + gradeset = grades.grade(enrolled_students[0], request, course, keep_raw_scores=get_raw_scores) # log.debug('student %s gradeset %s' % (enrolled_students[0], gradeset)) if get_raw_scores: header += [score.section for score in gradeset['raw_scores']] @@ -196,7 +199,7 @@ def get_student_grade_summary_data(request, course, course_id, get_grades=True, data = [] for student in enrolled_students: - datarow = [ student.id, student.username, student.profile.name, student.email ] + datarow = [ student.id, student.username, student.profile.name, student.email ] try: datarow.append(student.externalauthmap.external_email) except: # ExternalAuthMap.DoesNotExist @@ -214,6 +217,7 @@ def get_student_grade_summary_data(request, course, course_id, get_grades=True, datatable['data'] = data return datatable + @cache_control(no_cache=True, no_store=True, must_revalidate=True) def gradebook(request, course_id): """ @@ -240,7 +244,7 @@ def gradebook(request, course_id): 'course': course, 'course_id': course_id, # Checked above - 'staff_access': True,}) + 'staff_access': True, }) @cache_control(no_cache=True, no_store=True, must_revalidate=True) @@ -250,9 +254,10 @@ def grade_summary(request, course_id): # For now, just a static page context = {'course': course, - 'staff_access': True,} + 'staff_access': True, } return render_to_response('courseware/grade_summary.html', context) + @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) def enroll_students(request, course_id): @@ -269,7 +274,7 @@ def enroll_students(request, course_id): ''' course = get_course_with_access(request.user, course_id, 'staff') - existing_students = [ce.user.email for ce in CourseEnrollment.objects.filter(course_id = course_id)] + existing_students = [ce.user.email for ce in CourseEnrollment.objects.filter(course_id=course_id)] if 'new_students' in request.POST: new_students = request.POST['new_students'].split('\n') @@ -282,20 +287,21 @@ def enroll_students(request, course_id): for student in new_students: try: - nce = CourseEnrollment(user=User.objects.get(email = student), course_id = course_id) + nce = CourseEnrollment(user=User.objects.get(email=student), course_id=course_id) nce.save() added_students.append(student) except: rejected_students.append(student) - return render_to_response("enroll_students.html", {'course':course_id, + return render_to_response("enroll_students.html", {'course': course_id, 'existing_students': existing_students, 'added_students': added_students, 'rejected_students': rejected_students, - 'debug':new_students}) + 'debug': new_students}) #----------------------------------------------------------------------------- + def compute_course_stats(course): ''' Compute course statistics, including number of problems, videos, html. @@ -308,23 +314,15 @@ def compute_course_stats(course): counts = defaultdict(int) - print "hello world" - def walk(module): children = module.get_children() if not children: - category = module.__class__.__name__ # HtmlDescriptor, CapaDescriptor, ... + category = module.__class__.__name__ # HtmlDescriptor, CapaDescriptor, ... counts[category] += 1 return for c in children: - # print c.__class__.__name__ walk(c) walk(course) - - print "course %s counts=%s" % (course.display_name,counts) - stats = dict(counts) # number of each kind of module - return stats - From 4341ea7689ee4570749312c7e80392737f5fca12 Mon Sep 17 00:00:00 2001 From: ichuang Date: Sun, 2 Sep 2012 22:40:50 -0400 Subject: [PATCH 06/23] fix bug in modulestore reload: etree parser must be specified explicitly --- common/lib/xmodule/xmodule/xml_module.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 25dc4e0c6e..e8ecfa785a 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -12,6 +12,9 @@ import sys log = logging.getLogger(__name__) +edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False, + remove_comments=True, remove_blank_text=True) + def name_to_pathname(name): """ Convert a location name for use in a path: replace ':' with '/'. @@ -146,7 +149,7 @@ class XmlDescriptor(XModuleDescriptor): Returns an lxml Element """ - return etree.parse(file_object).getroot() + return etree.parse(file_object, parser=edx_xml_parser).getroot() @classmethod def load_file(cls, filepath, fs, location): From 91dcef7fd0fac0f5afcf50418158e4812dee7667 Mon Sep 17 00:00:00 2001 From: ichuang Date: Sun, 2 Sep 2012 22:41:27 -0400 Subject: [PATCH 07/23] add gitreload to instructor dashboard --- lms/djangoapps/instructor/views.py | 16 ++++++++++++++-- .../courseware/instructor_dashboard.html | 3 ++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index bfbfab6e25..5a3611285c 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -1,10 +1,11 @@ # ======== Instructor views ============================================================================= import csv +import itertools import json import logging +import os import urllib -import itertools from functools import partial from collections import defaultdict @@ -85,7 +86,18 @@ def instructor_dashboard(request, course_id): # process actions from form POST action = request.POST.get('action', '') - if 'Reload' in action: + if 'GIT pull' in action: + data_dir = course.metadata['data_dir'] + log.debug('git pull %s' % (data_dir)) + gdir = settings.DATA_DIR / data_dir + if not os.path.exists(gdir): + msg += "====> ERROR in gitreload - no such directory %s" % gdir + else: + cmd = "cd %s; git reset --hard HEAD; git clean -f -d; git pull origin; chmod g+w course.xml" % gdir + msg += "git pull on %s:

" % data_dir + msg += "

%s

" % escape(os.popen(cmd).read()) + + if 'Reload course' in action: log.debug('reloading %s (%s)' % (course_id, course)) try: data_dir = course.metadata['data_dir'] diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html index 49b16cf122..29397e5c41 100644 --- a/lms/templates/courseware/instructor_dashboard.html +++ b/lms/templates/courseware/instructor_dashboard.html @@ -70,7 +70,8 @@ table.stat_table td { %if admin_access:

- + + %endif From 9b056b284d185a48ee2db61de455a960ab7e270e Mon Sep 17 00:00:00 2001 From: ichuang Date: Sun, 2 Sep 2012 23:17:45 -0400 Subject: [PATCH 08/23] add tracking logging of instructor dashboard actions --- lms/djangoapps/instructor/views.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index 5a3611285c..5a596d3274 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -7,6 +7,8 @@ import logging import os import urllib +import track.views + from functools import partial from collections import defaultdict @@ -96,6 +98,7 @@ def instructor_dashboard(request, course_id): cmd = "cd %s; git reset --hard HEAD; git clean -f -d; git pull origin; chmod g+w course.xml" % gdir msg += "git pull on %s:

" % data_dir msg += "

%s

" % escape(os.popen(cmd).read()) + track.views.server_track(request, 'git pull %s' % data_dir, {}, page='idashboard') if 'Reload course' in action: log.debug('reloading %s (%s)' % (course_id, course)) @@ -103,6 +106,7 @@ def instructor_dashboard(request, course_id): data_dir = course.metadata['data_dir'] modulestore().try_load_course(data_dir) msg += "

Course reloaded from %s

" % data_dir + track.views.server_track(request, 'reload %s' % data_dir, {}, page='idashboard') except Exception as err: msg += '

Error: %s

' % escape(err) @@ -110,23 +114,28 @@ def instructor_dashboard(request, course_id): log.debug(action) datatable = get_student_grade_summary_data(request, course, course_id, get_grades=False) datatable['title'] = 'List of students enrolled in %s' % course_id + track.views.server_track(request, 'list-students', {}, page='idashboard') elif 'Dump Grades' in action: log.debug(action) datatable = get_student_grade_summary_data(request, course, course_id, get_grades=True) datatable['title'] = 'Summary Grades of students enrolled in %s' % course_id + track.views.server_track(request, 'dump-grades', {}, page='idashboard') elif 'Dump all RAW grades' in action: log.debug(action) datatable = get_student_grade_summary_data(request, course, course_id, get_grades=True, get_raw_scores=True) datatable['title'] = 'Raw Grades of students enrolled in %s' % course_id + track.views.server_track(request, 'dump-grades-raw', {}, page='idashboard') elif 'Download CSV of all student grades' in action: + track.views.server_track(request, 'dump-grades-csv', {}, page='idashboard') return return_csv('grades_%s.csv' % course_id, get_student_grade_summary_data(request, course, course_id)) elif 'Download CSV of all RAW grades' in action: + track.views.server_track(request, 'dump-grades-csv-raw', {}, page='idashboard') return return_csv('grades_%s_raw.csv' % course_id, get_student_grade_summary_data(request, course, course_id, get_raw_scores=True)) @@ -138,6 +147,7 @@ def instructor_dashboard(request, course_id): datatable = {'header': ['Username', 'Full name']} datatable['data'] = [[x.username, x.profile.name] for x in uset] datatable['title'] = 'List of Staff in course %s' % course_id + track.views.server_track(request, 'list-staff', {}, page='idashboard') elif action == 'Add course staff': uname = request.POST['staffuser'] @@ -151,6 +161,7 @@ def instructor_dashboard(request, course_id): msg += 'Added %s to staff group = %s' % (user, group.name) log.debug('staffgrp=%s' % group.name) user.groups.add(group) + track.views.server_track(request, 'add-staff %s' % user, {}, page='idashboard') elif action == 'Remove course staff': uname = request.POST['staffuser'] @@ -164,6 +175,7 @@ def instructor_dashboard(request, course_id): msg += 'Removed %s from staff group = %s' % (user, group.name) log.debug('staffgrp=%s' % group.name) user.groups.remove(group) + track.views.server_track(request, 'remove-staff %s' % user, {}, page='idashboard') # For now, mostly a static page context = {'course': course, From 4e2ddd578a1ee16c72740c7ae6a79e371c49adfb Mon Sep 17 00:00:00 2001 From: ichuang Date: Mon, 3 Sep 2012 12:32:41 -0400 Subject: [PATCH 09/23] unit tests for instructor dashboard (grades CSV download) --- lms/djangoapps/instructor/tests.py | 85 ++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 lms/djangoapps/instructor/tests.py diff --git a/lms/djangoapps/instructor/tests.py b/lms/djangoapps/instructor/tests.py new file mode 100644 index 0000000000..8b8449dbee --- /dev/null +++ b/lms/djangoapps/instructor/tests.py @@ -0,0 +1,85 @@ +""" +Unit tests for instructor dashboard + +Based on (and depends on) unit tests for courseware. + +Notes for running by hand: + +django-admin.py test --settings=lms.envs.test --pythonpath=. lms/djangoapps/instructor +""" + +import courseware.tests.tests as ct + +from nose import SkipTest +from mock import patch, Mock +from override_settings import override_settings + +# Need access to internal func to put users in the right group +from courseware.access import _course_staff_group_name +from django.contrib.auth.models import User, Group +from django.conf import settings +from django.core.urlresolvers import reverse + +import xmodule.modulestore.django + +from xmodule.modulestore.django import modulestore + + +@override_settings(MODULESTORE=ct.TEST_DATA_XML_MODULESTORE) +class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader): + ''' + Check for download of csv + ''' + + def setUp(self): + xmodule.modulestore.django._MODULESTORES = {} + courses = modulestore().get_courses() + + def find_course(name): + """Assumes the course is present""" + return [c for c in courses if c.location.course==name][0] + + self.full = find_course("full") + self.toy = find_course("toy") + + # Create two accounts + self.student = 'view@test.com' + self.instructor = 'view2@test.com' + self.password = 'foo' + self.create_account('u1', self.student, self.password) + self.create_account('u2', self.instructor, self.password) + self.activate_user(self.student) + self.activate_user(self.instructor) + + group_name = _course_staff_group_name(self.toy.location) + g = Group.objects.create(name=group_name) + g.user_set.add(ct.user(self.instructor)) + + self.logout() + self.login(self.instructor, self.password) + self.enroll(self.toy) + + + def test_download_grades_csv(self): + print "running test_download_grades_csv" + course = self.toy + url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) + msg = "url = %s\n" % url + resp = self.client.post(url, { + 'action': 'Download CSV of all student grades for this course', + }) + msg += "instructor dashboard download csv grades: resp = '%s'" % resp + + respstr = str(resp).replace('\r','') + #open('idtest.out','w').write(respstr) + + expected_resp = '''Vary: Cookie +Content-Type: text/csv +Content-Disposition: attachment; filename=grades_edX/toy/2012_Fall.csv +Cache-Control: no-cache, no-store, must-revalidate + +"ID","Username","Full Name","edX email","External email","HW 01","HW 02","HW 03","HW 04","HW 05","HW 06","HW 07","HW 08","HW 09","HW 10","HW 11","HW 12","HW Avg","Lab 01","Lab 02","Lab 03","Lab 04","Lab 05","Lab 06","Lab 07","Lab 08","Lab 09","Lab 10","Lab 11","Lab 12","Lab Avg","Midterm","Final" +"2","u2","Fred Weasley","view2@test.com","","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0.0","0.0" +''' + + self.assertEqual(respstr, expected_resp, msg) From b0397ab0b5b5539bce31df8e4513f84c65bef945 Mon Sep 17 00:00:00 2001 From: ichuang Date: Mon, 3 Sep 2012 12:43:48 -0400 Subject: [PATCH 10/23] fix to be compatible with jenkins test --- lms/djangoapps/instructor/tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/djangoapps/instructor/tests.py b/lms/djangoapps/instructor/tests.py index 8b8449dbee..4b7b86f3b7 100644 --- a/lms/djangoapps/instructor/tests.py +++ b/lms/djangoapps/instructor/tests.py @@ -71,6 +71,7 @@ class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader): msg += "instructor dashboard download csv grades: resp = '%s'" % resp respstr = str(resp).replace('\r','') + respstr = respstr.replace('TT_2012','2012') # jenkins course_id is TT_2012_Fall instead of 2012_Fall? #open('idtest.out','w').write(respstr) expected_resp = '''Vary: Cookie From 02d58a7ca24318392ebdbda0a62f21836fc048a6 Mon Sep 17 00:00:00 2001 From: ichuang Date: Mon, 3 Sep 2012 16:49:34 -0400 Subject: [PATCH 11/23] display course load error messages on modulestore reload by instructor --- lms/djangoapps/instructor/views.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index 5a596d3274..96acd66e8e 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -107,6 +107,11 @@ def instructor_dashboard(request, course_id): modulestore().try_load_course(data_dir) msg += "

Course reloaded from %s

" % data_dir track.views.server_track(request, 'reload %s' % data_dir, {}, page='idashboard') + course_errors = modulestore().get_item_errors(course.location) + msg += '
    ' + for cmsg, cerr in course_errors: + msg += "
  • %s: 
    %s
    " % (cmsg,escape(cerr)) + msg += '
' except Exception as err: msg += '

Error: %s

' % escape(err) From a973c1c21ebdc442b50abb27ceed2753aa574d5d Mon Sep 17 00:00:00 2001 From: ichuang Date: Mon, 3 Sep 2012 16:49:55 -0400 Subject: [PATCH 12/23] use module.descriptor.start for start date in xmodule_modifiers --- common/djangoapps/xmodule_modifiers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 6c3c237ea6..9ce6a0235d 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -121,9 +121,9 @@ def add_histogram(get_html, module, user): # useful to indicate to staff if problem has been released or not now = time.gmtime() is_released = "unknown" - if hasattr(module,'start'): - if module.start is not None: - is_released = "Yes!" if (now > module.start) else "Not yet" + mstart = getattr(module.descriptor,'start') + if mstart is not None: + is_released = "Yes!" if (now > mstart) else "Not yet" staff_context = {'definition': module.definition.get('data'), 'metadata': json.dumps(module.metadata, indent=4), From dbeca659bd2f607569dcb42e21b0d81094239171 Mon Sep 17 00:00:00 2001 From: ichuang Date: Mon, 3 Sep 2012 20:32:07 -0400 Subject: [PATCH 13/23] suppress gunicorn error which is being caught by errortracker. The gunicorn error is of this form: Traceback (most recent call last): File "/home/ike/mitx_all/python/local/lib/python2.7/site-packages/gunicorn/workers/sync.py", line 33, in run client, addr = self.socket.accept() File "/usr/lib/python2.7/socket.py", line 202, in accept sock, addr = self._sock.accept() error: [Errno 11] Resource temporarily unavailable --- common/lib/xmodule/xmodule/errortracker.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/lib/xmodule/xmodule/errortracker.py b/common/lib/xmodule/xmodule/errortracker.py index 8ac2903149..e9ea233e09 100644 --- a/common/lib/xmodule/xmodule/errortracker.py +++ b/common/lib/xmodule/xmodule/errortracker.py @@ -35,6 +35,11 @@ def make_error_tracker(): if in_exception_handler(): exc_str = exc_info_to_str(sys.exc_info()) + # don't display irrelevant gunicorn sync error + if (('mitx_all/python/local/lib/python2.7/site-packages/gunicorn/workers/sync.py' in exc_str) and + ('[Errno 11] Resource temporarily unavailable' in exc_str)): + exc_str = '' + errors.append((msg, exc_str)) return ErrorLog(error_tracker, errors) From 520d5620d050b57bf2caab38f3fe53fb952f6b21 Mon Sep 17 00:00:00 2001 From: ichuang Date: Mon, 3 Sep 2012 20:37:33 -0400 Subject: [PATCH 14/23] more general catch for gunicorn sync error --- common/lib/xmodule/xmodule/errortracker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/errortracker.py b/common/lib/xmodule/xmodule/errortracker.py index e9ea233e09..6accc8b8a7 100644 --- a/common/lib/xmodule/xmodule/errortracker.py +++ b/common/lib/xmodule/xmodule/errortracker.py @@ -36,7 +36,7 @@ def make_error_tracker(): exc_str = exc_info_to_str(sys.exc_info()) # don't display irrelevant gunicorn sync error - if (('mitx_all/python/local/lib/python2.7/site-packages/gunicorn/workers/sync.py' in exc_str) and + if (('python2.7/site-packages/gunicorn/workers/sync.py' in exc_str) and ('[Errno 11] Resource temporarily unavailable' in exc_str)): exc_str = '' From 258274f2a7d265e2af88079fb87d2346b8444128 Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 10:25:28 -0400 Subject: [PATCH 15/23] add string to model class unicode --- common/djangoapps/student/models.py | 2 +- lms/templates/courses.html | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 529a6f1298..c37b9fce16 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -187,7 +187,7 @@ class CourseEnrollment(models.Model): unique_together = (('user', 'course_id'), ) def __unicode__(self): - return "%s: %s (%s)" % (self.user, self.course_id, self.created) + return "[CourseEnrollment] %s: %s (%s)" % (self.user, self.course_id, self.created) @receiver(post_save, sender=CourseEnrollment) diff --git a/lms/templates/courses.html b/lms/templates/courses.html index 25930184a9..414292012c 100644 --- a/lms/templates/courses.html +++ b/lms/templates/courses.html @@ -19,6 +19,12 @@
## I'm removing this for now since we aren't using it for the fall. ## <%include file="course_filter.html" /> + +<%! +from django.core.urlresolvers import reverse +%> +

For 8.01 Reading Questions, click on Find Courses

+
%for course in universities['MITx']: From 4f8161df84b22839f1435d333136480331545256 Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 11:10:22 -0400 Subject: [PATCH 16/23] address two comments in https://github.com/MITx/mitx/pull/615 --- common/djangoapps/xmodule_modifiers.py | 1 + doc/development.md | 2 ++ 2 files changed, 3 insertions(+) diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 9ce6a0235d..066d83ed3e 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -119,6 +119,7 @@ def add_histogram(get_html, module, user): source_file = module.metadata.get('source_file','') # source used to generate the problem XML, eg latex or word # useful to indicate to staff if problem has been released or not + # TODO (ichuang): use _has_access_descriptor.can_load in lms.courseware.access, instead of now>mstart comparison here now = time.gmtime() is_released = "unknown" mstart = getattr(module.descriptor,'start') diff --git a/doc/development.md b/doc/development.md index fcf0b7a40f..7770ef1a11 100644 --- a/doc/development.md +++ b/doc/development.md @@ -94,3 +94,5 @@ course content can be setup to trigger an automatic reload when changes are push The mitx server will then do "git reset --hard HEAD; git clean -f -d; git pull origin" in that directory. After the pull, it will reload the modulestore for that course. + +Note that the gitreload-based workflow is not meant for deployments on AWS (or elsewhere) which use collectstatic, since collectstatic is not run by a gitreload event. From d6430570527d9d2ae9af85b32259d2ffab43e8a9 Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 11:21:18 -0400 Subject: [PATCH 17/23] count all types of modules, not just childless ones --- lms/djangoapps/instructor/views.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index 96acd66e8e..92b2401216 100644 --- a/lms/djangoapps/instructor/views.py +++ b/lms/djangoapps/instructor/views.py @@ -345,10 +345,8 @@ def compute_course_stats(course): def walk(module): children = module.get_children() - if not children: - category = module.__class__.__name__ # HtmlDescriptor, CapaDescriptor, ... - counts[category] += 1 - return + category = module.__class__.__name__ # HtmlDescriptor, CapaDescriptor, ... + counts[category] += 1 for c in children: walk(c) From 4cbdf90d5e2f77ea209616d7fb9ed51b603f07e2 Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 18:26:22 -0400 Subject: [PATCH 18/23] cleaned up access.py - now has _has_instructor_access_to_loation --- lms/djangoapps/courseware/access.py | 62 +++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 4f35aa98fa..2612efedb9 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -153,7 +153,7 @@ def _has_access_course_desc(user, course, action): 'enroll': can_enroll, 'see_exists': see_exists, 'staff': lambda: _has_staff_access_to_descriptor(user, course), - 'instructor': lambda: _has_staff_access_to_descriptor(user, course, require_instructor=True), + 'instructor': lambda: _has_instructor_access_to_descriptor(user, course), } return _dispatch(checkers, action, user, course) @@ -314,6 +314,7 @@ def _course_staff_group_name(location): """ return 'staff_%s' % Location(location).course + def _course_instructor_group_name(location): """ Get the name of the instructor group for a location. Right now, that's instructor_COURSE. @@ -323,6 +324,7 @@ def _course_instructor_group_name(location): """ return 'instructor_%s' % Location(location).course + def _has_global_staff_access(user): if user.is_staff: debug("Allow: user.is_staff") @@ -332,19 +334,28 @@ def _has_global_staff_access(user): return False -def _has_staff_access_to_location(user, location, require_instructor=False): +def _has_instructor_access_to_location(user, location): + return _has_access_to_location(user, location, 'instructor') + + +def _has_staff_access_to_location(user, location): + return _has_access_to_location(user, location, 'staff') + + +def _has_access_to_location(user, location, access_level): ''' - Returns True if the given user has staff access to a location. For now this - is equivalent to having staff access to the course location.course. + Returns True if the given user has access_level (= staff or + instructor) access to a location. For now this is equivalent to + having staff / instructor access to the course location.course. - If require_instructor=True, then user must be in instructor_* group. - - This means that user is in the staff_* group, or is an overall admin. + This means that user is in the staff_* group or instructor_* 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 a string: the course field of the location being accessed. + location = location + access_level = string, either "staff" or "instructor" ''' if user is None or (not user.is_authenticated()): debug("Deny: no user or anon user") @@ -355,29 +366,46 @@ def _has_staff_access_to_location(user, location, require_instructor=False): # If not global staff, is the user in the Auth group for this class? user_groups = [g.name for g in user.groups.all()] - staff_group = _course_staff_group_name(location) - if not require_instructor: + + if access_level == 'staff': + staff_group = _course_staff_group_name(location) if staff_group in user_groups: debug("Allow: user in group %s", staff_group) return True - instructor_group = _course_instructor_group_name(location) - if instructor_group in user_groups: - debug("Allow: user in group %s", instructor_group) - return True - debug("Deny: user not in group %s", staff_group) + debug("Deny: user not in group %s", staff_group) + + if access_level == 'instructor' or access_level == 'staff': # instructors get staff privileges + instructor_group = _course_instructor_group_name(location) + if instructor_group in user_groups: + debug("Allow: user in group %s", instructor_group) + return True + debug("Deny: user not in group %s", instructor_group) + + else: + log.debug("Error in access._has_access_to_location access_level=%s unknown" % access_level) + 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_location(user, loc) -def _has_staff_access_to_descriptor(user, descriptor, require_instructor=False): +def _has_instructor_access_to_descriptor(user, descriptor): """Helper method that checks whether the user has staff access to the course of the location. - location: something that can be passed to Location + descriptor: something that has a location attribute """ - return _has_staff_access_to_location(user, descriptor.location, require_instructor=require_instructor) + return _has_instructor_access_to_location(user, descriptor.location) + +def _has_staff_access_to_descriptor(user, descriptor): + """Helper method that checks whether the user has staff access to + the course of the location. + + descriptor: something that has a location attribute + """ + return _has_staff_access_to_location(user, descriptor.location) From 0c3d0542532540e3eb29482d4f85d759b7b762be Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 18:36:26 -0400 Subject: [PATCH 19/23] fix: university domain should not be determined by HTTP_HOST for dev or behind proxy --- common/djangoapps/student/views.py | 5 ++++- lms/envs/common.py | 3 +++ lms/envs/dev.py | 1 + lms/envs/dev_ike.py | 4 ++++ 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 27adf485f0..486aba12f6 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -75,8 +75,11 @@ def index(request, extra_context={}, user=None): entry.summary = soup.getText() # The course selection work is done in courseware.courses. + domain = settings.MITX_FEATURES.get('FORCE_UNIVERSITY_DOMAIN') # normally False + if not domain: + domain = request.META.get('HTTP_HOST') universities = get_courses_by_university(None, - domain=request.META.get('HTTP_HOST')) + domain=domain) context = {'universities': universities, 'entries': entries} context.update(extra_context) return render_to_response('index.html', context) diff --git a/lms/envs/common.py b/lms/envs/common.py index cf9a767d9f..7e868f0a34 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -64,6 +64,9 @@ MITX_FEATURES = { # university to use for branding purposes 'SUBDOMAIN_BRANDING': False, + 'FORCE_UNIVERSITY_DOMAIN': False, # set this to the university domain to use, as an override to HTTP_HOST + # set to None to do no university selection + 'ENABLE_TEXTBOOK' : True, 'ENABLE_DISCUSSION' : False, 'ENABLE_DISCUSSION_SERVICE': True, diff --git a/lms/envs/dev.py b/lms/envs/dev.py index 974b8c9fd6..0427938b70 100644 --- a/lms/envs/dev.py +++ b/lms/envs/dev.py @@ -17,6 +17,7 @@ MITX_FEATURES['DISABLE_START_DATES'] = True MITX_FEATURES['ENABLE_SQL_TRACKING_LOGS'] = True MITX_FEATURES['SUBDOMAIN_COURSE_LISTINGS'] = False # Enable to test subdomains--otherwise, want all courses to show up MITX_FEATURES['SUBDOMAIN_BRANDING'] = True +MITX_FEATURES['FORCE_UNIVERSITY_DOMAIN'] = None # show all university courses if in dev (ie don't use HTTP_HOST) WIKI_ENABLED = True diff --git a/lms/envs/dev_ike.py b/lms/envs/dev_ike.py index 297b179fae..0be9146fd4 100644 --- a/lms/envs/dev_ike.py +++ b/lms/envs/dev_ike.py @@ -18,6 +18,7 @@ MITX_FEATURES['ENABLE_DISCUSSION'] = False MITX_FEATURES['ACCESS_REQUIRE_STAFF_FOR_COURSE'] = True # require that user be in the staff_* group to be able to enroll MITX_FEATURES['SUBDOMAIN_COURSE_LISTINGS'] = False MITX_FEATURES['SUBDOMAIN_BRANDING'] = False +MITX_FEATURES['FORCE_UNIVERSITY_DOMAIN'] = None # show all university courses if in dev (ie don't use HTTP_HOST) MITX_FEATURES['DISABLE_START_DATES'] = True # MITX_FEATURES['USE_DJANGO_PIPELINE']=False # don't recompile scss @@ -28,6 +29,9 @@ if ('edxvm' in myhost) or ('ocw' in myhost): MITX_FEATURES['USE_XQA_SERVER'] = 'https://qisx.mit.edu/xqa' # needs to be ssl or browser blocks it MITX_FEATURES['USE_DJANGO_PIPELINE']=False # don't recompile scss +if ('ocw' in myhost): + MITX_FEATURES['ACCESS_REQUIRE_STAFF_FOR_COURSE'] = False + if ('domU' in myhost): EMAIL_BACKEND = 'django.core.mail.backends.smtp.EmailBackend' MITX_FEATURES['REROUTE_ACTIVATION_EMAIL'] = 'ichuang@mitx.mit.edu' # nonempty string = address for all activation emails From 98eb7645259de80f4247ea4b6a46b428d906742b Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 18:56:49 -0400 Subject: [PATCH 20/23] cleaned up instructor dashboard CSV unit test code, per https://github.com/MITx/mitx/pull/615 --- lms/djangoapps/instructor/tests.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/instructor/tests.py b/lms/djangoapps/instructor/tests.py index 4b7b86f3b7..e948771d6d 100644 --- a/lms/djangoapps/instructor/tests.py +++ b/lms/djangoapps/instructor/tests.py @@ -65,22 +65,20 @@ class TestInstructorDashboardGradeDownloadCSV(ct.PageLoader): course = self.toy url = reverse('instructor_dashboard', kwargs={'course_id': course.id}) msg = "url = %s\n" % url - resp = self.client.post(url, { - 'action': 'Download CSV of all student grades for this course', - }) - msg += "instructor dashboard download csv grades: resp = '%s'" % resp + response = self.client.post(url, {'action': 'Download CSV of all student grades for this course', + }) + msg += "instructor dashboard download csv grades: response = '%s'\n" % response - respstr = str(resp).replace('\r','') - respstr = respstr.replace('TT_2012','2012') # jenkins course_id is TT_2012_Fall instead of 2012_Fall? - #open('idtest.out','w').write(respstr) + self.assertEqual(response['Content-Type'],'text/csv',msg) - expected_resp = '''Vary: Cookie -Content-Type: text/csv -Content-Disposition: attachment; filename=grades_edX/toy/2012_Fall.csv -Cache-Control: no-cache, no-store, must-revalidate + cdisp = response['Content-Disposition'].replace('TT_2012','2012') # jenkins course_id is TT_2012_Fall instead of 2012_Fall? + msg += "cdisp = '%s'\n" % cdisp + self.assertEqual(cdisp,'attachment; filename=grades_edX/toy/2012_Fall.csv',msg) -"ID","Username","Full Name","edX email","External email","HW 01","HW 02","HW 03","HW 04","HW 05","HW 06","HW 07","HW 08","HW 09","HW 10","HW 11","HW 12","HW Avg","Lab 01","Lab 02","Lab 03","Lab 04","Lab 05","Lab 06","Lab 07","Lab 08","Lab 09","Lab 10","Lab 11","Lab 12","Lab Avg","Midterm","Final" + body = response.content.replace('\r','') + msg += "body = '%s'\n" % body + + expected_body = '''"ID","Username","Full Name","edX email","External email","HW 01","HW 02","HW 03","HW 04","HW 05","HW 06","HW 07","HW 08","HW 09","HW 10","HW 11","HW 12","HW Avg","Lab 01","Lab 02","Lab 03","Lab 04","Lab 05","Lab 06","Lab 07","Lab 08","Lab 09","Lab 10","Lab 11","Lab 12","Lab Avg","Midterm","Final" "2","u2","Fred Weasley","view2@test.com","","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0","0.0","0.0" ''' - - self.assertEqual(respstr, expected_resp, msg) + self.assertEqual(body, expected_body, msg) From 5c90ec19d9552c132979c3b04b6e00fe49fe3270 Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 19:08:28 -0400 Subject: [PATCH 21/23] clarify that /gitreload is behind ENABLE_LMS_MIGRATION in docs --- doc/development.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/development.md b/doc/development.md index 7770ef1a11..b4ac52d202 100644 --- a/doc/development.md +++ b/doc/development.md @@ -96,3 +96,6 @@ course content can be setup to trigger an automatic reload when changes are push it will reload the modulestore for that course. Note that the gitreload-based workflow is not meant for deployments on AWS (or elsewhere) which use collectstatic, since collectstatic is not run by a gitreload event. + +Also, the gitreload feature needs MITX_FEATURES['ENABLE_LMS_MIGRATION'] = True in the django settings. + From 566b9694c4f3b5de25906314c79bd0c3f9f56e0c Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 19:47:37 -0400 Subject: [PATCH 22/23] revert inadvertent change to course.html --- lms/templates/courses.html | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lms/templates/courses.html b/lms/templates/courses.html index 414292012c..25930184a9 100644 --- a/lms/templates/courses.html +++ b/lms/templates/courses.html @@ -19,12 +19,6 @@
## I'm removing this for now since we aren't using it for the fall. ## <%include file="course_filter.html" /> - -<%! -from django.core.urlresolvers import reverse -%> -

For 8.01 Reading Questions, click on Find Courses

-
%for course in universities['MITx']: From d56d235fc9284a208b986b509ae640ae8497aacc Mon Sep 17 00:00:00 2001 From: ichuang Date: Tue, 4 Sep 2012 23:02:51 -0400 Subject: [PATCH 23/23] minor change to create_user command, don't fail on kerberos lookup failure --- .../lms_migration/management/commands/create_user.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/lms_migration/management/commands/create_user.py b/lms/djangoapps/lms_migration/management/commands/create_user.py index 333608d467..7d39accc44 100644 --- a/lms/djangoapps/lms_migration/management/commands/create_user.py +++ b/lms/djangoapps/lms_migration/management/commands/create_user.py @@ -67,7 +67,10 @@ class Command(BaseCommand): password = GenPasswd(12) # get name from kerberos - kname = os.popen("finger %s | grep 'name:'" % email).read().strip().split('name: ')[1].strip() + try: + kname = os.popen("finger %s | grep 'name:'" % email).read().strip().split('name: ')[1].strip() + except: + kname = '' name = raw_input('Full name: [%s] ' % kname).strip() if name=='': name = kname