Ensure LMS passes correct group_id when querying content

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

@@ -62,6 +62,8 @@ class CohortedDiscussionTestMixin(BaseDiscussionMixin):
         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 @@ class InlineDiscussionTest(UniqueCourseTest):
 
         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

@@ -13,6 +13,7 @@ from lms import startup
 
 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
@@ -43,111 +44,27 @@ class MockRequestSetupMixin(object):
 
 
 @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

@@ -38,7 +38,8 @@ from django_comment_client.utils import (
     get_discussion_categories_ids,
     get_discussion_categories_ids,
     permalink,
-    add_thread_group_name
+    add_thread_group_name,
+    get_group_id_for_comments_service
 )
 from util.html import strip_tags
 from django_comment_client.permissions import check_permissions_by_view, cached_has_permission
@@ -136,32 +137,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()
     event_data = {'forum_id': commentable_id, 'thread_id': thread.id}

lms/djangoapps/django_comment_client/forum/tests.py

@@ -10,7 +10,13 @@ from edxmako.tests import mako_middleware_process_request
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from django.core.urlresolvers import reverse
 from util.testing import UrlResetMixin
+from django_comment_client.tests.group_id import (
+    GroupIdAssertionMixin,
+    CohortedTopicGroupIdTestMixin,
+    NonCohortedTopicGroupIdTestMixin
+)
 from django_comment_client.tests.unicode import UnicodeTestMixin
+from django_comment_client.tests.utils import CohortedContentTestCase
 from django_comment_client.forum import views
 
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
@@ -267,17 +273,7 @@ class SingleThreadTestCase(ModuleStoreTestCase):
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 @patch('requests.request')
-class SingleCohortedThreadTestCase(ModuleStoreTestCase):
-    def setUp(self):
-        self.course = CourseFactory.create()
-        self.student = UserFactory.create()
-        CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id)
-        self.student_cohort = CourseUserGroup.objects.create(
-            name="student_cohort",
-            course_id=self.course.id,
-            group_type=CourseUserGroup.COHORT
-        )
-
+class SingleCohortedThreadTestCase(CohortedContentTestCase):
     def _create_mock_cohorted_thread(self, mock_request):
         self.mock_text = "dummy content"
         self.mock_thread_id = "test_thread_id"
@@ -333,6 +329,197 @@ class SingleCohortedThreadTestCase(ModuleStoreTestCase):
         self.assertRegexpMatches(html, r'"group_name": "student_cohort"')
 
 
