Merge pull request #5501 from edx/andya/disable-cohorts-ui-bug

Don't show cohort information when disabled

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

@@ -30,6 +30,7 @@ class BaseDiscussionMixin(object):
             thread_fixture.addResponse(Response(id=str(i), body=str(i)))
         thread_fixture.push()
         self.setup_thread_page(thread_id)
+        return thread_id
 
 
 class CohortTestMixin(object):
@@ -46,7 +47,19 @@ class CohortTestMixin(object):
                 u"cohort_config": {
                     "auto_cohort_groups": auto_cohort_groups or [],
                     "cohorted_discussions": [],
-                    "cohorted": True
+                    "cohorted": True,
+                },
+            },
+        })
+
+    def disable_cohorting(self, course_fixture):
+        """
+        Disables cohorting for the current course fixture.
+        """
+        course_fixture._update_xblock(course_fixture._course_location, {
+            "metadata": {
+                u"cohort_config": {
+                    "cohorted": False
                 },
             },
         })

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

@@ -45,12 +45,17 @@ class CohortedDiscussionTestMixin(BaseDiscussionMixin, CohortTestMixin):
     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.thread_id = self.setup_thread(1, group_id=self.cohort_1_id)
         self.assertEquals(
             self.thread_page.get_group_visibility_label(),
             "This post is visible only to {}.".format(self.cohort_1_name)
         )
 
+        # Disable cohorts and verify that the post now shows as visible to everyone.
+        self.disable_cohorting(self.course_fixture)
+        self.refresh_thread_page(self.thread_id)
+        self.assertEquals(self.thread_page.get_group_visibility_label(), "This post is visible to everyone.")
+
 
 class DiscussionTabSingleThreadTest(UniqueCourseTest):
     """
@@ -68,6 +73,11 @@ class DiscussionTabSingleThreadTest(UniqueCourseTest):
         self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, thread_id)  # pylint:disable=W0201
         self.thread_page.visit()
 
+    # pylint:disable=W0613
+    def refresh_thread_page(self, thread_id):
+        self.browser.refresh()
+        self.thread_page.wait_for_page()
+
 
 @attr('shard_1')
 class CohortedDiscussionTabSingleThreadTest(DiscussionTabSingleThreadTest, CohortedDiscussionTestMixin):
@@ -113,12 +123,19 @@ class InlineDiscussionTest(UniqueCourseTest):
 
     def setup_thread_page(self, thread_id):
         CoursewarePage(self.browser, self.course_id).visit()
+        self.show_thread(thread_id)
+
+    def show_thread(self, thread_id):
         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()
 
+    def refresh_thread_page(self, thread_id):
+        self.browser.refresh()
+        self.show_thread(thread_id)
+
 
 @attr('shard_1')
 class CohortedInlineDiscussionTest(InlineDiscussionTest, CohortedDiscussionTestMixin):

lms/djangoapps/django_comment_client/utils.py

+import json
 import pytz
 from collections import defaultdict
 import logging
@@ -8,19 +9,19 @@ from django.core.urlresolvers import reverse
 from django.db import connection
 from django.http import HttpResponse
 from django.utils import simplejson
+from django.utils.timezone import UTC
+
 from django_comment_common.models import Role, FORUM_ROLE_STUDENT
 from django_comment_client.permissions import check_permissions_by_view, cached_has_permission
 
 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.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted, is_course_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
 from opaque_keys.edx.keys import CourseKey
-import json
+from xmodule.modulestore.django import modulestore
 
 log = logging.getLogger(__name__)
 
@@ -425,7 +426,7 @@ def prepare_content(content, course_key, is_staff=False):
         # Only reveal endorser if requester can see author or if endorser is staff
         if (
             endorser and
-            ("username" in fields or cached_has_permission(endorser, "endorse_comment", course_id))
+            ("username" in fields or cached_has_permission(endorser, "endorse_comment", course_key))
         ):
             endorsement["username"] = endorser.username
         else:
@@ -438,9 +439,13 @@ def prepare_content(content, course_key, is_staff=False):
             ]
             content[child_content_key] = children
 
-    # Augment the specified thread info to include the group name if a group id is present.
-    if content.get('group_id') is not None:
-        content['group_name'] = get_cohort_by_id(course_key, content.get('group_id')).name
+    if is_course_cohorted(course_key):
+        # Augment the specified thread info to include the group name if a group id is present.
+        if content.get('group_id') is not None:
+            content['group_name'] = get_cohort_by_id(course_key, content.get('group_id')).name
+    else:
+        # Remove any cohort information that might remain if the course had previously been cohorted.
+        content.pop('group_id', None)
 
     return content