diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 8d44b98d95..04b263474c 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1377,7 +1377,9 @@ class ContentStoreTest(ModuleStoreTestCase): course_id = _get_course_id(test_course_data) self.assertTrue(are_permissions_roles_seeded(course_id)) delete_course_and_groups(course_id, commit=True) - self.assertFalse(are_permissions_roles_seeded(course_id)) + # should raise an exception for checking permissions on deleted course + with self.assertRaises(ItemNotFoundError): + are_permissions_roles_seeded(course_id) def test_forum_unseeding_with_multiple_courses(self): """Test new course creation and verify forum unseeding when there are multiple courses""" @@ -1387,12 +1389,31 @@ class ContentStoreTest(ModuleStoreTestCase): # unseed the forums for the first course course_id = _get_course_id(test_course_data) delete_course_and_groups(course_id, commit=True) - self.assertFalse(are_permissions_roles_seeded(course_id)) + # should raise an exception for checking permissions on deleted course + with self.assertRaises(ItemNotFoundError): + are_permissions_roles_seeded(course_id) second_course_id = _get_course_id(second_course_data) # permissions should still be there for the other course self.assertTrue(are_permissions_roles_seeded(second_course_id)) + def test_course_enrollments_and_roles_on_delete(self): + """ + Test that course deletion doesn't remove course enrollments or user's roles + """ + test_course_data = self.assert_created_course(number_suffix=uuid4().hex) + course_id = _get_course_id(test_course_data) + + # test that a user gets his enrollment and its 'student' role as default on creating a course + self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id)) + self.assertTrue(self.user.roles.filter(name="Student", course_id=course_id)) # pylint: disable=no-member + + delete_course_and_groups(course_id, commit=True) + # check that user's enrollment for this course is not deleted + self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id)) + # check that user has form role "Student" for this course even after deleting it + self.assertTrue(self.user.roles.filter(name="Student", course_id=course_id)) # pylint: disable=no-member + def test_create_course_duplicate_course(self): """Test new course creation - error path""" self.client.ajax_post('/course', self.course_data) diff --git a/cms/djangoapps/contentstore/tests/test_users_default_role.py b/cms/djangoapps/contentstore/tests/test_users_default_role.py new file mode 100644 index 0000000000..1ca97d1906 --- /dev/null +++ b/cms/djangoapps/contentstore/tests/test_users_default_role.py @@ -0,0 +1,125 @@ +""" +Unit tests for checking default forum role "Student" of a user when he creates a course or +after deleting it creates same course again +""" +from contentstore.tests.utils import AjaxEnabledTestClient +from contentstore.utils import delete_course_and_groups +from courseware.tests.factories import UserFactory +from xmodule.modulestore import Location +from xmodule.modulestore.django import loc_mapper +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + +from student.models import CourseEnrollment + + +class TestUsersDefaultRole(ModuleStoreTestCase): + """ + Unit tests for checking enrollment and default forum role "Student" of a logged in user + """ + def setUp(self): + """ + Add a user and a course + """ + super(TestUsersDefaultRole, self).setUp() + # create and log in a staff user. + self.user = UserFactory(is_staff=True) # pylint: disable=no-member + self.client = AjaxEnabledTestClient() + self.client.login(username=self.user.username, password='test') + + # create a course via the view handler to create course + self.course_location = Location(['i4x', 'Org_1', 'Course_1', 'course', 'Run_1']) + self._create_course_with_given_location(self.course_location) + + def _create_course_with_given_location(self, course_location): + """ + Create course at provided location + """ + course_locator = loc_mapper().translate_location( + course_location.course_id, course_location, False, True + ) + resp = self.client.ajax_post( + course_locator.url_reverse('course'), + { + 'org': course_location.org, + 'number': course_location.course, + 'display_name': 'test course', + 'run': course_location.name, + } + ) + return resp + + def tearDown(self): + """ + Reverse the setup + """ + self.client.logout() + super(TestUsersDefaultRole, self).tearDown() + + def test_user_forum_default_role_on_course_deletion(self): + """ + Test that a user enrolls and gets "Student" forum role for that course which he creates and remains + enrolled even the course is deleted and keeps its "Student" forum role for that course + """ + course_id = self.course_location.course_id + # check that user has enrollment for this course + self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id)) + + # check that user has his default "Student" forum role for this course + self.assertTrue(self.user.roles.filter(name="Student", course_id=course_id)) # pylint: disable=no-member + + delete_course_and_groups(course_id, commit=True) + + # check that user's enrollment for this course is not deleted + self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id)) + + # check that user has forum role for this course even after deleting it + self.assertTrue(self.user.roles.filter(name="Student", course_id=course_id)) # pylint: disable=no-member + + def test_user_role_on_course_recreate(self): + """ + Test that creating same course again after deleting it gives user his default + forum role "Student" for that course + """ + course_id = self.course_location.course_id + # check that user has enrollment and his default "Student" forum role for this course + self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id)) + self.assertTrue(self.user.roles.filter(name="Student", course_id=course_id)) # pylint: disable=no-member + + # delete this course and recreate this course with same user + delete_course_and_groups(course_id, commit=True) + resp = self._create_course_with_given_location(self.course_location) + self.assertEqual(resp.status_code, 200) + + # check that user has his enrollment for this course + self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_id)) + + # check that user has his default "Student" forum role for this course + self.assertTrue(self.user.roles.filter(name="Student", course_id=course_id)) # pylint: disable=no-member + + def test_user_role_on_course_recreate_with_change_name_case(self): + """ + Test that creating same course again with different name case after deleting it gives user + his default forum role "Student" for that course + """ + course_location = self.course_location + # check that user has enrollment and his default "Student" forum role for this course + self.assertTrue(CourseEnrollment.is_enrolled(self.user, course_location.course_id)) + # delete this course and recreate this course with same user + delete_course_and_groups(course_location.course_id, commit=True) + + # now create same course with different name case ('uppercase') + new_course_location = Location( + ['i4x', course_location.org, course_location.course.upper(), 'course', course_location.name] + ) + resp = self._create_course_with_given_location(new_course_location) + self.assertEqual(resp.status_code, 200) + + # check that user has his default "Student" forum role again for this course (with changed name case) + self.assertTrue( + self.user.roles.filter(name="Student", course_id=new_course_location.course_id) # pylint: disable=no-member + ) + + # Disabled due to case-sensitive test db (sqlite3) + # # check that there user has only one "Student" forum role (with new updated course_id) + # self.assertEqual(self.user.roles.filter(name='Student').count(), 1) # pylint: disable=no-member + # self.assertEqual(self.user.roles.filter(name='Student')[0].course_id, new_course_location.course_id) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index b152880732..c93efa34c2 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -40,8 +40,6 @@ def delete_course_and_groups(course_id, commit=False): loc = CourseDescriptor.id_to_location(course_id) if delete_course(module_store, content_store, loc, commit): - print 'removing forums permissions and roles...' - unseed_permissions_roles(course_id) print 'removing User permissions from course....' # in the django layer, we need to remove all the user permissions groups associated with this course diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index ad8b239fd7..9b8efd4c39 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -43,6 +43,7 @@ from .component import ( OPEN_ENDED_COMPONENT_TYPES, NOTE_COMPONENT_TYPES, ADVANCED_COMPONENT_POLICY_KEY) +from django_comment_common.models import assign_default_role from django_comment_common.utils import seed_permissions_roles from student.models import CourseEnrollment @@ -206,7 +207,7 @@ def _accessible_courses_list_from_groups(request): course_ids.add(course_id.replace('/', '.').lower()) for course_id in course_ids: - # get course_location with lowercase idget_item + # get course_location with lowercase id course_location = loc_mapper().translate_locator_to_location( CourseLocator(package_id=course_id), get_course=True, lower_only=True ) @@ -407,10 +408,20 @@ def create_new_course(request): # auto-enroll the course creator in the course so that "View Live" will # work. CourseEnrollment.enroll(request.user, new_course.location.course_id) + _users_assign_default_role(new_course.location) return JsonResponse({'url': new_location.url_reverse("course/", "")}) +def _users_assign_default_role(course_location): + """ + Assign 'Student' role to all previous users (if any) for this course + """ + enrollments = CourseEnrollment.objects.filter(course_id=course_location.course_id) + for enrollment in enrollments: + assign_default_role(course_location.course_id, enrollment.user) + + # pylint: disable=unused-argument @login_required @ensure_csrf_cookie diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index 9c71c12561..0479b7ab28 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -18,7 +18,10 @@ FORUM_ROLE_STUDENT = ugettext_noop('Student') @receiver(post_save, sender=CourseEnrollment) -def assign_default_role(sender, instance, **kwargs): +def assign_default_role_on_enrollment(sender, instance, **kwargs): + """ + Assign forum default role 'Student' + """ # The code below would remove all forum Roles from a user when they unenroll # from a course. Concerns were raised that it should apply only to students, # or that even the history of student roles is important for research @@ -33,8 +36,15 @@ def assign_default_role(sender, instance, **kwargs): # return # We've enrolled the student, so make sure they have the Student role - role = Role.objects.get_or_create(course_id=instance.course_id, name="Student")[0] - instance.user.roles.add(role) + assign_default_role(instance.course_id, instance.user) + + +def assign_default_role(course_id, user): + """ + Assign forum default role 'Student' to user + """ + role, __ = Role.objects.get_or_create(course_id=course_id, name="Student") + user.roles.add(role) class Role(models.Model): diff --git a/common/djangoapps/django_comment_common/utils.py b/common/djangoapps/django_comment_common/utils.py index 388474395c..75da2453dc 100644 --- a/common/djangoapps/django_comment_common/utils.py +++ b/common/djangoapps/django_comment_common/utils.py @@ -9,11 +9,28 @@ _MODERATOR_ROLE_PERMISSIONS = ["edit_content", "delete_thread", "openclose_threa _ADMINISTRATOR_ROLE_PERMISSIONS = ["manage_moderator"] + +def _save_forum_role(course_id, name): + """ + Save and Update 'course_id' for all roles which are already created to keep course_id same + as actual passed course id + """ + role, created = Role.objects.get_or_create(name=name, course_id=course_id) + if created is False: + role.course_id = course_id + role.save() + + return role + + def seed_permissions_roles(course_id): - administrator_role = Role.objects.get_or_create(name="Administrator", course_id=course_id)[0] - moderator_role = Role.objects.get_or_create(name="Moderator", course_id=course_id)[0] - community_ta_role = Role.objects.get_or_create(name="Community TA", course_id=course_id)[0] - student_role = Role.objects.get_or_create(name="Student", course_id=course_id)[0] + """ + Create and assign permissions for forum roles + """ + administrator_role = _save_forum_role(course_id, "Administrator") + moderator_role = _save_forum_role(course_id, "Moderator") + community_ta_role = _save_forum_role(course_id, "Community TA") + student_role = _save_forum_role(course_id, "Student") for per in _STUDENT_ROLE_PERMISSIONS: student_role.add_permission(per) diff --git a/lms/djangoapps/django_comment_client/management/commands/assign_roles_for_course.py b/lms/djangoapps/django_comment_client/management/commands/assign_roles_for_course.py index 2a58e370af..4701774a2e 100644 --- a/lms/djangoapps/django_comment_client/management/commands/assign_roles_for_course.py +++ b/lms/djangoapps/django_comment_client/management/commands/assign_roles_for_course.py @@ -7,7 +7,7 @@ Enrollments. from django.core.management.base import BaseCommand, CommandError from student.models import CourseEnrollment -from django_comment_common.models import assign_default_role +from django_comment_common.models import assign_default_role_on_enrollment class Command(BaseCommand): @@ -23,7 +23,7 @@ class Command(BaseCommand): print "Updated roles for ", for i, enrollment in enumerate(CourseEnrollment.objects.filter(course_id=course_id, is_active=1), start=1): - assign_default_role(None, enrollment) + assign_default_role_on_enrollment(None, enrollment) if i % 1000 == 0: print "{0}...".format(i), print diff --git a/lms/djangoapps/django_comment_client/management/commands/create_roles_for_existing.py b/lms/djangoapps/django_comment_client/management/commands/create_roles_for_existing.py index 0516c61c7c..575d9666e1 100644 --- a/lms/djangoapps/django_comment_client/management/commands/create_roles_for_existing.py +++ b/lms/djangoapps/django_comment_client/management/commands/create_roles_for_existing.py @@ -7,7 +7,7 @@ Enrollments. from django.core.management.base import BaseCommand, CommandError from student.models import CourseEnrollment -from django_comment_common.models import assign_default_role +from django_comment_common.models import assign_default_role_on_enrollment class Command(BaseCommand): @@ -20,7 +20,7 @@ class Command(BaseCommand): print "Updated roles for ", for i, enrollment in enumerate(CourseEnrollment.objects.filter(is_active=1), start=1): - assign_default_role(None, enrollment) + assign_default_role_on_enrollment(None, enrollment) if i % 1000 == 0: print "{0}...".format(i), print