+@patch('lms.lib.comment_client.utils.requests.request')
+class SingleThreadAccessTestCase(CohortedContentTestCase):
+    def call_view(self, mock_request, commentable_id, user, group_id, thread_group_id=None, pass_group_id=True):
+        thread_id = "test_thread_id"
+        mock_request.side_effect = make_mock_request_impl("dummy context", thread_id, group_id=thread_group_id)
+
+        request_data = {}
+        if pass_group_id:
+            request_data["group_id"] = group_id
+        request = RequestFactory().get(
+            "dummy_url",
+            data=request_data,
+            HTTP_X_REQUESTED_WITH="XMLHttpRequest"
+        )
+        request.user = user
+        return views.single_thread(
+            request,
+            self.course.id.to_deprecated_string(),
+            commentable_id,
+            thread_id
+        )
+
+    def test_student_non_cohorted(self, mock_request):
+        resp = self.call_view(mock_request, "non_cohorted_topic", self.student, self.student_cohort.id)
+        self.assertEqual(resp.status_code, 200)
+
+    def test_student_same_cohort(self, mock_request):
+        resp = self.call_view(
+            mock_request,
+            "cohorted_topic",
+            self.student,
+            self.student_cohort.id,
+            thread_group_id=self.student_cohort.id
+        )
+        self.assertEqual(resp.status_code, 200)
+
+    def test_student_different_cohort(self, mock_request):
+        self.assertRaises(
+            Http404,
+            lambda: self.call_view(
+                mock_request,
+                "cohorted_topic",
+                self.student,
+                self.student_cohort.id,
+                thread_group_id=self.moderator_cohort.id
+            )
+        )
+
+    def test_moderator_non_cohorted(self, mock_request):
+        resp = self.call_view(mock_request, "non_cohorted_topic", self.moderator, self.moderator_cohort.id)
+        self.assertEqual(resp.status_code, 200)
+
+    def test_moderator_same_cohort(self, mock_request):
+        resp = self.call_view(
+            mock_request,
+            "cohorted_topic",
+            self.moderator,
+            self.moderator_cohort.id,
+            thread_group_id=self.moderator_cohort.id
+        )
+        self.assertEqual(resp.status_code, 200)
+
+    def test_moderator_different_cohort(self, mock_request):
+        resp = self.call_view(
+            mock_request,
+            "cohorted_topic",
+            self.moderator,
+            self.moderator_cohort.id,
+            thread_group_id=self.student_cohort.id
+        )
+        self.assertEqual(resp.status_code, 200)
+
+
+@patch('lms.lib.comment_client.utils.requests.request')
+class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin):
+    cs_endpoint = "/threads"
+
+    def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
+        mock_request.side_effect = make_mock_request_impl("dummy context", group_id=self.student_cohort.id)
+
+        request_data = {}
+        if pass_group_id:
+            request_data["group_id"] = group_id
+        request = RequestFactory().get(
+            "dummy_url",
+            data=request_data
+        )
+        request.user = user
+        mako_middleware_process_request(request)
+        return views.single_thread(
+            request,
+            self.course.id.to_deprecated_string(),
+            "dummy_discussion_id",
+            "dummy_thread_id"
+        )
+
+
+@patch('lms.lib.comment_client.utils.requests.request')
+class InlineDiscussionGroupIdTestCase(
+        CohortedContentTestCase,
+        CohortedTopicGroupIdTestMixin,
+        NonCohortedTopicGroupIdTestMixin
+):
+    cs_endpoint = "/threads"
+
+    def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
+        mock_request.side_effect = make_mock_request_impl("dummy content")
+
+        request_data = {}
+        if pass_group_id:
+            request_data["group_id"] = group_id
+        request = RequestFactory().get(
+            "dummy_url",
+            data=request_data
+        )
+        request.user = user
+        return views.inline_discussion(
+            request,
+            self.course.id.to_deprecated_string(),
+            commentable_id
+        )
+
+
+@patch('lms.lib.comment_client.utils.requests.request')
+class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin):
+    cs_endpoint = "/threads"
+
+    def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
+        mock_request.side_effect = make_mock_request_impl("dummy content")
+
+        request_data = {}
+        if pass_group_id:
+            request_data["group_id"] = group_id
+        request = RequestFactory().get(
+            "dummy_url",
+            data=request_data
+        )
+        request.user = user
+        mako_middleware_process_request(request)
+        return views.forum_form_discussion(
+            request,
+            self.course.id.to_deprecated_string()
+        )
+
+
+@patch('lms.lib.comment_client.utils.requests.request')
+class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin):
+    cs_endpoint = "/active_threads"
+
+    def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
+        mock_request.side_effect = make_mock_request_impl("dummy content")
+
+        request_data = {}
+        if pass_group_id:
+            request_data["group_id"] = group_id
+        request = RequestFactory().get(
+            "dummy_url",
+            data=request_data
+        )
+        request.user = user
+        mako_middleware_process_request(request)
+        return views.user_profile(
+            request,
+            self.course.id.to_deprecated_string(),
+            user.id
+        )
+
+
+@patch('lms.lib.comment_client.utils.requests.request')
+class FollowedThreadsDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin):
+    cs_endpoint = "/subscribed_threads"
+
+    def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
+        mock_request.side_effect = make_mock_request_impl("dummy content")
+
+        request_data = {}
+        if pass_group_id:
+            request_data["group_id"] = group_id
+        request = RequestFactory().get(
+            "dummy_url",
+            data=request_data,
+            HTTP_X_REQUESTED_WITH="XMLHttpRequest"
+        )
+        request.user = user
+        return views.followed_threads(
+            request,
+            self.course.id.to_deprecated_string(),
+            user.id
+        )
+
+
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
 @patch('requests.request')
 class UserProfileTestCase(ModuleStoreTestCase):

lms/djangoapps/django_comment_client/forum/views.py

@@ -5,8 +5,7 @@ import xml.sax.saxutils as saxutils
 from django.contrib.auth.decorators import login_required
 from django.core.context_processors import csrf
 from django.contrib.auth.models import User
-from django.http import Http404
-from django.utils.translation import ugettext as _
+from django.http import Http404, HttpResponseBadRequest
 from django.views.decorators.http import require_GET
 import newrelic.agent
 
