From 969bfa59166edcfe551a4d8e1348c3607e4fef56 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Wed, 18 Feb 2015 17:11:38 +0500 Subject: [PATCH] use 'commit_on_success' transaction decorator to avoid registering users in multiple cohort groups of a particular course TNL-1331 --- .../core/djangoapps/course_groups/cohorts.py | 2 + .../course_groups/management/__init__.py | 0 .../management/commands/__init__.py | 0 .../remove_users_from_multiple_cohorts.py | 56 +++++++++++++ ...test_remove_users_from_multiple_cohorts.py | 82 +++++++++++++++++++ 5 files changed, 140 insertions(+) create mode 100644 openedx/core/djangoapps/course_groups/management/__init__.py create mode 100644 openedx/core/djangoapps/course_groups/management/commands/__init__.py create mode 100644 openedx/core/djangoapps/course_groups/management/commands/remove_users_from_multiple_cohorts.py create mode 100644 openedx/core/djangoapps/course_groups/management/commands/tests/test_remove_users_from_multiple_cohorts.py diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index ed98ebf80e..442ea506f4 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -6,6 +6,7 @@ forums, and to the cohort admin views. import logging import random +from django.db import transaction from django.db.models.signals import post_save, m2m_changed from django.dispatch import receiver from django.http import Http404 @@ -197,6 +198,7 @@ def get_cohorted_commentables(course_key): return ans +@transaction.commit_on_success def get_cohort(user, course_key, assign=True): """ Given a Django user and a CourseKey, return the user's cohort in that diff --git a/openedx/core/djangoapps/course_groups/management/__init__.py b/openedx/core/djangoapps/course_groups/management/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/course_groups/management/commands/__init__.py b/openedx/core/djangoapps/course_groups/management/commands/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/course_groups/management/commands/remove_users_from_multiple_cohorts.py b/openedx/core/djangoapps/course_groups/management/commands/remove_users_from_multiple_cohorts.py new file mode 100644 index 0000000000..46a753ea41 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/management/commands/remove_users_from_multiple_cohorts.py @@ -0,0 +1,56 @@ +""" +Script for removing users with multiple cohorts of a course from cohorts +to ensure user's uniqueness for a course cohorts +""" +from django.contrib.auth.models import User +from django.core.management.base import BaseCommand +from django.db.models import Count + +from openedx.core.djangoapps.course_groups.models import CourseUserGroup + + +class Command(BaseCommand): + """ + Remove users with multiple cohorts of a course from all cohorts + """ + help = 'Remove all users from multiple cohorts (except one) of each course' + + def handle(self, *args, **options): + """ + Execute the command + """ + # Get entries of cohorts which have same user added multiple times for a single course + multiple_objects_cohorts = CourseUserGroup.objects.filter(group_type=CourseUserGroup.COHORT).\ + values_list('users', 'course_id').annotate(user_count=Count('users')).filter(user_count__gt=1).\ + order_by('users') + multiple_objects_cohorts_count = multiple_objects_cohorts.count() + multiple_course_cohorts_users = set(multiple_objects_cohorts.values_list('users', flat=True)) + users_failed_to_cleanup = [] + + for user in User.objects.filter(id__in=multiple_course_cohorts_users): + print u"Removing user with id '{0}' from cohort groups".format(user.id) + try: + # remove user from only cohorts + user.course_groups.remove(*user.course_groups.filter(group_type=CourseUserGroup.COHORT)) + except AttributeError as err: + users_failed_to_cleanup.append(user.email) + print u"Failed to remove user with id {0} from cohort groups, error: {1}".format(user.id, err) + + print "=" * 80 + print u"=" * 30 + u"> Cohorts summary" + print( + u"Total number of CourseUserGroup of type '{0}' with multiple users: {1}".format( + CourseUserGroup.COHORT, multiple_objects_cohorts_count + ) + ) + print( + u"Total number of unique users with multiple course cohorts: {0}".format( + len(multiple_course_cohorts_users) + ) + ) + print( + u"Users which failed on cohorts cleanup [{0}]: [{1}]".format( + len(users_failed_to_cleanup), (', '.join(users_failed_to_cleanup)) + ) + ) + print "=" * 80 diff --git a/openedx/core/djangoapps/course_groups/management/commands/tests/test_remove_users_from_multiple_cohorts.py b/openedx/core/djangoapps/course_groups/management/commands/tests/test_remove_users_from_multiple_cohorts.py new file mode 100644 index 0000000000..afd233b046 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/management/commands/tests/test_remove_users_from_multiple_cohorts.py @@ -0,0 +1,82 @@ +""" +Tests for cleanup of users which are added in multiple cohorts of a course +""" +from django.core.exceptions import MultipleObjectsReturned +from django.core.management import call_command +from django.test.client import RequestFactory + +from openedx.core.djangoapps.course_groups.views import cohort_handler +from openedx.core.djangoapps.course_groups.cohorts import get_cohort, get_cohort_by_name +from openedx.core.djangoapps.course_groups.tests.helpers import config_course_cohorts +from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory + + +class TestMultipleCohortUsers(ModuleStoreTestCase): + """ + Base class for testing users with multiple cohorts + """ + def setUp(self): + """ + setup course, user and request for tests + """ + super(TestMultipleCohortUsers, self).setUp() + self.course1 = CourseFactory.create() + self.course2 = CourseFactory.create() + self.user1 = UserFactory(is_staff=True) + self.user2 = UserFactory(is_staff=True) + self.request = RequestFactory().get("dummy_url") + self.request.user = self.user1 + + def test_users_with_multiple_cohorts_cleanup(self): + """ + Test that user which have been added in multiple cohorts of a course, + can get cohorts without error after running cohorts cleanup command + """ + # set two auto_cohort_groups for both courses + config_course_cohorts( + self.course1, [], cohorted=True, auto_cohort_groups=["Course1AutoGroup1", "Course1AutoGroup2"] + ) + config_course_cohorts( + self.course2, [], cohorted=True, auto_cohort_groups=["Course2AutoGroup1", "Course2AutoGroup2"] + ) + + # get the cohorts from the courses, which will cause auto cohorts to be created + cohort_handler(self.request, unicode(self.course1.id)) + cohort_handler(self.request, unicode(self.course2.id)) + course_1_auto_cohort_1 = get_cohort_by_name(self.course1.id, "Course1AutoGroup1") + course_1_auto_cohort_2 = get_cohort_by_name(self.course1.id, "Course1AutoGroup2") + course_2_auto_cohort_1 = get_cohort_by_name(self.course2.id, "Course2AutoGroup1") + + # forcefully add user1 in two auto cohorts + course_1_auto_cohort_1.users.add(self.user1) + course_1_auto_cohort_2.users.add(self.user1) + # forcefully add user2 in auto cohorts of both courses + course_1_auto_cohort_1.users.add(self.user2) + course_2_auto_cohort_1.users.add(self.user2) + + # now check that when user1 goes on discussion page and tries to get + # cohorts 'MultipleObjectsReturned' exception is returned + with self.assertRaises(MultipleObjectsReturned): + get_cohort(self.user1, self.course1.id) + # also check that user 2 can go on discussion page of both courses + # without any exception + get_cohort(self.user2, self.course1.id) + get_cohort(self.user2, self.course2.id) + + # call command to remove users added in multiple cohorts of a course + # are removed from all cohort groups + call_command('remove_users_from_multiple_cohorts') + + # check that only user1 (with multiple cohorts) is removed from cohorts + # and user2 is still in auto cohorts of both course after running + # 'remove_users_from_multiple_cohorts' management command + self.assertEqual(self.user1.course_groups.count(), 0) + self.assertEqual(self.user2.course_groups.count(), 2) + user2_cohorts = list(self.user2.course_groups.values_list('name', flat=True)) + self.assertEqual(user2_cohorts, ['Course1AutoGroup1', 'Course2AutoGroup1']) + + # now check that user1 can get cohorts in which he is added + response = cohort_handler(self.request, unicode(self.course1.id)) + self.assertEqual(response.status_code, 200)