Merge pull request #5026 from edx/dan-f/test-lms-passes-group-id

Test passing `group_id` in thread creation

lms/djangoapps/django_comment_client/base/tests.py

@@ -12,6 +12,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from django_comment_client.base import views
+from django_comment_client.tests.utils import CohortedContentTestCase
 from django_comment_client.tests.unicode import UnicodeTestMixin
 from django_comment_common.models import Role, FORUM_ROLE_STUDENT
 from django_comment_common.utils import seed_permissions_roles
@@ -20,6 +21,7 @@ from util.testing import UrlResetMixin
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
+
 log = logging.getLogger(__name__)
 
 CS_PREFIX = "http://localhost:4567/api/v1"
@@ -33,6 +35,114 @@ class MockRequestSetupMixin(object):
         mock_request.return_value = self._create_repsonse_mock(data)
 
 
+@patch('lms.lib.comment_client.utils.requests.request')
+class CreateCohortedThreadTestCase(CohortedContentTestCase):
+    """
+    Tests how `views.create_thread` passes `group_id` to the comments service
+    for cohorted topics.
+    """
+    def _create_thread_in_cohorted_topic(
+            self,
+            user,
+            mock_request,
+            group_id,
+            pass_group_id=True,
+            expected_status_code=200
+    ):
+        self._create_thread(user, "cohorted_topic", mock_request, group_id, pass_group_id, expected_status_code)
+
+    def test_student_without_group_id(self, mock_request):
+        self._create_thread_in_cohorted_topic(self.student, mock_request, None, pass_group_id=False)
+        self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_student_none_group_id(self, mock_request):
+        self._create_thread_in_cohorted_topic(self.student, mock_request, None)
+        self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_student_with_own_group_id(self, mock_request):
+        self._create_thread_in_cohorted_topic(self.student, mock_request, self.student_cohort.id)
+        self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_student_with_other_group_id(self, mock_request):
+        self._create_thread_in_cohorted_topic(self.student, mock_request, self.moderator_cohort.id)
+        self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_moderator_without_group_id(self, mock_request):
+        self._create_thread_in_cohorted_topic(self.moderator, mock_request, None, pass_group_id=False)
+        self._assert_mock_request_called_with_group_id(mock_request, self.moderator_cohort.id)
+
+    def test_moderator_none_group_id(self, mock_request):
+        self._create_thread_in_cohorted_topic(self.moderator, mock_request, None, expected_status_code=400)
+        self.assertFalse(mock_request.called)
+
+    def test_moderator_with_own_group_id(self, mock_request):
+        self._create_thread_in_cohorted_topic(self.moderator, mock_request, self.moderator_cohort.id)
+        self._assert_mock_request_called_with_group_id(mock_request, self.moderator_cohort.id)
+
+    def test_moderator_with_other_group_id(self, mock_request):
+        self._create_thread_in_cohorted_topic(self.moderator, mock_request, self.student_cohort.id)
+        self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_moderator_with_invalid_group_id(self, mock_request):
+        invalid_id = self.student_cohort.id + self.moderator_cohort.id
+        self._create_thread_in_cohorted_topic(self.moderator, mock_request, invalid_id, expected_status_code=400)
+        self.assertFalse(mock_request.called)
+
+
+@patch('lms.lib.comment_client.utils.requests.request')
+class CreateNonCohortedThreadTestCase(CohortedContentTestCase):
+    """
+    Tests how `views.create_thread` passes `group_id` to the comments service
+    for non-cohorted topics.
+    """
+    def _create_thread_in_non_cohorted_topic(
+            self,
+            user,
+            mock_request,
+            group_id,
+            pass_group_id=True,
+            expected_status_code=200
+    ):
+        self._create_thread(user, "non_cohorted_topic", mock_request, group_id, pass_group_id, expected_status_code)
+
+    def test_student_without_group_id(self, mock_request):
+        self._create_thread_in_non_cohorted_topic(self.student, mock_request, None, pass_group_id=False)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+    def test_student_none_group_id(self, mock_request):
+        self._create_thread_in_non_cohorted_topic(self.student, mock_request, None)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+    def test_student_with_own_group_id(self, mock_request):
+        self._create_thread_in_non_cohorted_topic(self.student, mock_request, self.student_cohort.id)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+    def test_student_with_other_group_id(self, mock_request):
+        self._create_thread_in_non_cohorted_topic(self.student, mock_request, self.moderator_cohort.id)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+    def test_moderator_without_group_id(self, mock_request):
+        self._create_thread_in_non_cohorted_topic(self.moderator, mock_request, None, pass_group_id=False)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+    def test_moderator_none_group_id(self, mock_request):
+        self._create_thread_in_non_cohorted_topic(self.student, mock_request, None)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+    def test_moderator_with_own_group_id(self, mock_request):
+        self._create_thread_in_non_cohorted_topic(self.moderator, mock_request, self.moderator_cohort.id)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+    def test_moderator_with_other_group_id(self, mock_request):
+        self._create_thread_in_non_cohorted_topic(self.moderator, mock_request, self.student_cohort.id)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+    def test_moderator_with_invalid_group_id(self, mock_request):
+        invalid_id = self.student_cohort.id + self.moderator_cohort.id
+        self._create_thread_in_non_cohorted_topic(self.moderator, mock_request, invalid_id)
+        self._assert_mock_request_called_without_group_id(mock_request)
+
+
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 @patch('lms.lib.comment_client.utils.requests.request')
 class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
