diff --git a/common/test/acceptance/tests/discussion/test_cohorts.py b/common/test/acceptance/tests/discussion/test_cohorts.py index 60cd25923c90..047978342a75 100644 --- a/common/test/acceptance/tests/discussion/test_cohorts.py +++ b/common/test/acceptance/tests/discussion/test_cohorts.py @@ -62,6 +62,8 @@ def setup_cohorts(self): self.cohort_1_id = self.add_cohort(self.cohort_1_name) def test_cohort_visibility_label(self): + # Must be moderator to view content in a cohort other than your own + AutoAuthPage(self.browser, course_id=self.course_id, roles="Moderator").visit() self.setup_thread(1, group_id=self.cohort_1_id) self.assertEquals( self.thread_page.get_group_visibility_label(), @@ -128,13 +130,11 @@ def setUp(self): self.user_id = AutoAuthPage(self.browser, course_id=self.course_id).visit().get_user_id() - self.courseware_page = CoursewarePage(self.browser, self.course_id) - self.courseware_page.visit() - self.discussion_page = InlineDiscussionPage(self.browser, self.discussion_id) - def setup_thread_page(self, thread_id): - self.discussion_page.expand_discussion() - self.assertEqual(self.discussion_page.get_num_displayed_threads(), 1) + CoursewarePage(self.browser, self.course_id).visit() + discussion_page = InlineDiscussionPage(self.browser, self.discussion_id) + discussion_page.expand_discussion() + self.assertEqual(discussion_page.get_num_displayed_threads(), 1) self.thread_page = InlineDiscussionThreadPage(self.browser, thread_id) # pylint:disable=W0201 self.thread_page.expand() diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index bbb3c3d13a31..531d6c87e2ce 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -12,6 +12,7 @@ from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE from django_comment_client.base import views +from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin from django_comment_client.tests.utils import CohortedContentTestCase from django_comment_client.tests.unicode import UnicodeTestMixin from django_comment_common.models import Role, FORUM_ROLE_STUDENT @@ -36,111 +37,27 @@ def _set_mock_request_data(self, mock_request, data): @patch('lms.lib.comment_client.utils.requests.request') -class CreateCohortedThreadTestCase(CohortedContentTestCase): - """ - Tests how `views.create_thread` passes `group_id` to the comments service - for cohorted topics. - """ - def _create_thread_in_cohorted_topic( - self, - user, - mock_request, - group_id, - pass_group_id=True, - expected_status_code=200 - ): - self._create_thread(user, "cohorted_topic", mock_request, group_id, pass_group_id, expected_status_code) - - def test_student_without_group_id(self, mock_request): - self._create_thread_in_cohorted_topic(self.student, mock_request, None, pass_group_id=False) - self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id) - - def test_student_none_group_id(self, mock_request): - self._create_thread_in_cohorted_topic(self.student, mock_request, None) - self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id) - - def test_student_with_own_group_id(self, mock_request): - self._create_thread_in_cohorted_topic(self.student, mock_request, self.student_cohort.id) - self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id) - - def test_student_with_other_group_id(self, mock_request): - self._create_thread_in_cohorted_topic(self.student, mock_request, self.moderator_cohort.id) - self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id) - - def test_moderator_without_group_id(self, mock_request): - self._create_thread_in_cohorted_topic(self.moderator, mock_request, None, pass_group_id=False) - self._assert_mock_request_called_with_group_id(mock_request, self.moderator_cohort.id) - - def test_moderator_none_group_id(self, mock_request): - self._create_thread_in_cohorted_topic(self.moderator, mock_request, None, expected_status_code=400) - self.assertFalse(mock_request.called) - - def test_moderator_with_own_group_id(self, mock_request): - self._create_thread_in_cohorted_topic(self.moderator, mock_request, self.moderator_cohort.id) - self._assert_mock_request_called_with_group_id(mock_request, self.moderator_cohort.id) - - def test_moderator_with_other_group_id(self, mock_request): - self._create_thread_in_cohorted_topic(self.moderator, mock_request, self.student_cohort.id) - self._assert_mock_request_called_with_group_id(mock_request, self.student_cohort.id) - - def test_moderator_with_invalid_group_id(self, mock_request): - invalid_id = self.student_cohort.id + self.moderator_cohort.id - self._create_thread_in_cohorted_topic(self.moderator, mock_request, invalid_id, expected_status_code=400) - self.assertFalse(mock_request.called) - +class CreateThreadGroupIdTestCase( + CohortedContentTestCase, + CohortedTopicGroupIdTestMixin, + NonCohortedTopicGroupIdTestMixin +): + cs_endpoint = "/threads" + + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + mock_request.return_value.status_code = 200 + request_data = {"body": "body", "title": "title", "thread_type": "discussion"} + if pass_group_id: + request_data["group_id"] = group_id + request = RequestFactory().post("dummy_url", request_data) + request.user = user + request.view_name = "create_thread" -@patch('lms.lib.comment_client.utils.requests.request') -class CreateNonCohortedThreadTestCase(CohortedContentTestCase): - """ - Tests how `views.create_thread` passes `group_id` to the comments service - for non-cohorted topics. - """ - def _create_thread_in_non_cohorted_topic( - self, - user, - mock_request, - group_id, - pass_group_id=True, - expected_status_code=200 - ): - self._create_thread(user, "non_cohorted_topic", mock_request, group_id, pass_group_id, expected_status_code) - - def test_student_without_group_id(self, mock_request): - self._create_thread_in_non_cohorted_topic(self.student, mock_request, None, pass_group_id=False) - self._assert_mock_request_called_without_group_id(mock_request) - - def test_student_none_group_id(self, mock_request): - self._create_thread_in_non_cohorted_topic(self.student, mock_request, None) - self._assert_mock_request_called_without_group_id(mock_request) - - def test_student_with_own_group_id(self, mock_request): - self._create_thread_in_non_cohorted_topic(self.student, mock_request, self.student_cohort.id) - self._assert_mock_request_called_without_group_id(mock_request) - - def test_student_with_other_group_id(self, mock_request): - self._create_thread_in_non_cohorted_topic(self.student, mock_request, self.moderator_cohort.id) - self._assert_mock_request_called_without_group_id(mock_request) - - def test_moderator_without_group_id(self, mock_request): - self._create_thread_in_non_cohorted_topic(self.moderator, mock_request, None, pass_group_id=False) - self._assert_mock_request_called_without_group_id(mock_request) - - def test_moderator_none_group_id(self, mock_request): - self._create_thread_in_non_cohorted_topic(self.student, mock_request, None) - self._assert_mock_request_called_without_group_id(mock_request) - - def test_moderator_with_own_group_id(self, mock_request): - self._create_thread_in_non_cohorted_topic(self.moderator, mock_request, self.moderator_cohort.id) - self._assert_mock_request_called_without_group_id(mock_request) - - def test_moderator_with_other_group_id(self, mock_request): - self._create_thread_in_non_cohorted_topic(self.moderator, mock_request, self.student_cohort.id) - self._assert_mock_request_called_without_group_id(mock_request) - - def test_moderator_with_invalid_group_id(self, mock_request): - invalid_id = self.student_cohort.id + self.moderator_cohort.id - self._create_thread_in_non_cohorted_topic(self.moderator, mock_request, invalid_id) - self._assert_mock_request_called_without_group_id(mock_request) + return views.create_thread( + request, + course_id=self.course.id.to_deprecated_string(), + commentable_id=commentable_id + ) @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index a8f1968056ee..fe95d7dc3568 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -28,7 +28,9 @@ JsonError, JsonResponse, safe_content, - add_thread_group_name) + add_thread_group_name, + get_group_id_for_comments_service +) from django_comment_client.permissions import check_permissions_by_view, cached_has_permission import lms.lib.comment_client as cc @@ -102,32 +104,13 @@ def create_thread(request, course_id, commentable_id): title=post["title"] ) - user = cc.User.from_django_user(request.user) - - #kevinchugh because the new requirement is that all groups will be determined - #by the group id in the request this all goes away - #not anymore, only for admins - - # Cohort the thread if the commentable is cohorted. - if is_commentable_cohorted(course_key, commentable_id): - user_group_id = get_cohort_id(user, course_key) - - # TODO (vshnayder): once we have more than just cohorts, we'll want to - # change this to a single get_group_for_user_and_commentable function - # that can do different things depending on the commentable_id - if cached_has_permission(request.user, "see_all_cohorts", course_key): - # admins can optionally choose what group to post as - try: - group_id = int(post.get('group_id', user_group_id)) - get_cohort_by_id(course_key, group_id) - except (ValueError, CourseUserGroup.DoesNotExist): - return HttpResponseBadRequest("Invalid cohort id") - else: - # regular users always post with their own id. - group_id = user_group_id - - if group_id: - thread.group_id = group_id + # Cohort the thread if required + try: + group_id = get_group_id_for_comments_service(request, course_key, commentable_id) + except ValueError: + return HttpResponseBadRequest("Invalid cohort id") + if group_id is not None: + thread.group_id = group_id thread.save() diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 87cf77838b6a..c11da0a0dc6d 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -10,7 +10,13 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from django.core.urlresolvers import reverse from util.testing import UrlResetMixin +from django_comment_client.tests.group_id import ( + GroupIdAssertionMixin, + CohortedTopicGroupIdTestMixin, + NonCohortedTopicGroupIdTestMixin +) from django_comment_client.tests.unicode import UnicodeTestMixin +from django_comment_client.tests.utils import CohortedContentTestCase from django_comment_client.forum import views from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE @@ -267,17 +273,7 @@ def test_not_found(self, mock_request): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @patch('requests.request') -class SingleCohortedThreadTestCase(ModuleStoreTestCase): - def setUp(self): - self.course = CourseFactory.create() - self.student = UserFactory.create() - CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) - self.student_cohort = CourseUserGroup.objects.create( - name="student_cohort", - course_id=self.course.id, - group_type=CourseUserGroup.COHORT - ) - +class SingleCohortedThreadTestCase(CohortedContentTestCase): def _create_mock_cohorted_thread(self, mock_request): self.mock_text = "dummy content" self.mock_thread_id = "test_thread_id" @@ -333,6 +329,197 @@ def test_html(self, mock_request): self.assertRegexpMatches(html, r'"group_name": "student_cohort"') +@patch('lms.lib.comment_client.utils.requests.request') +class SingleThreadAccessTestCase(CohortedContentTestCase): + 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) + + request_data = {} + if pass_group_id: + request_data["group_id"] = group_id + request = RequestFactory().get( + "dummy_url", + data=request_data, + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + request.user = user + return views.single_thread( + request, + self.course.id.to_deprecated_string(), + commentable_id, + thread_id + ) + + def test_student_non_cohorted(self, mock_request): + resp = self.call_view(mock_request, "non_cohorted_topic", self.student, self.student_cohort.id) + self.assertEqual(resp.status_code, 200) + + def test_student_same_cohort(self, mock_request): + resp = self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id, + thread_group_id=self.student_cohort.id + ) + self.assertEqual(resp.status_code, 200) + + def test_student_different_cohort(self, mock_request): + self.assertRaises( + Http404, + lambda: self.call_view( + mock_request, + "cohorted_topic", + self.student, + self.student_cohort.id, + thread_group_id=self.moderator_cohort.id + ) + ) + + def test_moderator_non_cohorted(self, mock_request): + resp = self.call_view(mock_request, "non_cohorted_topic", self.moderator, self.moderator_cohort.id) + self.assertEqual(resp.status_code, 200) + + def test_moderator_same_cohort(self, mock_request): + resp = self.call_view( + mock_request, + "cohorted_topic", + self.moderator, + self.moderator_cohort.id, + thread_group_id=self.moderator_cohort.id + ) + self.assertEqual(resp.status_code, 200) + + def test_moderator_different_cohort(self, mock_request): + resp = self.call_view( + mock_request, + "cohorted_topic", + self.moderator, + self.moderator_cohort.id, + thread_group_id=self.student_cohort.id + ) + self.assertEqual(resp.status_code, 200) + + +@patch('lms.lib.comment_client.utils.requests.request') +class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): + cs_endpoint = "/threads" + + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + mock_request.side_effect = make_mock_request_impl("dummy context", group_id=self.student_cohort.id) + + request_data = {} + if pass_group_id: + request_data["group_id"] = group_id + request = RequestFactory().get( + "dummy_url", + data=request_data + ) + request.user = user + mako_middleware_process_request(request) + return views.single_thread( + request, + self.course.id.to_deprecated_string(), + "dummy_discussion_id", + "dummy_thread_id" + ) + + +@patch('lms.lib.comment_client.utils.requests.request') +class InlineDiscussionGroupIdTestCase( + CohortedContentTestCase, + CohortedTopicGroupIdTestMixin, + NonCohortedTopicGroupIdTestMixin +): + cs_endpoint = "/threads" + + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + mock_request.side_effect = make_mock_request_impl("dummy content") + + request_data = {} + if pass_group_id: + request_data["group_id"] = group_id + request = RequestFactory().get( + "dummy_url", + data=request_data + ) + request.user = user + return views.inline_discussion( + request, + self.course.id.to_deprecated_string(), + commentable_id + ) + + +@patch('lms.lib.comment_client.utils.requests.request') +class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): + cs_endpoint = "/threads" + + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + mock_request.side_effect = make_mock_request_impl("dummy content") + + request_data = {} + if pass_group_id: + request_data["group_id"] = group_id + request = RequestFactory().get( + "dummy_url", + data=request_data + ) + request.user = user + mako_middleware_process_request(request) + return views.forum_form_discussion( + request, + self.course.id.to_deprecated_string() + ) + + +@patch('lms.lib.comment_client.utils.requests.request') +class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): + cs_endpoint = "/active_threads" + + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + mock_request.side_effect = make_mock_request_impl("dummy content") + + request_data = {} + if pass_group_id: + request_data["group_id"] = group_id + request = RequestFactory().get( + "dummy_url", + data=request_data + ) + request.user = user + mako_middleware_process_request(request) + return views.user_profile( + request, + self.course.id.to_deprecated_string(), + user.id + ) + + +@patch('lms.lib.comment_client.utils.requests.request') +class FollowedThreadsDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin): + cs_endpoint = "/subscribed_threads" + + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + mock_request.side_effect = make_mock_request_impl("dummy content") + + request_data = {} + if pass_group_id: + request_data["group_id"] = group_id + request = RequestFactory().get( + "dummy_url", + data=request_data, + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + request.user = user + return views.followed_threads( + request, + self.course.id.to_deprecated_string(), + user.id + ) + + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @patch('requests.request') class UserProfileTestCase(ModuleStoreTestCase): diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index f30837435371..615654f0e29e 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -5,8 +5,7 @@ from django.contrib.auth.decorators import login_required from django.core.context_processors import csrf from django.contrib.auth.models import User -from django.http import Http404 -from django.utils.translation import ugettext as _ +from django.http import Http404, HttpResponseBadRequest from django.views.decorators.http import require_GET import newrelic.agent @@ -17,7 +16,14 @@ from courseware.access import has_access from django_comment_client.permissions import cached_has_permission -from django_comment_client.utils import (merge_dict, extract, strip_none, add_courseware_context, add_thread_group_name) +from django_comment_client.utils import ( + merge_dict, + extract, + strip_none, + add_courseware_context, + add_thread_group_name, + get_group_id_for_comments_service +) import django_comment_client.utils as utils import lms.lib.comment_client as cc @@ -58,7 +64,7 @@ def make_course_settings(course, include_category_map=False): def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PAGE): """ This may raise an appropriate subclass of cc.utils.CommentClientError - if something goes wrong. + if something goes wrong, or ValueError if the group_id is invalid. """ default_query_params = { 'page': 1, @@ -69,6 +75,7 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA 'commentable_id': discussion_id, 'course_id': course_key.to_deprecated_string(), 'user_id': request.user.id, + 'group_id': get_group_id_for_comments_service(request, course_key, discussion_id), # may raise ValueError } if not request.GET.get('sort_key'): @@ -87,22 +94,6 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA #is user a moderator #did the user request a group - #if the user requested a group explicitly, give them that group, otherwise, if mod, show all, else if student, use cohort - - group_id = request.GET.get('group_id') - - if group_id == "all": - group_id = None - - if not group_id: - if not cached_has_permission(request.user, "see_all_cohorts", course_key): - group_id = get_cohort_id(request.user, course_key) - - if group_id: - default_query_params["group_id"] = group_id - - #so by default, a moderator sees all items, and a student sees his cohort - query_params = merge_dict( default_query_params, strip_none( @@ -148,11 +139,14 @@ def inline_discussion(request, course_id, discussion_id): course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) course = get_course_with_access(request.user, 'load_forum', course_id) - - threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE) cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() + try: + threads, query_params = get_threads(request, course_id, discussion_id, per_page=INLINE_THREADS_PER_PAGE) + except ValueError: + return HttpResponseBadRequest("Invalid group_id") + with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) @@ -177,6 +171,9 @@ def forum_form_discussion(request, course_id): course = get_course_with_access(request.user, 'load_forum', course_id) course_settings = make_course_settings(course, include_category_map=True) + user = cc.User.from_django_user(request.user) + user_info = user.to_dict() + try: unsafethreads, query_params = get_threads(request, course_id) # This might process a search query is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) @@ -184,9 +181,8 @@ def forum_form_discussion(request, course_id): except cc.utils.CommentClientMaintenanceError: log.warning("Forum is in maintenance mode") return render_to_response('discussion/maintenance.html', {}) - - user = cc.User.from_django_user(request.user) - user_info = user.to_dict() + except ValueError: + return HttpResponseBadRequest("Invalid group_id") with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) @@ -240,6 +236,7 @@ def single_thread(request, course_id, discussion_id, thread_id): course_settings = make_course_settings(course, include_category_map=True) cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() + is_moderator = cached_has_permission(request.user, "see_all_cohorts", course_key) # 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 @@ -256,6 +253,12 @@ def single_thread(request, course_id, discussion_id, thread_id): raise Http404 raise + # verify that the thread belongs to the requesting student's cohort + if is_commentable_cohorted(course_key, discussion_id) and not is_moderator: + user_group_id = get_cohort_id(request.user, course_key) + if hasattr(thread, "group_id") and user_group_id != thread.group_id: + raise Http404 + is_staff = cached_has_permission(request.user, 'openclose_thread', course.id) if request.is_ajax(): with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"): @@ -270,7 +273,10 @@ def single_thread(request, course_id, discussion_id, thread_id): }) else: - threads, query_params = get_threads(request, course_key) + try: + threads, query_params = get_threads(request, course_key) + except ValueError: + return HttpResponseBadRequest("Invalid group_id") threads.append(thread.to_dict()) with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"): @@ -303,7 +309,7 @@ def single_thread(request, course_id, discussion_id, thread_id): 'thread_id': thread_id, 'threads': _attr_safe_json(threads), 'roles': _attr_safe_json(utils.get_role_ids(course_key)), - 'is_moderator': cached_has_permission(request.user, "see_all_cohorts", course_key), + 'is_moderator': is_moderator, 'thread_pages': query_params['num_pages'], 'is_course_cohorted': is_course_cohorted(course_key), 'flag_moderator': cached_has_permission(request.user, 'openclose_thread', course.id) or has_access(request.user, 'staff', course), @@ -331,6 +337,13 @@ def user_profile(request, course_id, user_id): 'per_page': THREADS_PER_PAGE, # more than threads_per_page to show more activities } + try: + group_id = get_group_id_for_comments_service(request, course_id) + except ValueError: + return HttpResponseBadRequest("Invalid group_id") + if group_id is not None: + query_params['group_id'] = group_id + threads, page, num_pages = profiled_user.active_threads(query_params) query_params['page'] = page query_params['num_pages'] = num_pages @@ -399,6 +412,13 @@ def followed_threads(request, course_id, user_id): ) ) + try: + group_id = get_group_id_for_comments_service(request, course_id) + except ValueError: + return HttpResponseBadRequest("Invalid group_id") + if group_id is not None: + query_params['group_id'] = group_id + threads, page, num_pages = profiled_user.subscribed_threads(query_params) query_params['page'] = page query_params['num_pages'] = num_pages diff --git a/lms/djangoapps/django_comment_client/tests/group_id.py b/lms/djangoapps/django_comment_client/tests/group_id.py new file mode 100644 index 000000000000..c60761074fb7 --- /dev/null +++ b/lms/djangoapps/django_comment_client/tests/group_id.py @@ -0,0 +1,118 @@ +class GroupIdAssertionMixin(object): + def _data_or_params_cs_request(self, mock_request): + """ + Returns the data or params dict that `mock_request` was called with. + """ + call = [call for call in mock_request.call_args_list if call[0][1].endswith(self.cs_endpoint)][0] + if call[0][0] == "get": + return call[1]["params"] + elif call[0][0] == "post": + return call[1]["data"] + + def _assert_comments_service_called_with_group_id(self, mock_request, group_id): + self.assertTrue(mock_request.called) + self.assertEqual(self._data_or_params_cs_request(mock_request)["group_id"], group_id) + + def _assert_comments_service_called_without_group_id(self, mock_request): + self.assertTrue(mock_request.called) + self.assertNotIn("group_id", self._data_or_params_cs_request(mock_request)) + + +class CohortedTopicGroupIdTestMixin(GroupIdAssertionMixin): + """ + Provides test cases to verify that views pass the correct `group_id` to + the comments service when requesting content in cohorted discussions. + """ + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + """ + Call the view for the implementing test class, constructing a request + from the parameters. + """ + pass + + def test_cohorted_topic_student_without_group_id(self, mock_request): + self.call_view(mock_request, "cohorted_topic", self.student, None, pass_group_id=False) + self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id) + + def test_cohorted_topic_student_none_group_id(self, mock_request): + self.call_view(mock_request, "cohorted_topic", self.student, "") + self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id) + + def test_cohorted_topic_student_with_own_group_id(self, mock_request): + self.call_view(mock_request, "cohorted_topic", self.student, self.student_cohort.id) + self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id) + + def test_cohorted_topic_student_with_other_group_id(self, mock_request): + self.call_view(mock_request, "cohorted_topic", self.student, self.moderator_cohort.id) + self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id) + + def test_cohorted_topic_moderator_without_group_id(self, mock_request): + self.call_view(mock_request, "cohorted_topic", self.moderator, None, pass_group_id=False) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_cohorted_topic_moderator_none_group_id(self, mock_request): + self.call_view(mock_request, "cohorted_topic", self.moderator, "") + self._assert_comments_service_called_without_group_id(mock_request) + + def test_cohorted_topic_moderator_with_own_group_id(self, mock_request): + self.call_view(mock_request, "cohorted_topic", self.moderator, self.moderator_cohort.id) + self._assert_comments_service_called_with_group_id(mock_request, self.moderator_cohort.id) + + def test_cohorted_topic_moderator_with_other_group_id(self, mock_request): + self.call_view(mock_request, "cohorted_topic", self.moderator, self.student_cohort.id) + self._assert_comments_service_called_with_group_id(mock_request, self.student_cohort.id) + + def test_cohorted_topic_moderator_with_invalid_group_id(self, mock_request): + invalid_id = self.student_cohort.id + self.moderator_cohort.id + response = self.call_view(mock_request, "cohorted_topic", self.moderator, invalid_id) + self.assertEqual(response.status_code, 400) + + +class NonCohortedTopicGroupIdTestMixin(GroupIdAssertionMixin): + """ + Provides test cases to verify that views pass the correct `group_id` to + the comments service when requesting content in non-cohorted discussions. + """ + def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True): + """ + Call the view for the implementing test class, constructing a request + from the parameters. + """ + pass + + def test_non_cohorted_topic_student_without_group_id(self, mock_request): + self.call_view(mock_request, "non_cohorted_topic", self.student, None, pass_group_id=False) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_non_cohorted_topic_student_none_group_id(self, mock_request): + self.call_view(mock_request, "non_cohorted_topic", self.student, None) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_non_cohorted_topic_student_with_own_group_id(self, mock_request): + self.call_view(mock_request, "non_cohorted_topic", self.student, self.student_cohort.id) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_non_cohorted_topic_student_with_other_group_id(self, mock_request): + self.call_view(mock_request, "non_cohorted_topic", self.student, self.moderator_cohort.id) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_non_cohorted_topic_moderator_without_group_id(self, mock_request): + self.call_view(mock_request, "non_cohorted_topic", self.moderator, None, pass_group_id=False) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_non_cohorted_topic_moderator_none_group_id(self, mock_request): + self.call_view(mock_request, "non_cohorted_topic", self.moderator, None) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_non_cohorted_topic_moderator_with_own_group_id(self, mock_request): + self.call_view(mock_request, "non_cohorted_topic", self.moderator, self.moderator_cohort.id) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_non_cohorted_topic_moderator_with_other_group_id(self, mock_request): + self.call_view(mock_request, "non_cohorted_topic", self.moderator, self.student_cohort.id) + self._assert_comments_service_called_without_group_id(mock_request) + + def test_non_cohorted_topic_moderator_with_invalid_group_id(self, mock_request): + invalid_id = self.student_cohort.id + self.moderator_cohort.id + self.call_view(mock_request, "non_cohorted_topic", self.moderator, invalid_id) + self._assert_comments_service_called_without_group_id(mock_request) diff --git a/lms/djangoapps/django_comment_client/tests/utils.py b/lms/djangoapps/django_comment_client/tests/utils.py index f637abed3f58..2c1cc371ecad 100644 --- a/lms/djangoapps/django_comment_client/tests/utils.py +++ b/lms/djangoapps/django_comment_client/tests/utils.py @@ -1,9 +1,7 @@ -from django.test.client import RequestFactory from django.test.utils import override_settings from course_groups.models import CourseUserGroup from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE -from django_comment_client import base from django_comment_common.models import Role from django_comment_common.utils import seed_permissions_roles from mock import patch @@ -49,35 +47,3 @@ def setUp(self): 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) - - def _create_thread( - self, - user, - commentable_id, - mock_request, - group_id, - pass_group_id=True, - expected_status_code=200 - ): - mock_request.return_value.status_code = 200 - request_data = {"body": "body", "title": "title", "thread_type": "discussion"} - if pass_group_id: - request_data["group_id"] = group_id - request = RequestFactory().post("dummy_url", request_data) - request.user = user - request.view_name = "create_thread" - - response = base.views.create_thread( - request, - course_id=self.course.id.to_deprecated_string(), - commentable_id=commentable_id - ) - self.assertEqual(response.status_code, expected_status_code) - - def _assert_mock_request_called_with_group_id(self, mock_request, group_id): - self.assertTrue(mock_request.called) - self.assertEqual(mock_request.call_args[1]["data"]["group_id"], group_id) - - def _assert_mock_request_called_without_group_id(self, mock_request): - self.assertTrue(mock_request.called) - self.assertNotIn("group_id", mock_request.call_args[1]["data"]) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index c054c38323c3..bff4c2de6983 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -3,7 +3,6 @@ import logging from datetime import datetime -from course_groups.cohorts import get_cohort_by_id from django.contrib.auth.models import User from django.core.urlresolvers import reverse from django.db import connection @@ -15,6 +14,8 @@ from edxmako import lookup_template import pystache_custom as pystache +from course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted +from course_groups.models import CourseUserGroup from xmodule.modulestore.django import modulestore from django.utils.timezone import UTC from opaque_keys.edx.locations import i4xEncoder @@ -420,3 +421,38 @@ def add_thread_group_name(thread_info, course_key): """ if thread_info.get('group_id') is not None: thread_info['group_name'] = get_cohort_by_id(course_key, thread_info.get('group_id')).name + + +def get_group_id_for_comments_service(request, course_key, commentable_id=None): + """ + Given a user requesting content within a `commentable_id`, determine the + group_id which should be passed to the comments service. + + Returns: + int: the group_id to pass to the comments service or None if nothing + should be passed + + Raises: + ValueError if the requested group_id is invalid + """ + if commentable_id is None or is_commentable_cohorted(course_key, commentable_id): + if request.method == "GET": + requested_group_id = request.GET.get('group_id') + elif request.method == "POST": + requested_group_id = request.POST.get('group_id') + if cached_has_permission(request.user, "see_all_cohorts", course_key): + if not requested_group_id: + return None + try: + group_id = int(requested_group_id) + get_cohort_by_id(course_key, group_id) + except CourseUserGroup.DoesNotExist: + raise ValueError + else: + # regular users always query with their own id. + group_id = get_cohort_id(request.user, course_key) + return group_id + else: + # Never pass a group_id to the comments service for a non-cohorted + # commentable + return None