Merge pull request #7266 from edx/dan-f/forums-filter-discussion-modules-3

Filter inline discussion topics and threads in the forums by access

common/test/acceptance/pages/lms/discussion.py

@@ -311,13 +311,15 @@ class DiscussionSortPreferencePage(CoursePage):
 
 
 class DiscussionTabSingleThreadPage(CoursePage):
-    def __init__(self, browser, course_id, thread_id):
+    def __init__(self, browser, course_id, discussion_id, thread_id):
         super(DiscussionTabSingleThreadPage, self).__init__(browser, course_id)
         self.thread_page = DiscussionThreadPage(
             browser,
             "body.discussion .discussion-article[data-id='{thread_id}']".format(thread_id=thread_id)
         )
-        self.url_path = "discussion/forum/dummy/threads/" + thread_id
+        self.url_path = "discussion/forum/{discussion_id}/threads/{thread_id}".format(
+            discussion_id=discussion_id, thread_id=thread_id
+        )
 
     def is_browser_on_page(self):
         return self.thread_page.is_browser_on_page()

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

@@ -5,12 +5,15 @@ Helper functions and classes for discussion tests.
 from uuid import uuid4
 import json
 
+from ...fixtures import LMS_BASE_URL
+from ...fixtures.course import CourseFixture
 from ...fixtures.discussion import (
     SingleThreadViewFixture,
     Thread,
     Response,
 )
-from ...fixtures import LMS_BASE_URL
+from ...pages.lms.discussion import DiscussionTabSingleThreadPage
+from ...tests.helpers import UniqueCourseTest
 
 
 class BaseDiscussionMixin(object):
@@ -83,3 +86,22 @@ class CohortTestMixin(object):
         data = {"users": username}
         response = course_fixture.session.post(url, data=data, headers=course_fixture.headers)
         self.assertTrue(response.ok, "Failed to add user to cohort")
+
+
+class BaseDiscussionTestCase(UniqueCourseTest):
+    def setUp(self):
+        super(BaseDiscussionTestCase, self).setUp()
+
+        self.discussion_id = "test_discussion_{}".format(uuid4().hex)
+        self.course_fixture = CourseFixture(**self.course_info)
+        self.course_fixture.add_advanced_settings(
+            {'discussion_topics': {'value': {'Test Discussion Topic': {'id': self.discussion_id}}}}
+        )
+        self.course_fixture.install()
+
+    def create_single_thread_page(self, thread_id):
+        """
+        Sets up a `DiscussionTabSingleThreadPage` for a given
+        `thread_id`.
+        """
+        return DiscussionTabSingleThreadPage(self.browser, self.course_id, self.discussion_id, thread_id)

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

