Merge pull request #2364 from edx/zub/bugfix/std1140-seedforumpermissions

assign default role 'Student' to user on recreating course

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)

cms/djangoapps/contentstore/tests/test_users_default_role.py

+"""
+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)

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

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

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):

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)

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

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