cleanup, fix permissions, rename functions, add more api tests

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]

lms/djangoapps/analytics/distributions.py

 """
 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

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))

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':

lms/djangoapps/instructor/tests/test_api.py

 """
-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)

lms/djangoapps/instructor/views/api.py

 """
 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)
+        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
+                }
+            }
+        ]
+    }
     """
-    course = get_course_with_access(
-        request.user, course_id, 'staff', depth=None
-    )
-
     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.
-
-    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
+    Respond with json of the distribution of students over selected features which have choices.
 
-    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])})
+    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']
 
-    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
-    )
-
-    try:
-        features = json.loads(request.GET.get('features'))
-    except Exception:
-        features = [request.GET.get('features')]
-
-    feature_results = {}
+    feature = request.GET.get('feature')
+    # alternate notations of None
+    if feature in (None, 'null', ''):
+        feature = None
+    else:
+        feature = str(feature)
 
-    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"):

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

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
 

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 = $ '<option/>',
-            text: data.display_names[feature]
+            text: data.feature_display_names[feature]
             data:
               feature: feature
 
@@ -53,64 +54,60 @@ class Analytics
     feature = opt.data 'feature'
 
     @reset_display()
+    # only proceed if there is a feature attached to the selected option.
     return unless feature
-    @get_profile_distributions [feature],
+    @get_profile_distributions feature,
       error: std_ajax_err => @$request_response_error.text "Error getting distribution for '#{feature}'."
       success: (data) =>
-        feature_res = data.feature_results[feature]
-        # feature response format: {'error': 'optional error string', 'type': 'SOME_TYPE', 'data': [stuff]}
-        if feature_res.error
-          console.warn(feature_res.error)
-          @$display_text.text 'Error fetching data'
+        feature_res = data.feature_results
+        if feature_res.type is 'EASY_CHOICE'
+          # display on SlickGrid
+          options =
+            enableCellNavigation: true
+            enableColumnReorder: false
+            forceFitColumns: true
+
+          columns = [
+            id: feature
+            field: feature
+            name: feature
+          ,
+            id: 'count'
+            field: 'count'
+            name: 'Count'
+          ]
+
+          grid_data = _.map feature_res.data, (value, key) ->
+            datapoint = {}
+            datapoint[feature] = feature_res.choices_display_names[key]
+            datapoint['count'] = value
+            datapoint
+
+          table_placeholder = $ '<div/>', class: 'slickgrid'
+          @$display_table.append table_placeholder
+          grid = new Slick.Grid(table_placeholder, grid_data, columns, options)
+        else if feature_res.feature is 'year_of_birth'
+          graph_placeholder = $ '<div/>', class: 'year-of-birth'
+          @$display_graph.append graph_placeholder
+
+          graph_data = _.map feature_res.data, (value, key) -> [parseInt(key), value]
+
+          $.plot graph_placeholder, [
+            data: graph_data
+          ]
         else
-          if feature_res.type is 'EASY_CHOICE'
-            # display on SlickGrid
-            options =
-              enableCellNavigation: true
-              enableColumnReorder: false
-              forceFitColumns: true
-
-            columns = [
-              id: feature
-              field: feature
-              name: feature
-            ,
-              id: 'count'
-              field: 'count'
-              name: 'Count'
-            ]
-
-            grid_data = _.map feature_res.data, (value, key) ->
-              datapoint = {}
-              datapoint[feature] = feature_res.display_names[key]
-              datapoint['count'] = value
-              datapoint
-
-            table_placeholder = $ '<div/>', class: 'slickgrid'
-            @$display_table.append table_placeholder
-            grid = new Slick.Grid(table_placeholder, grid_data, columns, options)
-          else if feature is 'year_of_birth'
-            graph_placeholder = $ '<div/>', class: 'year-of-birth'
-            @$display_graph.append graph_placeholder
-
-            graph_data = _.map feature_res.data, (value, key) -> [parseInt(key), value]
-
-            $.plot graph_placeholder, [
-              data: graph_data
-            ]
-          else
-            console.warn("don't know how to show #{feature_res.type}")
-            @$display_text.text 'Unavailable Metric\n' + JSON.stringify(feature_res)
+          console.warn("unable to show distribution #{feature_res.type}")
+          @$display_text.text 'Unavailable Metric Display\n' + JSON.stringify(feature_res)
 
 
   # fetch distribution data from server.
   # `handler` can be either a callback for success
   # or a mapping e.g. {success: ->, error: ->, complete: ->}
-  get_profile_distributions: (featurelist, handler) ->
+  get_profile_distributions: (feature, handler) ->
     settings =
       dataType: 'json'
       url: @$distribution_select.data 'endpoint'
-      data: features: JSON.stringify featurelist
+      data: feature: feature
 
     if typeof handler is 'function'
       _.extend settings, success: handler

lms/static/coffee/src/instructor_dashboard/instructor_dashboard.coffee

@@ -36,9 +36,7 @@ HASH_LINK_PREFIX = '#view-'
 # once we're ready, check if this page is the instructor dashboard
 $ =>
   instructor_dashboard_content = $ ".#{CSS_INSTRUCTOR_CONTENT}"
-  console.log 'checking if we are on the instructor dashboard'
   if instructor_dashboard_content.length > 0
-    console.log 'we are on the instructor dashboard'
     setup_instructor_dashboard          instructor_dashboard_content
     setup_instructor_dashboard_sections instructor_dashboard_content
 

