Ensure LMS passes correct group_id when querying content

common/test/acceptance/tests/discussion/test_cohorts.py

@@ -62,6 +62,8 @@ def setup_cohorts(self):
         self.cohort_1_id = self.add_cohort(self.cohort_1_name)
 
     def test_cohort_visibility_label(self):
+        # Must be moderator to view content in a cohort other than your own
+        AutoAuthPage(self.browser, course_id=self.course_id, roles="Moderator").visit()
         self.setup_thread(1, group_id=self.cohort_1_id)
         self.assertEquals(
             self.thread_page.get_group_visibility_label(),
@@ -128,13 +130,11 @@ def setUp(self):
 
         self.user_id = AutoAuthPage(self.browser, course_id=self.course_id).visit().get_user_id()
 
-        self.courseware_page = CoursewarePage(self.browser, self.course_id)
-        self.courseware_page.visit()
-        self.discussion_page = InlineDiscussionPage(self.browser, self.discussion_id)
-
     def setup_thread_page(self, thread_id):
-        self.discussion_page.expand_discussion()
-        self.assertEqual(self.discussion_page.get_num_displayed_threads(), 1)
+        CoursewarePage(self.browser, self.course_id).visit()
+        discussion_page = InlineDiscussionPage(self.browser, self.discussion_id)
+        discussion_page.expand_discussion()
+        self.assertEqual(discussion_page.get_num_displayed_threads(), 1)
         self.thread_page = InlineDiscussionThreadPage(self.browser, thread_id)  # pylint:disable=W0201
         self.thread_page.expand()
 

lms/djangoapps/django_comment_client/base/tests.py

@@ -12,6 +12,7 @@
 
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from django_comment_client.base import views
+from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin
 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
@@ -36,111 +37,27 @@ def _set_mock_request_data(self, mock_request, 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)
-
+class CreateThreadGroupIdTestCase(
+        CohortedContentTestCase,
+        CohortedTopicGroupIdTestMixin,
+        NonCohortedTopicGroupIdTestMixin
+):
+    cs_endpoint = "/threads"
+
+    def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
+        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"
 
-@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)
+        return views.create_thread(
+            request,
+            course_id=self.course.id.to_deprecated_string(),
+            commentable_id=commentable_id
+        )
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)

lms/djangoapps/django_comment_client/base/views.py

@@ -28,7 +28,9 @@
     JsonError,
     JsonResponse,
     safe_content,
-    add_thread_group_name)
+    add_thread_group_name,
+    get_group_id_for_comments_service
+)
 from django_comment_client.permissions import check_permissions_by_view, cached_has_permission
 import lms.lib.comment_client as cc
 
@@ -102,32 +104,13 @@ def create_thread(request, course_id, commentable_id):
         title=post["title"]
     )
 
-    user = cc.User.from_django_user(request.user)
-
-    #kevinchugh because the new requirement is that all groups will be determined
-    #by the group id in the request this all goes away
-    #not anymore, only for admins
-
-    # Cohort the thread if the commentable is cohorted.
-    if is_commentable_cohorted(course_key, commentable_id):
-        user_group_id = get_cohort_id(user, course_key)
-
-        # TODO (vshnayder): once we have more than just cohorts, we'll want to
-        # change this to a single get_group_for_user_and_commentable function
-        # 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
-            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
-
-        if group_id:
-            thread.group_id = group_id
+    # Cohort the thread if required
+    try:
+        group_id = get_group_id_for_comments_service(request, course_key, commentable_id)
+    except ValueError:
+        return HttpResponseBadRequest("Invalid cohort id")
+    if group_id is not None:
+        thread.group_id = group_id
 
     thread.save()