diff --git a/lms/djangoapps/analytics/basic.py b/lms/djangoapps/analytics/basic.py index 498cfd22c7..b1c96122dc 100644 --- a/lms/djangoapps/analytics/basic.py +++ b/lms/djangoapps/analytics/basic.py @@ -36,9 +36,10 @@ def enrolled_students_features(course_id, features): student_dict = dict((feature, getattr(student, feature)) for feature in student_features) profile = student.profile - profile_dict = dict((feature, getattr(profile, feature)) - for feature in profile_features) - student_dict.update(profile_dict) + if not profile is None: + profile_dict = dict((feature, getattr(profile, feature)) + for feature in profile_features) + student_dict.update(profile_dict) return student_dict return [extract_student(student, features) for student in students] diff --git a/lms/djangoapps/analytics/distributions.py b/lms/djangoapps/analytics/distributions.py index 3acd569709..e44b44308b 100644 --- a/lms/djangoapps/analytics/distributions.py +++ b/lms/djangoapps/analytics/distributions.py @@ -1,5 +1,24 @@ """ Profile Distributions + +Aggregate sums for values of fields in students profiles. + +For example: +The distribution in a course for gender might look like: +'gender': { + 'type': 'EASY_CHOICE', + 'data': { + 'no_data': 1234, + 'm': 5678, + 'o': 2134, + 'f': 5678 + }, + 'display_names': { + 'no_data': 'No Data', + 'm': 'Male', + 'o': 'Other', + 'f': 'Female' +} """ from django.db.models import Count @@ -8,6 +27,48 @@ from student.models import CourseEnrollment, UserProfile _EASY_CHOICE_FEATURES = ('gender', 'level_of_education') _OPEN_CHOICE_FEATURES = ('year_of_birth',) AVAILABLE_PROFILE_FEATURES = _EASY_CHOICE_FEATURES + _OPEN_CHOICE_FEATURES +DISPLAY_NAMES = { + 'gender': 'Gender', + 'level_of_education': 'Level of Education', + 'year_of_birth': 'Year Of Birth', +} + + +class ProfileDistribution(object): + """ + Container for profile distribution data + + `feature` is the name of the distribution feature + `feature_display_name` is the display name of feature + `data` is a dictionary of the distribution + `type` is either 'EASY_CHOICE' or 'OPEN_CHOICE' + `choices_display_names` is a dict if the distribution is an 'EASY_CHOICE' + """ + + class ValidationError(ValueError): + """ Error thrown if validation fails. """ + pass + + def __init__(self, feature): + self.feature = feature + self.feature_display_name = DISPLAY_NAMES[feature] + + def validate(self): + """ + Validate this profile distribution. + + Throws ProfileDistribution.ValidationError + """ + def validation_assert(predicate): + if not predicate: + raise ProfileDistribution.ValidationError() + + validation_assert(isinstance(self.feature, str)) + validation_assert(isinstance(self.feature_display_name, str)) + validation_assert(self.type in ['EASY_CHOICE', 'OPEN_CHOICE']) + validation_assert(isinstance(self.data, dict)) + if self.type == 'EASY_CHOICE': + validation_assert(isinstance(self.choices_display_names, dict)) def profile_distribution(course_id, feature): @@ -15,37 +76,35 @@ def profile_distribution(course_id, feature): Retrieve distribution of students over a given feature. feature is one of AVAILABLE_PROFILE_FEATURES. - Return a dictionary { - 'type': 'SOME_TYPE', - 'data': {'key': 'val'}, - 'display_names': {'key': 'displaynameval'} - } + Returns a ProfileDistribution instance. - display_names is only return for EASY_CHOICE type eatuers - note no_data instead of None to be compatible with the json spec. - data types e.g. + NOTE: no_data will appear as a key instead of None to be compatible with the json spec. + data types are EASY_CHOICE - choices with a restricted domain, e.g. level_of_education OPEN_CHOICE - choices with a larger domain e.g. year_of_birth """ - feature_results = {} - if not feature in AVAILABLE_PROFILE_FEATURES: raise ValueError( "unsupported feature requested for distribution '{}'".format( feature) ) + prd = ProfileDistribution(feature) + if feature in _EASY_CHOICE_FEATURES: + prd.type = 'EASY_CHOICE' + if feature == 'gender': raw_choices = UserProfile.GENDER_CHOICES elif feature == 'level_of_education': raw_choices = UserProfile.LEVEL_OF_EDUCATION_CHOICES + # short name and display nae (full) of the choices. choices = [(short, full) for (short, full) in raw_choices] + [('no_data', 'No Data')] - data = {} + distribution = {} for (short, full) in choices: if feature == 'gender': count = CourseEnrollment.objects.filter( @@ -55,12 +114,12 @@ def profile_distribution(course_id, feature): count = CourseEnrollment.objects.filter( course_id=course_id, user__profile__level_of_education=short ).count() - data[short] = count + distribution[short] = count - feature_results['data'] = data - feature_results['type'] = 'EASY_CHOICE' - feature_results['display_names'] = dict(choices) + prd.data = distribution + prd.choices_display_names = dict(choices) elif feature in _OPEN_CHOICE_FEATURES: + prd.type = 'OPEN_CHOICE' profiles = UserProfile.objects.filter( user__courseenrollment__course_id=course_id) query_distribution = profiles.values( @@ -81,7 +140,7 @@ def profile_distribution(course_id, feature): **{feature: None} ).count() - feature_results['data'] = distribution - feature_results['type'] = 'OPEN_CHOICE' + prd.data = distribution - return feature_results + prd.validate() + return prd diff --git a/lms/djangoapps/analytics/tests/test_distributions.py b/lms/djangoapps/analytics/tests/test_distributions.py index 0c565cf756..61f19ff72c 100644 --- a/lms/djangoapps/analytics/tests/test_distributions.py +++ b/lms/djangoapps/analytics/tests/test_distributions.py @@ -31,20 +31,20 @@ class TestAnalyticsDistributions(TestCase): feature = 'gender' self.assertIn(feature, AVAILABLE_PROFILE_FEATURES) distribution = profile_distribution(self.course_id, feature) - self.assertEqual(distribution['type'], 'EASY_CHOICE') - self.assertEqual(distribution['data']['no_data'], 0) - self.assertEqual(distribution['data']['m'], len(self.users) / 3) - self.assertEqual(distribution['display_names']['m'], 'Male') + self.assertEqual(distribution.type, 'EASY_CHOICE') + self.assertEqual(distribution.data['no_data'], 0) + self.assertEqual(distribution.data['m'], len(self.users) / 3) + self.assertEqual(distribution.choices_display_names['m'], 'Male') def test_profile_distribution_open_choice(self): feature = 'year_of_birth' self.assertIn(feature, AVAILABLE_PROFILE_FEATURES) distribution = profile_distribution(self.course_id, feature) print distribution - self.assertEqual(distribution['type'], 'OPEN_CHOICE') - self.assertNotIn('display_names', distribution) - self.assertNotIn('no_data', distribution['data']) - self.assertEqual(distribution['data'][1930], 1) + self.assertEqual(distribution.type, 'OPEN_CHOICE') + self.assertFalse(hasattr(distribution, 'choices_display_names')) + self.assertNotIn('no_data', distribution.data) + self.assertEqual(distribution.data[1930], 1) class TestAnalyticsDistributionsNoData(TestCase): @@ -70,7 +70,7 @@ class TestAnalyticsDistributionsNoData(TestCase): self.assertIn(feature, AVAILABLE_PROFILE_FEATURES) distribution = profile_distribution(self.course_id, feature) print distribution - self.assertEqual(distribution['type'], 'OPEN_CHOICE') - self.assertNotIn('display_names', distribution) - self.assertIn('no_data', distribution['data']) - self.assertEqual(distribution['data']['no_data'], len(self.nodata_users)) + self.assertEqual(distribution.type, 'OPEN_CHOICE') + self.assertFalse(hasattr(distribution, 'choices_display_names')) + self.assertIn('no_data', distribution.data) + self.assertEqual(distribution.data['no_data'], len(self.nodata_users)) diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index b6f21a0fb8..7b42d58556 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -58,6 +58,8 @@ def _change_access(course, user, level, mode): level is one of ['instructor', 'staff', 'beta'] mode is one of ['allow', 'revoke'] + + NOTE: will NOT create a group that does not yet exist. """ if level is 'beta': diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 76bcc65a44..f4bfdbde7f 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1,10 +1,194 @@ """ -Unit tests for instructor.enrollment methods. +Unit tests for instructor.api methods. """ +import json +from urllib import quote from django.test import TestCase +from nose.tools import raises +from django.test.utils import override_settings +from django.core.urlresolvers import reverse -from instructor.views.api import _split_input_list +from courseware.tests.modulestore_config import TEST_DATA_MONGO_MODULESTORE +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from courseware.tests.helpers import LoginEnrollmentTestCase +from xmodule.modulestore.tests.factories import CourseFactory +from student.tests.factories import UserFactory, AdminFactory + +from student.models import CourseEnrollment + +from instructor.views.api import _split_input_list, _msk_from_problem_urlname + + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class TestInstructorAPIDenyLevels(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Ensure that users cannot access endpoints they shouldn't be able to. + """ + def setUp(self): + self.user = UserFactory.create() + self.course = CourseFactory.create() + CourseEnrollment.objects.create(user=self.user, course_id=self.course.id) + self.client.login(username=self.user.username, password='test') + + def test_deny_students_update_enrollment(self): + url = reverse('students_update_enrollment', kwargs={'course_id': self.course.id}) + response = self.client.get(url, {}) + self.assertEqual(response.status_code, 403) + + def test_staff_level(self): + """ + Ensure that an enrolled student can't access staff or instructor endpoints. + """ + staff_level_endpoints = [ + 'students_update_enrollment', + 'modify_access', + 'list_course_role_members', + 'get_grading_config', + 'get_students_features', + 'get_distribution', + 'get_student_progress_url', + 'reset_student_attempts', + 'rescore_problem', + 'list_instructor_tasks', + 'list_forum_members', + 'update_forum_role_membership', + ] + for endpoint in staff_level_endpoints: + url = reverse(endpoint, kwargs={'course_id': self.course.id}) + response = self.client.get(url, {}) + self.assertEqual(response.status_code, 403) + + def test_instructor_level(self): + """ + Ensure that a staff member can't access instructor endpoints. + """ + instructor_level_endpoints = [ + 'modify_access', + 'list_course_role_members', + 'reset_student_attempts', + 'list_instructor_tasks', + 'update_forum_role_membership', + ] + for endpoint in instructor_level_endpoints: + url = reverse(endpoint, kwargs={'course_id': self.course.id}) + response = self.client.get(url, {}) + self.assertEqual(response.status_code, 403) + +# class TestInstructorAPILevelsEnrollment +# # students_update_enrollment + +# class TestInstructorAPILevelsAccess +# # modify_access +# # list_course_role_members + + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCase): + """ + Test endpoints that show data without side effects. + """ + def setUp(self): + self.instructor = AdminFactory.create() + self.course = CourseFactory.create() + self.client.login(username=self.instructor.username, password='test') + + # self.students = [UserFactory(email="foobar{}@robot.org".format(i)) for i in xrange(6)] + self.students = [UserFactory() for _ in xrange(6)] + for student in self.students: + CourseEnrollment.objects.create(user=student, course_id=self.course.id) + + def test_get_students_features(self): + """ + Test that some minimum of information is formatted + correctly in the response to get_students_features. + """ + url = reverse('get_students_features', kwargs={'course_id': self.course.id}) + response = self.client.get(url, {}) + res_json = json.loads(response.content) + self.assertIn('students', res_json) + for student in self.students: + student_json = [ + x for x in res_json['students'] + if x['username'] == student.username + ][0] + self.assertEqual(student_json['username'], student.username) + self.assertEqual(student_json['email'], student.email) + + def test_get_distribution_no_feature(self): + """ + Test that get_distribution lists available features + when supplied no feature quparameter. + """ + url = reverse('get_distribution', kwargs={'course_id': self.course.id}) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content) + self.assertEqual(type(res_json['available_features']), list) + + url = reverse('get_distribution', kwargs={'course_id': self.course.id}) + response = self.client.get(url + u'?feature=') + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content) + self.assertEqual(type(res_json['available_features']), list) + + def test_get_distribution_unavailable_feature(self): + """ + Test that get_distribution fails gracefully with + an unavailable feature. + """ + url = reverse('get_distribution', kwargs={'course_id': self.course.id}) + response = self.client.get(url, {'feature': 'robot-not-a-real-feature'}) + self.assertEqual(response.status_code, 400) + + def test_get_distribution_gender(self): + """ + Test that get_distribution fails gracefully with + an unavailable feature. + """ + url = reverse('get_distribution', kwargs={'course_id': self.course.id}) + response = self.client.get(url, {'feature': 'gender'}) + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content) + print res_json + self.assertEqual(res_json['feature_results']['data']['m'], 6) + self.assertEqual(res_json['feature_results']['choices_display_names']['m'], 'Male') + self.assertEqual(res_json['feature_results']['data']['no_data'], 0) + self.assertEqual(res_json['feature_results']['choices_display_names']['no_data'], 'No Data') + + def test_get_student_progress_url(self): + """ Test that progress_url is in the successful response. """ + url = reverse('get_student_progress_url', kwargs={'course_id': self.course.id}) + url += "?student_email={}".format( + quote(self.students[0].email.encode("utf-8")) + ) + print url + response = self.client.get(url) + print response + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content) + self.assertIn('progress_url', res_json) + + def test_get_student_progress_url_noparams(self): + """ Test that the endpoint 404's without the required query params. """ + url = reverse('get_student_progress_url', kwargs={'course_id': self.course.id}) + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + + def test_get_student_progress_url_nostudent(self): + """ Test that the endpoint 400's when requesting an unknown email. """ + url = reverse('get_student_progress_url', kwargs={'course_id': self.course.id}) + response = self.client.get(url) + self.assertEqual(response.status_code, 400) + +# class TestInstructorAPILevelsGrade modification & tasks +# # reset_student_attempts +# # rescore_problem +# # list_instructor_tasks + +# class TestInstructorAPILevelsForums +# # list_forum_members +# # update_forum_role_membership class TestInstructorAPIHelpers(TestCase): @@ -24,3 +208,13 @@ class TestInstructorAPIHelpers(TestCase): self.assertEqual(_split_input_list(u'robot@robot.edu, robot2@robot.edu'), [u'robot@robot.edu', 'robot2@robot.edu']) scary_unistuff = unichr(40960) + u'abcd' + unichr(1972) self.assertEqual(_split_input_list(scary_unistuff), [scary_unistuff]) + + def test_msk_from_problem_urlname(self): + args = ('MITx/6.002x/2013_Spring', 'L2Node1') + output = 'i4x://MITx/6.002x/problem/L2Node1' + self.assertEqual(_msk_from_problem_urlname(*args), output) + + @raises(ValueError) + def test_msk_from_problem_urlname_error(self): + args = ('notagoodcourse', 'L2Node1') + _msk_from_problem_urlname(*args) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 8741fe6e9e..0d02121e11 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1,20 +1,21 @@ """ Instructor Dashboard API views -Non-html views which the instructor dashboard requests. +JSON views which the instructor dashboard requests. TODO a lot of these GETs should be PUTs """ import re import json +import logging from django_future.csrf import ensure_csrf_cookie from django.views.decorators.cache import cache_control from django.core.urlresolvers import reverse -from django.http import HttpResponse, HttpResponseBadRequest +from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbidden from courseware.access import has_access -from courseware.courses import get_course_with_access +from courseware.courses import get_course_with_access, get_course_by_id from django.contrib.auth.models import User from django_comment_common.models import (Role, FORUM_ROLE_ADMINISTRATOR, @@ -30,6 +31,7 @@ import analytics.basic import analytics.distributions import analytics.csvs +log = logging.getLogger(__name__) def common_exceptions_400(func): """ @@ -85,8 +87,38 @@ def require_query_params(*args, **kwargs): return decorator +def require_level(level): + """ + Decorator with argument that requires an access level of the requesting + user. If the requirement is not satisfied, returns an + HttpResponseForbidden (403). + + Assumes that request is in args[0]. + Assumes that course_id is in kwargs['course_id']. + + `level` is in ['instructor', 'staff'] + if `level` is 'staff', instructors will also be allowed, even + if they are not int he staff group. + """ + if level not in ['instructor', 'staff']: + raise ValueError("unrecognized level '{}'".format(level)) + + def decorator(func): + def wrapped(*args, **kwargs): + request = args[0] + course = get_course_by_id(kwargs['course_id']) + + if has_access(request.user, course, level): + return func(*args, **kwargs) + else: + return HttpResponseForbidden() + return wrapped + return decorator + + @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('staff') @require_query_params(action="enroll or unenroll", emails="stringified list of emails") def students_update_enrollment(request, course_id): """ @@ -97,45 +129,60 @@ def students_update_enrollment(request, course_id): - action in ['enroll', 'unenroll'] - emails is string containing a list of emails separated by anything split_input_list can handle. - auto_enroll is a boolean (defaults to false) - """ - course = get_course_with_access( - request.user, course_id, 'staff', depth=None - ) + If auto_enroll is false, students will be allowed to enroll. + If auto_enroll is true, students will be enroled as soon as they register. + Returns an analog to this JSON structure: { + "action": "enroll", + "auto_enroll": false + "results": [ + { + "email": "testemail@test.org", + "before": { + "enrollment": false, + "auto_enroll": false, + "user": true, + "allowed": false + }, + "after": { + "enrollment": true, + "auto_enroll": false, + "user": true, + "allowed": false + } + } + ] + } + """ action = request.GET.get('action') emails_raw = request.GET.get('emails') - print "@@@@" - print type(emails_raw) emails = _split_input_list(emails_raw) auto_enroll = request.GET.get('auto_enroll') in ['true', 'True', True] - def format_result(func, email): - """ Act on a single email and format response or errors. """ + results = [] + for email in emails: try: - before, after = func() - return { + if action == 'enroll': + before, after = enroll_email(course_id, email, auto_enroll) + elif action == 'unenroll': + before, after = unenroll_email(course_id, email) + else: + return HttpResponseBadRequest("Unrecognized action '{}'".format(action)) + + results.append({ 'email': email, 'before': before.to_dict(), 'after': after.to_dict(), - } - except Exception: - return { + }) + # catch and log any exceptions + # so that one error doesn't cause a 500. + except Exception as exc: + log.exception("Error while #{}ing student") + log.exception(exc) + results.append({ 'email': email, 'error': True, - } - - if action == 'enroll': - results = [format_result( - lambda: enroll_email(course_id, email, auto_enroll), - email - ) for email in emails] - elif action == 'unenroll': - results = [format_result( - lambda: unenroll_email(course_id, email), - email - ) for email in emails] - else: - return HttpResponseBadRequest("Unrecognized action '{}'".format(action)) + }) response_payload = { 'action': action, @@ -150,13 +197,14 @@ def students_update_enrollment(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('instructor') @common_exceptions_400 @require_query_params( email="user email", rolename="'instructor', 'staff', or 'beta'", mode="'allow' or 'revoke'" ) -def access_allow_revoke(request, course_id): +def modify_access(request, course_id): """ Modify staff/instructor access. Requires instructor access. @@ -174,6 +222,11 @@ def access_allow_revoke(request, course_id): rolename = request.GET.get('rolename') mode = request.GET.get('mode') + if not rolename in ['instructor', 'staff', 'beta']: + return HttpResponseBadRequest( + "unknown rolename '{}'".format(rolename) + ) + user = User.objects.get(email=email) if mode == 'allow': @@ -184,7 +237,10 @@ def access_allow_revoke(request, course_id): raise ValueError("unrecognized mode '{}'".format(mode)) response_payload = { - 'DONE': 'YES', + 'email': email, + 'rolename': rolename, + 'mode': mode, + 'success': 'yes', } response = HttpResponse( json.dumps(response_payload), content_type="application/json" @@ -194,6 +250,7 @@ def access_allow_revoke(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('instructor') @require_query_params(rolename="'instructor', 'staff', or 'beta'") def list_course_role_members(request, course_id): """ @@ -212,6 +269,7 @@ def list_course_role_members(request, course_id): return HttpResponseBadRequest() def extract_user_info(user): + """ convert user into dicts for json view """ return { 'username': user.username, 'email': user.email, @@ -233,11 +291,10 @@ def list_course_role_members(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) -def grading_config(request, course_id): +@require_level('staff') +def get_grading_config(request, course_id): """ Respond with json which contains a html formatted grade summary. - - TODO this shouldn't be html already """ course = get_course_with_access( request.user, course_id, 'staff', depth=None @@ -256,18 +313,16 @@ def grading_config(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) -def enrolled_students_features(request, course_id, csv=False): +@require_level('staff') +def get_students_features(request, course_id, csv=False): """ Respond with json which contains a summary of all enrolled students profile information. - Response {"students": [{-student-info-}, ...]} + Responds with JSON + {"students": [{-student-info-}, ...]} - TODO accept requests for different attribute sets + TODO accept requests for different attribute sets. """ - course = get_course_with_access( - request.user, course_id, 'staff', depth=None - ) - available_features = analytics.basic.AVAILABLE_FEATURES query_features = ['username', 'name', 'email', 'language', 'location', 'year_of_birth', 'gender', 'level_of_education', 'mailing_address', 'goals'] @@ -293,53 +348,52 @@ def enrolled_students_features(request, course_id, csv=False): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) -def profile_distribution(request, course_id): +@require_level('staff') +def get_distribution(request, course_id): """ - Respond with json of the distribution of students over selected fields which have choices. + Respond with json of the distribution of students over selected features which have choices. - Ask for features through the 'features' query parameter. - The features query parameter can be either a single feature name, or a json string of feature names. - e.g. - http://localhost:8000/courses/MITx/6.002x/2013_Spring/instructor_dashboard/api/profile_distribution?features=level_of_education - http://localhost:8000/courses/MITx/6.002x/2013_Spring/instructor_dashboard/api/profile_distribution?features=%5B%22year_of_birth%22%2C%22gender%22%5D + Ask for a feature through the `feature` query parameter. + If no `feature` is supplied, will return response with an + empty response['feature_results'] object. + A list of available will be available in the response['available_features'] - Example js query: - $.get("http://localhost:8000/courses/MITx/6.002x/2013_Spring/instructor_dashboard/api/profile_distribution", - {'features': JSON.stringify(['year_of_birth', 'gender'])}, - function(){console.log(arguments[0])}) - - TODO how should query parameter interpretation work? TODO respond to csv requests as well """ - course = get_course_with_access( - request.user, course_id, 'staff', depth=None - ) + feature = request.GET.get('feature') + # alternate notations of None + if feature in (None, 'null', ''): + feature = None + else: + feature = str(feature) - try: - features = json.loads(request.GET.get('features')) - except Exception: - features = [request.GET.get('features')] - - feature_results = {} - - for feature in features: - try: - feature_results[feature] = analytics.distributions.profile_distribution(course_id, feature) - except Exception as e: - feature_results[feature] = {'error': "Error finding distribution for distribution for '{}'.".format(feature)} - raise e + AVAILABLE_FEATURES = analytics.distributions.AVAILABLE_PROFILE_FEATURES + # allow None so that requests for no feature can list available features + if not feature in AVAILABLE_FEATURES + (None,): + return HttpResponseBadRequest( + "feature '{}' not available.".format(feature) + ) response_payload = { 'course_id': course_id, - 'queried_features': features, - 'available_features': analytics.distributions.AVAILABLE_PROFILE_FEATURES, - 'display_names': { - 'gender': 'Gender', - 'level_of_education': 'Level of Education', - 'year_of_birth': 'Year Of Birth', - }, - 'feature_results': feature_results, + 'queried_feature': feature, + 'available_features': AVAILABLE_FEATURES, + 'feature_display_names': analytics.distributions.DISPLAY_NAMES, } + + p_dist = None + if not feature is None: + p_dist = analytics.distributions.profile_distribution(course_id, feature) + response_payload['feature_results'] = { + 'feature': p_dist.feature, + 'feature_display_name': p_dist.feature_display_name, + 'data': p_dist.data, + 'type': p_dist.type, + } + + if p_dist.type == 'EASY_CHOICE': + response_payload['feature_results']['choices_display_names'] = p_dist.choices_display_names + response = HttpResponse( json.dumps(response_payload), content_type="application/json" ) @@ -348,6 +402,8 @@ def profile_distribution(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@common_exceptions_400 +@require_level('staff') @require_query_params( student_email="email of student for whom to get progress url" ) @@ -361,10 +417,6 @@ def get_student_progress_url(request, course_id): 'progress_url': '/../...' } """ - course = get_course_with_access( - request.user, course_id, 'staff', depth=None - ) - student_email = request.GET.get('student_email') user = User.objects.get(email=student_email) @@ -382,6 +434,10 @@ def get_student_progress_url(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('staff') +@require_query_params( + student_email="email of student for whom to reset attempts" +) @common_exceptions_400 def reset_student_attempts(request, course_id): """ @@ -438,6 +494,8 @@ def reset_student_attempts(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('instructor') +@require_query_params(problem_to_reset="problem urlname to reset") @common_exceptions_400 def rescore_problem(request, course_id): """ @@ -451,10 +509,6 @@ def rescore_problem(request, course_id): all_students will be ignored if student_email is present """ - course = get_course_with_access( - request.user, course_id, 'instructor', depth=None - ) - problem_to_reset = request.GET.get('problem_to_reset') student_email = request.GET.get('student_email', False) all_students = request.GET.get('all_students') in ['true', 'True', True] @@ -486,6 +540,7 @@ def rescore_problem(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('instructor') def list_instructor_tasks(request, course_id): """ List instructor tasks. @@ -495,10 +550,6 @@ def list_instructor_tasks(request, course_id): - (optional) problem_urlname (same format as problem_to_reset in other api methods) - (optional) student_email """ - course = get_course_with_access( - request.user, course_id, 'instructor', depth=None - ) - problem_urlname = request.GET.get('problem_urlname', False) student_email = request.GET.get('student_email', False) @@ -516,6 +567,7 @@ def list_instructor_tasks(request, course_id): tasks = instructor_task.api.get_running_instructor_tasks(course_id) def extract_task_features(task): + """ Convert task to dict for json rendering """ FEATURES = ['task_type', 'task_input', 'task_id', 'requester', 'created', 'task_state'] return dict((feature, str(getattr(task, feature))) for feature in FEATURES) @@ -530,17 +582,15 @@ def list_instructor_tasks(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('staff') +@require_query_params('rolename') def list_forum_members(request, course_id): """ - Resets a students attempts counter. Optionally deletes student state for a problem. + Lists forum members of a certain rolename. Limited to staff access. - Takes query parameter rolename + Takes query parameter `rolename` """ - course = get_course_with_access( - request.user, course_id, 'staff', depth=None - ) - rolename = request.GET.get('rolename') if not rolename in [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA]: @@ -553,6 +603,7 @@ def list_forum_members(request, course_id): users = [] def extract_user_info(user): + """ Convert user to dict for json rendering. """ return { 'username': user.username, 'email': user.email, @@ -572,20 +623,22 @@ def list_forum_members(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('instructor') +@require_query_params( + email="the target users email", + rolename="the forum role", + mode="'allow' or 'revoke'", +) @common_exceptions_400 def update_forum_role_membership(request, course_id): """ - Modify forum role access. + Modify user's forum role. Query parameters: email is the target users email rolename is one of [FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA] mode is one of ['allow', 'revoke'] """ - course = get_course_with_access( - request.user, course_id, 'instructor', depth=None - ) - email = request.GET.get('email') rolename = request.GET.get('rolename') mode = request.GET.get('mode') @@ -602,7 +655,6 @@ def update_forum_role_membership(request, course_id): response_payload = { 'course_id': course_id, 'mode': mode, - 'DONE': 'YES', } response = HttpResponse( json.dumps(response_payload), content_type="application/json" @@ -631,7 +683,7 @@ def _split_input_list(str_list): def _msk_from_problem_urlname(course_id, urlname): """ - Convert a 'problem urlname' (instructor input name) + Convert a 'problem urlname' (name that instructor's input into dashboard) to a module state key (db field) """ if urlname.endswith(".xml"): diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 9e106906b1..3acc6fd17b 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -108,7 +108,7 @@ def _section_membership(course_id, access): 'enroll_button_url': reverse('students_update_enrollment', kwargs={'course_id': course_id}), 'unenroll_button_url': reverse('students_update_enrollment', kwargs={'course_id': course_id}), 'list_course_role_members_url': reverse('list_course_role_members', kwargs={'course_id': course_id}), - 'access_allow_revoke_url': reverse('access_allow_revoke', kwargs={'course_id': course_id}), + 'modify_access_url': reverse('modify_access', kwargs={'course_id': course_id}), 'list_forum_members_url': reverse('list_forum_members', kwargs={'course_id': course_id}), 'update_forum_role_membership_url': reverse('update_forum_role_membership', kwargs={'course_id': course_id}), } @@ -121,7 +121,7 @@ def _section_student_admin(course_id, access): 'section_key': 'student_admin', 'section_display_name': 'Student Admin', 'access': access, - 'get_student_progress_url': reverse('get_student_progress_url', kwargs={'course_id': course_id}), + 'get_student_progress_url_url': reverse('get_student_progress_url', kwargs={'course_id': course_id}), 'enrollment_url': reverse('students_update_enrollment', kwargs={'course_id': course_id}), 'reset_student_attempts_url': reverse('reset_student_attempts', kwargs={'course_id': course_id}), 'rescore_problem_url': reverse('rescore_problem', kwargs={'course_id': course_id}), @@ -135,8 +135,8 @@ def _section_data_download(course_id): section_data = { 'section_key': 'data_download', 'section_display_name': 'Data Download', - 'grading_config_url': reverse('grading_config', kwargs={'course_id': course_id}), - 'enrolled_students_features_url': reverse('enrolled_students_features', kwargs={'course_id': course_id}), + 'get_grading_config_url': reverse('get_grading_config', kwargs={'course_id': course_id}), + 'get_students_features_url': reverse('get_students_features', kwargs={'course_id': course_id}), } return section_data @@ -146,6 +146,6 @@ def _section_analytics(course_id): section_data = { 'section_key': 'analytics', 'section_display_name': 'Analytics', - 'profile_distributions_url': reverse('profile_distribution', kwargs={'course_id': course_id}), + 'get_distribution_url': reverse('get_distribution', kwargs={'course_id': course_id}), } return section_data diff --git a/lms/envs/test.py b/lms/envs/test.py index 81505ab0b3..1df9c0491f 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -29,6 +29,8 @@ MITX_FEATURES['ENABLE_SERVICE_STATUS'] = True MITX_FEATURES['ENABLE_HINTER_INSTRUCTOR_VIEW'] = True +MITX_FEATURES['ENABLE_INSTRUCTOR_BETA_DASHBOARD'] = True + # Need wiki for courseware views to work. TODO (vshnayder): shouldn't need it. WIKI_ENABLED = True diff --git a/lms/static/coffee/src/instructor_dashboard/analytics.coffee b/lms/static/coffee/src/instructor_dashboard/analytics.coffee index b4deb93f8a..ca9b9e46b4 100644 --- a/lms/static/coffee/src/instructor_dashboard/analytics.coffee +++ b/lms/static/coffee/src/instructor_dashboard/analytics.coffee @@ -28,7 +28,8 @@ class Analytics # fetch and list available distributions # `cb` is a callback to be run after populate_selector: (cb) -> - @get_profile_distributions [], + # ask for no particular distribution to get list of available distribuitions. + @get_profile_distributions undefined, # on error, print to console and dom. error: std_ajax_err => @$request_response_error.text "Error getting available distributions." success: (data) => @@ -38,7 +39,7 @@ class Analytics # add all fetched available features to drop-down for feature in data.available_features opt = $ '