@@ -575,6 +685,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):
 
         assert_equal(response.status_code, 200)
 
+
 @patch("lms.lib.comment_client.utils.requests.request")
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin):

lms/djangoapps/django_comment_client/base/views.py

@@ -9,7 +9,7 @@ from django.contrib.auth.decorators import login_required
 from django.contrib.auth.models import User
 from django.core import exceptions
 from django.core.files.storage import get_storage_class
-from django.http import Http404
+from django.http import Http404, HttpResponseBadRequest
 from django.utils.translation import ugettext as _
 from django.views.decorators import csrf
 from django.views.decorators.http import require_GET, require_POST
@@ -18,7 +18,8 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
 from courseware.access import has_access
 from courseware.courses import get_course_with_access, get_course_by_id
-from course_groups.cohorts import get_cohort_id, is_commentable_cohorted
+from course_groups.models import CourseUserGroup
+from course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted
 import django_comment_client.settings as cc_settings
 from django_comment_client.utils import (
     add_courseware_context,
@@ -116,7 +117,11 @@ def create_thread(request, course_id, commentable_id):
         # that can do different things depending on the commentable_id
         if cached_has_permission(request.user, "see_all_cohorts", course_key):
             # admins can optionally choose what group to post as
-            group_id = post.get('group_id', user_group_id)
+            try:
+                group_id = int(post.get('group_id', user_group_id))
+                get_cohort_by_id(course_key, group_id)
+            except (ValueError, CourseUserGroup.DoesNotExist):
+                return HttpResponseBadRequest("Invalid cohort id")
         else:
             # regular users always post with their own id.
             group_id = user_group_id

lms/djangoapps/django_comment_client/tests/utils.py

@@ -0,0 +1,83 @@
+from django.test.client import RequestFactory
+from django.test.utils import override_settings
+
+from course_groups.models import CourseUserGroup
+from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from django_comment_client import base
+from django_comment_common.models import Role
+from django_comment_common.utils import seed_permissions_roles
+from mock import patch
+from student.tests.factories import CourseEnrollmentFactory, UserFactory
+from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+class CohortedContentTestCase(ModuleStoreTestCase):
+    """
+    Sets up a course with a student, a moderator and their cohorts.
+    """
+    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
+    def setUp(self):
+        super(CohortedContentTestCase, self).setUp()
+
+        self.course = CourseFactory.create(
+            discussion_topics={
+                "cohorted topic": {"id": "cohorted_topic"},
+                "non-cohorted topic": {"id": "non_cohorted_topic"},
+            },
+            cohort_config={
+                "cohorted": True,
+                "cohorted_discussions": ["cohorted_topic"]
+            }
+        )
+        self.student_cohort = CourseUserGroup.objects.create(
+            name="student_cohort",
+            course_id=self.course.id,
+            group_type=CourseUserGroup.COHORT
+        )
+        self.moderator_cohort = CourseUserGroup.objects.create(
+            name="moderator_cohort",
+            course_id=self.course.id,
+            group_type=CourseUserGroup.COHORT
+        )
+
+        seed_permissions_roles(self.course.id)
+        self.student = UserFactory.create()
+        self.moderator = UserFactory.create()
+        CourseEnrollmentFactory(user=self.student, course_id=self.course.id)
+        CourseEnrollmentFactory(user=self.moderator, course_id=self.course.id)
+        self.moderator.roles.add(Role.objects.get(name="Moderator", course_id=self.course.id))
+        self.student_cohort.users.add(self.student)
+        self.moderator_cohort.users.add(self.moderator)
+
+    def _create_thread(
+            self,
+            user,
+            commentable_id,
+            mock_request,
+            group_id,
+            pass_group_id=True,
+            expected_status_code=200
+    ):
+        mock_request.return_value.status_code = 200
+        request_data = {"body": "body", "title": "title", "thread_type": "discussion"}
+        if pass_group_id:
+            request_data["group_id"] = group_id
+        request = RequestFactory().post("dummy_url", request_data)
+        request.user = user
+        request.view_name = "create_thread"
+
+        response = base.views.create_thread(
+            request,
+            course_id=self.course.id.to_deprecated_string(),
+            commentable_id=commentable_id
+        )
+        self.assertEqual(response.status_code, expected_status_code)
+
+    def _assert_mock_request_called_with_group_id(self, mock_request, group_id):
+        self.assertTrue(mock_request.called)
+        self.assertEqual(mock_request.call_args[1]["data"]["group_id"], group_id)
+
+    def _assert_mock_request_called_without_group_id(self, mock_request):
+        self.assertTrue(mock_request.called)
+        self.assertNotIn("group_id", mock_request.call_args[1]["data"])