lms/templates/courseware/instructor_dashboard_2/analytics.html

 <%page args="section_data"/>
 
 <h2>Distributions</h2>
-<select id="distributions" data-endpoint="${ section_data['profile_distributions_url'] }">
+<select id="distributions" data-endpoint="${ section_data['get_distribution_url'] }">
   <option> Getting available distributions... </option>
 </select>
 <div class="distribution-display">

lms/templates/courseware/instructor_dashboard_2/data_download.html

 <%page args="section_data"/>
 
-<input type="button" name="list-profiles" value="List enrolled students with profile information" data-endpoint="${ section_data['enrolled_students_features_url'] }" >
-<input type="button" name="list-profiles" value="CSV" data-csv="true" class="csv" data-endpoint="${ section_data['enrolled_students_features_url'] }" >
+<input type="button" name="list-profiles" value="List enrolled students with profile information" data-endpoint="${ section_data['get_students_features_url'] }" >
+<input type="button" name="list-profiles" value="CSV" data-csv="true" class="csv" data-endpoint="${ section_data['get_students_features_url'] }" >
 <br>
 ## <input type="button" name="list-grades" value="Student grades">
 ## <input type="button" name="list-profiles" value="CSV" data-csv="true" class="csv">
 ## <br>
 ## <input type="button" name="list-answer-distributions" value="Answer distributions (x students got y points)">
 ## <br>
-<input type="button" name="dump-gradeconf" value="Grading Configuration" data-endpoint="${ section_data['grading_config_url'] }">
+<input type="button" name="dump-gradeconf" value="Grading Configuration" data-endpoint="${ section_data['get_grading_config_url'] }">
 
 <div class="data-display">
   <div class="data-display-text"></div>

lms/templates/courseware/instructor_dashboard_2/membership.html

@@ -29,7 +29,7 @@
   %if section_data['access']['instructor']:
     <div class="auth-list-container" data-rolename="staff" data-display-name="Staff">
       <div class="auth-list-table" data-endpoint="${ section_data['list_course_role_members_url'] }"></div>
-      <div class="auth-list-add" data-endpoint="${ section_data['access_allow_revoke_url'] }">
+      <div class="auth-list-add" data-endpoint="${ section_data['modify_access_url'] }">
         <input type="text" name="email" placeholder="Enter Email" spellcheck="false">
         <input type="button" name="allow" value="Grant Staff Access">
       </div>
@@ -39,7 +39,7 @@
     %if section_data['access']['instructor']:
       <div class="auth-list-container" data-rolename="instructor" data-display-name="Instructors">
         <div class="auth-list-table" data-endpoint="${ section_data['list_course_role_members_url'] }"></div>
-        <div class="auth-list-add" data-endpoint="${ section_data['access_allow_revoke_url'] }">
+        <div class="auth-list-add" data-endpoint="${ section_data['modify_access_url'] }">
           <input type="text" name="email" placeholder="Enter Email" spellcheck="false">
           <input type="button" name="allow" value="Grant Instructor Access">
         </div>
@@ -49,7 +49,7 @@
 
     <div class="auth-list-container" data-rolename="beta" data-display-name="Beta Testers">
       <div class="auth-list-table" data-endpoint="${ section_data['list_course_role_members_url'] }"></div>
-      <div class="auth-list-add" data-endpoint="${ section_data['access_allow_revoke_url'] }">
+      <div class="auth-list-add" data-endpoint="${ section_data['modify_access_url'] }">
         <input type="text" name="email" placeholder="Enter Email" spellcheck="false">
         <input type="button" name="allow" value="Grant Beta Tester Access">
       </div>

lms/templates/courseware/instructor_dashboard_2/student_admin.html

@@ -8,7 +8,7 @@
   <br>
 
   <div class="progress-link-wrapper">
-    <a href="" class="progress-link" data-endpoint="${ section_data['get_student_progress_url'] }">Student Progress Page</a>
+    <a href="" class="progress-link" data-endpoint="${ section_data['get_student_progress_url_url'] }">Student Progress Page</a>
   </div>
 
   <br>

lms/urls.py

@@ -350,14 +350,14 @@ if settings.COURSEWARE_ENABLED and settings.MITX_FEATURES.get('ENABLE_INSTRUCTOR
             'instructor.views.api.students_update_enrollment', name="students_update_enrollment"),
         url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/list_course_role_members$',
             'instructor.views.api.list_course_role_members', name="list_course_role_members"),
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/access_allow_revoke$',
-            'instructor.views.api.access_allow_revoke', name="access_allow_revoke"),
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/grading_config$',
-            'instructor.views.api.grading_config', name="grading_config"),
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/enrolled_students_features(?P<csv>/csv)?$',
-            'instructor.views.api.enrolled_students_features', name="enrolled_students_features"),
-        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/profile_distribution$',
-            'instructor.views.api.profile_distribution', name="profile_distribution"),
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/modify_access$',
+            'instructor.views.api.modify_access', name="modify_access"),
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/get_grading_config$',
+            'instructor.views.api.get_grading_config', name="get_grading_config"),
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/get_students_features(?P<csv>/csv)?$',
+            'instructor.views.api.get_students_features', name="get_students_features"),
+        url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/get_distribution$',
+            'instructor.views.api.get_distribution', name="get_distribution"),
         url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/get_student_progress_url$',
             'instructor.views.api.get_student_progress_url', name="get_student_progress_url"),
         url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/instructor_dashboard/api/reset_student_attempts$',