diff --git a/lms/djangoapps/analytics/distributions.py b/lms/djangoapps/analytics/distributions.py index fb0cd5437d..4f24d0308a 100644 --- a/lms/djangoapps/analytics/distributions.py +++ b/lms/djangoapps/analytics/distributions.py @@ -56,6 +56,11 @@ class ProfileDistribution(object): self.feature = feature self.feature_display_name = DISPLAY_NAMES.get(feature, feature) + # to be set later + self.type = None + self.data = None + self.choices_display_names = None + def validate(self): """ Validate this profile distribution. @@ -63,6 +68,7 @@ class ProfileDistribution(object): Throws ProfileDistribution.ValidationError """ def validation_assert(predicate): + """ Throw a ValidationError if false. """ if not predicate: raise ProfileDistribution.ValidationError() diff --git a/lms/djangoapps/analytics/tests/test_distributions.py b/lms/djangoapps/analytics/tests/test_distributions.py index a72d89cfb3..61f948c26d 100644 --- a/lms/djangoapps/analytics/tests/test_distributions.py +++ b/lms/djangoapps/analytics/tests/test_distributions.py @@ -45,7 +45,8 @@ class TestAnalyticsDistributions(TestCase): distribution = profile_distribution(self.course_id, feature) print distribution self.assertEqual(distribution.type, 'OPEN_CHOICE') - self.assertFalse(hasattr(distribution, 'choices_display_names')) + self.assertTrue(hasattr(distribution, 'choices_display_names')) + self.assertEqual(distribution.choices_display_names, None) self.assertNotIn('no_data', distribution.data) self.assertEqual(distribution.data[1930], 1) @@ -76,6 +77,7 @@ class TestAnalyticsDistributionsNoData(TestCase): print distribution self.assertEqual(distribution.type, 'EASY_CHOICE') self.assertTrue(hasattr(distribution, 'choices_display_names')) + self.assertNotEqual(distribution.choices_display_names, None) self.assertIn('no_data', distribution.data) self.assertEqual(distribution.data['no_data'], len(self.nodata_users)) @@ -85,6 +87,7 @@ class TestAnalyticsDistributionsNoData(TestCase): distribution = profile_distribution(self.course_id, feature) print distribution self.assertEqual(distribution.type, 'OPEN_CHOICE') - self.assertFalse(hasattr(distribution, 'choices_display_names')) + self.assertTrue(hasattr(distribution, 'choices_display_names')) + self.assertEqual(distribution.choices_display_names, None) 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 3bdb9ae216..dfe8eed23b 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -3,7 +3,7 @@ Access control operations for use by instructor APIs. Does not include any access control, be sure to check access before calling. -TODO sync instructor and staff flags +TO DO sync instructor and staff flags e.g. should these be possible? {instructor: true, staff: false} {instructor: true, staff: true} diff --git a/lms/djangoapps/instructor/tests/test_access.py b/lms/djangoapps/instructor/tests/test_access.py index 74e7d48e79..688ed89dad 100644 --- a/lms/djangoapps/instructor/tests/test_access.py +++ b/lms/djangoapps/instructor/tests/test_access.py @@ -11,8 +11,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from django.test.utils import override_settings from courseware.tests.modulestore_config import TEST_DATA_MONGO_MODULESTORE -from courseware.access import (get_access_group_name, - course_beta_test_group_name) +from courseware.access import get_access_group_name from django_comment_common.models import (Role, FORUM_ROLE_MODERATOR) from instructor.access import (allow_access, diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 675f7fff8e..d8bea4e4c7 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -103,9 +103,10 @@ class TestInstructorAPIEnrollment(ModuleStoreTestCase, LoginEnrollmentTestCase): self.notregistered_email = 'robot-not-an-email-yet@robot.org' self.assertEqual(User.objects.filter(email=self.notregistered_email).count(), 0) - # enable printing of large diffs + # uncomment to enable enable printing of large diffs # from failed assertions in the event of a test failure. - self.maxDiff = None + # (comment because pylint C0103) + # self.maxDiff = None def test_missing_params(self): """ Test missing all query parameters. """ @@ -605,11 +606,12 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase): """ Fake task object """ FEATURES = ['task_type', 'task_input', 'task_id', 'requester', 'created', 'task_state'] - def __init__(self, i): + def __init__(self): for feature in self.FEATURES: setattr(self, feature, 'expected') def to_dict(self): + """ Convert fake task to dictionary representation. """ return {key: 'expected' for key in self.FEATURES} def setUp(self): @@ -631,7 +633,7 @@ class TestInstructorAPITaskLists(ModuleStoreTestCase, LoginEnrollmentTestCase): state=json.dumps({'attempts': 10}), ) - self.tasks = [self.FakeTask(i) for i in xrange(6)] + self.tasks = [self.FakeTask() for _ in xrange(6)] def test_list_instructor_tasks_running(self): """ Test list of all running tasks. """ diff --git a/lms/djangoapps/instructor/tests/test_enrollment.py b/lms/djangoapps/instructor/tests/test_enrollment.py index ce32f62b7b..46f677fe88 100644 --- a/lms/djangoapps/instructor/tests/test_enrollment.py +++ b/lms/djangoapps/instructor/tests/test_enrollment.py @@ -30,8 +30,9 @@ class TestSettableEnrollmentState(TestCase): allowed=False, auto_enroll=False ) - email, user, cenr, cea = mes.create_user(self.course_id) - ees = EmailEnrollmentState(self.course_id, email) + # enrollment objects + eobjs = mes.create_user(self.course_id) + ees = EmailEnrollmentState(self.course_id, eobjs.email) self.assertEqual(mes, ees) @@ -58,23 +59,20 @@ class TestEnrollmentChangeBase(TestCase): `action` should transition the world from before_ideal to after_ideal `action` will be supplied the following arguments (None-able arguments) `email` is an email string - `user` is a User - `cenr` is a CourseEnrollment - `cea` is a CourseEnrollmentAllowed """ # initialize & check before print "checking initialization..." - email, user, cenr, cea = before_ideal.create_user(self.course_id) - before = EmailEnrollmentState(self.course_id, email) + eobjs = before_ideal.create_user(self.course_id) + before = EmailEnrollmentState(self.course_id, eobjs.email) self.assertEqual(before, before_ideal) # do action print "running action..." - action(email, user, cenr, cea) + action(eobjs.email) # check after print "checking effects..." - after = EmailEnrollmentState(self.course_id, email) + after = EmailEnrollmentState(self.course_id, eobjs.email) self.assertEqual(after, after_ideal) @@ -95,8 +93,7 @@ class TestInstructorEnrollDB(TestEnrollmentChangeBase): auto_enroll=False ) - def action(email, user, cenr, cea): - enroll_email(self.course_id, email) + action = lambda email: enroll_email(self.course_id, email) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -115,8 +112,7 @@ class TestInstructorEnrollDB(TestEnrollmentChangeBase): auto_enroll=False, ) - def action(email, user, cenr, cea): - enroll_email(self.course_id, email) + action = lambda email: enroll_email(self.course_id, email) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -135,8 +131,7 @@ class TestInstructorEnrollDB(TestEnrollmentChangeBase): auto_enroll=False, ) - def action(email, user, cenr, cea): - enroll_email(self.course_id, email) + action = lambda email: enroll_email(self.course_id, email) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -155,8 +150,7 @@ class TestInstructorEnrollDB(TestEnrollmentChangeBase): auto_enroll=False, ) - def action(email, user, cenr, cea): - enroll_email(self.course_id, email) + action = lambda email: enroll_email(self.course_id, email) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -175,8 +169,7 @@ class TestInstructorEnrollDB(TestEnrollmentChangeBase): auto_enroll=True, ) - def action(email, user, cenr, cea): - enroll_email(self.course_id, email, auto_enroll=True) + action = lambda email: enroll_email(self.course_id, email, auto_enroll=True) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -195,8 +188,7 @@ class TestInstructorEnrollDB(TestEnrollmentChangeBase): auto_enroll=False, ) - def action(email, user, cenr, cea): - enroll_email(self.course_id, email, auto_enroll=False) + action = lambda email: enroll_email(self.course_id, email, auto_enroll=False) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -218,8 +210,7 @@ class TestInstructorUnenrollDB(TestEnrollmentChangeBase): auto_enroll=False ) - def action(email, user, cenr, cea): - unenroll_email(self.course_id, email) + action = lambda email: unenroll_email(self.course_id, email) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -238,8 +229,7 @@ class TestInstructorUnenrollDB(TestEnrollmentChangeBase): auto_enroll=False ) - def action(email, user, cenr, cea): - unenroll_email(self.course_id, email) + action = lambda email: unenroll_email(self.course_id, email) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -258,8 +248,7 @@ class TestInstructorUnenrollDB(TestEnrollmentChangeBase): auto_enroll=False ) - def action(email, user, cenr, cea): - unenroll_email(self.course_id, email) + action = lambda email: unenroll_email(self.course_id, email) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -278,8 +267,7 @@ class TestInstructorUnenrollDB(TestEnrollmentChangeBase): auto_enroll=False ) - def action(email, user, cenr, cea): - unenroll_email(self.course_id, email) + action = lambda email: unenroll_email(self.course_id, email) return self._run_state_change_test(before_ideal, after_ideal, action) @@ -310,6 +298,24 @@ class TestInstructorEnrollmentStudentModule(TestCase): self.assertEqual(StudentModule.objects.filter(student=user, course_id=self.course_id, module_state_key=msk).count(), 0) +class EnrollmentObjects(object): + """ + Container for enrollment objects. + + `email` - student email + `user` - student User object + `cenr` - CourseEnrollment object + `cea` - CourseEnrollmentAllowed object + + Any of the objects except email can be None. + """ + def __init__(self, email, user, cenr, cea): + self.email = email + self.user = user + self.cenr = cenr + self.cea = cea + + class SettableEnrollmentState(EmailEnrollmentState): """ Settable enrollment state. @@ -318,7 +324,7 @@ class SettableEnrollmentState(EmailEnrollmentState): a call to create_user will make objects which correspond to the state represented in the SettableEnrollmentState. """ - def __init__(self, user=False, enrollment=False, allowed=False, auto_enroll=False): + def __init__(self, user=False, enrollment=False, allowed=False, auto_enroll=False): # pylint: disable=W0231 self.user = user self.enrollment = enrollment self.allowed = allowed @@ -351,15 +357,15 @@ class SettableEnrollmentState(EmailEnrollmentState): user=user, course_id=course_id ) - return (email, user, cenr, None) + return EnrollmentObjects(email, user, cenr, None) else: - return (email, user, None, None) + return EnrollmentObjects(email, user, None, None) elif self.allowed: cea = CourseEnrollmentAllowed.objects.create( email=email, course_id=course_id, auto_enroll=self.auto_enroll, ) - return (email, None, None, cea) + return EnrollmentObjects(email, None, None, cea) else: - return (email, None, None, None) + return EnrollmentObjects(email, None, None, None) diff --git a/lms/djangoapps/instructor/tests/test_legacy_download_csv.py b/lms/djangoapps/instructor/tests/test_legacy_download_csv.py index 5a934d7a23..b05746f015 100644 --- a/lms/djangoapps/instructor/tests/test_legacy_download_csv.py +++ b/lms/djangoapps/instructor/tests/test_legacy_download_csv.py @@ -44,9 +44,10 @@ class TestInstructorDashboardGradeDownloadCSV(LoginEnrollmentTestCase): self.activate_user(self.instructor) def make_instructor(course): + """ Create an instructor for the course. """ group_name = _course_staff_group_name(course.location) - g = Group.objects.create(name=group_name) - g.user_set.add(User.objects.get(email=self.instructor)) + group = Group.objects.create(name=group_name) + group.user_set.add(User.objects.get(email=self.instructor)) make_instructor(self.toy) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 48b3de62e1..34698225c5 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -3,7 +3,7 @@ Instructor Dashboard API views JSON views which the instructor dashboard requests. -TODO a lot of these GETs should be PUTs +Many of these GETs may become PUTs in the future. """ import re @@ -41,7 +41,7 @@ def common_exceptions_400(func): Catches common exceptions and renders matching 400 errors. (decorator without arguments) """ - def wrapped(*args, **kwargs): + def wrapped(*args, **kwargs): # pylint: disable=C0111 try: return func(*args, **kwargs) except User.DoesNotExist: @@ -65,8 +65,8 @@ def require_query_params(*args, **kwargs): required_params += [(key, kwargs[key]) for key in kwargs] # required_params = e.g. [('action', 'enroll or unenroll'), ['emails', None]] - def decorator(func): - def wrapped(*args, **kwargs): + def decorator(func): # pylint: disable=C0111 + def wrapped(*args, **kwargs): # pylint: disable=C0111 request = args[0] error_response_data = { @@ -108,8 +108,8 @@ def require_level(level): if level not in ['instructor', 'staff']: raise ValueError("unrecognized level '{}'".format(level)) - def decorator(func): - def wrapped(*args, **kwargs): + def decorator(func): # pylint: disable=C0111 + def wrapped(*args, **kwargs): # pylint: disable=C0111 request = args[0] course = get_course_by_id(kwargs['course_id']) @@ -339,14 +339,14 @@ def get_grading_config(request, course_id): @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_level('staff') -def get_students_features(request, course_id, csv=False): +def get_students_features(request, course_id, csv=False): # pylint: disable=W0613 """ Respond with json which contains a summary of all enrolled students profile information. Responds with JSON {"students": [{-student-info-}, ...]} - TODO accept requests for different attribute sets. + TO DO accept requests for different attribute sets. """ available_features = analytics.basic.AVAILABLE_FEATURES query_features = ['username', 'name', 'email', 'language', 'location', 'year_of_birth', 'gender', @@ -382,8 +382,6 @@ def get_distribution(request, course_id): 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 respond to csv requests as well """ feature = request.GET.get('feature') # alternate notations of None @@ -392,9 +390,9 @@ def get_distribution(request, course_id): else: feature = str(feature) - AVAILABLE_FEATURES = analytics.distributions.AVAILABLE_PROFILE_FEATURES + 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,): + if not feature in available_features + (None,): return HttpResponseBadRequest( "feature '{}' not available.".format(feature) ) @@ -402,7 +400,7 @@ def get_distribution(request, course_id): response_payload = { 'course_id': course_id, 'queried_feature': feature, - 'available_features': AVAILABLE_FEATURES, + 'available_features': available_features, 'feature_display_names': analytics.distributions.DISPLAY_NAMES, } @@ -518,7 +516,7 @@ def reset_student_attempts(request, course_id): except StudentModule.DoesNotExist: return HttpResponseBadRequest("Module does not exist.") elif all_students: - task = instructor_task.api.submit_reset_problem_attempts_for_all_students(request, course_id, module_state_key) + instructor_task.api.submit_reset_problem_attempts_for_all_students(request, course_id, module_state_key) response_payload['task'] = 'created' else: return HttpResponseBadRequest() @@ -566,10 +564,10 @@ def rescore_problem(request, course_id): if student_email: response_payload['student_email'] = student_email student = User.objects.get(email=student_email) - task = instructor_task.api.submit_rescore_problem_for_student(request, course_id, module_state_key, student) + instructor_task.api.submit_rescore_problem_for_student(request, course_id, module_state_key, student) response_payload['task'] = 'created' elif all_students: - task = instructor_task.api.submit_rescore_problem_for_all_students(request, course_id, module_state_key) + instructor_task.api.submit_rescore_problem_for_all_students(request, course_id, module_state_key) response_payload['task'] = 'created' else: return HttpResponseBadRequest() @@ -614,8 +612,8 @@ def list_instructor_tasks(request, 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) + features = ['task_type', 'task_input', 'task_id', 'requester', 'created', 'task_state'] + return dict((feature, str(getattr(task, feature))) for feature in features) response_payload = { 'tasks': map(extract_task_features, tasks), diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 1c2f0db30f..ca89545ec0 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -12,10 +12,7 @@ from django.http import Http404 from courseware.access import has_access from courseware.courses import get_course_by_id from django_comment_client.utils import has_forum_access -from django_comment_common.models import (Role, - FORUM_ROLE_ADMINISTRATOR, - FORUM_ROLE_MODERATOR, - FORUM_ROLE_COMMUNITY_TA) +from django_comment_common.models import FORUM_ROLE_ADMINISTRATOR from xmodule.modulestore.django import modulestore from student.models import CourseEnrollment @@ -64,9 +61,9 @@ The dictionary must include at least { 'section_display_name': 'Circus Expo' } +section_key will be used as a css attribute, javascript tie-in, and template import filename. section_display_name will be used to generate link titles in the nav bar. -sek will be used as a css attribute, javascript tie-in, and template import filename. -""" +""" # pylint: disable=W0105 def _section_course_info(course_id): @@ -81,16 +78,16 @@ def _section_course_info(course_id): section_data['enrollment_count'] = CourseEnrollment.objects.filter(course_id=course_id).count() section_data['has_started'] = course.has_started() section_data['has_ended'] = course.has_ended() + try: - def next(memo, (letter, score)): - return "{}: {}, ".format(letter, score) + memo - section_data['grade_cutoffs'] = reduce(next, course.grade_cutoffs.items(), "")[:-2] - except: + advance = lambda memo, (letter, score): "{}: {}, ".format(letter, score) + memo + section_data['grade_cutoffs'] = reduce(advance, course.grade_cutoffs.items(), "")[:-2] + except Exception: section_data['grade_cutoffs'] = "Not Available" # section_data['offline_grades'] = offline_grades_available(course_id) try: - section_data['course_errors'] = [(escape(a), '') for (a, b) in modulestore().get_item_errors(course.location)] + section_data['course_errors'] = [(escape(a), '') for (a, _) in modulestore().get_item_errors(course.location)] except Exception: section_data['course_errors'] = [('Error fetching errors', '')]