@@ -3,7 +3,7 @@ Tests related to the cohorting feature.
 """
 from uuid import uuid4
 
-from .helpers import BaseDiscussionMixin
+from .helpers import BaseDiscussionMixin, BaseDiscussionTestCase
 from .helpers import CohortTestMixin
 from ..helpers import UniqueCourseTest
 from ...pages.lms.auto_auth import AutoAuthPage
@@ -57,20 +57,17 @@ class CohortedDiscussionTestMixin(BaseDiscussionMixin, CohortTestMixin):
         self.assertEquals(self.thread_page.get_group_visibility_label(), "This post is visible to everyone.")
 
 
-class DiscussionTabSingleThreadTest(UniqueCourseTest):
+class DiscussionTabSingleThreadTest(BaseDiscussionTestCase):
     """
     Tests for the discussion page displaying a single thread.
     """
     def setUp(self):
         super(DiscussionTabSingleThreadTest, self).setUp()
-        self.discussion_id = "test_discussion_{}".format(uuid4().hex)
-        # Create a course to register for
-        self.course_fixture = CourseFixture(**self.course_info).install()
         self.setup_cohorts()
         AutoAuthPage(self.browser, course_id=self.course_id).visit()
 
     def setup_thread_page(self, thread_id):
-        self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, thread_id)  # pylint: disable=attribute-defined-outside-init
+        self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, self.discussion_id, thread_id)  # pylint: disable=attribute-defined-outside-init
         self.thread_page.visit()
 
     # pylint: disable=unused-argument

lms/djangoapps/django_comment_client/base/tests.py

@@ -14,7 +14,7 @@ from lms.lib.comment_client import Thread
 from xmodule.modulestore.tests.django_utils import TEST_DATA_MOCK_MODULESTORE
 from django_comment_client.base import views
 from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin, GroupIdAssertionMixin
-from django_comment_client.tests.utils import CohortedContentTestCase
+from django_comment_client.tests.utils import CohortedTestCase
 from django_comment_client.tests.unicode import UnicodeTestMixin
 from django_comment_common.models import Role
 from django_comment_common.utils import seed_permissions_roles
@@ -42,7 +42,7 @@ class MockRequestSetupMixin(object):
 @patch('lms.lib.comment_client.utils.requests.request')
 class CreateThreadGroupIdTestCase(
         MockRequestSetupMixin,
-        CohortedContentTestCase,
+        CohortedTestCase,
         CohortedTopicGroupIdTestMixin,
         NonCohortedTopicGroupIdTestMixin
 ):
@@ -77,7 +77,7 @@ class CreateThreadGroupIdTestCase(
 @patch('lms.lib.comment_client.utils.requests.request')
 class ThreadActionGroupIdTestCase(
         MockRequestSetupMixin,
-        CohortedContentTestCase,
+        CohortedTestCase,
         GroupIdAssertionMixin
 ):
     def call_view(

lms/djangoapps/django_comment_client/base/views.py

@@ -74,7 +74,7 @@ def track_forum_event(request, event_name, course, obj, data, id_map=None):
     user = request.user
     data['id'] = obj.id
     if id_map is None:
-        id_map = get_discussion_id_map(course)
+        id_map = get_discussion_id_map(course, user)
 
     commentable_id = data['commentable_id']
     if commentable_id in id_map:
@@ -173,9 +173,9 @@ def create_thread(request, course_id, commentable_id):
 
     # Calls to id map are expensive, but we need this more than once.
     # Prefetch it.
-    id_map = get_discussion_id_map(course)
+    id_map = get_discussion_id_map(course, request.user)
 
-    add_courseware_context([data], course, id_map=id_map)
+    add_courseware_context([data], course, request.user, id_map=id_map)
 
     track_forum_event(request, 'edx.forum.thread.created',
                       course, thread, event_data, id_map=id_map)
@@ -208,7 +208,7 @@ def update_thread(request, course_id, thread_id):
         thread.thread_type = request.POST["thread_type"]
     if "commentable_id" in request.POST:
         course = get_course_with_access(request.user, 'load', course_key)
-        commentable_ids = get_discussion_categories_ids(course)
+        commentable_ids = get_discussion_categories_ids(course, request.user)
         if request.POST.get("commentable_id") in commentable_ids:
             thread.commentable_id = request.POST["commentable_id"]
         else:

lms/djangoapps/django_comment_client/forum/views.py

@@ -52,7 +52,7 @@ def _attr_safe_json(obj):
 
 
 @newrelic.agent.function_trace()
-def make_course_settings(course):
+def make_course_settings(course, user):
     """
     Generate a JSON-serializable model for course settings, which will be used to initialize a
     DiscussionCourseSettings object on the client.
@@ -63,14 +63,14 @@ def make_course_settings(course):
         'allow_anonymous': course.allow_anonymous,
         'allow_anonymous_to_peers': course.allow_anonymous_to_peers,
         'cohorts': [{"id": str(g.id), "name": g.name} for g in get_course_cohorts(course)],
-        'category_map': utils.get_discussion_category_map(course)
+        'category_map': utils.get_discussion_category_map(course, user)
     }
 
     return obj
 
 
 @newrelic.agent.function_trace()
