Implement forum cohort visibility labels correctly

TNL-25

lms/djangoapps/django_comment_client/base/views.py

@@ -37,7 +37,8 @@ from django_comment_client.utils import (
     safe_content,
     get_discussion_categories_ids,
     get_discussion_categories_ids,
-    permalink
+    permalink,
+    add_thread_group_name
 )
 from util.html import strip_tags
 from django_comment_client.permissions import check_permissions_by_view, cached_has_permission
@@ -196,7 +197,8 @@ def create_thread(request, course_id, commentable_id):
     # call into the social_engagement django app to
     # rescore this user
     _update_user_engagement_score(course_key, request.user.id)
-
+    
+    add_thread_group_name(data, course_key)
     add_courseware_context([data], course)
     if request.is_ajax():
         return ajax_content_response(request, course_key, data)

lms/djangoapps/django_comment_client/forum/tests.py

 import json
+import logging
+
 from django.http import Http404
 from django.test.utils import override_settings
 from django.test.client import Client, RequestFactory
@@ -15,7 +17,7 @@ from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
 from nose.tools import assert_true  # pylint: disable=E0611
 from mock import patch, Mock, ANY, call
 
-import logging
+from course_groups.models import CourseUserGroup
 
 log = logging.getLogger(__name__)
 
@@ -87,7 +89,7 @@ class ViewsExceptionTestCase(UrlResetMixin, ModuleStoreTestCase):
         self.assertEqual(self.response.status_code, 404)
 
 
-def make_mock_thread_data(text, thread_id, include_children):
+def make_mock_thread_data(text, thread_id, include_children, group_id=None, group_name=None):
     thread_data = {
         "id": thread_id,
         "type": "thread",
@@ -98,6 +100,9 @@ def make_mock_thread_data(text, thread_id, include_children):
         "resp_skip": 25,
         "resp_limit": 5,
     }