@@ -18,7 +17,14 @@ from course_groups.cohorts import (is_course_cohorted, get_cohort_id, get_cohort
 from courseware.access import has_access
 
 from django_comment_client.permissions import cached_has_permission
-from django_comment_client.utils import (merge_dict, extract, strip_none, add_courseware_context, add_thread_group_name)
+from django_comment_client.utils import (
+    merge_dict,
+    extract,
+    strip_none,
+    add_courseware_context,
+    add_thread_group_name,
+    get_group_id_for_comments_service
+)
 import django_comment_client.utils as utils
 import lms.lib.comment_client as cc
 
@@ -94,9 +100,9 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA
         group_id = None
 
     if not group_id:
-        if not cached_has_permission(request.user, "see_all_cohorts", course_id):
-            group_id = get_cohort_id(request.user, course_id)
-            if not group_id and get_cohorted_threads_privacy(course_id) == 'cohort-only':
+        if not cached_has_permission(request.user, "see_all_cohorts", course_key):
+            group_id = get_cohort_id(request.user, course_key)
+            if not group_id and get_cohorted_threads_privacy(course_key) == 'cohort-only':
                 default_query_params['exclude_groups'] = True
 
     if group_id:
@@ -124,7 +130,7 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA
     )
 
     threads, page, num_pages, corrected_text = cc.Thread.search(query_params)
-    threads = _set_group_names(course_id, threads)
+    threads = _set_group_names(course_key, threads)
 
     query_params['page'] = page
     query_params['num_pages'] = num_pages
@@ -161,11 +167,14 @@ def inline_discussion(request, course_id, discussion_id):
     course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id)
 
     course = get_course_with_access(request.user, 'load_forum', course_id)
-
-    threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
     cc_user = cc.User.from_django_user(request.user)
     user_info = cc_user.to_dict()
 
+    try:
+        threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
+    except ValueError:
+        return HttpResponseBadRequest("Invalid group_id")
+
     with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"):
         annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info)
     is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
@@ -191,6 +200,9 @@ def forum_form_discussion(request, course_id):
     course = get_course_with_access(request.user, 'load_forum', course_id)
     course_settings = make_course_settings(course)
 
+    user = cc.User.from_django_user(request.user)
+    user_info = user.to_dict()
+
     try:
         unsafethreads, query_params = get_threads(request, course_id)   # This might process a search query
         is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
@@ -198,9 +210,8 @@ def forum_form_discussion(request, course_id):
     except cc.utils.CommentClientMaintenanceError:
         log.warning("Forum is in maintenance mode")
         return render_to_response('discussion/maintenance.html', {})
-
-    user = cc.User.from_django_user(request.user)
-    user_info = user.to_dict()
+    except ValueError:
+        return HttpResponseBadRequest("Invalid group_id")
 
     with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"):
         annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info)
@@ -255,6 +266,7 @@ def single_thread(request, course_id, discussion_id, thread_id):
     course_settings = make_course_settings(course, include_category_map=True)
     cc_user = cc.User.from_django_user(request.user)
     user_info = cc_user.to_dict()
+    is_moderator = cached_has_permission(request.user, "see_all_cohorts", course_key)
 
     # Currently, the front end always loads responses via AJAX, even for this
     # page; it would be a nice optimization to avoid that extra round trip to
@@ -271,6 +283,12 @@ def single_thread(request, course_id, discussion_id, thread_id):
             raise Http404
         raise
 
+    # verify that the thread belongs to the requesting student's cohort
+    if is_commentable_cohorted(course_key, discussion_id) and not is_moderator:
+        user_group_id = get_cohort_id(request.user, course_key)
+        if hasattr(thread, "group_id") and user_group_id != thread.group_id:
+            raise Http404
+
     is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
     if request.is_ajax():
         with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"):
@@ -285,7 +303,10 @@ def single_thread(request, course_id, discussion_id, thread_id):
         })
 
     else:
-        threads, query_params = get_threads(request, course_key)
+        try:
+            threads, query_params = get_threads(request, course_key)
+        except ValueError:
+            return HttpResponseBadRequest("Invalid group_id")
         threads.append(thread.to_dict())
 
         with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
@@ -318,7 +339,7 @@ def single_thread(request, course_id, discussion_id, thread_id):
             'thread_id': thread_id,
             'threads': _attr_safe_json(threads),
             'roles': _attr_safe_json(utils.get_role_ids(course_key)),