-def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PAGE):
+def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
     """
     This may raise an appropriate subclass of cc.utils.CommentClientError
     if something goes wrong, or ValueError if the group_id is invalid.
@@ -81,12 +81,16 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA
         'sort_key': 'date',
         'sort_order': 'desc',
         'text': '',
-        'commentable_id': discussion_id,
-        'course_id': course_key.to_deprecated_string(),
+        'course_id': unicode(course.id),
         'user_id': request.user.id,
-        'group_id': get_group_id_for_comments_service(request, course_key, discussion_id),  # may raise ValueError
+        'group_id': get_group_id_for_comments_service(request, course.id, discussion_id),  # may raise ValueError
     }
 
+    # If provided with a discussion id, filter by discussion id in the
+    # comments_service.
+    if discussion_id is not None:
+        default_query_params['commentable_id'] = discussion_id
+
     if not request.GET.get('sort_key'):
         # If the user did not select a sort key, use their last used sort key
         cc_user = cc.User.from_django_user(request.user)
@@ -124,6 +128,15 @@ 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)
 
+    # If not provided with a discussion id, filter threads by commentable ids
+    # which are accessible to the current user.
+    if discussion_id is None:
+        discussion_category_ids = set(utils.get_discussion_categories_ids(course, request.user))
+        threads = [
+            thread for thread in threads
+            if thread.get('commentable_id') in discussion_category_ids
+        ]
+
     for thread in threads:
         # patch for backward compatibility to comments service
         if 'pinned' not in thread:
@@ -163,7 +176,7 @@ def inline_discussion(request, course_key, discussion_id):
     user_info = cc_user.to_dict()
 
     try:
-        threads, query_params = get_threads(request, course_key, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
+        threads, query_params = get_threads(request, course, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
     except ValueError:
         return HttpResponseBadRequest("Invalid group_id")
 
@@ -172,7 +185,7 @@ def inline_discussion(request, course_key, discussion_id):
     is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
     threads = [utils.prepare_content(thread, course_key, is_staff) for thread in threads]
     with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
-        add_courseware_context(threads, course)
+        add_courseware_context(threads, course, request.user)
     return utils.JsonResponse({
         'is_commentable_cohorted': is_commentable_cohorted(course_key, discussion_id),
         'discussion_data': threads,
@@ -181,7 +194,7 @@ def inline_discussion(request, course_key, discussion_id):
         'page': query_params['page'],
         'num_pages': query_params['num_pages'],
         'roles': utils.get_role_ids(course_key),
-        'course_settings': make_course_settings(course)
+        'course_settings': make_course_settings(course, request.user)
     })
 
 
@@ -194,13 +207,13 @@ def forum_form_discussion(request, course_key):
     nr_transaction = newrelic.agent.current_transaction()
 
     course = get_course_with_access(request.user, 'load_forum', course_key, check_if_enrolled=True)
-    course_settings = make_course_settings(course)
+    course_settings = make_course_settings(course, request.user)
 
     user = cc.User.from_django_user(request.user)
     user_info = user.to_dict()
 
     try:
-        unsafethreads, query_params = get_threads(request, course_key)   # This might process a search query
+        unsafethreads, query_params = get_threads(request, course)   # This might process a search query
         is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
         threads = [utils.prepare_content(thread, course_key, is_staff) for thread in unsafethreads]
     except cc.utils.CommentClientMaintenanceError:
@@ -213,7 +226,7 @@ def forum_form_discussion(request, course_key):
         annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info)
 
     with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
-        add_courseware_context(threads, course)
+        add_courseware_context(threads, course, request.user)
 
     if request.is_ajax():
         return utils.JsonResponse({
@@ -258,15 +271,18 @@ def single_thread(request, course_key, discussion_id, thread_id):
     """
     Renders a response to display a single discussion thread.
     """
-
     nr_transaction = newrelic.agent.current_transaction()
 
     course = get_course_with_access(request.user, 'load_forum', course_key)
-    course_settings = make_course_settings(course)
+    course_settings = make_course_settings(course, request.user)
     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)
 
+    # Verify that the student has access to this thread if belongs to a discussion module
+    if discussion_id not in utils.get_discussion_categories_ids(course, request.user):
+        raise Http404
+
     # 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
     # the comments service.
@@ -294,7 +310,7 @@ def single_thread(request, course_key, discussion_id, thread_id):
             annotated_content_info = utils.get_annotated_content_infos(course_key, thread, request.user, user_info=user_info)
         content = utils.prepare_content(thread.to_dict(), course_key, is_staff)
         with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
-            add_courseware_context([content], course)
+            add_courseware_context([content], course, request.user)
         return utils.JsonResponse({
             'content': content,
             'annotated_content_info': annotated_content_info,
@@ -302,13 +318,13 @@ def single_thread(request, course_key, discussion_id, thread_id):
 
     else:
         try:
-            threads, query_params = get_threads(request, course_key)
+            threads, query_params = get_threads(request, course)
         except ValueError:
             return HttpResponseBadRequest("Invalid group_id")
         threads.append(thread.to_dict())
 
         with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
-            add_courseware_context(threads, course)
+            add_courseware_context(threads, course, request.user)
 
         for thread in threads:
             # patch for backward compatibility with comments service

lms/djangoapps/django_comment_client/tests/utils.py

+"""
+Utilities for tests within the django_comment_client module.
+"""
+from datetime import datetime
 from mock import patch
+from pytz import UTC
 
-from openedx.core.djangoapps.course_groups.models import CourseUserGroup
+from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup
+from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory
 from django_comment_common.models import Role
 from django_comment_common.utils import seed_permissions_roles
 from student.tests.factories import CourseEnrollmentFactory, UserFactory
-from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.partitions.partitions import UserPartition, Group
 
 
-class CohortedContentTestCase(ModuleStoreTestCase):
+class CohortedTestCase(ModuleStoreTestCase):
     """
     Sets up a course with a student, a moderator and their cohorts.
     """
     @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
     def setUp(self):
-        super(CohortedContentTestCase, self).setUp()
+        super(CohortedTestCase, self).setUp()
 
         self.course = CourseFactory.create(
-            discussion_topics={
-                "cohorted topic": {"id": "cohorted_topic"},
-                "non-cohorted topic": {"id": "non_cohorted_topic"},
-            },
             cohort_config={
                 "cohorted": True,
                 "cohorted_discussions": ["cohorted_topic"]
             }
         )
-        self.student_cohort = CourseUserGroup.objects.create(
+        self.student_cohort = CohortFactory.create(
             name="student_cohort",
-            course_id=self.course.id,
-            group_type=CourseUserGroup.COHORT
+            course_id=self.course.id
         )
-        self.moderator_cohort = CourseUserGroup.objects.create(
+        self.moderator_cohort = CohortFactory.create(
             name="moderator_cohort",
-            course_id=self.course.id,
-            group_type=CourseUserGroup.COHORT
+            course_id=self.course.id
         )
+        self.course.discussion_topics["cohorted topic"] = {"id": "cohorted_topic"}
+        self.course.discussion_topics["non-cohorted topic"] = {"id": "non_cohorted_topic"}
+        self.store.update_item(self.course, self.user.id)
 
         seed_permissions_roles(self.course.id)
         self.student = UserFactory.create()
@@ -45,3 +49,82 @@ 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)
+
+
+class ContentGroupTestCase(ModuleStoreTestCase):
+    """
+    Sets up discussion modules visible to content groups 'Alpha' and
+    'Beta', as well as a module visible to all students.  Creates a
+    staff user, users with access to Alpha/Beta (by way of cohorts),
+    and a non-cohorted user with no special access.
+    """
+    def setUp(self):
+        super(ContentGroupTestCase, self).setUp()
+
+        self.course = CourseFactory.create(
+            org='org', number='number', run='run',
+            # This test needs to use a course that has already started --
+            # discussion topics only show up if the course has already started,
+            # and the default start date for courses is Jan 1, 2030.
+            start=datetime(2012, 2, 3, tzinfo=UTC),
+            user_partitions=[
+                UserPartition(
+                    0,
+                    'Content Group Configuration',
+                    '',
+                    [Group(1, 'Alpha'), Group(2, 'Beta')],
+                    scheme_id='cohort'
+                )
+            ],
+            cohort_config={'cohorted': True},
+            discussion_topics={}
+        )
+
+        self.staff_user = UserFactory.create(is_staff=True)
+        self.alpha_user = UserFactory.create()
+        self.beta_user = UserFactory.create()
+        self.non_cohorted_user = UserFactory.create()
+        for user in [self.staff_user, self.alpha_user, self.beta_user, self.non_cohorted_user]:
+            CourseEnrollmentFactory.create(user=user, course_id=self.course.id)
+
+        alpha_cohort = CohortFactory(
+            course_id=self.course.id,
+            name='Cohort Alpha',
+            users=[self.alpha_user]
+        )
+        beta_cohort = CohortFactory(
+            course_id=self.course.id,
+            name='Cohort Beta',
+            users=[self.beta_user]
+        )
+        CourseUserGroupPartitionGroup.objects.create(
+            course_user_group=alpha_cohort,
+            partition_id=self.course.user_partitions[0].id,
+            group_id=self.course.user_partitions[0].groups[0].id
+        )
+        CourseUserGroupPartitionGroup.objects.create(
+            course_user_group=beta_cohort,
+            partition_id=self.course.user_partitions[0].id,
+            group_id=self.course.user_partitions[0].groups[1].id
+        )
+        self.alpha_module = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='alpha_group_discussion',
+            discussion_target='Visible to Alpha',
+            group_access={self.course.user_partitions[0].id: [self.course.user_partitions[0].groups[0].id]}
+        )
+        self.beta_module = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='beta_group_discussion',
+            discussion_target='Visible to Beta',
+            group_access={self.course.user_partitions[0].id: [self.course.user_partitions[0].groups[1].id]}
+        )
+        self.global_module = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='global_group_discussion',
+            discussion_target='Visible to Everyone'
+        )
+        self.course = self.store.get_item(self.course.location)

lms/djangoapps/django_comment_client/utils.py

@@ -18,6 +18,7 @@ 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
 
+from courseware.access import has_access
 from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted, \
     is_course_cohorted
 from openedx.core.djangoapps.course_groups.models import CourseUserGroup
@@ -59,9 +60,11 @@ def has_forum_access(uname, course_id, rolename):
     return role.users.filter(username=uname).exists()
 
 
-def _get_discussion_modules(course):
+# pylint: disable=invalid-name
+def get_accessible_discussion_modules(course, user):
     """