+    if group_id is not None:
+        thread_data['group_id'] = group_id
+        thread_data['group_name'] = group_name
     if include_children:
         thread_data["children"] = [{
             "id": "dummy_comment_id",
@@ -107,16 +112,16 @@ def make_mock_thread_data(text, thread_id, include_children):
     return thread_data
 
 
-def make_mock_request_impl(text, thread_id="dummy_thread_id"):
+def make_mock_request_impl(text, thread_id="dummy_thread_id", group_id=None):
     def mock_request_impl(*args, **kwargs):
         url = args[1]
         data = None
         if url.endswith("threads"):
             data = {
-                "collection": [make_mock_thread_data(text, thread_id, False)]
+                "collection": [make_mock_thread_data(text, thread_id, False, group_id=group_id)]
             }
         elif thread_id and url.endswith(thread_id):
-            data = make_mock_thread_data(text, thread_id, True)
+            data = make_mock_thread_data(text, thread_id, True, group_id=group_id)
         elif "/users/" in url:
             data = {
                 "default_sort_key": "date",
@@ -262,6 +267,74 @@ 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
+        )
+
+    def _create_mock_cohorted_thread(self, mock_request):
+        self.mock_text = "dummy content"
+        self.mock_thread_id = "test_thread_id"
+        mock_request.side_effect = make_mock_request_impl(
+            self.mock_text, self.mock_thread_id,
+            group_id=self.student_cohort.id
+        )
+
+    def test_ajax(self, mock_request):
+        self._create_mock_cohorted_thread(mock_request)
+
+        request = RequestFactory().get(
+            "dummy_url",
+            HTTP_X_REQUESTED_WITH="XMLHttpRequest"
+        )
+        request.user = self.student
+        response = views.single_thread(
+            request,
+            self.course.id.to_deprecated_string(),
+            "dummy_discussion_id",
+            self.mock_thread_id
+        )
+
+        self.assertEquals(response.status_code, 200)
+        response_data = json.loads(response.content)
+        self.assertEquals(
+            response_data["content"],
+            make_mock_thread_data(
+                self.mock_text, self.mock_thread_id, True,
+                group_id=self.student_cohort.id,
+                group_name=self.student_cohort.name,
+            )
+        )
+
+    def test_html(self, mock_request):
+        self._create_mock_cohorted_thread(mock_request)
+
+        request = RequestFactory().get("dummy_url")
+        request.user = self.student
+        mako_middleware_process_request(request)
+        response = views.single_thread(
+            request,
+            self.course.id.to_deprecated_string(),
+            "dummy_discussion_id",
+            self.mock_thread_id
+        )
+
+        self.assertEquals(response.status_code, 200)
+        self.assertEqual(response['Content-Type'], 'text/html; charset=utf-8')
+        html = response.content
+
+        # Verify that the group name is correctly included in the HTML
+        self.assertRegexpMatches(html, r'"group_name": "student_cohort"')
+
+
+@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
+@patch('requests.request')
 class UserProfileTestCase(ModuleStoreTestCase):
 
     TEST_THREAD_TEXT = 'userprofile-test-text'

lms/djangoapps/django_comment_client/forum/views.py

@@ -18,7 +18,7 @@ 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)
+from django_comment_client.utils import (merge_dict, extract, strip_none, add_courseware_context, add_thread_group_name)
 import django_comment_client.utils as utils
 import lms.lib.comment_client as cc
 
@@ -54,7 +54,7 @@ def make_course_settings(course):
     return obj
 
 @newrelic.agent.function_trace()
-def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAGE):
+def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PAGE):
     """
     This may raise an appropriate subclass of cc.utils.CommentClientError
     if something goes wrong.
@@ -66,7 +66,7 @@ def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAG
         'sort_order': 'desc',
         'text': '',
         'commentable_id': discussion_id,
-        'course_id': course_id.to_deprecated_string(),
+        'course_id': course_key.to_deprecated_string(),
         'user_id': request.user.id,
     }
 
@@ -86,7 +86,7 @@ def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAG
     #is user a moderator
     #did the user request a group
 
-    #if the user requested a group explicitly, give them that group, othewrise, if mod, show all, else if student, use cohort
+    #if the user requested a group explicitly, give them that group, otherwise, if mod, show all, else if student, use cohort
 
     group_id = request.GET.get('group_id')
 
@@ -248,11 +248,11 @@ def forum_form_discussion(request, course_id):
 @require_GET
 @login_required
 def single_thread(request, course_id, discussion_id, thread_id):
-    course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
     nr_transaction = newrelic.agent.current_transaction()
 
-    course = get_course_with_access(request.user, 'load_forum', course_id)
-    course_settings = make_course_settings(course)
+    course = get_course_with_access(request.user, 'load_forum', course_key)
+    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()
 
@@ -274,8 +274,9 @@ def single_thread(request, course_id, discussion_id, thread_id):
     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"):
-            annotated_content_info = utils.get_annotated_content_infos(course_id, thread, request.user, user_info=user_info)
-        content = utils.safe_content(thread.to_dict(), course_id, is_staff)
+            annotated_content_info = utils.get_annotated_content_infos(course_key, thread, request.user, user_info=user_info)
+        content = utils.safe_content(thread.to_dict(), course_key, is_staff)
+        add_thread_group_name(content, course_key)
         with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
             add_courseware_context([content], course)
         return utils.JsonResponse({
@@ -284,27 +285,26 @@ def single_thread(request, course_id, discussion_id, thread_id):
         })
 
     else:
-        threads, query_params = get_threads(request, course_id)
+        threads, query_params = get_threads(request, course_key)
         threads.append(thread.to_dict())
 
         with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
             add_courseware_context(threads, course)
 
         for thread in threads:
-            if thread.get('group_id') and not thread.get('group_name'):
-                thread['group_name'] = get_cohort_by_id(course_id, thread.get('group_id')).name
+            add_thread_group_name(thread, course_key)
 
             #patch for backward compatibility with comments service
             if not "pinned" in thread:
                 thread["pinned"] = False
 
-        threads = [utils.safe_content(thread, course_id, is_staff) for thread in threads]
+        threads = [utils.safe_content(thread, course_key, is_staff) for thread in threads]
 
         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)
+            annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info)
 
         with newrelic.agent.FunctionTrace(nr_transaction, "get_cohort_info"):
-            user_cohort = get_cohort_id(request.user, course_id)
+            user_cohort = get_cohort_id(request.user, course_key)
 
         context = {
             'discussion_id': discussion_id,
@@ -317,10 +317,10 @@ def single_thread(request, course_id, discussion_id, thread_id):
             'course_id': course.id.to_deprecated_string(),   # TODO: Why pass both course and course.id to template?
             'thread_id': thread_id,
             'threads': _attr_safe_json(threads),
-            'roles': _attr_safe_json(utils.get_role_ids(course_id)),
-            'is_moderator': cached_has_permission(request.user, "see_all_cohorts", course_id),
+            'roles': _attr_safe_json(utils.get_role_ids(course_key)),
+            'is_moderator': cached_has_permission(request.user, "see_all_cohorts", course_key),
             'thread_pages': query_params['num_pages'],
-            'is_course_cohorted': is_course_cohorted(course_id),
+            '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),
             'cohorts': course_settings["cohorts"],
             'user_cohort': user_cohort,

lms/djangoapps/django_comment_client/utils.py

@@ -6,6 +6,7 @@ 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
@@ -17,7 +18,6 @@ from django_comment_client.permissions import check_permissions_by_view, cached_
 from edxmako import lookup_template
 import pystache_custom as pystache
 
-
 from xmodule.modulestore.django import modulestore
 from django.utils.timezone import UTC
 from opaque_keys.edx.locations import i4xEncoder
@@ -400,7 +400,7 @@ def safe_content(content, course_id, is_staff=False):
         'updated_at', 'depth', 'type', 'commentable_id', 'comments_count',
         'at_position_list', 'children', 'highlighted_title', 'highlighted_body',
         'courseware_title', 'courseware_url', 'unread_comments_count',
-        'read', 'group_id', 'group_name', 'group_string', 'pinned', 'abuse_flaggers',
+        'read', 'group_id', 'group_name', 'pinned', 'abuse_flaggers',
         'stats', 'resp_skip', 'resp_limit', 'resp_total', 'thread_type',
         'endorsed_responses', 'non_endorsed_responses', 'non_endorsed_resp_total',
         'endorsement',
@@ -442,6 +442,14 @@ def safe_content(content, course_id, is_staff=False):
     return content
 
 
+def add_thread_group_name(thread_info, course_key):
+    """
+    Augment the specified thread info to include the group name if a group id is present.
+    """
+    if thread_info.get('group_id') is not None:
+        thread_info['group_name'] = get_cohort_by_id(course_key, thread_info.get('group_id')).name
+
+
 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