-            'is_moderator': cached_has_permission(request.user, "see_all_cohorts", course_key),
+            'is_moderator': is_moderator,
             'thread_pages': query_params['num_pages'],
             'is_course_cohorted': is_course_cohorted(course_key),
             'flag_moderator': cached_has_permission(request.user, 'openclose_thread', course.id) or has_access(request.user, 'staff', course),
@@ -347,6 +368,13 @@ def user_profile(request, course_id, user_id):
             'per_page': THREADS_PER_PAGE,   # more than threads_per_page to show more activities
         }
 
+        try:
+            group_id = get_group_id_for_comments_service(request, course_id)
+        except ValueError:
+            return HttpResponseBadRequest("Invalid group_id")
+        if group_id is not None:
+            query_params['group_id'] = group_id
+
         threads, page, num_pages = profiled_user.active_threads(query_params)
         threads = _set_group_names(course_id, threads)
         query_params['page'] = page
@@ -416,6 +444,13 @@ def followed_threads(request, course_id, user_id):
             )
         )
 
+        try:
+            group_id = get_group_id_for_comments_service(request, course_id)
+        except ValueError:
+            return HttpResponseBadRequest("Invalid group_id")
+        if group_id is not None:
+            query_params['group_id'] = group_id
+
         threads, page, num_pages = profiled_user.subscribed_threads(query_params)
         threads = _set_group_names(course_id, threads)
         query_params['page'] = page

lms/djangoapps/django_comment_client/tests/group_id.py

+class GroupIdAssertionMixin(object):
+    def _data_or_params_cs_request(self, mock_request):
+        """
+        Returns the data or params dict that `mock_request` was called with.
+        """
+        call = [call for call in mock_request.call_args_list if call[0][1].endswith(self.cs_endpoint)][0]
+        if call[0][0] == "get":
+            return call[1]["params"]
+        elif call[0][0] == "post":
+            return call[1]["data"]
+
+    def _assert_comments_service_called_with_group_id(self, mock_request, group_id):
+        self.assertTrue(mock_request.called)
+        self.assertEqual(self._data_or_params_cs_request(mock_request)["group_id"], group_id)
+
+    def _assert_comments_service_called_without_group_id(self, mock_request):
+        self.assertTrue(mock_request.called)
+        self.assertNotIn("group_id", self._data_or_params_cs_request(mock_request))
+
+
+class CohortedTopicGroupIdTestMixin(GroupIdAssertionMixin):
+    """
+    Provides test cases to verify that views pass the correct `group_id` to
+    the comments service when requesting content in cohorted discussions.
+    """
+    def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
+        """
+        Call the view for the implementing test class, constructing a request
+        from the parameters.
+        """
+        pass
+
+    def test_cohorted_topic_student_without_group_id(self, mock_request):
+        self.call_view(mock_request, "cohorted_topic", self.student, None, pass_group_id=False)
+        self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_cohorted_topic_student_none_group_id(self, mock_request):
+        self.call_view(mock_request, "cohorted_topic", self.student, "")
+        self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_cohorted_topic_student_with_own_group_id(self, mock_request):
+        self.call_view(mock_request, "cohorted_topic", self.student, self.student_cohort.id)
+        self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_cohorted_topic_student_with_other_group_id(self, mock_request):
+        self.call_view(mock_request, "cohorted_topic", self.student, self.moderator_cohort.id)
+        self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_cohorted_topic_moderator_without_group_id(self, mock_request):
+        self.call_view(mock_request, "cohorted_topic", self.moderator, None, pass_group_id=False)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_cohorted_topic_moderator_none_group_id(self, mock_request):
+        self.call_view(mock_request, "cohorted_topic", self.moderator, "")
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_cohorted_topic_moderator_with_own_group_id(self, mock_request):
+        self.call_view(mock_request, "cohorted_topic", self.moderator, self.moderator_cohort.id)
+        self._assert_comments_service_called_with_group_id(mock_request, self.moderator_cohort.id)
+
+    def test_cohorted_topic_moderator_with_other_group_id(self, mock_request):
+        self.call_view(mock_request, "cohorted_topic", self.moderator, self.student_cohort.id)
+        self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id)
+
+    def test_cohorted_topic_moderator_with_invalid_group_id(self, mock_request):
+        invalid_id = self.student_cohort.id + self.moderator_cohort.id
+        response = self.call_view(mock_request, "cohorted_topic", self.moderator, invalid_id)
+        self.assertEqual(response.status_code, 400)
+
+
+class NonCohortedTopicGroupIdTestMixin(GroupIdAssertionMixin):
+    """
+    Provides test cases to verify that views pass the correct `group_id` to
+    the comments service when requesting content in non-cohorted discussions.
+    """
+    def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
+        """
+        Call the view for the implementing test class, constructing a request
+        from the parameters.
+        """
+        pass
+
+    def test_non_cohorted_topic_student_without_group_id(self, mock_request):
+        self.call_view(mock_request, "non_cohorted_topic", self.student, None, pass_group_id=False)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_non_cohorted_topic_student_none_group_id(self, mock_request):
+        self.call_view(mock_request, "non_cohorted_topic", self.student, None)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_non_cohorted_topic_student_with_own_group_id(self, mock_request):
+        self.call_view(mock_request, "non_cohorted_topic", self.student, self.student_cohort.id)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_non_cohorted_topic_student_with_other_group_id(self, mock_request):
+        self.call_view(mock_request, "non_cohorted_topic", self.student, self.moderator_cohort.id)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_non_cohorted_topic_moderator_without_group_id(self, mock_request):
+        self.call_view(mock_request, "non_cohorted_topic", self.moderator, None, pass_group_id=False)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_non_cohorted_topic_moderator_none_group_id(self, mock_request):
+        self.call_view(mock_request, "non_cohorted_topic", self.moderator, None)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_non_cohorted_topic_moderator_with_own_group_id(self, mock_request):
+        self.call_view(mock_request, "non_cohorted_topic", self.moderator, self.moderator_cohort.id)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_non_cohorted_topic_moderator_with_other_group_id(self, mock_request):
+        self.call_view(mock_request, "non_cohorted_topic", self.moderator, self.student_cohort.id)
+        self._assert_comments_service_called_without_group_id(mock_request)
+
+    def test_non_cohorted_topic_moderator_with_invalid_group_id(self, mock_request):
+        invalid_id = self.student_cohort.id + self.moderator_cohort.id
+        self.call_view(mock_request, "non_cohorted_topic", self.moderator, invalid_id)
+        self._assert_comments_service_called_without_group_id(mock_request)