-    Return a list of all valid discussion modules in this course.
+    Return a list of all valid discussion modules in this course that
+    are accessible to the given user.
     """
     all_modules = modulestore().get_items(course.id, qualifiers={'category': 'discussion'})
 
@@ -72,12 +75,16 @@ def _get_discussion_modules(course):
                 return False
         return True
 
-    return filter(has_required_keys, all_modules)
+    return [
+        module for module in all_modules
+        if has_required_keys(module) and has_access(user, 'load', module, course.id)
+    ]
 
 
-def get_discussion_id_map(course):
+def get_discussion_id_map(course, user):
     """
-    Transform the list of this course's discussion modules into a dictionary of metadata keyed by discussion_id.
+    Transform the list of this course's discussion modules (visible to a given user) into a dictionary of metadata keyed
+    by discussion_id.
     """
     def get_entry(module):  # pylint: disable=missing-docstring
         discussion_id = module.discussion_id
@@ -85,7 +92,7 @@ def get_discussion_id_map(course):
         last_category = module.discussion_category.split("/")[-1].strip()
         return (discussion_id, {"location": module.location, "title": last_category + " / " + title})
 
-    return dict(map(get_entry, _get_discussion_modules(course)))
+    return dict(map(get_entry, get_accessible_discussion_modules(course, user)))
 
 
 def _filter_unstarted_categories(category_map):
@@ -138,14 +145,14 @@ def _sort_map_entries(category_map, sort_alpha):
     category_map["children"] = [x[0] for x in sorted(things, key=lambda x: x[1]["sort_key"])]
 
 
-def get_discussion_category_map(course):
+def get_discussion_category_map(course, user):
     """
     Transform the list of this course's discussion modules into a recursive dictionary structure.  This is used
