From 707ee5f2c745821885510da84aec8b73141e16a0 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 4 Apr 2014 18:53:00 -0400 Subject: [PATCH] Change behavior of cohort addition interface Instead of noting users that are already in a cohort and not changing such users, clobber the previous cohort and display a message indicating the previous cohort membership. JIRA: FOR-452 --- common/djangoapps/course_groups/cohorts.py | 13 +- .../tests/{tests.py => test_cohorts.py} | 0 .../course_groups/tests/test_views.py | 187 ++++++++++++++++++ common/djangoapps/course_groups/views.py | 39 ++-- common/static/js/course_groups/cohorts.js | 49 +++-- 5 files changed, 245 insertions(+), 43 deletions(-) rename common/djangoapps/course_groups/tests/{tests.py => test_cohorts.py} (100%) create mode 100644 common/djangoapps/course_groups/tests/test_views.py diff --git a/common/djangoapps/course_groups/cohorts.py b/common/djangoapps/course_groups/cohorts.py index c6b93e563490..7e6925879ff6 100644 --- a/common/djangoapps/course_groups/cohorts.py +++ b/common/djangoapps/course_groups/cohorts.py @@ -226,18 +226,16 @@ def add_user_to_cohort(cohort, username_or_email): username_or_email: string. Treated as email if has '@' Returns: - User object. + Tuple of User object and string (or None) indicating previous cohort Raises: User.DoesNotExist if can't find user. ValueError if user already present in this cohort. - - CohortConflict if user already in another cohort. """ user = get_user_by_username_or_email(username_or_email) + previous_cohort = None - # If user in any cohorts in this course already, complain course_cohorts = CourseUserGroup.objects.filter( course_id=cohort.course_id, users__id=user.id, @@ -248,12 +246,11 @@ def add_user_to_cohort(cohort, username_or_email): user.username, cohort.name)) else: - raise CohortConflict("User {0} is in another cohort {1} in course" - .format(user.username, - course_cohorts[0].name)) + previous_cohort = course_cohorts[0].name + course_cohorts[0].users.remove(user) cohort.users.add(user) - return user + return (user, previous_cohort) def get_course_cohort_names(course_id): diff --git a/common/djangoapps/course_groups/tests/tests.py b/common/djangoapps/course_groups/tests/test_cohorts.py similarity index 100% rename from common/djangoapps/course_groups/tests/tests.py rename to common/djangoapps/course_groups/tests/test_cohorts.py diff --git a/common/djangoapps/course_groups/tests/test_views.py b/common/djangoapps/course_groups/tests/test_views.py new file mode 100644 index 000000000000..dfd79651710a --- /dev/null +++ b/common/djangoapps/course_groups/tests/test_views.py @@ -0,0 +1,187 @@ +import json + +from django.test.client import RequestFactory +from django.test.utils import override_settings +from factory import post_generation, Sequence +from factory.django import DjangoModelFactory + +from course_groups.models import CourseUserGroup +from course_groups.views import add_users_to_cohort +from courseware.tests.tests import TEST_DATA_MIXED_MODULESTORE +from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory + + +class CohortFactory(DjangoModelFactory): + FACTORY_FOR = CourseUserGroup + + name = Sequence("cohort{}".format) + course_id = "dummy_id" + group_type = CourseUserGroup.COHORT + + @post_generation + def users(self, create, extracted, **kwargs): # pylint: disable=W0613 + self.users.add(*extracted) + + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class AddUsersToCohortTestCase(ModuleStoreTestCase): + def setUp(self): + self.course = CourseFactory.create() + self.staff_user = UserFactory.create(is_staff=True) + self.cohort1_users = [UserFactory.create() for _ in range(3)] + self.cohort2_users = [UserFactory.create() for _ in range(2)] + self.cohort3_users = [UserFactory.create() for _ in range(2)] + self.cohortless_users = [UserFactory.create() for _ in range(3)] + self.cohort1 = CohortFactory.create(course_id=self.course.id, users=self.cohort1_users) + self.cohort2 = CohortFactory.create(course_id=self.course.id, users=self.cohort2_users) + self.cohort3 = CohortFactory.create(course_id=self.course.id, users=self.cohort3_users) + + def check_request( + self, + users_string, + expected_added=None, + expected_changed=None, + expected_present=None, + expected_unknown=None + ): + """ + Check that add_users_to_cohort returns the expected result and has the + expected side effects. The given users will be added to cohort1. + + users_string is the string input entered by the client + + expected_added is a list of users + + expected_changed is a list of (user, previous_cohort) tuples + + expected_present is a list of (user, email/username) tuples where + email/username corresponds to the input + + expected_unknown is a list of strings corresponding to the input + """ + expected_added = expected_added or [] + expected_changed = expected_changed or [] + expected_present = expected_present or [] + expected_unknown = expected_unknown or [] + request = RequestFactory().post("dummy_url", {"users": users_string}) + request.user = self.staff_user + response = add_users_to_cohort(request, self.course.id, self.cohort1.id) + self.assertEqual(response.status_code, 200) + result = json.loads(response.content) + self.assertEqual(result.get("success"), True) + self.assertItemsEqual( + result.get("added"), + [ + {"username": user.username, "name": user.profile.name, "email": user.email} + for user in expected_added + ] + ) + self.assertItemsEqual( + result.get("changed"), + [ + { + "username": user.username, + "name": user.profile.name, + "email": user.email, + "previous_cohort": previous_cohort + } + for (user, previous_cohort) in expected_changed + ] + ) + self.assertItemsEqual( + result.get("present"), + [username_or_email for (_, username_or_email) in expected_present] + ) + self.assertItemsEqual(result.get("unknown"), expected_unknown) + for user in expected_added + [user for (user, _) in expected_changed + expected_present]: + self.assertEqual( + CourseUserGroup.objects.get( + course_id=self.course.id, + group_type=CourseUserGroup.COHORT, + users__id=user.id + ), + self.cohort1 + ) + + def test_empty(self): + self.check_request("") + + def test_only_added(self): + self.check_request( + ",".join([user.username for user in self.cohortless_users]), + expected_added=self.cohortless_users + ) + + def test_only_changed(self): + self.check_request( + ",".join([user.username for user in self.cohort2_users + self.cohort3_users]), + expected_changed=( + [(user, self.cohort2.name) for user in self.cohort2_users] + + [(user, self.cohort3.name) for user in self.cohort3_users] + ) + ) + + def test_only_present(self): + usernames = [user.username for user in self.cohort1_users] + self.check_request( + ",".join(usernames), + expected_present=[(user, user.username) for user in self.cohort1_users] + ) + + def test_only_unknown(self): + usernames = ["unknown_user{}".format(i) for i in range(3)] + self.check_request( + ",".join(usernames), + expected_unknown=usernames + ) + + def test_all(self): + unknowns = ["unknown_user{}".format(i) for i in range(3)] + self.check_request( + ",".join( + unknowns + + [ + user.username + for user in self.cohortless_users + self.cohort1_users + self.cohort2_users + self.cohort3_users + ] + ), + self.cohortless_users, + ( + [(user, self.cohort2.name) for user in self.cohort2_users] + + [(user, self.cohort3.name) for user in self.cohort3_users] + ), + [(user, user.username) for user in self.cohort1_users], + unknowns + ) + + def test_emails(self): + unknown = "unknown_user@example.com" + self.check_request( + ",".join([ + self.cohort1_users[0].email, + self.cohort2_users[0].email, + self.cohortless_users[0].email, + unknown + ]), + [self.cohortless_users[0]], + [(self.cohort2_users[0], self.cohort2.name)], + [(self.cohort1_users[0], self.cohort1_users[0].email)], + [unknown] + ) + + def test_delimiters(self): + unknown = "unknown_user" + self.check_request( + " {} {}\t{}, \r\n{}".format( + unknown, + self.cohort1_users[0].username, + self.cohort2_users[0].username, + self.cohortless_users[0].username + ), + [self.cohortless_users[0]], + [(self.cohort2_users[0], self.cohort2.name)], + [(self.cohort1_users[0], self.cohort1_users[0].username)], + [unknown] + ) diff --git a/common/djangoapps/course_groups/views.py b/common/djangoapps/course_groups/views.py index fb1a99a6aa02..9dc9cf523c09 100644 --- a/common/djangoapps/course_groups/views.py +++ b/common/djangoapps/course_groups/views.py @@ -134,11 +134,13 @@ def add_users_to_cohort(request, course_id, cohort_id): Return json dict of: {'success': True, - 'added': [{'username': username, - 'name': name, - 'email': email}, ...], - 'conflict': [{'username_or_email': ..., - 'msg': ...}], # in another cohort + 'added': [{'username': ..., + 'name': ..., + 'email': ...}, ...], + 'changed': [{'username': ..., + 'name': ..., + 'email': ..., + 'previous_cohort': ...}, ...], 'present': [str1, str2, ...], # already there 'unknown': [str1, str2, ...]} """ @@ -148,29 +150,34 @@ def add_users_to_cohort(request, course_id, cohort_id): users = request.POST.get('users', '') added = [] + changed = [] present = [] - conflict = [] unknown = [] for username_or_email in split_by_comma_and_whitespace(users): + if not username_or_email: + continue + try: - user = cohorts.add_user_to_cohort(cohort, username_or_email) - added.append({'username': user.username, - 'name': "{0} {1}".format(user.first_name, user.last_name), - 'email': user.email, - }) + (user, previous_cohort) = cohorts.add_user_to_cohort(cohort, username_or_email) + info = { + 'username': user.username, + 'name': user.profile.name, + 'email': user.email, + } + if previous_cohort: + info['previous_cohort'] = previous_cohort + changed.append(info) + else: + added.append(info) except ValueError: present.append(username_or_email) except User.DoesNotExist: unknown.append(username_or_email) - except cohorts.CohortConflict as err: - conflict.append({'username_or_email': username_or_email, - 'msg': str(err)}) - return json_http_response({'success': True, 'added': added, + 'changed': changed, 'present': present, - 'conflict': conflict, 'unknown': unknown}) diff --git a/common/static/js/course_groups/cohorts.js b/common/static/js/course_groups/cohorts.js index 2c9904ef4003..2707b7693185 100644 --- a/common/static/js/course_groups/cohorts.js +++ b/common/static/js/course_groups/cohorts.js @@ -160,32 +160,43 @@ var CohortManager = (function ($) { log_error(response.msg || "There was an error loading users for " + cohort.title); } + op_results.empty(); detail.show(); } function added_users(response) { - function adder(note, color) { - return function(item) { - var li = $('
  • ') - if (typeof item === "object" && item.username) { - li.text(note + ' ' + item.name + ', ' + item.username + ', ' + item.email); - } else if (typeof item === "object" && item.msg) { - li.text(note + ' ' + item.username_or_email + ', ' + item.msg); - } else { - // string - li.text(note + ' ' + item); - } - li.css('color', color); - op_results.append(li); - } - } op_results.empty(); if (response && response.success) { - response.added.forEach(adder("Added", "green")); - response.present.forEach(adder("Already present:", "black")); - response.conflict.forEach(adder("In another cohort:", "purple")); - response.unknown.forEach(adder("Unknown user:", "red")); + function add_to_list(text, color) { + op_results.append($("
  • ").text(text).css("color", color)); + } + response.added.forEach( + function(item) { + add_to_list( + "Added: " + item.name + ", " + item.username + ", " + item.email, + "green" + ); + } + ); + response.changed.forEach( + function(item) { + add_to_list( + "Moved from cohort " + item.previous_cohort + ": " + item.name + ", " + item.username + ", " + item.email, + "purple" + ) + } + ); + response.present.forEach( + function(item) { + add_to_list("Already present: " + item, "black"); + } + ); + response.unknown.forEach( + function(item) { + add_to_list("Unknown user: " + item, "red") + } + ); users_area.val('') } else { log_error(response.msg || "There was an error adding users");