lms/djangoapps/django_comment_client/tests/utils.py

-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
@@ -49,35 +47,3 @@ class CohortedContentTestCase(ModuleStoreTestCase):
         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"])

lms/djangoapps/django_comment_client/utils.py

@@ -6,7 +6,6 @@ from collections import defaultdict
 import logging
 from datetime import datetime
 
-from course_groups.cohorts import get_cohort_by_id
 from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 from django.db import connection
@@ -18,6 +17,8 @@ from django_comment_client.permissions import check_permissions_by_view, cached_
 from edxmako import lookup_template
 import pystache_custom as pystache
 
+from course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted
+from course_groups.models import CourseUserGroup
 from xmodule.modulestore.django import modulestore
 from django.utils.timezone import UTC
 from opaque_keys.edx.locations import i4xEncoder
@@ -450,6 +451,41 @@ def add_thread_group_name(thread_info, course_key):
         thread_info['group_name'] = get_cohort_by_id(course_key, thread_info.get('group_id')).name
 
 
+def get_group_id_for_comments_service(request, course_key, commentable_id=None):
+    """
+    Given a user requesting content within a `commentable_id`, determine the
+    group_id which should be passed to the comments service.
+
+    Returns:
+        int: the group_id to pass to the comments service or None if nothing
+        should be passed
+
+    Raises:
+        ValueError if the requested group_id is invalid
+    """
+    if commentable_id is None or is_commentable_cohorted(course_key, commentable_id):
+        if request.method == "GET":
+            requested_group_id = request.GET.get('group_id')
+        elif request.method == "POST":
+            requested_group_id = request.POST.get('group_id')
+        if cached_has_permission(request.user, "see_all_cohorts", course_key):
+            if not requested_group_id:
+                return None
+            try:
+                group_id = int(requested_group_id)
+                get_cohort_by_id(course_key, group_id)
+            except CourseUserGroup.DoesNotExist:
+                raise ValueError
+        else:
+            # regular users always query with their own id.
+            group_id = get_cohort_id(request.user, course_key)
+        return group_id
+    else:
+        # Never pass a group_id to the comments service for a non-cohorted
+        # commentable
+        return None
+
+
 def format_filename(filename):
     """Take a string and return a valid filename constructed from the string.
     Uses a whitelist approach: any characters not present in valid_chars are