-    to render the discussion category map in the discussion tab sidebar.
+    to render the discussion category map in the discussion tab sidebar for a given user.
     """
     unexpanded_category_map = defaultdict(list)
 
-    modules = _get_discussion_modules(course)
+    modules = get_accessible_discussion_modules(course, user)
 
     is_course_cohorted = course.is_cohorted
     cohorted_discussion_ids = course.cohorted_discussions
@@ -218,21 +225,15 @@ def get_discussion_category_map(course):
     return _filter_unstarted_categories(category_map)
 
 
-def get_discussion_categories_ids(course):
+def get_discussion_categories_ids(course, user):
     """
-    Returns a list of available ids of categories for the course.
+    Returns a list of available ids of categories for the course that
+    are accessible to the given user.
     """
-    ids = []
-    queue = [get_discussion_category_map(course)]
-    while queue:
-        category_map = queue.pop()
-        for child in category_map["children"]:
-            if child in category_map["entries"]:
-                ids.append(category_map["entries"][child]["id"])
-            else:
-                queue.append(category_map["subcategories"][child])
-
-    return ids
+    accessible_discussion_ids = [
+        module.discussion_id for module in get_accessible_discussion_modules(course, user)
+    ]
+    return course.top_level_discussion_topic_ids + accessible_discussion_ids
 
 
 class JsonResponse(HttpResponse):
@@ -382,12 +383,12 @@ def extend_content(content):
     return merge_dict(content, content_info)
 
 
-def add_courseware_context(content_list, course, id_map=None):
+def add_courseware_context(content_list, course, user, id_map=None):
     """
     Decorates `content_list` with courseware metadata.
     """
     if id_map is None:
-        id_map = get_discussion_id_map(course)
+        id_map = get_discussion_id_map(course, user)
 
     for content in content_list:
         commentable_id = content['commentable_id']