From 934abf3c1959fbb41e971eb89bd0abb1368b83da Mon Sep 17 00:00:00 2001 From: Daniel Friedman Date: Mon, 26 Jan 2015 21:55:25 -0500 Subject: [PATCH] Check access for discussion modules in forums TNL-650 Conflicts: lms/djangoapps/django_comment_client/base/views.py lms/djangoapps/django_comment_client/tests/test_utils.py lms/djangoapps/django_comment_client/tests/utils.py lms/djangoapps/django_comment_client/utils.py --- .../test/acceptance/pages/lms/discussion.py | 6 +- .../acceptance/tests/discussion/helpers.py | 24 +- .../tests/discussion/test_cohorts.py | 9 +- .../tests/discussion/test_discussion.py | 68 ++--- .../django_comment_client/base/tests.py | 6 +- .../django_comment_client/base/views.py | 8 +- .../django_comment_client/forum/tests.py | 276 +++++++++++++----- .../django_comment_client/forum/views.py | 50 ++-- .../django_comment_client/tests/test_utils.py | 235 +++++++++++++-- .../django_comment_client/tests/utils.py | 111 ++++++- lms/djangoapps/django_comment_client/utils.py | 49 ++-- 11 files changed, 625 insertions(+), 217 deletions(-) diff --git a/common/test/acceptance/pages/lms/discussion.py b/common/test/acceptance/pages/lms/discussion.py index b026a9bdb8..5fe900079e 100644 --- a/common/test/acceptance/pages/lms/discussion.py +++ b/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() diff --git a/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py index 46cdba1155..f0722cd236 100644 --- a/common/test/acceptance/tests/discussion/helpers.py +++ b/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) diff --git a/common/test/acceptance/tests/discussion/test_cohorts.py b/common/test/acceptance/tests/discussion/test_cohorts.py index 3c567a15b9..9bc1a3f52f 100644 --- a/common/test/acceptance/tests/discussion/test_cohorts.py +++ b/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 diff --git a/common/test/acceptance/tests/discussion/test_discussion.py b/common/test/acceptance/tests/discussion/test_discussion.py index 9efcadb629..8aebf75efc 100644 --- a/common/test/acceptance/tests/discussion/test_discussion.py +++ b/common/test/acceptance/tests/discussion/test_discussion.py @@ -7,6 +7,7 @@ from pytz import UTC from uuid import uuid4 from nose.plugins.attrib import attr +from .helpers import BaseDiscussionTestCase from ..helpers import UniqueCourseTest from ...pages.lms.auto_auth import AutoAuthPage from ...pages.lms.courseware import CoursewarePage @@ -139,22 +140,17 @@ class DiscussionHomePageTest(UniqueCourseTest): @attr('shard_1') -class DiscussionTabSingleThreadTest(UniqueCourseTest, DiscussionResponsePaginationTestMixin): +class DiscussionTabSingleThreadTest(BaseDiscussionTestCase, DiscussionResponsePaginationTestMixin): """ 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 - CourseFixture(**self.course_info).install() - 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 = self.create_single_thread_page(thread_id) # pylint: disable=attribute-defined-outside-init self.thread_page.visit() def test_marked_answer_comments(self): @@ -180,7 +176,7 @@ class DiscussionTabSingleThreadTest(UniqueCourseTest, DiscussionResponsePaginati @attr('shard_1') -class DiscussionOpenClosedThreadTest(UniqueCourseTest): +class DiscussionOpenClosedThreadTest(BaseDiscussionTestCase): """ Tests for checking the display of attributes on open and closed threads """ @@ -188,8 +184,6 @@ class DiscussionOpenClosedThreadTest(UniqueCourseTest): def setUp(self): super(DiscussionOpenClosedThreadTest, self).setUp() - # Create a course to register for - CourseFixture(**self.course_info).install() self.thread_id = "test_thread_{}".format(uuid4().hex) def setup_user(self, roles=[]): @@ -197,6 +191,7 @@ class DiscussionOpenClosedThreadTest(UniqueCourseTest): self.user_id = AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str).visit().get_user_id() def setup_view(self, **thread_kwargs): + thread_kwargs.update({'commentable_id': self.discussion_id}) view = SingleThreadViewFixture( Thread(id=self.thread_id, **thread_kwargs) ) @@ -209,7 +204,7 @@ class DiscussionOpenClosedThreadTest(UniqueCourseTest): self.setup_view(closed=True) else: self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, self.thread_id) + page = self.create_single_thread_page(self.thread_id) page.visit() page.close_open_thread() return page @@ -230,23 +225,16 @@ class DiscussionOpenClosedThreadTest(UniqueCourseTest): @attr('shard_1') -class DiscussionCommentDeletionTest(UniqueCourseTest): +class DiscussionCommentDeletionTest(BaseDiscussionTestCase): """ Tests for deleting comments displayed beneath responses in the single thread view. """ - - def setUp(self): - super(DiscussionCommentDeletionTest, self).setUp() - - # Create a course to register for - CourseFixture(**self.course_info).install() - def setup_user(self, roles=[]): roles_str = ','.join(roles) self.user_id = AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str).visit().get_user_id() def setup_view(self): - view = SingleThreadViewFixture(Thread(id="comment_deletion_test_thread")) + view = SingleThreadViewFixture(Thread(id="comment_deletion_test_thread", commentable_id=self.discussion_id)) view.addResponse( Response(id="response1"), [Comment(id="comment_other_author", user_id="other"), Comment(id="comment_self_author", user_id=self.user_id)]) @@ -255,7 +243,7 @@ class DiscussionCommentDeletionTest(UniqueCourseTest): def test_comment_deletion_as_student(self): self.setup_user() self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_deletion_test_thread") + page = self.create_single_thread_page("comment_deletion_test_thread") page.visit() self.assertTrue(page.is_comment_deletable("comment_self_author")) self.assertTrue(page.is_comment_visible("comment_other_author")) @@ -265,7 +253,7 @@ class DiscussionCommentDeletionTest(UniqueCourseTest): def test_comment_deletion_as_moderator(self): self.setup_user(roles=['Moderator']) self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_deletion_test_thread") + page = self.create_single_thread_page("comment_deletion_test_thread") page.visit() self.assertTrue(page.is_comment_deletable("comment_self_author")) self.assertTrue(page.is_comment_deletable("comment_other_author")) @@ -274,23 +262,16 @@ class DiscussionCommentDeletionTest(UniqueCourseTest): @attr('shard_1') -class DiscussionResponseEditTest(UniqueCourseTest): +class DiscussionResponseEditTest(BaseDiscussionTestCase): """ Tests for editing responses displayed beneath thread in the single thread view. """ - - def setUp(self): - super(DiscussionResponseEditTest, self).setUp() - - # Create a course to register for - CourseFixture(**self.course_info).install() - def setup_user(self, roles=[]): roles_str = ','.join(roles) self.user_id = AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str).visit().get_user_id() def setup_view(self): - view = SingleThreadViewFixture(Thread(id="response_edit_test_thread")) + view = SingleThreadViewFixture(Thread(id="response_edit_test_thread", commentable_id=self.discussion_id)) view.addResponse( Response(id="response_other_author", user_id="other", thread_id="response_edit_test_thread"), ) @@ -317,7 +298,7 @@ class DiscussionResponseEditTest(UniqueCourseTest): """ self.setup_user() self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "response_edit_test_thread") + page = self.create_single_thread_page("response_edit_test_thread") page.visit() self.assertTrue(page.is_response_visible("response_other_author")) self.assertFalse(page.is_response_editable("response_other_author")) @@ -334,7 +315,7 @@ class DiscussionResponseEditTest(UniqueCourseTest): """ self.setup_user(roles=["Moderator"]) self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "response_edit_test_thread") + page = self.create_single_thread_page("response_edit_test_thread") page.visit() self.edit_response(page, "response_self_author") self.edit_response(page, "response_other_author") @@ -362,7 +343,7 @@ class DiscussionResponseEditTest(UniqueCourseTest): """ self.setup_user(roles=["Moderator"]) self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "response_edit_test_thread") + page = self.create_single_thread_page("response_edit_test_thread") page.visit() self.edit_response(page, "response_self_author") self.edit_response(page, "response_other_author") @@ -375,23 +356,16 @@ class DiscussionResponseEditTest(UniqueCourseTest): @attr('shard_1') -class DiscussionCommentEditTest(UniqueCourseTest): +class DiscussionCommentEditTest(BaseDiscussionTestCase): """ Tests for editing comments displayed beneath responses in the single thread view. """ - - def setUp(self): - super(DiscussionCommentEditTest, self).setUp() - - # Create a course to register for - CourseFixture(**self.course_info).install() - def setup_user(self, roles=[]): roles_str = ','.join(roles) self.user_id = AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str).visit().get_user_id() def setup_view(self): - view = SingleThreadViewFixture(Thread(id="comment_edit_test_thread")) + view = SingleThreadViewFixture(Thread(id="comment_edit_test_thread", commentable_id=self.discussion_id)) view.addResponse( Response(id="response1"), [Comment(id="comment_other_author", user_id="other"), Comment(id="comment_self_author", user_id=self.user_id)]) @@ -406,7 +380,7 @@ class DiscussionCommentEditTest(UniqueCourseTest): def test_edit_comment_as_student(self): self.setup_user() self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_edit_test_thread") + page = self.create_single_thread_page("comment_edit_test_thread") page.visit() self.assertTrue(page.is_comment_editable("comment_self_author")) self.assertTrue(page.is_comment_visible("comment_other_author")) @@ -416,7 +390,7 @@ class DiscussionCommentEditTest(UniqueCourseTest): def test_edit_comment_as_moderator(self): self.setup_user(roles=["Moderator"]) self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_edit_test_thread") + page = self.create_single_thread_page("comment_edit_test_thread") page.visit() self.assertTrue(page.is_comment_editable("comment_self_author")) self.assertTrue(page.is_comment_editable("comment_other_author")) @@ -426,7 +400,7 @@ class DiscussionCommentEditTest(UniqueCourseTest): def test_cancel_comment_edit(self): self.setup_user() self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_edit_test_thread") + page = self.create_single_thread_page("comment_edit_test_thread") page.visit() self.assertTrue(page.is_comment_editable("comment_self_author")) original_body = page.get_comment_body("comment_self_author") @@ -438,7 +412,7 @@ class DiscussionCommentEditTest(UniqueCourseTest): """Only one editor should be visible at a time within a single response""" self.setup_user(roles=["Moderator"]) self.setup_view() - page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_edit_test_thread") + page = self.create_single_thread_page("comment_edit_test_thread") page.visit() self.assertTrue(page.is_comment_editable("comment_self_author")) self.assertTrue(page.is_comment_editable("comment_other_author")) diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index da31e2e7bb..f949f434f9 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/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( diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 133f5af161..dc41404e28 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/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: diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 8e2ec25ed4..b7c3c731bd 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -2,6 +2,7 @@ import json import logging import ddt +from django.core import cache from django.core.urlresolvers import reverse from django.http import Http404 from django.test.client import Client, RequestFactory @@ -14,7 +15,7 @@ from django_comment_client.tests.group_id import ( NonCohortedTopicGroupIdTestMixin ) from django_comment_client.tests.unicode import UnicodeTestMixin -from django_comment_client.tests.utils import CohortedContentTestCase +from django_comment_client.tests.utils import CohortedTestCase, ContentGroupTestCase from django_comment_client.utils import strip_none from student.tests.factories import UserFactory, CourseEnrollmentFactory from util.testing import UrlResetMixin @@ -104,13 +105,15 @@ class ViewsExceptionTestCase(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(self.response.status_code, 404) -def make_mock_thread_data(text, thread_id, num_children, group_id=None, group_name=None, commentable_id=None): +def make_mock_thread_data(course, text, thread_id, num_children, group_id=None, group_name=None, commentable_id=None): thread_data = { "id": thread_id, "type": "thread", "title": text, "body": text, - "commentable_id": commentable_id or "dummy_commentable_id", + "commentable_id": ( + commentable_id or course.discussion_topics.get('General', {}).get('id') or "dummy_commentable_id" + ), "resp_total": 42, "resp_skip": 25, "resp_limit": 5, @@ -128,21 +131,33 @@ def make_mock_thread_data(text, thread_id, num_children, group_id=None, group_na def make_mock_request_impl( - text, - thread_id="dummy_thread_id", - group_id=None, - commentable_id=None, - num_thread_responses=1, + course, + text, + thread_id="dummy_thread_id", + group_id=None, + commentable_id=None, + num_thread_responses=1 ): def mock_request_impl(*args, **kwargs): url = args[1] data = None if url.endswith("threads") or url.endswith("user_profile"): data = { - "collection": [make_mock_thread_data(text, thread_id, None, group_id=group_id, commentable_id=commentable_id)] + "collection": [ + make_mock_thread_data( + course=course, + text=text, + thread_id=thread_id, + num_children=None, + group_id=group_id, + commentable_id=commentable_id + ) + ] } elif thread_id and url.endswith(thread_id): - data = make_mock_thread_data(text, thread_id, num_thread_responses, group_id=group_id) + data = make_mock_thread_data( + course=course, text=text, thread_id=thread_id, num_children=num_thread_responses, group_id=group_id + ) elif "/users/" in url: data = { "default_sort_key": "date", @@ -186,14 +201,14 @@ class SingleThreadTestCase(ModuleStoreTestCase): def setUp(self): super(SingleThreadTestCase, self).setUp(create_user=False) - self.course = CourseFactory.create() + self.course = CourseFactory.create(discussion_topics={'dummy discussion': {'id': 'dummy_discussion_id'}}) self.student = UserFactory.create() CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) def test_ajax(self, mock_request): text = "dummy content" thread_id = "test_thread_id" - mock_request.side_effect = make_mock_request_impl(text, thread_id) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text, thread_id=thread_id) request = RequestFactory().get( "dummy_url", @@ -213,7 +228,7 @@ class SingleThreadTestCase(ModuleStoreTestCase): # django view performs prior to writing thread data to the response self.assertEquals( response_data["content"], - strip_none(make_mock_thread_data(text, thread_id, 1)) + strip_none(make_mock_thread_data(course=self.course, text=text, thread_id=thread_id, num_children=1)) ) mock_request.assert_called_with( "get", @@ -229,7 +244,7 @@ class SingleThreadTestCase(ModuleStoreTestCase): thread_id = "test_thread_id" response_skip = "45" response_limit = "15" - mock_request.side_effect = make_mock_request_impl(text, thread_id) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text, thread_id=thread_id) request = RequestFactory().get( "dummy_url", @@ -249,7 +264,7 @@ class SingleThreadTestCase(ModuleStoreTestCase): # django view performs prior to writing thread data to the response self.assertEquals( response_data["content"], - strip_none(make_mock_thread_data(text, thread_id, 1)) + strip_none(make_mock_thread_data(course=self.course, text=text, thread_id=thread_id, num_children=1)) ) mock_request.assert_called_with( "get", @@ -280,7 +295,7 @@ class SingleThreadTestCase(ModuleStoreTestCase): request = RequestFactory().get("dummy_url") request.user = self.student # Mock request to return 404 for thread request - mock_request.side_effect = make_mock_request_impl("dummy", thread_id=None) + mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy", thread_id=None) self.assertRaises( Http404, views.single_thread, @@ -301,34 +316,42 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase): MODULESTORE = TEST_DATA_MONGO_MODULESTORE @ddt.data( - # old mongo: number of responses plus 16. TODO: O(n)! - (ModuleStoreEnum.Type.mongo, 1, 17), - (ModuleStoreEnum.Type.mongo, 50, 66), + # old mongo with cache: number of responses plus 17. TODO: O(n)! + (ModuleStoreEnum.Type.mongo, 1, 23, 18), + (ModuleStoreEnum.Type.mongo, 50, 366, 67), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, 1, 3), - (ModuleStoreEnum.Type.split, 50, 3), + (ModuleStoreEnum.Type.split, 1, 3, 3), + (ModuleStoreEnum.Type.split, 50, 3, 3), ) @ddt.unpack - def test_number_of_mongo_queries(self, default_store, num_thread_responses, num_mongo_calls, mock_request): - + def test_number_of_mongo_queries( + self, + default_store, + num_thread_responses, + num_uncached_mongo_calls, + num_cached_mongo_calls, + mock_request + ): with modulestore().default_store(default_store): - course = CourseFactory.create() + course = CourseFactory.create(discussion_topics={'dummy discussion': {'id': 'dummy_discussion_id'}}) student = UserFactory.create() CourseEnrollmentFactory.create(user=student, course_id=course.id) test_thread_id = "test_thread_id" mock_request.side_effect = make_mock_request_impl( - "dummy content", - test_thread_id, - num_thread_responses=num_thread_responses, + course=course, text="dummy content", thread_id=test_thread_id, num_thread_responses=num_thread_responses ) request = RequestFactory().get( "dummy_url", HTTP_X_REQUESTED_WITH="XMLHttpRequest" ) request.user = student - with check_mongo_calls(num_mongo_calls): + + def call_single_thread(): + """ + Call single_thread and assert that it returns what we expect. + """ response = views.single_thread( request, course.id.to_deprecated_string(), @@ -338,15 +361,35 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase): self.assertEquals(response.status_code, 200) self.assertEquals(len(json.loads(response.content)["content"]["children"]), num_thread_responses) + # TODO: update this once django cache is disabled in tests + # Test with and without cache, clearing before and after use. + single_thread_local_cache = cache.get_cache( + backend='default', + LOCATION='single_thread_local_cache' + ) + single_thread_dummy_cache = cache.get_cache( + backend='django.core.cache.backends.dummy.DummyCache', + LOCATION='single_thread_local_cache' + ) + cached_calls = { + single_thread_dummy_cache: num_uncached_mongo_calls, + single_thread_local_cache: num_cached_mongo_calls + } + for single_thread_cache, expected_calls in cached_calls.items(): + single_thread_cache.clear() + with patch("django_comment_client.permissions.CACHE", single_thread_cache): + with check_mongo_calls(expected_calls): + call_single_thread() + single_thread_cache.clear() + @patch('requests.request') -class SingleCohortedThreadTestCase(CohortedContentTestCase): +class SingleCohortedThreadTestCase(CohortedTestCase): 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 + course=self.course, text=self.mock_text, thread_id=self.mock_thread_id, group_id=self.student_cohort.id ) def test_ajax(self, mock_request): @@ -360,7 +403,7 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase): response = views.single_thread( request, self.course.id.to_deprecated_string(), - "dummy_discussion_id", + "cohorted_topic", self.mock_thread_id ) @@ -369,9 +412,12 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase): self.assertEquals( response_data["content"], make_mock_thread_data( - self.mock_text, self.mock_thread_id, 1, + course=self.course, + text=self.mock_text, + thread_id=self.mock_thread_id, + num_children=1, group_id=self.student_cohort.id, - group_name=self.student_cohort.name, + group_name=self.student_cohort.name ) ) @@ -384,7 +430,7 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase): response = views.single_thread( request, self.course.id.to_deprecated_string(), - "dummy_discussion_id", + "cohorted_topic", self.mock_thread_id ) @@ -397,10 +443,12 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase): @patch('lms.lib.comment_client.utils.requests.request') -class SingleThreadAccessTestCase(CohortedContentTestCase): +class SingleThreadAccessTestCase(CohortedTestCase): 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) + mock_request.side_effect = make_mock_request_impl( + course=self.course, text="dummy context", thread_id=thread_id, group_id=thread_group_id + ) request_data = {} if pass_group_id: @@ -482,11 +530,13 @@ class SingleThreadAccessTestCase(CohortedContentTestCase): @patch('lms.lib.comment_client.utils.requests.request') -class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): +class SingleThreadGroupIdTestCase(CohortedTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/threads" def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True, is_ajax=False): - mock_request.side_effect = make_mock_request_impl("dummy context", group_id=self.student_cohort.id) + mock_request.side_effect = make_mock_request_impl( + course=self.course, text="dummy context", group_id=self.student_cohort.id + ) request_data = {} if pass_group_id: @@ -504,7 +554,7 @@ class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdT return views.single_thread( request, self.course.id.to_deprecated_string(), - "dummy_discussion_id", + commentable_id, "dummy_thread_id" ) @@ -531,16 +581,98 @@ class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdT ) +@patch('requests.request') +class SingleThreadContentGroupTestCase(ContentGroupTestCase): + def assert_can_access(self, user, discussion_id, thread_id, should_have_access): + """ + Verify that a user has access to a thread within a given + discussion_id when should_have_access is True, otherwise + verify that the user does not have access to that thread. + """ + request = RequestFactory().get("dummy_url") + request.user = user + mako_middleware_process_request(request) + + def call_single_thread(): + return views.single_thread( + request, + unicode(self.course.id), + discussion_id, + thread_id + ) + + if should_have_access: + self.assertEqual(call_single_thread().status_code, 200) + else: + with self.assertRaises(Http404): + call_single_thread() + + def test_staff_user(self, mock_request): + """ + Verify that the staff user can access threads in the alpha, + beta, and global discussion modules. + """ + thread_id = "test_thread_id" + mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id) + + for discussion_module in [self.alpha_module, self.beta_module, self.global_module]: + self.assert_can_access(self.staff_user, discussion_module.discussion_id, thread_id, True) + + def test_alpha_user(self, mock_request): + """ + Verify that the alpha user can access threads in the alpha and + global discussion modules. + """ + thread_id = "test_thread_id" + mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id) + + for discussion_module in [self.alpha_module, self.global_module]: + self.assert_can_access(self.alpha_user, discussion_module.discussion_id, thread_id, True) + + self.assert_can_access(self.alpha_user, self.beta_module.discussion_id, thread_id, False) + + def test_beta_user(self, mock_request): + """ + Verify that the beta user can access threads in the beta and + global discussion modules. + """ + thread_id = "test_thread_id" + mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id) + + for discussion_module in [self.beta_module, self.global_module]: + self.assert_can_access(self.beta_user, discussion_module.discussion_id, thread_id, True) + + self.assert_can_access(self.beta_user, self.alpha_module.discussion_id, thread_id, False) + + def test_non_cohorted_user(self, mock_request): + """ + Verify that the non-cohorted user can access threads in just the + global discussion module. + """ + thread_id = "test_thread_id" + mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id) + + self.assert_can_access(self.non_cohorted_user, self.global_module.discussion_id, thread_id, True) + + self.assert_can_access(self.non_cohorted_user, self.alpha_module.discussion_id, thread_id, False) + + self.assert_can_access(self.non_cohorted_user, self.beta_module.discussion_id, thread_id, False) + + @patch('lms.lib.comment_client.utils.requests.request') class InlineDiscussionGroupIdTestCase( - CohortedContentTestCase, + CohortedTestCase, CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin ): cs_endpoint = "/threads" + def setUp(self): + super(InlineDiscussionGroupIdTestCase, self).setUp() + self.cohorted_commentable_id = 'cohorted_topic' + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): - kwargs = {} + kwargs = {'commentable_id': self.cohorted_commentable_id} if group_id: # avoid causing a server error when the LMS chokes attempting # to find a group name for the group_id, when we're testing with @@ -550,7 +682,7 @@ class InlineDiscussionGroupIdTestCase( kwargs['group_id'] = group_id except CourseUserGroup.DoesNotExist: pass - mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs) + mock_request.side_effect = make_mock_request_impl(self.course, "dummy content", **kwargs) request_data = {} if pass_group_id: @@ -569,7 +701,7 @@ class InlineDiscussionGroupIdTestCase( def test_group_info_in_ajax_response(self, mock_request): response = self.call_view( mock_request, - "cohorted_topic", + self.cohorted_commentable_id, self.student, self.student_cohort.id ) @@ -579,14 +711,14 @@ class InlineDiscussionGroupIdTestCase( @patch('lms.lib.comment_client.utils.requests.request') -class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): +class ForumFormDiscussionGroupIdTestCase(CohortedTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/threads" def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True, is_ajax=False): kwargs = {} if group_id: kwargs['group_id'] = group_id - mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs) + mock_request.side_effect = make_mock_request_impl(self.course, "dummy content", **kwargs) request_data = {} if pass_group_id: @@ -629,7 +761,7 @@ class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicG @patch('lms.lib.comment_client.utils.requests.request') -class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): +class UserProfileDiscussionGroupIdTestCase(CohortedTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/active_threads" def call_view_for_profiled_user( @@ -642,7 +774,7 @@ class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopi kwargs = {} if group_id: kwargs['group_id'] = group_id - mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs) + mock_request.side_effect = make_mock_request_impl(self.course, "dummy content", **kwargs) request_data = {} if pass_group_id: @@ -795,14 +927,14 @@ class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopi @patch('lms.lib.comment_client.utils.requests.request') -class FollowedThreadsDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): +class FollowedThreadsDiscussionGroupIdTestCase(CohortedTestCase, CohortedTopicGroupIdTestMixin): cs_endpoint = "/subscribed_threads" def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): kwargs = {} if group_id: kwargs['group_id'] = group_id - mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs) + mock_request.side_effect = make_mock_request_impl(self.course, "dummy content", **kwargs) request_data = {} if pass_group_id: @@ -851,9 +983,13 @@ class InlineDiscussionTestCase(ModuleStoreTestCase): def test_courseware_data(self, mock_request): request = RequestFactory().get("dummy_url") request.user = self.student - mock_request.side_effect = make_mock_request_impl("dummy content", commentable_id=self.discussion1.discussion_id) + mock_request.side_effect = make_mock_request_impl( + course=self.course, text="dummy content", commentable_id=self.discussion1.discussion_id + ) - response = views.inline_discussion(request, self.course.id.to_deprecated_string(), "dummy_discussion_id") + response = views.inline_discussion( + request, self.course.id.to_deprecated_string(), self.discussion1.discussion_id + ) self.assertEqual(response.status_code, 200) response_data = json.loads(response.content) expected_courseware_url = '/courses/TestX/101/Test_Course/jump_to/i4x://TestX/101/discussion/Discussion1' @@ -877,7 +1013,9 @@ class UserProfileTestCase(ModuleStoreTestCase): CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) def get_response(self, mock_request, params, **headers): - mock_request.side_effect = make_mock_request_impl(self.TEST_THREAD_TEXT, self.TEST_THREAD_ID) + mock_request.side_effect = make_mock_request_impl( + course=self.course, text=self.TEST_THREAD_TEXT, thread_id=self.TEST_THREAD_ID + ) request = RequestFactory().get("dummy_url", data=params, **headers) request.user = self.student @@ -964,7 +1102,9 @@ class UserProfileTestCase(ModuleStoreTestCase): ) def test_post(self, mock_request): - mock_request.side_effect = make_mock_request_impl(self.TEST_THREAD_TEXT, self.TEST_THREAD_ID) + mock_request.side_effect = make_mock_request_impl( + course=self.course, text=self.TEST_THREAD_TEXT, thread_id=self.TEST_THREAD_ID + ) request = RequestFactory().post("dummy_url") request.user = self.student response = views.user_profile( @@ -986,7 +1126,7 @@ class CommentsServiceRequestHeadersTestCase(UrlResetMixin, ModuleStoreTestCase): # Invoke UrlResetMixin super(CommentsServiceRequestHeadersTestCase, self).setUp(create_user=False) - self.course = CourseFactory.create() + self.course = CourseFactory.create(discussion_topics={'dummy discussion': {'id': 'dummy_discussion_id'}}) self.student = UserFactory.create(username=username, password=password) CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) self.assertTrue( @@ -1009,14 +1149,14 @@ class CommentsServiceRequestHeadersTestCase(UrlResetMixin, ModuleStoreTestCase): lang = "eo" text = "dummy content" thread_id = "test_thread_id" - mock_request.side_effect = make_mock_request_impl(text, thread_id) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text, thread_id=thread_id) self.client.get( reverse( "django_comment_client.forum.views.single_thread", kwargs={ "course_id": self.course.id.to_deprecated_string(), - "discussion_id": "dummy", + "discussion_id": "dummy_discussion_id", "thread_id": thread_id, } ), @@ -1026,7 +1166,7 @@ class CommentsServiceRequestHeadersTestCase(UrlResetMixin, ModuleStoreTestCase): @override_settings(COMMENTS_SERVICE_KEY="test_api_key") def test_api_key(self, mock_request): - mock_request.side_effect = make_mock_request_impl("dummy", "dummy") + mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy", thread_id="dummy") self.client.get( reverse( @@ -1047,11 +1187,13 @@ class InlineDiscussionUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.side_effect = make_mock_request_impl(text) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) request = RequestFactory().get("dummy_url") request.user = self.student - response = views.inline_discussion(request, self.course.id.to_deprecated_string(), "dummy_discussion_id") + response = views.inline_discussion( + request, self.course.id.to_deprecated_string(), self.course.discussion_topics['General']['id'] + ) self.assertEqual(response.status_code, 200) response_data = json.loads(response.content) self.assertEqual(response_data["discussion_data"][0]["title"], text) @@ -1068,7 +1210,7 @@ class ForumFormDiscussionUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.side_effect = make_mock_request_impl(text) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) request = RequestFactory().get("dummy_url") request.user = self.student request.META["HTTP_X_REQUESTED_WITH"] = "XMLHttpRequest" # so request.is_ajax() == True @@ -1090,7 +1232,7 @@ class ForumDiscussionSearchUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.side_effect = make_mock_request_impl(text) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) data = { "ajax": 1, "text": text, @@ -1110,14 +1252,14 @@ class SingleThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): def setUp(self): super(SingleThreadUnicodeTestCase, self).setUp() - self.course = CourseFactory.create() + self.course = CourseFactory.create(discussion_topics={'dummy_discussion_id': {'id': 'dummy_discussion_id'}}) self.student = UserFactory.create() CourseEnrollmentFactory(user=self.student, course_id=self.course.id) @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): thread_id = "test_thread_id" - mock_request.side_effect = make_mock_request_impl(text, thread_id) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text, thread_id=thread_id) request = RequestFactory().get("dummy_url") request.user = self.student request.META["HTTP_X_REQUESTED_WITH"] = "XMLHttpRequest" # so request.is_ajax() == True @@ -1139,7 +1281,7 @@ class UserProfileUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.side_effect = make_mock_request_impl(text) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) request = RequestFactory().get("dummy_url") request.user = self.student request.META["HTTP_X_REQUESTED_WITH"] = "XMLHttpRequest" # so request.is_ajax() == True @@ -1161,7 +1303,7 @@ class FollowedThreadsUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.side_effect = make_mock_request_impl(text) + mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) request = RequestFactory().get("dummy_url") request.user = self.student request.META["HTTP_X_REQUESTED_WITH"] = "XMLHttpRequest" # so request.is_ajax() == True @@ -1188,7 +1330,7 @@ class EnrollmentTestCase(ModuleStoreTestCase): @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @patch('lms.lib.comment_client.utils.requests.request') def test_unenrolled(self, mock_request): - mock_request.side_effect = make_mock_request_impl('dummy') + mock_request.side_effect = make_mock_request_impl(course=self.course, text='dummy') request = RequestFactory().get('dummy_url') request.user = self.student with self.assertRaises(UserNotEnrolled): diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index d1cb4e83da..57f61e1de0 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/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 diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index c84d15c1ee..79d4cadf0e 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -10,6 +10,7 @@ import mock from django_comment_client.tests.factories import RoleFactory from django_comment_client.tests.unicode import UnicodeTestMixin +from django_comment_client.tests.utils import ContentGroupTestCase import django_comment_client.utils as utils from student.tests.factories import UserFactory, CourseEnrollmentFactory from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -88,7 +89,7 @@ class CoursewareContextTestCase(ModuleStoreTestCase): comment client service integration """ def setUp(self): - super(CoursewareContextTestCase, self).setUp() + super(CoursewareContextTestCase, self).setUp(create_user=True) self.course = CourseFactory.create(org="TestX", number="101", display_name="Test Course") self.discussion1 = ItemFactory.create( @@ -107,12 +108,12 @@ class CoursewareContextTestCase(ModuleStoreTestCase): ) def test_empty(self): - utils.add_courseware_context([], self.course) + utils.add_courseware_context([], self.course, self.user) def test_missing_commentable_id(self): orig = {"commentable_id": "non-inline"} modified = dict(orig) - utils.add_courseware_context([modified], self.course) + utils.add_courseware_context([modified], self.course, self.user) self.assertEqual(modified, orig) def test_basic(self): @@ -120,7 +121,7 @@ class CoursewareContextTestCase(ModuleStoreTestCase): {"commentable_id": self.discussion1.discussion_id}, {"commentable_id": self.discussion2.discussion_id} ] - utils.add_courseware_context(threads, self.course) + utils.add_courseware_context(threads, self.course, self.user) def assertThreadCorrect(thread, discussion, expected_title): # pylint: disable=invalid-name """Asserts that the given thread has the expected set of properties""" @@ -144,13 +145,29 @@ class CoursewareContextTestCase(ModuleStoreTestCase): assertThreadCorrect(threads[1], self.discussion2, "Subsection / Discussion 2") -class CategoryMapTestCase(ModuleStoreTestCase): +class CategoryMapTestMixin(object): + """ + Provides functionality for classes that test + `get_discussion_category_map`. + """ + def assert_category_map_equals(self, expected, requesting_user=None): + """ + Call `get_discussion_category_map`, and verify that it returns + what is expected. + """ + self.assertEqual( + utils.get_discussion_category_map(self.course, requesting_user or self.user), + expected + ) + + +class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase): """ Base testcase class for discussion categories for the comment client service integration """ def setUp(self): - super(CategoryMapTestCase, self).setUp() + super(CategoryMapTestCase, self).setUp(create_user=True) self.course = CourseFactory.create( org="TestX", number="101", display_name="Test Course", @@ -176,20 +193,8 @@ class CategoryMapTestCase(ModuleStoreTestCase): **kwargs ) - def assertCategoryMapEquals(self, expected): - """ - Compare a manually-constructed category map to the actual result from `utils.get_discussion_category_map` - """ - self.assertEqual( - utils.get_discussion_category_map(self.course), - expected - ) - def test_empty(self): - self.assertEqual( - utils.get_discussion_category_map(self.course), - {"entries": {}, "subcategories": {}, "children": []} - ) + self.assert_category_map_equals({"entries": {}, "subcategories": {}, "children": []}) def test_configured_topics(self): self.course.discussion_topics = { @@ -199,7 +204,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): } def check_cohorted_topics(expected_ids): # pylint: disable=missing-docstring - self.assertCategoryMapEquals( + self.assert_category_map_equals( { "entries": { "Topic A": {"id": "Topic_A", "sort_key": "Topic A", "is_cohorted": "Topic_A" in expected_ids}, @@ -231,7 +236,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): def test_single_inline(self): self.create_discussion("Chapter", "Discussion") - self.assertCategoryMapEquals( + self.assert_category_map_equals( { "entries": {}, "subcategories": { @@ -261,7 +266,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): def check_cohorted(is_cohorted): - self.assertCategoryMapEquals( + self.assert_category_map_equals( { "entries": {}, "subcategories": { @@ -362,7 +367,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): self.create_discussion("Chapter 2 / Section 1 / Subsection 1", "Discussion") self.create_discussion("Chapter 2 / Section 1 / Subsection 1", "Discussion") # duplicate - category_map = utils.get_discussion_category_map(self.course) + category_map = utils.get_discussion_category_map(self.course, self.user) chapter1 = category_map["subcategories"]["Chapter 1"] chapter1_discussions = set(["Discussion A", "Discussion B", "Discussion A (1)", "Discussion A (2)"]) @@ -385,7 +390,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): self.create_discussion("Chapter 2 / Section 1 / Subsection 2", "Discussion", start=later) self.create_discussion("Chapter 3 / Section 1", "Discussion", start=later) - self.assertCategoryMapEquals( + self.assert_category_map_equals( { "entries": {}, "subcategories": { @@ -423,7 +428,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): self.create_discussion("Chapter", "Discussion 4", sort_key="C") self.create_discussion("Chapter", "Discussion 5", sort_key="B") - self.assertCategoryMapEquals( + self.assert_category_map_equals( { "entries": {}, "subcategories": { @@ -475,7 +480,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): "Topic B": {"id": "Topic_B", "sort_key": "C"}, "Topic C": {"id": "Topic_C", "sort_key": "A"} } - self.assertCategoryMapEquals( + self.assert_category_map_equals( { "entries": { "Topic A": {"id": "Topic_A", "sort_key": "B", "is_cohorted": False}, @@ -496,7 +501,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): self.create_discussion("Chapter", "Discussion C") self.create_discussion("Chapter", "Discussion B") - self.assertCategoryMapEquals( + self.assert_category_map_equals( { "entries": {}, "subcategories": { @@ -549,7 +554,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): self.create_discussion("Chapter B", "Discussion 1") self.create_discussion("Chapter A", "Discussion 2") - self.assertCategoryMapEquals( + self.assert_category_map_equals( { "entries": {}, "subcategories": { @@ -602,7 +607,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): ) def test_ids_empty(self): - self.assertEqual(utils.get_discussion_categories_ids(self.course), []) + self.assertEqual(utils.get_discussion_categories_ids(self.course, self.user), []) def test_ids_configured_topics(self): self.course.discussion_topics = { @@ -611,7 +616,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): "Topic C": {"id": "Topic_C"} } self.assertItemsEqual( - utils.get_discussion_categories_ids(self.course), + utils.get_discussion_categories_ids(self.course, self.user), ["Topic_A", "Topic_B", "Topic_C"] ) @@ -623,7 +628,7 @@ class CategoryMapTestCase(ModuleStoreTestCase): self.create_discussion("Chapter 2 / Section 1 / Subsection 2", "Discussion") self.create_discussion("Chapter 3 / Section 1", "Discussion") self.assertItemsEqual( - utils.get_discussion_categories_ids(self.course), + utils.get_discussion_categories_ids(self.course, self.user), ["discussion1", "discussion2", "discussion3", "discussion4", "discussion5", "discussion6"] ) @@ -637,11 +642,177 @@ class CategoryMapTestCase(ModuleStoreTestCase): self.create_discussion("Chapter 2", "Discussion") self.create_discussion("Chapter 2 / Section 1 / Subsection 1", "Discussion") self.assertItemsEqual( - utils.get_discussion_categories_ids(self.course), + utils.get_discussion_categories_ids(self.course, self.user), ["Topic_A", "Topic_B", "Topic_C", "discussion1", "discussion2", "discussion3"] ) +class ContentGroupCategoryMapTestCase(CategoryMapTestMixin, ContentGroupTestCase): + """ + Tests `get_discussion_category_map` on discussion modules which are + only visible to some content groups. + """ + def test_staff_user(self): + """ + Verify that the staff user can access the alpha, beta, and + global discussion topics. + """ + self.assert_category_map_equals( + { + 'subcategories': { + 'Week 1': { + 'subcategories': {}, + 'children': [ + 'Visible to Alpha', + 'Visible to Beta', + 'Visible to Everyone' + ], + 'entries': { + 'Visible to Alpha': { + 'sort_key': None, + 'is_cohorted': True, + 'id': 'alpha_group_discussion' + }, + 'Visible to Beta': { + 'sort_key': None, + 'is_cohorted': True, + 'id': 'beta_group_discussion' + }, + 'Visible to Everyone': { + 'sort_key': None, + 'is_cohorted': True, + 'id': 'global_group_discussion' + } + } + } + }, + 'children': ['General', 'Week 1'], + 'entries': { + 'General': { + 'sort_key': 'General', + 'is_cohorted': False, + 'id': 'i4x-org-number-course-run' + } + } + }, + requesting_user=self.staff_user + ) + + def test_alpha_user(self): + """ + Verify that the alpha user can access the alpha and global + discussion topics. + """ + self.assert_category_map_equals( + { + 'subcategories': { + 'Week 1': { + 'subcategories': {}, + 'children': [ + 'Visible to Alpha', + 'Visible to Everyone' + ], + 'entries': { + 'Visible to Alpha': { + 'sort_key': None, + 'is_cohorted': True, + 'id': 'alpha_group_discussion' + }, + 'Visible to Everyone': { + 'sort_key': None, + 'is_cohorted': True, + 'id': 'global_group_discussion' + } + } + } + }, + 'children': ['General', 'Week 1'], + 'entries': { + 'General': { + 'sort_key': 'General', + 'is_cohorted': False, + 'id': 'i4x-org-number-course-run' + } + } + }, + requesting_user=self.alpha_user + ) + + def test_beta_user(self): + """ + Verify that the beta user can access the beta and global + discussion topics. + """ + self.assert_category_map_equals( + { + 'subcategories': { + 'Week 1': { + 'subcategories': {}, + 'children': [ + 'Visible to Beta', + 'Visible to Everyone' + ], + 'entries': { + 'Visible to Beta': { + 'sort_key': None, + 'is_cohorted': True, + 'id': 'beta_group_discussion' + }, + 'Visible to Everyone': { + 'sort_key': None, + 'is_cohorted': True, + 'id': 'global_group_discussion' + } + } + } + }, + 'children': ['General', 'Week 1'], + 'entries': { + 'General': { + 'sort_key': 'General', + 'is_cohorted': False, + 'id': 'i4x-org-number-course-run' + } + } + }, + requesting_user=self.beta_user + ) + + def test_non_cohorted_user(self): + """ + Verify that the non-cohorted user can access the global + discussion topic. + """ + self.assert_category_map_equals( + { + 'subcategories': { + 'Week 1': { + 'subcategories': {}, + 'children': [ + 'Visible to Everyone' + ], + 'entries': { + 'Visible to Everyone': { + 'sort_key': None, + 'is_cohorted': True, + 'id': 'global_group_discussion' + } + } + } + }, + 'children': ['General', 'Week 1'], + 'entries': { + 'General': { + 'sort_key': 'General', + 'is_cohorted': False, + 'id': 'i4x-org-number-course-run' + } + } + }, + requesting_user=self.non_cohorted_user + ) + + class JsonResponseTestCase(TestCase, UnicodeTestMixin): def _test_unicode_data(self, text): response = utils.JsonResponse(text) diff --git a/lms/djangoapps/django_comment_client/tests/utils.py b/lms/djangoapps/django_comment_client/tests/utils.py index 557e36c202..084c78cbfd 100644 --- a/lms/djangoapps/django_comment_client/tests/utils.py +++ b/lms/djangoapps/django_comment_client/tests/utils.py @@ -1,41 +1,45 @@ +""" +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) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 966e335eed..0a139850fa 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/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']