diff --git a/common/lib/xmodule/xmodule/partitions/partitions_service.py b/common/lib/xmodule/xmodule/partitions/partitions_service.py index bb23ca8225..5eafa11259 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions_service.py +++ b/common/lib/xmodule/xmodule/partitions/partitions_service.py @@ -21,8 +21,9 @@ class PartitionService(object): """ raise NotImplementedError('Subclasses must implement course_partition') - def __init__(self, runtime, track_function): - self.runtime = runtime + def __init__(self, user, course_id, track_function=None): + self._user = user + self._course_id = course_id self._track_function = track_function def get_user_group_id_for_partition(self, user_partition_id): @@ -50,10 +51,10 @@ class PartitionService(object): if user_partition is None: raise ValueError( "Configuration problem! No user_partition with id {0} " - "in course {1}".format(user_partition_id, self.runtime.course_id) + "in course {1}".format(user_partition_id, self._course_id) ) - group = self._get_group(user_partition) + group = self.get_group(user_partition) return group.id if group else None def _get_user_partition(self, user_partition_id): @@ -69,13 +70,12 @@ class PartitionService(object): return None - def _get_group(self, user_partition): + def get_group(self, user_partition, assign=True): """ Returns the group from the specified user partition to which the user is assigned. If the user has not yet been assigned, a group will be chosen for them based upon the partition's scheme. """ - user = self.runtime.get_real_user(self.runtime.anonymous_student_id) return user_partition.scheme.get_group_for_user( - self.runtime.course_id, user, user_partition, track_function=self._track_function + self._course_id, self._user, user_partition, assign=assign, track_function=self._track_function ) diff --git a/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py b/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py index 6b13b7cb4e..f52f968ca9 100644 --- a/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py +++ b/common/lib/xmodule/xmodule/partitions/tests/test_partitions.py @@ -6,10 +6,10 @@ Test the partitions and partitions service from unittest import TestCase from mock import Mock +from opaque_keys.edx.locations import SlashSeparatedCourseKey from stevedore.extension import Extension, ExtensionManager from xmodule.partitions.partitions import Group, UserPartition, UserPartitionError, USER_PARTITION_SCHEME_NAMESPACE from xmodule.partitions.partitions_service import PartitionService -from xmodule.tests import get_test_system class TestGroup(TestCase): @@ -91,7 +91,7 @@ class MockUserPartitionScheme(object): self.name = name self.current_group = current_group - def get_group_for_user(self, course_id, user, user_partition, track_function=None): # pylint: disable=unused-argument + def get_group_for_user(self, course_id, user, user_partition, assign=True, track_function=None): # pylint: disable=unused-argument """ Returns the current group if set, else the first group from the specified user partition. """ @@ -280,9 +280,11 @@ class TestPartitionService(PartitionTestCase): def setUp(self): super(TestPartitionService, self).setUp() + course = Mock(id=SlashSeparatedCourseKey('org_0', 'course_0', 'run_0')) self.partition_service = StaticPartitionService( [self.user_partition], - runtime=get_test_system(), + user=Mock(username='ma', email='ma@edx.org', is_staff=False, is_active=True), + course_id=course.id, track_function=Mock() ) @@ -300,3 +302,19 @@ class TestPartitionService(PartitionTestCase): self.user_partition.scheme.current_group = groups[1] # pylint: disable=no-member group2_id = self.partition_service.get_user_group_id_for_partition(user_partition_id) self.assertEqual(group2_id, groups[1].id) # pylint: disable=no-member + + def test_get_group(self): + """ + Test that a partition group is assigned to a user. + """ + groups = self.user_partition.groups # pylint: disable=no-member + + # assign first group and verify that it is returned for the user + self.user_partition.scheme.current_group = groups[0] # pylint: disable=no-member + group1 = self.partition_service.get_group(self.user_partition) + self.assertEqual(group1, groups[0]) # pylint: disable=no-member + + # switch to the second group and verify that it is returned for the user + self.user_partition.scheme.current_group = groups[1] # pylint: disable=no-member + group2 = self.partition_service.get_group(self.user_partition) + self.assertEqual(group2, groups[1]) # pylint: disable=no-member diff --git a/common/lib/xmodule/xmodule/tests/test_split_test_module.py b/common/lib/xmodule/xmodule/tests/test_split_test_module.py index 700145c8d6..df8275b180 100644 --- a/common/lib/xmodule/xmodule/tests/test_split_test_module.py +++ b/common/lib/xmodule/xmodule/tests/test_split_test_module.py @@ -59,7 +59,8 @@ class SplitTestModuleTest(XModuleXmlImportTest, PartitionTestCase): MockUserPartitionScheme() ) ], - runtime=self.module_system, + user=Mock(username='ma', email='ma@edx.org', is_staff=False, is_active=True), + course_id=self.course.id, track_function=Mock(name='track_function'), ) self.module_system._services['partitions'] = self.partitions_service # pylint: disable=protected-access diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index 70a5e2d42d..4ed4e9000f 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -4,7 +4,6 @@ running state of a course. """ import json -import urllib from datetime import datetime from time import time import unicodecsv @@ -22,8 +21,7 @@ from track.views import task_track from util.file import course_filename_prefix_generator, UniversalNewlineIterator from xmodule.modulestore.django import modulestore -from openedx.core.djangoapps.course_groups.models import CourseUserGroup -from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort +from courseware.courses import get_course_by_id from courseware.grades import iterate_grades_for from courseware.models import StudentModule from courseware.model_data import FieldDataCache @@ -31,8 +29,13 @@ from courseware.module_render import get_module_for_descriptor_internal from instructor_analytics.basic import enrolled_students_features from instructor_analytics.csvs import format_dictlist from instructor_task.models import ReportStore, InstructorTask, PROGRESS +from lms.djangoapps.lms_xblock.runtime import LmsPartitionService +from openedx.core.djangoapps.course_groups.cohorts import get_cohort +from openedx.core.djangoapps.course_groups.models import CourseUserGroup +from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort from student.models import CourseEnrollment + # define different loggers for use within tasks and on client side TASK_LOG = get_task_logger(__name__) @@ -547,6 +550,13 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, enrolled_students = CourseEnrollment.users_enrolled_in(course_id) task_progress = TaskProgress(action_name, enrolled_students.count(), start_time) + course = get_course_by_id(course_id) + cohorts_header = ['Cohort Group Name'] if course.is_cohorted else [] + + partition_service = LmsPartitionService(user=None, course_id=course_id) + partitions = partition_service.course_partitions + group_configs_header = ['Group Configuration Group Name ({})'.format(partition.name) for partition in partitions] + # Loop over all our students and build our CSV lists in memory header = None rows = [] @@ -564,7 +574,9 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, if not header: # Encode the header row in utf-8 encoding in case there are unicode characters header = [section['label'].encode('utf-8') for section in gradeset[u'section_breakdown']] - rows.append(["id", "email", "username", "grade"] + header) + rows.append( + ["id", "email", "username", "grade"] + header + cohorts_header + group_configs_header + ) percents = { section['label']: section.get('percent', 0.0) @@ -572,6 +584,16 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, if 'label' in section } + cohorts_group_name = [] + if course.is_cohorted: + group = get_cohort(student, course_id, assign=False) + cohorts_group_name.append(group.name if group else '') + + group_configs_group_names = [] + for partition in partitions: + group = LmsPartitionService(student, course_id).get_group(partition, assign=False) + group_configs_group_names.append(group.name if group else '') + # Not everybody has the same gradable items. If the item is not # found in the user's gradeset, just assume it's a 0. The aggregated # grades for their sections and overall course will be calculated @@ -579,7 +601,10 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, # possible for a student to have a 0.0 show up in their row but # still have 100% for the course. row_percents = [percents.get(label, 0.0) for label in header] - rows.append([student.id, student.email, student.username, gradeset['percent']] + row_percents) + rows.append( + [student.id, student.email, student.username, gradeset['percent']] + + row_percents + cohorts_group_name + group_configs_group_names + ) else: # An empty gradeset means we failed to grade a student. task_progress.failed += 1 diff --git a/lms/djangoapps/instructor_task/tests/test_integration.py b/lms/djangoapps/instructor_task/tests/test_integration.py index 72580de00b..bb164350cb 100644 --- a/lms/djangoapps/instructor_task/tests/test_integration.py +++ b/lms/djangoapps/instructor_task/tests/test_integration.py @@ -611,15 +611,30 @@ class TestGradeReportConditionalContent(TestReportMixin, TestIntegrationTask): representing their grades we expect to see in the CSV. For example: [student_a: {'grade': 1.0, 'HW': 1.0}] """ - def merge_dicts(dict_1, dict_2): - """Return the union of dict_1 and dict_2""" - return dict(dict_1.items() + dict_2.items()) + def merge_dicts(*dicts): + """ + Return the union of dicts + + Arguments: + dicts: tuple of dicts + """ + return dict([item for d in dicts for item in d.items()]) + + def user_partition_group(user): + """Return a dict having single key with value equals to students group in partition""" + group_config_hdr_tpl = 'Group Configuration Group Name ({})' + return { + group_config_hdr_tpl.format(self.partition.name): self.partition.scheme.get_group_for_user( # pylint: disable=E1101 + self.course.id, user, self.partition, track_function=None + ).name + } self.verify_rows_in_csv( [ merge_dicts( {'id': str(student.id), 'username': student.username, 'email': student.email}, - grades + grades, + user_partition_group(student) ) for student_grades in students_grades for student, grades in student_grades.iteritems() ] diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 7c324be9c8..7002a0a69e 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1,3 +1,5 @@ +# -*- coding: utf-8 -*- + """ Unit tests for LMS instructor-initiated background tasks helper functions. @@ -7,8 +9,12 @@ Tests that CSV grade report generation works with unicode emails. import ddt from mock import Mock, patch import tempfile +import unicodecsv from xmodule.modulestore.tests.factories import CourseFactory +from student.tests.factories import UserFactory +from student.models import CourseEnrollment +from xmodule.partitions.partitions import Group, UserPartition from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from instructor_task.models import ReportStore @@ -57,6 +63,82 @@ class TestInstructorGradeReport(TestReportMixin, InstructorTaskCourseTestCase): report_store = ReportStore.from_config() self.assertTrue(any('grade_report_err' in item[0] for item in report_store.links_for(self.course.id))) + def _verify_cohort_data(self, course_id, expected_cohort_groups): + """ + Verify cohort data. + """ + cohort_groups_in_csv = [] + with patch('instructor_task.tasks_helper._get_current_task'): + result = upload_grades_csv(None, None, course_id, None, 'graded') + self.assertDictContainsSubset({'attempted': 2, 'succeeded': 2, 'failed': 0}, result) + report_store = ReportStore.from_config() + report_csv_filename = report_store.links_for(course_id)[0][0] + with open(report_store.path_to(course_id, report_csv_filename)) as csv_file: + for row in unicodecsv.DictReader(csv_file): + cohort_groups_in_csv.append(row['Cohort Group Name']) + + self.assertEqual(cohort_groups_in_csv, expected_cohort_groups) + + def test_cohort_data_in_grading(self): + """ + Test that cohort data is included in grades csv if cohort configuration is enabled for course. + """ + cohort_groups = ['cohort 1', 'cohort 2'] + course = CourseFactory.create(cohort_config={'cohorted': True, 'auto_cohort': True, + 'auto_cohort_groups': cohort_groups}) + for _ in range(2): + CourseEnrollment.enroll(UserFactory.create(), course.id) + + # In auto cohorting a group will be assigned to a user only when user visits a problem + # In grading calculation we only add a group in csv if group is already assigned to + # user rather than creating a group automatically at runtime + expected_groups = ['', ''] + self._verify_cohort_data(course.id, expected_groups) + + def test_unicode_cohort_data_in_grading(self): + """ + Test that cohort groups can contain unicode characters. + """ + cohort_groups = [u'ÞrÖfessÖr X', u'MàgnëtÖ'] + course = CourseFactory.create(cohort_config={'cohorted': True}) + + # Create users and manually assign cohort groups + user1 = UserFactory.create(username='user1') + user2 = UserFactory.create(username='user2') + CourseEnrollment.enroll(user1, course.id) + CourseEnrollment.enroll(user2, course.id) + cohort1 = CohortFactory(course_id=course.id, name=u'ÞrÖfessÖr X') + cohort2 = CohortFactory(course_id=course.id, name=u'MàgnëtÖ') + cohort1.users.add(user1) + cohort2.users.add(user2) + + self._verify_cohort_data(course.id, cohort_groups) + + def test_unicode_user_partitions(self): + """ + Test that user partition groups can contain unicode characters. + """ + user_groups = [u'ÞrÖfessÖr X', u'MàgnëtÖ'] + user_partition = UserPartition( + 0, + 'x_man', + 'X Man', + [ + Group(0, user_groups[0]), + Group(1, user_groups[1]) + ] + ) + + # Create course with group configurations + self.initialize_course( + course_factory_kwargs={ + 'user_partitions': [user_partition] + } + ) + + _groups = [group.name for group in self.course.user_partitions[0].groups] + self.assertEqual(_groups, user_groups) + @ddt.ddt class TestStudentReport(TestReportMixin, InstructorTaskCourseTestCase): diff --git a/lms/djangoapps/lms_xblock/runtime.py b/lms/djangoapps/lms_xblock/runtime.py index c85e39eeeb..49d0abbf38 100644 --- a/lms/djangoapps/lms_xblock/runtime.py +++ b/lms/djangoapps/lms_xblock/runtime.py @@ -135,7 +135,7 @@ class LmsPartitionService(PartitionService): """ @property def course_partitions(self): - course = modulestore().get_course(self.runtime.course_id) + course = modulestore().get_course(self._course_id) return course.user_partitions @@ -195,7 +195,8 @@ class LmsModuleSystem(LmsHandlerUrls, ModuleSystem): # pylint: disable=abstract services = kwargs.setdefault('services', {}) services['user_tags'] = UserTagsService(self) services['partitions'] = LmsPartitionService( - runtime=self, + user=kwargs.get('user'), + course_id=kwargs.get('course_id'), track_function=kwargs.get('track_function', None), ) services['fs'] = xblock.reference.plugins.FSService() diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index 82a013d336..7fba71cafa 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -196,7 +196,7 @@ def get_cohorted_commentables(course_key): return ans -def get_cohort(user, course_key): +def get_cohort(user, course_key, assign=True): """ Given a Django user and a CourseKey, return the user's cohort in that cohort. @@ -204,6 +204,7 @@ def get_cohort(user, course_key): Arguments: user: a Django User object. course_key: CourseKey + assign (bool): if False then we don't assign a group to user Returns: A CourseUserGroup object if the course is cohorted and the User has a @@ -230,7 +231,8 @@ def get_cohort(user, course_key): ) except CourseUserGroup.DoesNotExist: # Didn't find the group. We'll go on to create one if needed. - pass + if not assign: + return None choices = course.auto_cohort_groups if len(choices) > 0: diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index 6c98a19c98..1707dd0530 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -207,6 +207,31 @@ class TestCohorts(TestCase): "other_user should be assigned to the default cohort" ) + def test_get_cohort_with_assign(self): + """ + Make sure cohorts.get_cohort() returns None if no group is already + assigned to a user instead of assigning/creating a group automatically + """ + course = modulestore().get_course(self.toy_course_key) + self.assertFalse(course.is_cohorted) + + user = UserFactory(username="test", email="a@b.com") + + # Add an auto_cohort_group to the course... + config_course_cohorts( + course, + discussions=[], + cohorted=True, + auto_cohort_groups=["AutoGroup"] + ) + + # get_cohort should return None as no group is assigned to user + self.assertIsNone(cohorts.get_cohort(user, course.id, assign=False)) + + # get_cohort should return a group for user + self.assertEquals(cohorts.get_cohort(user, course.id).name, "AutoGroup") + + def test_auto_cohorting(self): """ Make sure cohorts.get_cohort() does the right thing with auto_cohort_groups diff --git a/openedx/core/djangoapps/user_api/partition_schemes.py b/openedx/core/djangoapps/user_api/partition_schemes.py index b3b40212db..a664500b98 100644 --- a/openedx/core/djangoapps/user_api/partition_schemes.py +++ b/openedx/core/djangoapps/user_api/partition_schemes.py @@ -15,15 +15,16 @@ class RandomUserPartitionScheme(object): RANDOM = random.Random() @classmethod - def get_group_for_user(cls, course_id, user, user_partition, track_function=None): + def get_group_for_user(cls, course_id, user, user_partition, assign=True, track_function=None): """ Returns the group from the specified user position to which the user is assigned. - If the user has not yet been assigned, a group will be randomly chosen for them. + If the user has not yet been assigned, a group will be randomly chosen for them if assign flag is True. """ partition_key = cls._key_for_partition(user_partition) group_id = course_tag_api.get_course_tag(user, course_id, partition_key) group = user_partition.get_group(int(group_id)) if not group_id is None else None - if group is None: + + if group is None and assign: if not user_partition.groups: raise UserPartitionError('Cannot assign user to an empty user partition') diff --git a/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py b/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py index 7d7ce3135d..703b7be2af 100644 --- a/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py +++ b/openedx/core/djangoapps/user_api/tests/test_partition_schemes.py @@ -58,6 +58,24 @@ class TestRandomUserPartitionScheme(PartitionTestCase): group2_id = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, self.user_partition) self.assertEqual(group1_id, group2_id) + def test_get_group_for_user_with_assign(self): + """ + Make sure get_group_for_user returns None if no group is already + assigned to a user instead of assigning/creating a group automatically + """ + # We should not get any group because assign is False which will + # protect us from automatically creating a group for user + group = RandomUserPartitionScheme.get_group_for_user( + self.MOCK_COURSE_ID, self.user, self.user_partition, assign=False + ) + + self.assertIsNone(group) + + # We should get a group automatically assigned to user + group = RandomUserPartitionScheme.get_group_for_user(self.MOCK_COURSE_ID, self.user, self.user_partition) + + self.assertIsNotNone(group) + def test_empty_partition(self): empty_partition = UserPartition( self.TEST_ID,