From 6114cf05f4a3e6e7111b2d44d41e894906ecc87d Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Mon, 6 Oct 2014 16:59:05 -0400 Subject: [PATCH 1/3] Don't show cohort information when disabled TNL-552 --- .../acceptance/tests/discussion/helpers.py | 15 +++++++++++++- .../tests/discussion/test_cohorts.py | 18 ++++++++++++++++- lms/djangoapps/django_comment_client/utils.py | 20 +++++++++++++------ 3 files changed, 45 insertions(+), 8 deletions(-) diff --git a/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py index e8aad4a58e..b38a9a36a0 100644 --- a/common/test/acceptance/tests/discussion/helpers.py +++ b/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 }, }, }) diff --git a/common/test/acceptance/tests/discussion/test_cohorts.py b/common/test/acceptance/tests/discussion/test_cohorts.py index dce9e50994..8ca505c52b 100644 --- a/common/test/acceptance/tests/discussion/test_cohorts.py +++ b/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,10 @@ class DiscussionTabSingleThreadTest(UniqueCourseTest): self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, thread_id) # pylint:disable=W0201 self.thread_page.visit() + 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 +122,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): diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index f88fc3d82c..f0f91739c8 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -1,3 +1,4 @@ +import json import pytz from collections import defaultdict import logging @@ -8,6 +9,8 @@ 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 @@ -16,11 +19,10 @@ 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 courseware.courses import get_course_by_id 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__) @@ -393,6 +395,8 @@ def prepare_content(content, course_key, is_staff=False): @TODO: not all response pre-processing steps are currently integrated into this function. """ + course = get_course_by_id(course_key) + fields = [ 'id', 'title', 'body', 'course_id', 'anonymous', 'anonymous_to_peers', 'endorsed', 'parent_id', 'thread_id', 'votes', 'closed', 'created_at', @@ -438,9 +442,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 course.is_cohorted: + # 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 From fbeb57874ca9a66798d9bf4ab53ab2a711745376 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Wed, 8 Oct 2014 12:47:23 -0400 Subject: [PATCH 2/3] Address code review comments --- lms/djangoapps/django_comment_client/utils.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index f0f91739c8..84f01176dd 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -17,9 +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.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted, is_course_cohorted from course_groups.models import CourseUserGroup -from courseware.courses import get_course_by_id from opaque_keys.edx.locations import i4xEncoder from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.django import modulestore @@ -395,7 +394,6 @@ def prepare_content(content, course_key, is_staff=False): @TODO: not all response pre-processing steps are currently integrated into this function. """ - course = get_course_by_id(course_key) fields = [ 'id', 'title', 'body', 'course_id', 'anonymous', 'anonymous_to_peers', @@ -429,7 +427,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: @@ -442,7 +440,7 @@ def prepare_content(content, course_key, is_staff=False): ] content[child_content_key] = children - if course.is_cohorted: + 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 From 3af9eb25b2aff0ef117ad70f47466b8c4f5b981e Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Wed, 8 Oct 2014 15:14:11 -0400 Subject: [PATCH 3/3] Minor cleanups --- common/test/acceptance/tests/discussion/test_cohorts.py | 1 + lms/djangoapps/django_comment_client/utils.py | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/common/test/acceptance/tests/discussion/test_cohorts.py b/common/test/acceptance/tests/discussion/test_cohorts.py index 8ca505c52b..482a35e34d 100644 --- a/common/test/acceptance/tests/discussion/test_cohorts.py +++ b/common/test/acceptance/tests/discussion/test_cohorts.py @@ -73,6 +73,7 @@ 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() diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 84f01176dd..8539978779 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -394,7 +394,6 @@ def prepare_content(content, course_key, is_staff=False): @TODO: not all response pre-processing steps are currently integrated into this function. """ - fields = [ 'id', 'title', 'body', 'course_id', 'anonymous', 'anonymous_to_peers', 'endorsed', 'parent_id', 'thread_id', 'votes', 'closed', 'created_at',