diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 03a3b4025e..34ede494d7 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -12,6 +12,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE from django_comment_client.base import views +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 from django_comment_common.utils import seed_permissions_roles @@ -20,6 +21,7 @@ from util.testing import UrlResetMixin from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + log = logging.getLogger(__name__) CS_PREFIX = "http://localhost:4567/api/v1" @@ -33,6 +35,114 @@ class MockRequestSetupMixin(object): mock_request.return_value = self._create_repsonse_mock(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) + + +@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) + + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @patch('lms.lib.comment_client.utils.requests.request') class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): @@ -575,6 +685,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): assert_equal(response.status_code, 200) + @patch("lms.lib.comment_client.utils.requests.request") @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 8c168a9070..1957784624 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -9,7 +9,7 @@ from django.contrib.auth.decorators import login_required from django.contrib.auth.models import User from django.core import exceptions from django.core.files.storage import get_storage_class -from django.http import Http404 +from django.http import Http404, HttpResponseBadRequest from django.utils.translation import ugettext as _ from django.views.decorators import csrf from django.views.decorators.http import require_GET, require_POST @@ -18,7 +18,8 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from courseware.access import has_access from courseware.courses import get_course_with_access, get_course_by_id -from course_groups.cohorts import get_cohort_id, is_commentable_cohorted +from course_groups.models import CourseUserGroup +from course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted import django_comment_client.settings as cc_settings from django_comment_client.utils import ( add_courseware_context, @@ -116,7 +117,11 @@ def create_thread(request, course_id, commentable_id): # 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 - group_id = post.get('group_id', user_group_id) + 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 diff --git a/lms/djangoapps/django_comment_client/tests/utils.py b/lms/djangoapps/django_comment_client/tests/utils.py new file mode 100644 index 0000000000..f637abed3f --- /dev/null +++ b/lms/djangoapps/django_comment_client/tests/utils.py @@ -0,0 +1,83 @@ +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 +from student.tests.factories import CourseEnrollmentFactory, UserFactory +from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class CohortedContentTestCase(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() + + 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( + name="student_cohort", + course_id=self.course.id, + group_type=CourseUserGroup.COHORT + ) + self.moderator_cohort = CourseUserGroup.objects.create( + name="moderator_cohort", + course_id=self.course.id, + group_type=CourseUserGroup.COHORT + ) + + seed_permissions_roles(self.course.id) + self.student = UserFactory.create() + self.moderator = UserFactory.create() + CourseEnrollmentFactory(user=self.student, course_id=self.course.id) + CourseEnrollmentFactory(user=self.moderator, course_id=self.course.id) + 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"])