From 38fb6eaede0c26d91b83c3853dc5362e522c6c91 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Tue, 18 Aug 2015 15:00:57 -0400 Subject: [PATCH] Add signals for user's discussion activity. These signals are caught by the Teams app and used to update `last_activity_at` on both teams and individual users. TNL-2497 --- .../contentstore/tests/test_contentstore.py | 2 +- .../django_comment_common/signals.py | 15 ++ common/djangoapps/terrain/stubs/comments.py | 12 + .../xmodule/modulestore/tests/utils.py | 23 -- common/test/acceptance/fixtures/discussion.py | 2 +- .../tests/discussion/test_discussion.py | 7 +- common/test/utils.py | 72 ++++++ .../courseware/tests/test_course_survey.py | 2 +- lms/djangoapps/discussion_api/api.py | 25 +- .../discussion_api/tests/test_api.py | 92 ++++++-- .../discussion_api/tests/test_views.py | 8 + .../django_comment_client/base/tests.py | 221 +++++++++++++----- .../django_comment_client/base/views.py | 64 +++-- .../tests/views/test_instructor_dashboard.py | 2 +- .../shoppingcart/tests/test_views.py | 2 +- lms/djangoapps/teams/__init__.py | 3 + lms/djangoapps/teams/models.py | 74 ++++++ lms/djangoapps/teams/tests/test_models.py | 109 ++++++++- .../verify_student/tests/test_views.py | 2 +- lms/lib/comment_client/comment.py | 5 + 20 files changed, 621 insertions(+), 121 deletions(-) create mode 100644 common/djangoapps/django_comment_common/signals.py diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 0c8b4b6337..d25cf5ae98 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -23,6 +23,7 @@ from django.test import TestCase from django.test.utils import override_settings from openedx.core.lib.tempdir import mkdtemp_clean +from common.test.utils import XssTestMixin from contentstore.tests.utils import parse_json, AjaxEnabledTestClient, CourseTestCase from contentstore.views.component import ADVANCED_COMPONENT_TYPES @@ -37,7 +38,6 @@ from xmodule.modulestore.inheritance import own_metadata from opaque_keys.edx.keys import UsageKey, CourseKey from opaque_keys.edx.locations import AssetLocation, CourseLocator from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory, check_mongo_calls -from xmodule.modulestore.tests.utils import XssTestMixin from xmodule.modulestore.xml_exporter import export_course_to_xml from xmodule.modulestore.xml_importer import import_course_from_xml, perform_xlint diff --git a/common/djangoapps/django_comment_common/signals.py b/common/djangoapps/django_comment_common/signals.py new file mode 100644 index 0000000000..f58486a34b --- /dev/null +++ b/common/djangoapps/django_comment_common/signals.py @@ -0,0 +1,15 @@ +# pylint: disable=invalid-name +"""Signals related to the comments service.""" + +from django.dispatch import Signal + + +thread_created = Signal(providing_args=['user', 'post']) +thread_edited = Signal(providing_args=['user', 'post']) +thread_voted = Signal(providing_args=['user', 'post']) +thread_deleted = Signal(providing_args=['user', 'post']) +comment_created = Signal(providing_args=['user', 'post']) +comment_edited = Signal(providing_args=['user', 'post']) +comment_voted = Signal(providing_args=['user', 'post']) +comment_deleted = Signal(providing_args=['user', 'post']) +comment_endorsed = Signal(providing_args=['user', 'post']) diff --git a/common/djangoapps/terrain/stubs/comments.py b/common/djangoapps/terrain/stubs/comments.py index 660117954d..208604e4e8 100644 --- a/common/djangoapps/terrain/stubs/comments.py +++ b/common/djangoapps/terrain/stubs/comments.py @@ -52,6 +52,11 @@ class StubCommentsServiceHandler(StubHttpRequestHandler): self.send_json_response({'username': self.post_dict.get("username"), 'external_id': self.post_dict.get("external_id")}) def do_DELETE(self): + pattern_handlers = { + "/api/v1/comments/(?P\\w+)$": self.do_delete_comment + } + if self.match_pattern(pattern_handlers): + return self.send_json_response({}) def do_user(self, user_id): @@ -113,6 +118,13 @@ class StubCommentsServiceHandler(StubHttpRequestHandler): comment = self.server.config['comments'][comment_id] self.send_json_response(comment) + def do_delete_comment(self, comment_id): + """Handle comment deletion. Returns a JSON representation of the + deleted comment.""" + if comment_id in self.server.config.get('comments', {}): + comment = self.server.config['comments'][comment_id] + self.send_json_response(comment) + def do_commentable(self, commentable_id): self.send_json_response({ "collection": [ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/utils.py b/common/lib/xmodule/xmodule/modulestore/tests/utils.py index b1c0b17e4a..aa8832ba10 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/utils.py @@ -2,7 +2,6 @@ Helper classes and methods for running modulestore tests without Django. """ from importlib import import_module -from markupsafe import escape from opaque_keys.edx.keys import UsageKey from unittest import TestCase from xblock.fields import XBlockMixin @@ -175,25 +174,3 @@ class ProceduralCourseTestMixin(object): with self.store.bulk_operations(self.course.id, emit_signals=emit_signals): descend(self.course, ['chapter', 'sequential', 'vertical', 'problem']) - - -class XssTestMixin(object): - """ - Mixin for testing XSS vulnerabilities. - """ - - def assert_xss(self, response, xss_content): - """Assert that `xss_content` is not present in the content of - `response`, and that its escaped version is present. Uses the - same `markupsafe.escape` function as Mako templates. - - Args: - response (Response): The HTTP response - xss_content (str): The Javascript code to check for. - - Returns: - None - - """ - self.assertContains(response, escape(xss_content)) - self.assertNotContains(response, xss_content) diff --git a/common/test/acceptance/fixtures/discussion.py b/common/test/acceptance/fixtures/discussion.py index b99ba94f23..25cc034f6a 100644 --- a/common/test/acceptance/fixtures/discussion.py +++ b/common/test/acceptance/fixtures/discussion.py @@ -14,7 +14,7 @@ from . import COMMENTS_STUB_URL class ContentFactory(factory.Factory): FACTORY_FOR = dict id = None - user_id = "dummy-user-id" + user_id = "1234" username = "dummy-username" course_id = "dummy-course-id" commentable_id = "dummy-commentable-id" diff --git a/common/test/acceptance/tests/discussion/test_discussion.py b/common/test/acceptance/tests/discussion/test_discussion.py index 76d74e3926..7b31c14f5c 100644 --- a/common/test/acceptance/tests/discussion/test_discussion.py +++ b/common/test/acceptance/tests/discussion/test_discussion.py @@ -394,8 +394,11 @@ class DiscussionCommentDeletionTest(BaseDiscussionTestCase): def setup_view(self): 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)]) + Response(id="response1"), [ + Comment(id="comment_other_author"), + Comment(id="comment_self_author", user_id=self.user_id, thread_id="comment_deletion_test_thread") + ] + ) view.push() def test_comment_deletion_as_student(self): diff --git a/common/test/utils.py b/common/test/utils.py index 7409625e73..9ce575e658 100644 --- a/common/test/utils.py +++ b/common/test/utils.py @@ -3,6 +3,9 @@ General testing utilities. """ import sys from contextlib import contextmanager +from django.dispatch import Signal +from markupsafe import escape +from mock import Mock, patch @contextmanager @@ -24,3 +27,72 @@ def nostderr(): yield finally: sys.stderr = savestderr + + +class XssTestMixin(object): + """ + Mixin for testing XSS vulnerabilities. + """ + + def assert_xss(self, response, xss_content): + """Assert that `xss_content` is not present in the content of + `response`, and that its escaped version is present. Uses the + same `markupsafe.escape` function as Mako templates. + + Args: + response (Response): The HTTP response + xss_content (str): The Javascript code to check for. + + Returns: + None + + """ + self.assertContains(response, escape(xss_content)) + self.assertNotContains(response, xss_content) + + +def disable_signal(module, signal): + """Replace `signal` inside of `module` with a dummy signal. Can be + used as a method or class decorator, as well as a context manager.""" + return patch.object(module, signal, new=Signal()) + + +class MockSignalHandlerMixin(object): + """Mixin for testing sending of signals.""" + + @contextmanager + def assert_signal_sent(self, module, signal, *args, **kwargs): + """Assert that a signal was sent with the correct arguments. Since + Django calls signal handlers with the signal as an argument, + it is added to `kwargs`. + + Uses `mock.patch.object`, which requires the target to be + specified as a module along with a variable name inside that + module. + + Args: + module (module): The module in which to patch the given signal name. + signal (str): The name of the signal to patch. + *args, **kwargs: The arguments which should have been passed + along with the signal. If `exclude_args` is passed as a + keyword argument, its value should be a list of keyword + arguments passed to the signal whose values should be + ignored. + + """ + with patch.object(module, signal, new=Signal()) as mock_signal: + def handler(*args, **kwargs): # pylint: disable=unused-argument + """No-op signal handler.""" + pass + mock_handler = Mock(spec=handler) + mock_signal.connect(mock_handler) + yield + self.assertTrue(mock_handler.called) + mock_args, mock_kwargs = mock_handler.call_args # pylint: disable=unpacking-non-sequence + if 'exclude_args' in kwargs: + for key in kwargs['exclude_args']: + self.assertIn(key, mock_kwargs) + del mock_kwargs[key] + del kwargs['exclude_args'] + self.assertEqual(mock_args, args) + self.assertEqual(mock_kwargs, dict(kwargs, signal=mock_signal)) diff --git a/lms/djangoapps/courseware/tests/test_course_survey.py b/lms/djangoapps/courseware/tests/test_course_survey.py index a06a77b372..7fa1960f33 100644 --- a/lms/djangoapps/courseware/tests/test_course_survey.py +++ b/lms/djangoapps/courseware/tests/test_course_survey.py @@ -9,9 +9,9 @@ from django.core.urlresolvers import reverse from survey.models import SurveyForm +from common.test.utils import XssTestMixin from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.utils import XssTestMixin from courseware.tests.helpers import LoginEnrollmentTestCase diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index e0b8a9596a..a95e9bdb3b 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -31,6 +31,16 @@ from django_comment_client.base.views import ( get_thread_created_event_data, track_forum_event, ) +from django_comment_common.signals import ( + thread_created, + thread_edited, + thread_deleted, + thread_voted, + comment_created, + comment_edited, + comment_voted, + comment_deleted +) from django_comment_client.utils import get_accessible_discussion_modules, is_commentable_cohorted from lms.lib.comment_client.comment import Comment from lms.lib.comment_client.thread import Thread @@ -501,6 +511,8 @@ def _do_extra_actions(api_content, cc_content, request_fields, actions_form, con cc_content.unFlagAbuse(context["cc_requester"], cc_content, removeAll=False) else: assert field == "voted" + signal = thread_voted if cc_content.type == 'thread' else comment_voted + signal.send(sender=None, user=context["request"].user, post=cc_content) if form_value: context["cc_requester"].vote(cc_content, "up") else: @@ -524,11 +536,12 @@ def create_thread(request, thread_data): detail. """ course_id = thread_data.get("course_id") + user = request.user if not course_id: raise ValidationError({"course_id": ["This field is required."]}) try: course_key = CourseKey.from_string(course_id) - course = _get_course_or_404(course_key, request.user) + course = _get_course_or_404(course_key, user) except (Http404, InvalidKeyError): raise ValidationError({"course_id": ["Invalid value."]}) @@ -539,14 +552,14 @@ def create_thread(request, thread_data): is_commentable_cohorted(course_key, thread_data.get("topic_id")) ): thread_data = thread_data.copy() - thread_data["group_id"] = get_cohort_id(request.user, course_key) + thread_data["group_id"] = get_cohort_id(user, course_key) serializer = ThreadSerializer(data=thread_data, context=context) actions_form = ThreadActionsForm(thread_data) if not (serializer.is_valid() and actions_form.is_valid()): raise ValidationError(dict(serializer.errors.items() + actions_form.errors.items())) serializer.save() - cc_thread = serializer.object + thread_created.send(sender=None, user=user, post=cc_thread) api_thread = serializer.data _do_extra_actions(api_thread, cc_thread, thread_data.keys(), actions_form, context) @@ -591,8 +604,8 @@ def create_comment(request, comment_data): if not (serializer.is_valid() and actions_form.is_valid()): raise ValidationError(dict(serializer.errors.items() + actions_form.errors.items())) serializer.save() - cc_comment = serializer.object + comment_created.send(sender=None, user=request.user, post=cc_comment) api_comment = serializer.data _do_extra_actions(api_comment, cc_comment, comment_data.keys(), actions_form, context) @@ -634,6 +647,7 @@ def update_thread(request, thread_id, update_data): # Only save thread object if some of the edited fields are in the thread data, not extra actions if set(update_data) - set(actions_form.fields): serializer.save() + thread_edited.send(sender=None, user=request.user, post=cc_thread) api_thread = serializer.data _do_extra_actions(api_thread, cc_thread, update_data.keys(), actions_form, context) return api_thread @@ -677,6 +691,7 @@ def update_comment(request, comment_id, update_data): # Only save comment object if some of the edited fields are in the comment data, not extra actions if set(update_data) - set(actions_form.fields): serializer.save() + comment_edited.send(sender=None, user=request.user, post=cc_comment) api_comment = serializer.data _do_extra_actions(api_comment, cc_comment, update_data.keys(), actions_form, context) return api_comment @@ -701,6 +716,7 @@ def delete_thread(request, thread_id): cc_thread, context = _get_thread_and_context(request, thread_id) if can_delete(cc_thread, context): cc_thread.delete() + thread_deleted.send(sender=None, user=request.user, post=cc_thread) else: raise PermissionDenied @@ -724,5 +740,6 @@ def delete_comment(request, comment_id): cc_comment, context = _get_comment_and_context(request, comment_id) if can_delete(cc_comment, context): cc_comment.delete() + comment_deleted.send(sender=None, user=request.user, post=cc_comment) else: raise PermissionDenied diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 2b85f3661a..80b45d5183 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -19,7 +19,9 @@ from rest_framework.exceptions import PermissionDenied from opaque_keys.edx.locator import CourseLocator +from common.test.utils import MockSignalHandlerMixin, disable_signal from courseware.tests.factories import BetaTesterFactory, StaffFactory +from discussion_api import api from discussion_api.api import ( create_comment, create_thread, @@ -1328,7 +1330,14 @@ class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase): @ddt.ddt -class CreateThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): +@disable_signal(api, 'thread_created') +@disable_signal(api, 'thread_voted') +class CreateThreadTest( + CommentsServiceMockMixin, + UrlResetMixin, + SharedModuleStoreTestCase, + MockSignalHandlerMixin +): """Tests for create_thread""" @classmethod @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -1363,7 +1372,8 @@ class CreateThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStor "created_at": "2015-05-19T00:00:00Z", "updated_at": "2015-05-19T00:00:00Z", }) - actual = create_thread(self.request, self.minimal_data) + with self.assert_signal_sent(api, 'thread_created', sender=None, user=self.user, exclude_args=('post',)): + actual = create_thread(self.request, self.minimal_data) expected = { "id": "test_id", "course_id": unicode(self.course.id), @@ -1512,7 +1522,8 @@ class CreateThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStor self.register_thread_votes_response("test_id") data = self.minimal_data.copy() data["voted"] = "True" - result = create_thread(self.request, data) + with self.assert_signal_sent(api, 'thread_voted', sender=None, user=self.user, exclude_args=('post',)): + result = create_thread(self.request, data) self.assertEqual(result["voted"], True) cs_request = httpretty.last_request() self.assertEqual(urlparse(cs_request.path).path, "/api/v1/threads/test_id/votes") @@ -1570,7 +1581,14 @@ class CreateThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStor @ddt.ddt -class CreateCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): +@disable_signal(api, 'comment_created') +@disable_signal(api, 'comment_voted') +class CreateCommentTest( + CommentsServiceMockMixin, + UrlResetMixin, + SharedModuleStoreTestCase, + MockSignalHandlerMixin +): """Tests for create_comment""" @classmethod @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -1619,7 +1637,8 @@ class CreateCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto data = self.minimal_data.copy() if parent_id: data["parent_id"] = parent_id - actual = create_comment(self.request, data) + with self.assert_signal_sent(api, 'comment_created', sender=None, user=self.user, exclude_args=('post',)): + actual = create_comment(self.request, data) expected = { "id": "test_comment", "thread_id": "test_thread", @@ -1721,7 +1740,8 @@ class CreateCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto self.register_comment_votes_response("test_comment") data = self.minimal_data.copy() data["voted"] = "True" - result = create_comment(self.request, data) + with self.assert_signal_sent(api, 'comment_voted', sender=None, user=self.user, exclude_args=('post',)): + result = create_comment(self.request, data) self.assertEqual(result["voted"], True) cs_request = httpretty.last_request() self.assertEqual(urlparse(cs_request.path).path, "/api/v1/comments/test_comment/votes") @@ -1835,7 +1855,14 @@ class CreateCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto @ddt.ddt -class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): +@disable_signal(api, 'thread_edited') +@disable_signal(api, 'thread_voted') +class UpdateThreadTest( + CommentsServiceMockMixin, + UrlResetMixin, + SharedModuleStoreTestCase, + MockSignalHandlerMixin +): """Tests for update_thread""" @classmethod @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -1888,7 +1915,8 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStor def test_basic(self): self.register_thread() - actual = update_thread(self.request, "test_thread", {"raw_body": "Edited body"}) + with self.assert_signal_sent(api, 'thread_edited', sender=None, user=self.user, exclude_args=('post',)): + actual = update_thread(self.request, "test_thread", {"raw_body": "Edited body"}) expected = { "id": "test_thread", "course_id": unicode(self.course.id), @@ -2074,7 +2102,12 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStor self.register_thread_votes_response("test_thread") self.register_thread() data = {"voted": new_voted} - result = update_thread(self.request, "test_thread", data) + if old_voted == new_voted: + result = update_thread(self.request, "test_thread", data) + else: + # Vote signals should only be sent if the number of votes has changed + with self.assert_signal_sent(api, 'thread_voted', sender=None, user=self.user, exclude_args=('post',)): + result = update_thread(self.request, "test_thread", data) self.assertEqual(result["voted"], new_voted) last_request_path = urlparse(httpretty.last_request().path).path votes_url = "/api/v1/threads/test_thread/votes" @@ -2142,7 +2175,14 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStor @ddt.ddt -class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): +@disable_signal(api, 'comment_edited') +@disable_signal(api, 'comment_voted') +class UpdateCommentTest( + CommentsServiceMockMixin, + UrlResetMixin, + SharedModuleStoreTestCase, + MockSignalHandlerMixin +): """Tests for update_comment""" @classmethod @@ -2205,7 +2245,8 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto @ddt.data(None, "test_parent") def test_basic(self, parent_id): self.register_comment({"parent_id": parent_id}) - actual = update_comment(self.request, "test_comment", {"raw_body": "Edited body"}) + with self.assert_signal_sent(api, 'comment_edited', sender=None, user=self.user, exclude_args=('post',)): + actual = update_comment(self.request, "test_comment", {"raw_body": "Edited body"}) expected = { "id": "test_comment", "thread_id": "test_thread", @@ -2387,7 +2428,12 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto self.register_comment_votes_response("test_comment") self.register_comment() data = {"voted": new_voted} - result = update_comment(self.request, "test_comment", data) + if old_voted == new_voted: + result = update_comment(self.request, "test_comment", data) + else: + # Vote signals should only be sent if the number of votes has changed + with self.assert_signal_sent(api, 'comment_voted', sender=None, user=self.user, exclude_args=('post',)): + result = update_comment(self.request, "test_comment", data) self.assertEqual(result["voted"], new_voted) last_request_path = urlparse(httpretty.last_request().path).path votes_url = "/api/v1/comments/test_comment/votes" @@ -2446,7 +2492,13 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto @ddt.ddt -class DeleteThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): +@disable_signal(api, 'thread_deleted') +class DeleteThreadTest( + CommentsServiceMockMixin, + UrlResetMixin, + SharedModuleStoreTestCase, + MockSignalHandlerMixin +): """Tests for delete_thread""" @classmethod @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -2484,7 +2536,8 @@ class DeleteThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStor def test_basic(self): self.register_thread() - self.assertIsNone(delete_thread(self.request, self.thread_id)) + with self.assert_signal_sent(api, 'thread_deleted', sender=None, user=self.user, exclude_args=('post',)): + self.assertIsNone(delete_thread(self.request, self.thread_id)) self.assertEqual( urlparse(httpretty.last_request().path).path, "/api/v1/threads/{}".format(self.thread_id) @@ -2578,7 +2631,13 @@ class DeleteThreadTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStor @ddt.ddt -class DeleteCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): +@disable_signal(api, 'comment_deleted') +class DeleteCommentTest( + CommentsServiceMockMixin, + UrlResetMixin, + SharedModuleStoreTestCase, + MockSignalHandlerMixin +): """Tests for delete_comment""" @classmethod @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -2625,7 +2684,8 @@ class DeleteCommentTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto def test_basic(self): self.register_comment_and_thread() - self.assertIsNone(delete_comment(self.request, self.comment_id)) + with self.assert_signal_sent(api, 'comment_deleted', sender=None, user=self.user, exclude_args=('post',)): + self.assertIsNone(delete_comment(self.request, self.comment_id)) self.assertEqual( urlparse(httpretty.last_request().path).path, "/api/v1/comments/{}".format(self.comment_id) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 166940ad26..23abc7adba 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -14,6 +14,8 @@ from django.core.urlresolvers import reverse from rest_framework.test import APIClient +from common.test.utils import disable_signal +from discussion_api import api from discussion_api.tests.utils import ( CommentsServiceMockMixin, make_minimal_cs_comment, @@ -385,6 +387,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @httpretty.activate +@disable_signal(api, 'thread_created') class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for ThreadViewSet create""" def setUp(self): @@ -476,6 +479,7 @@ class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @httpretty.activate +@disable_signal(api, 'thread_edited') class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for ThreadViewSet partial_update""" def setUp(self): @@ -575,6 +579,7 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest @httpretty.activate +@disable_signal(api, 'thread_deleted') class ThreadViewSetDeleteTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for ThreadViewSet delete""" def setUp(self): @@ -738,6 +743,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @httpretty.activate +@disable_signal(api, 'comment_deleted') class CommentViewSetDeleteTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for ThreadViewSet delete""" @@ -778,6 +784,7 @@ class CommentViewSetDeleteTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @httpretty.activate +@disable_signal(api, 'comment_created') class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for CommentViewSet create""" def setUp(self): @@ -861,6 +868,7 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.assertEqual(response_data, expected_response_data) +@disable_signal(api, 'comment_edited') class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for CommentViewSet partial_update""" def setUp(self): diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 0b48675e31..b99182b58e 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -1,3 +1,5 @@ +"""Tests for django comment client views.""" +from contextlib import contextmanager import logging import json import ddt @@ -14,6 +16,7 @@ from nose.tools import assert_true, assert_equal # pylint: disable=no-name-in-m from opaque_keys.edx.locations import SlashSeparatedCourseKey from lms.lib.comment_client import Thread +from common.test.utils import MockSignalHandlerMixin, disable_signal 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 CohortedTestCase @@ -67,7 +70,7 @@ class CreateThreadGroupIdTestCase( return views.create_thread( request, - course_id=self.course.id.to_deprecated_string(), + course_id=unicode(self.course.id), commentable_id=commentable_id ) @@ -82,6 +85,9 @@ class CreateThreadGroupIdTestCase( @patch('lms.lib.comment_client.utils.requests.request') +@disable_signal(views, 'thread_edited') +@disable_signal(views, 'thread_voted') +@disable_signal(views, 'thread_deleted') class ThreadActionGroupIdTestCase( MockRequestSetupMixin, CohortedTestCase, @@ -112,7 +118,7 @@ class ThreadActionGroupIdTestCase( return getattr(views, view_name)( request, - course_id=self.course.id.to_deprecated_string(), + course_id=unicode(self.course.id), thread_id="dummy", **(view_args or {}) ) @@ -204,26 +210,33 @@ class ViewsTestCaseMixin(object): ) # seed the forums permissions and roles - call_command('seed_permissions_roles', self.course_id.to_deprecated_string()) + call_command('seed_permissions_roles', unicode(self.course_id)) # Patch the comment client user save method so it does not try # to create a new cc user when creating a django user with patch('student.models.cc.User.save'): uname = 'student' email = 'student@edx.org' - password = 'test' + self.password = 'test' # pylint: disable=attribute-defined-outside-init # Create the user and make them active so we can log them in. - self.student = User.objects.create_user(uname, email, password) + self.student = User.objects.create_user(uname, email, self.password) # pylint: disable=attribute-defined-outside-init self.student.is_active = True self.student.save() + # Add a discussion moderator + self.moderator = UserFactory.create(password=self.password) # pylint: disable=attribute-defined-outside-init + # Enroll the student in the course CourseEnrollmentFactory(user=self.student, course_id=self.course_id) + # Enroll the moderator and give them the appropriate roles + CourseEnrollmentFactory(user=self.moderator, course_id=self.course.id) + self.moderator.roles.add(Role.objects.get(name="Moderator", course_id=self.course.id)) + self.client = Client() - assert_true(self.client.login(username='student', password='test')) + assert_true(self.client.login(username='student', password=self.password)) def _setup_mock_request(self, mock_request, include_depth=False): """ @@ -286,7 +299,7 @@ class ViewsTestCaseMixin(object): if extra_request_data: thread.update(extra_request_data) url = reverse('create_thread', kwargs={'commentable_id': 'i4x-MITx-999-course-Robot_Super_Course', - 'course_id': self.course_id.to_deprecated_string()}) + 'course_id': unicode(self.course_id)}) response = self.client.post(url, data=thread) assert_true(mock_request.called) expected_data = { @@ -324,7 +337,7 @@ class ViewsTestCaseMixin(object): response = self.client.post( reverse("update_thread", kwargs={ "thread_id": "dummy", - "course_id": self.course_id.to_deprecated_string() + "course_id": unicode(self.course_id) }), data={"body": "foo", "title": "foo", "commentable_id": "some_topic"} ) @@ -337,6 +350,8 @@ class ViewsTestCaseMixin(object): @ddt.ddt @patch('lms.lib.comment_client.utils.requests.request') +@disable_signal(views, 'thread_created') +@disable_signal(views, 'thread_edited') class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, ViewsTestCaseMixin): @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -386,8 +401,15 @@ class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet self.update_thread_helper(mock_request) +@ddt.ddt @patch('lms.lib.comment_client.utils.requests.request') -class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, ViewsTestCaseMixin): +class ViewsTestCase( + UrlResetMixin, + ModuleStoreTestCase, + MockRequestSetupMixin, + ViewsTestCaseMixin, + MockSignalHandlerMixin +): @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): @@ -397,8 +419,16 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V super(ViewsTestCase, self).setUp(create_user=False) self.set_up_course() + @contextmanager + def assert_discussion_signals(self, signal, user=None): + if user is None: + user = self.student + with self.assert_signal_sent(views, signal, sender=None, user=user, exclude_args=('post',)): + yield + def test_create_thread(self, mock_request): - self.create_thread_helper(mock_request) + with self.assert_discussion_signals('thread_created'): + self.create_thread_helper(mock_request) def test_create_thread_standalone(self, mock_request): team = CourseTeamFactory.create( @@ -414,6 +444,24 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V # create_thread_helper verifies that extra data are passed through to the comments service self.create_thread_helper(mock_request, extra_response_data={'context': ThreadContext.STANDALONE}) + def test_delete_thread(self, mock_request): + self._set_mock_request_data(mock_request, { + "user_id": str(self.student.id), + "closed": False, + }) + test_thread_id = "test_thread_id" + request = RequestFactory().post("dummy_url", {"id": test_thread_id}) + request.user = self.student + request.view_name = "delete_thread" + with self.assert_discussion_signals('thread_deleted'): + response = views.delete_thread( + request, + course_id=unicode(self.course.id), + thread_id=test_thread_id + ) + self.assertEqual(response.status_code, 200) + self.assertTrue(mock_request.called) + def test_delete_comment(self, mock_request): self._set_mock_request_data(mock_request, { "user_id": str(self.student.id), @@ -423,8 +471,12 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V request = RequestFactory().post("dummy_url", {"id": test_comment_id}) request.user = self.student request.view_name = "delete_comment" - response = views.delete_comment(request, course_id=self.course.id.to_deprecated_string(), comment_id=test_comment_id) - + with self.assert_discussion_signals('comment_deleted'): + response = views.delete_comment( + request, + course_id=unicode(self.course.id), + comment_id=test_comment_id + ) self.assertEqual(response.status_code, 200) self.assertTrue(mock_request.called) args = mock_request.call_args[0] @@ -447,7 +499,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_create_thread_no_title(self, mock_request): self._test_request_error( "create_thread", - {"commentable_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"commentable_id": "dummy", "course_id": unicode(self.course_id)}, {"body": "foo"}, mock_request ) @@ -455,7 +507,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_create_thread_empty_title(self, mock_request): self._test_request_error( "create_thread", - {"commentable_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"commentable_id": "dummy", "course_id": unicode(self.course_id)}, {"body": "foo", "title": " "}, mock_request ) @@ -463,7 +515,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_create_thread_no_body(self, mock_request): self._test_request_error( "create_thread", - {"commentable_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"commentable_id": "dummy", "course_id": unicode(self.course_id)}, {"title": "foo"}, mock_request ) @@ -471,7 +523,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_create_thread_empty_body(self, mock_request): self._test_request_error( "create_thread", - {"commentable_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"commentable_id": "dummy", "course_id": unicode(self.course_id)}, {"body": " ", "title": "foo"}, mock_request ) @@ -479,7 +531,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_update_thread_no_title(self, mock_request): self._test_request_error( "update_thread", - {"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"thread_id": "dummy", "course_id": unicode(self.course_id)}, {"body": "foo"}, mock_request ) @@ -487,7 +539,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_update_thread_empty_title(self, mock_request): self._test_request_error( "update_thread", - {"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"thread_id": "dummy", "course_id": unicode(self.course_id)}, {"body": "foo", "title": " "}, mock_request ) @@ -495,7 +547,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_update_thread_no_body(self, mock_request): self._test_request_error( "update_thread", - {"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"thread_id": "dummy", "course_id": unicode(self.course_id)}, {"title": "foo"}, mock_request ) @@ -503,27 +555,40 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_update_thread_empty_body(self, mock_request): self._test_request_error( "update_thread", - {"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"thread_id": "dummy", "course_id": unicode(self.course_id)}, {"body": " ", "title": "foo"}, mock_request ) def test_update_thread_course_topic(self, mock_request): - self.update_thread_helper(mock_request) + with self.assert_discussion_signals('thread_edited'): + self.update_thread_helper(mock_request) @patch('django_comment_client.utils.get_discussion_categories_ids', return_value=["test_commentable"]) def test_update_thread_wrong_commentable_id(self, mock_get_discussion_id_map, mock_request): self._test_request_error( "update_thread", - {"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"thread_id": "dummy", "course_id": unicode(self.course_id)}, {"body": "foo", "title": "foo", "commentable_id": "wrong_commentable"}, mock_request ) + def test_create_comment(self, mock_request): + self._setup_mock_request(mock_request) + with self.assert_discussion_signals('comment_created'): + response = self.client.post( + reverse( + "create_comment", + kwargs={"course_id": unicode(self.course_id), "thread_id": "dummy"} + ), + data={"body": "body"} + ) + self.assertEqual(response.status_code, 200) + def test_create_comment_no_body(self, mock_request): self._test_request_error( "create_comment", - {"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"thread_id": "dummy", "course_id": unicode(self.course_id)}, {}, mock_request ) @@ -531,7 +596,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_create_comment_empty_body(self, mock_request): self._test_request_error( "create_comment", - {"thread_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"thread_id": "dummy", "course_id": unicode(self.course_id)}, {"body": " "}, mock_request ) @@ -539,7 +604,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_create_sub_comment_no_body(self, mock_request): self._test_request_error( "create_sub_comment", - {"comment_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"comment_id": "dummy", "course_id": unicode(self.course_id)}, {}, mock_request ) @@ -547,7 +612,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_create_sub_comment_empty_body(self, mock_request): self._test_request_error( "create_sub_comment", - {"comment_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"comment_id": "dummy", "course_id": unicode(self.course_id)}, {"body": " "}, mock_request ) @@ -555,7 +620,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_update_comment_no_body(self, mock_request): self._test_request_error( "update_comment", - {"comment_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"comment_id": "dummy", "course_id": unicode(self.course_id)}, {}, mock_request ) @@ -563,7 +628,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V def test_update_comment_empty_body(self, mock_request): self._test_request_error( "update_comment", - {"comment_id": "dummy", "course_id": self.course_id.to_deprecated_string()}, + {"comment_id": "dummy", "course_id": unicode(self.course_id)}, {"body": " "}, mock_request ) @@ -572,15 +637,14 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V self._setup_mock_request(mock_request) comment_id = "test_comment_id" updated_body = "updated body" - - response = self.client.post( - reverse( - "update_comment", - kwargs={"course_id": self.course_id.to_deprecated_string(), "comment_id": comment_id} - ), - data={"body": updated_body} - ) - + with self.assert_discussion_signals('comment_edited'): + response = self.client.post( + reverse( + "update_comment", + kwargs={"course_id": unicode(self.course_id), "comment_id": comment_id} + ), + data={"body": updated_body} + ) self.assertEqual(response.status_code, 200) mock_request.assert_called_with( "put", @@ -627,7 +691,10 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V "read": False, "comments_count": 0, }) - url = reverse('flag_abuse_for_thread', kwargs={'thread_id': '518d4237b023791dca00000d', 'course_id': self.course_id.to_deprecated_string()}) + url = reverse('flag_abuse_for_thread', kwargs={ + 'thread_id': '518d4237b023791dca00000d', + 'course_id': unicode(self.course_id) + }) response = self.client.post(url) assert_true(mock_request.called) @@ -702,7 +769,10 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V "read": False, "comments_count": 0 }) - url = reverse('un_flag_abuse_for_thread', kwargs={'thread_id': '518d4237b023791dca00000d', 'course_id': self.course_id.to_deprecated_string()}) + url = reverse('un_flag_abuse_for_thread', kwargs={ + 'thread_id': '518d4237b023791dca00000d', + 'course_id': unicode(self.course_id) + }) response = self.client.post(url) assert_true(mock_request.called) @@ -771,7 +841,10 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V "type": "comment", "endorsed": False }) - url = reverse('flag_abuse_for_comment', kwargs={'comment_id': '518d4237b023791dca00000d', 'course_id': self.course_id.to_deprecated_string()}) + url = reverse('flag_abuse_for_comment', kwargs={ + 'comment_id': '518d4237b023791dca00000d', + 'course_id': unicode(self.course_id) + }) response = self.client.post(url) assert_true(mock_request.called) @@ -840,7 +913,10 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V "type": "comment", "endorsed": False }) - url = reverse('un_flag_abuse_for_comment', kwargs={'comment_id': '518d4237b023791dca00000d', 'course_id': self.course_id.to_deprecated_string()}) + url = reverse('un_flag_abuse_for_comment', kwargs={ + 'comment_id': '518d4237b023791dca00000d', + 'course_id': unicode(self.course_id) + }) response = self.client.post(url) assert_true(mock_request.called) @@ -878,8 +954,39 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, V assert_equal(response.status_code, 200) + @ddt.data( + ('upvote_thread', 'thread_id', 'thread_voted'), + ('upvote_comment', 'comment_id', 'comment_voted'), + ('downvote_thread', 'thread_id', 'thread_voted'), + ('downvote_comment', 'comment_id', 'comment_voted') + ) + @ddt.unpack + def test_voting(self, view_name, item_id, signal, mock_request): + self._setup_mock_request(mock_request) + with self.assert_discussion_signals(signal): + response = self.client.post( + reverse( + view_name, + kwargs={item_id: 'dummy', 'course_id': unicode(self.course_id)} + ) + ) + self.assertEqual(response.status_code, 200) + + def test_endorse_comment(self, mock_request): + self._setup_mock_request(mock_request) + self.client.login(username=self.moderator.username, password=self.password) + with self.assert_discussion_signals('comment_endorsed', user=self.moderator): + response = self.client.post( + reverse( + 'endorse_comment', + kwargs={'comment_id': 'dummy', 'course_id': unicode(self.course_id)} + ) + ) + self.assertEqual(response.status_code, 200) + @patch("lms.lib.comment_client.utils.requests.request") +@disable_signal(views, 'comment_endorsed') class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): @@ -897,7 +1004,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet self._set_mock_request_data(mock_request, {}) self.client.login(username=self.student.username, password=self.password) response = self.client.post( - reverse("pin_thread", kwargs={"course_id": self.course.id.to_deprecated_string(), "thread_id": "dummy"}) + reverse("pin_thread", kwargs={"course_id": unicode(self.course.id), "thread_id": "dummy"}) ) self.assertEqual(response.status_code, 401) @@ -905,7 +1012,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet self._set_mock_request_data(mock_request, {}) self.client.login(username=self.moderator.username, password=self.password) response = self.client.post( - reverse("pin_thread", kwargs={"course_id": self.course.id.to_deprecated_string(), "thread_id": "dummy"}) + reverse("pin_thread", kwargs={"course_id": unicode(self.course.id), "thread_id": "dummy"}) ) self.assertEqual(response.status_code, 200) @@ -913,7 +1020,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet self._set_mock_request_data(mock_request, {}) self.client.login(username=self.student.username, password=self.password) response = self.client.post( - reverse("un_pin_thread", kwargs={"course_id": self.course.id.to_deprecated_string(), "thread_id": "dummy"}) + reverse("un_pin_thread", kwargs={"course_id": unicode(self.course.id), "thread_id": "dummy"}) ) self.assertEqual(response.status_code, 401) @@ -921,7 +1028,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet self._set_mock_request_data(mock_request, {}) self.client.login(username=self.moderator.username, password=self.password) response = self.client.post( - reverse("un_pin_thread", kwargs={"course_id": self.course.id.to_deprecated_string(), "thread_id": "dummy"}) + reverse("un_pin_thread", kwargs={"course_id": unicode(self.course.id), "thread_id": "dummy"}) ) self.assertEqual(response.status_code, 200) @@ -944,7 +1051,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet ) self.client.login(username=self.moderator.username, password=self.password) response = self.client.post( - reverse("endorse_comment", kwargs={"course_id": self.course.id.to_deprecated_string(), "comment_id": "dummy"}) + reverse("endorse_comment", kwargs={"course_id": unicode(self.course.id), "comment_id": "dummy"}) ) self.assertEqual(response.status_code, 200) @@ -956,7 +1063,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet ) self.client.login(username=self.student.username, password=self.password) response = self.client.post( - reverse("endorse_comment", kwargs={"course_id": self.course.id.to_deprecated_string(), "comment_id": "dummy"}) + reverse("endorse_comment", kwargs={"course_id": unicode(self.course.id), "comment_id": "dummy"}) ) self.assertEqual(response.status_code, 401) @@ -968,7 +1075,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet ) self.client.login(username=self.student.username, password=self.password) response = self.client.post( - reverse("endorse_comment", kwargs={"course_id": self.course.id.to_deprecated_string(), "comment_id": "dummy"}) + reverse("endorse_comment", kwargs={"course_id": unicode(self.course.id), "comment_id": "dummy"}) ) self.assertEqual(response.status_code, 200) @@ -992,7 +1099,7 @@ class CreateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockReq request.user = self.student request.view_name = "create_thread" response = views.create_thread( - request, course_id=self.course.id.to_deprecated_string(), commentable_id="non_team_dummy_id" + request, course_id=unicode(self.course.id), commentable_id="non_team_dummy_id" ) self.assertEqual(response.status_code, 200) @@ -1001,6 +1108,7 @@ class CreateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockReq self.assertEqual(mock_request.call_args[1]["data"]["title"], text) +@disable_signal(views, 'thread_edited') class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): def setUp(self): super(UpdateThreadUnicodeTestCase, self).setUp() @@ -1020,7 +1128,7 @@ class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockReq request = RequestFactory().post("dummy_url", {"body": text, "title": text, "thread_type": "question", "commentable_id": "test_commentable"}) request.user = self.student request.view_name = "update_thread" - response = views.update_thread(request, course_id=self.course.id.to_deprecated_string(), thread_id="dummy_thread_id") + response = views.update_thread(request, course_id=unicode(self.course.id), thread_id="dummy_thread_id") self.assertEqual(response.status_code, 200) self.assertTrue(mock_request.called) @@ -1030,6 +1138,7 @@ class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockReq self.assertEqual(mock_request.call_args[1]["data"]["commentable_id"], "test_commentable") +@disable_signal(views, 'comment_created') class CreateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): def setUp(self): super(CreateCommentUnicodeTestCase, self).setUp() @@ -1064,6 +1173,7 @@ class CreateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRe del Thread.commentable_id +@disable_signal(views, 'comment_edited') class UpdateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): def setUp(self): super(UpdateCommentUnicodeTestCase, self).setUp() @@ -1082,13 +1192,14 @@ class UpdateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRe request = RequestFactory().post("dummy_url", {"body": text}) request.user = self.student request.view_name = "update_comment" - response = views.update_comment(request, course_id=self.course.id.to_deprecated_string(), comment_id="dummy_comment_id") + response = views.update_comment(request, course_id=unicode(self.course.id), comment_id="dummy_comment_id") self.assertEqual(response.status_code, 200) self.assertTrue(mock_request.called) self.assertEqual(mock_request.call_args[1]["data"]["body"], text) +@disable_signal(views, 'comment_created') class CreateSubCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): """ Make sure comments under a response can handle unicode. @@ -1130,6 +1241,11 @@ class CreateSubCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, Moc @ddt.ddt @patch("lms.lib.comment_client.utils.requests.request") +@disable_signal(views, 'thread_voted') +@disable_signal(views, 'thread_edited') +@disable_signal(views, 'comment_created') +@disable_signal(views, 'comment_voted') +@disable_signal(views, 'comment_deleted') class TeamsPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): # Most of the test points use the same ddt data. # args: user, commentable_id, status_code @@ -1380,6 +1496,7 @@ class TeamsPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSe self.assertEqual(response.status_code, status_code) +@disable_signal(views, 'comment_created') class ForumEventTestCase(ModuleStoreTestCase, MockRequestSetupMixin): """ Forum actions are expected to launch analytics events. Test these here. @@ -1437,7 +1554,7 @@ class ForumEventTestCase(ModuleStoreTestCase, MockRequestSetupMixin): request = RequestFactory().post("dummy_url", {"body": "Test comment", 'auto_subscribe': True}) request.user = self.student request.view_name = "create_comment" - views.create_comment(request, course_id=self.course.id.to_deprecated_string(), thread_id='test_thread_id') + views.create_comment(request, course_id=unicode(self.course.id), thread_id='test_thread_id') event_name, event = mock_emit.call_args[0] self.assertEqual(event_name, 'edx.forum.response.created') diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 9778fb4392..b24409112c 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -18,6 +18,17 @@ from courseware.access import has_access from util.file import store_uploaded_file from courseware.courses import get_course_with_access, get_course_by_id import django_comment_client.settings as cc_settings +from django_comment_common.signals import ( + thread_created, + thread_edited, + thread_voted, + thread_deleted, + comment_created, + comment_edited, + comment_voted, + comment_deleted, + comment_endorsed +) from django_comment_common.utils import ThreadContext from django_comment_client.utils import ( add_courseware_context, @@ -161,6 +172,7 @@ def create_thread(request, course_id, commentable_id): course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) course = get_course_with_access(request.user, 'load', course_key) post = request.POST + user = request.user if course.allow_anonymous: anonymous = post.get('anonymous', 'false').lower() == 'true' @@ -182,7 +194,7 @@ def create_thread(request, course_id, commentable_id): 'anonymous_to_peers': anonymous_to_peers, 'commentable_id': commentable_id, 'course_id': course_key.to_deprecated_string(), - 'user_id': request.user.id, + 'user_id': user.id, 'thread_type': post["thread_type"], 'body': post["body"], 'title': post["title"], @@ -206,6 +218,8 @@ def create_thread(request, course_id, commentable_id): thread.save() + thread_created.send(sender=None, user=user, post=thread) + # patch for backward compatibility to comments service if 'pinned' not in thread.attributes: thread['pinned'] = False @@ -213,13 +227,13 @@ def create_thread(request, course_id, commentable_id): follow = post.get('auto_subscribe', 'false').lower() == 'true' if follow: - user = cc.User.from_django_user(request.user) - user.follow(thread) + cc_user = cc.User.from_django_user(user) + cc_user.follow(thread) event_data = get_thread_created_event_data(thread, follow) data = thread.to_dict() - add_courseware_context([data], course, request.user) + add_courseware_context([data], course, user) track_forum_event(request, THREAD_CREATED_EVENT_NAME, course, thread, event_data) @@ -247,19 +261,23 @@ def update_thread(request, course_id, thread_id): thread_context = getattr(thread, "context", "course") thread.body = request.POST["body"] thread.title = request.POST["title"] + user = request.user # The following checks should avoid issues we've seen during deploys, where end users are hitting an updated server # while their browser still has the old client code. This will avoid erasing present values in those cases. if "thread_type" in request.POST: thread.thread_type = request.POST["thread_type"] if "commentable_id" in request.POST: commentable_id = request.POST["commentable_id"] - course = get_course_with_access(request.user, 'load', course_key) - if thread_context == "course" and not discussion_category_id_access(course, request.user, commentable_id): + course = get_course_with_access(user, 'load', course_key) + if thread_context == "course" and not discussion_category_id_access(course, user, commentable_id): return JsonError(_("Topic doesn't exist")) else: thread.commentable_id = commentable_id thread.save() + + thread_edited.send(sender=None, user=user, post=thread) + if request.is_ajax(): return ajax_content_response(request, course_key, thread.to_dict()) else: @@ -273,11 +291,12 @@ def _create_comment(request, course_key, thread_id=None, parent_id=None): """ assert isinstance(course_key, CourseKey) post = request.POST + user = request.user if 'body' not in post or not post['body'].strip(): return JsonError(_("Body can't be empty")) - course = get_course_with_access(request.user, 'load', course_key) + course = get_course_with_access(user, 'load', course_key) if course.allow_anonymous: anonymous = post.get('anonymous', 'false').lower() == 'true' else: @@ -291,7 +310,7 @@ def _create_comment(request, course_key, thread_id=None, parent_id=None): comment = cc.Comment( anonymous=anonymous, anonymous_to_peers=anonymous_to_peers, - user_id=request.user.id, + user_id=user.id, course_id=course_key.to_deprecated_string(), thread_id=thread_id, parent_id=parent_id, @@ -299,11 +318,13 @@ def _create_comment(request, course_key, thread_id=None, parent_id=None): ) comment.save() + comment_created.send(sender=None, user=user, post=comment) + followed = post.get('auto_subscribe', 'false').lower() == 'true' if followed: - user = cc.User.from_django_user(request.user) - user.follow(comment.thread) + cc_user = cc.User.from_django_user(request.user) + cc_user.follow(comment.thread) event_name = get_comment_created_event_name(comment) event_data = get_comment_created_event_data(comment, comment.thread.commentable_id, followed) @@ -339,7 +360,7 @@ def delete_thread(request, course_id, thread_id): # pylint: disable=unused-argu course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) thread = cc.Thread.find(thread_id) thread.delete() - + thread_deleted.send(sender=None, user=request.user, post=thread) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -357,6 +378,9 @@ def update_comment(request, course_id, comment_id): return JsonError(_("Body can't be empty")) comment.body = request.POST["body"] comment.save() + + comment_edited.send(sender=None, user=request.user, post=comment) + if request.is_ajax(): return ajax_content_response(request, course_key, comment.to_dict()) else: @@ -373,9 +397,11 @@ def endorse_comment(request, course_id, comment_id): """ course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) comment = cc.Comment.find(comment_id) + user = request.user comment.endorsed = request.POST.get('endorsed', 'false').lower() == 'true' - comment.endorsement_user_id = request.user.id + comment.endorsement_user_id = user.id comment.save() + comment_endorsed.send(sender=None, user=user, post=comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -422,6 +448,7 @@ def delete_comment(request, course_id, comment_id): course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) comment = cc.Comment.find(comment_id) comment.delete() + comment_deleted.send(sender=None, user=request.user, post=comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -433,9 +460,11 @@ def vote_for_comment(request, course_id, comment_id, value): given a course_id and comment_id, """ course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - user = cc.User.from_django_user(request.user) + user = request.user + cc_user = cc.User.from_django_user(user) comment = cc.Comment.find(comment_id) - user.vote(comment, value) + cc_user.vote(comment, value) + comment_voted.send(sender=None, user=user, post=comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -463,10 +492,11 @@ def vote_for_thread(request, course_id, thread_id, value): ajax only """ course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - user = cc.User.from_django_user(request.user) + user = request.user + cc_user = cc.User.from_django_user(user) thread = cc.Thread.find(thread_id) - user.vote(thread, value) - + cc_user.vote(thread, value) + thread_voted.send(sender=None, user=user, post=thread) return JsonResponse(prepare_content(thread.to_dict(), course_key)) diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 297e3bd774..4fed6a075e 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -13,9 +13,9 @@ from courseware.tabs import get_course_tab_list from courseware.tests.factories import UserFactory from courseware.tests.helpers import LoginEnrollmentTestCase +from common.test.utils import XssTestMixin from student.tests.factories import AdminFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.utils import XssTestMixin from xmodule.modulestore.tests.factories import CourseFactory from shoppingcart.models import PaidCourseRegistration, Order, CourseRegCodeItem from course_modes.models import CourseMode diff --git a/lms/djangoapps/shoppingcart/tests/test_views.py b/lms/djangoapps/shoppingcart/tests/test_views.py index bf1d9448af..ef9251c20e 100644 --- a/lms/djangoapps/shoppingcart/tests/test_views.py +++ b/lms/djangoapps/shoppingcart/tests/test_views.py @@ -24,9 +24,9 @@ from datetime import datetime, timedelta from mock import patch, Mock import ddt +from common.test.utils import XssTestMixin from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.tests.utils import XssTestMixin from student.roles import CourseSalesAdminRole from util.date_utils import get_default_time_display from util.testing import UrlResetMixin diff --git a/lms/djangoapps/teams/__init__.py b/lms/djangoapps/teams/__init__.py index 0aaad373bf..4bd41fb2fd 100644 --- a/lms/djangoapps/teams/__init__.py +++ b/lms/djangoapps/teams/__init__.py @@ -5,6 +5,9 @@ Defines common methods shared by Teams classes from django.conf import settings +TEAM_DISCUSSION_CONTEXT = 'standalone' + + def is_feature_enabled(course): """ Returns True if the teams feature is enabled. diff --git a/lms/djangoapps/teams/models.py b/lms/djangoapps/teams/models.py index a0d872e1ae..27b238a68d 100644 --- a/lms/djangoapps/teams/models.py +++ b/lms/djangoapps/teams/models.py @@ -1,18 +1,73 @@ """Django models related to teams functionality.""" +from datetime import datetime from uuid import uuid4 import pytz from datetime import datetime +from django.core.exceptions import ObjectDoesNotExist from django.contrib.auth.models import User from django.db import models +from django.dispatch import receiver from django.utils.translation import ugettext_lazy from django_countries.fields import CountryField +from django_comment_common.signals import ( + thread_created, + thread_edited, + thread_deleted, + thread_voted, + comment_created, + comment_edited, + comment_deleted, + comment_voted, + comment_endorsed +) from xmodule_django.models import CourseKeyField from util.model_utils import generate_unique_readable_id from student.models import LanguageField, CourseEnrollment from .errors import AlreadyOnTeamInCourse, NotEnrolledInCourseForTeam +from teams import TEAM_DISCUSSION_CONTEXT + + +@receiver(thread_voted) +@receiver(thread_created) +@receiver(comment_voted) +@receiver(comment_created) +def post_create_vote_handler(sender, **kwargs): # pylint: disable=unused-argument + """Update the user's last activity date upon creating or voting for a + post.""" + handle_activity(kwargs['user'], kwargs['post']) + + +@receiver(thread_edited) +@receiver(thread_deleted) +@receiver(comment_edited) +@receiver(comment_deleted) +def post_edit_delete_handler(sender, **kwargs): # pylint: disable=unused-argument + """Update the user's last activity date upon editing or deleting a + post.""" + post = kwargs['post'] + handle_activity(kwargs['user'], post, long(post.user_id)) + + +@receiver(comment_endorsed) +def comment_endorsed_handler(sender, **kwargs): # pylint: disable=unused-argument + """Update the user's last activity date upon endorsing a comment.""" + comment = kwargs['post'] + handle_activity(kwargs['user'], comment, long(comment.thread.user_id)) + + +def handle_activity(user, post, original_author_id=None): + """Handle user activity from django_comment_client and discussion_api + and update the user's last activity date. Checks if the user who + performed the action is the original author, and that the + discussion has the team context. + """ + if original_author_id is not None and user.id != original_author_id: + return + if getattr(post, "context", "course") == TEAM_DISCUSSION_CONTEXT: + CourseTeamMembership.update_last_activity(user, post.commentable_id) class CourseTeam(models.Model): @@ -134,3 +189,22 @@ class CourseTeamMembership(models.Model): False if not """ return cls.objects.filter(user=user, team__course_id=course_id).exists() + + @classmethod + def update_last_activity(cls, user, discussion_topic_id): + """Set the `last_activity_at` for both this user and their team in the + given discussion topic. No-op if the user is not a member of + the team for this discussion. + """ + try: + membership = cls.objects.get(user=user, team__discussion_topic_id=discussion_topic_id) + # If a privileged user is active in the discussion of a team + # they do not belong to, do not update their last activity + # information. + except ObjectDoesNotExist: + return + now = datetime.utcnow().replace(tzinfo=pytz.utc) + membership.last_activity_at = now + membership.team.last_activity_at = now + membership.team.save() + membership.save() diff --git a/lms/djangoapps/teams/tests/test_models.py b/lms/djangoapps/teams/tests/test_models.py index ba82f7f053..39c645dd78 100644 --- a/lms/djangoapps/teams/tests/test_models.py +++ b/lms/djangoapps/teams/tests/test_models.py @@ -1,13 +1,30 @@ # -*- coding: utf-8 -*- """Tests for the teams API at the HTTP request level.""" +from contextlib import contextmanager +from datetime import datetime import ddt +import itertools +from mock import Mock +import pytz +from django_comment_common.signals import ( + thread_created, + thread_edited, + thread_deleted, + thread_voted, + comment_created, + comment_edited, + comment_deleted, + comment_voted, + comment_endorsed +) from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from opaque_keys.edx.keys import CourseKey from student.tests.factories import UserFactory from .factories import CourseTeamFactory, CourseTeamMembershipFactory -from ..models import CourseTeamMembership +from ..models import CourseTeam, CourseTeamMembership +from teams import TEAM_DISCUSSION_CONTEXT COURSE_KEY1 = CourseKey.from_string('edx/history/1') COURSE_KEY2 = CourseKey.from_string('edx/history/2') @@ -73,3 +90,93 @@ class TeamMembershipTest(SharedModuleStoreTestCase): CourseTeamMembership.user_in_team_for_course(user, course_id), expected_value ) + + +@ddt.ddt +class TeamSignalsTest(SharedModuleStoreTestCase): + """Tests for handling of team-related signals.""" + + SIGNALS_LIST = ( + thread_created, + thread_edited, + thread_deleted, + thread_voted, + comment_created, + comment_edited, + comment_deleted, + comment_voted, + comment_endorsed + ) + + DISCUSSION_TOPIC_ID = 'test_topic' + + def setUp(self): + """Create a user with a team to test signals.""" + super(TeamSignalsTest, self).setUp() + self.user = UserFactory.create(username="user") + self.moderator = UserFactory.create(username="moderator") + self.team = CourseTeamFactory(discussion_topic_id=self.DISCUSSION_TOPIC_ID) + self.team_membership = CourseTeamMembershipFactory(user=self.user, team=self.team) + + def mock_comment(self, context=TEAM_DISCUSSION_CONTEXT, user=None): + """Create a mock comment service object with the given context.""" + if user is None: + user = self.user + return Mock( + user_id=user.id, + commentable_id=self.DISCUSSION_TOPIC_ID, + context=context, + **{'thread.user_id': self.user.id} + ) + + @contextmanager + def assert_last_activity_updated(self, should_update): + """If `should_update` is True, assert that the team and team + membership have had their `last_activity_at` updated. Otherwise, + assert that it was not updated. + """ + team_last_activity = self.team.last_activity_at + team_membership_last_activity = self.team_membership.last_activity_at + yield + # Reload team and team membership from the database in order to pick up changes + team = CourseTeam.objects.get(id=self.team.id) # pylint: disable=maybe-no-member + team_membership = CourseTeamMembership.objects.get(id=self.team_membership.id) # pylint: disable=maybe-no-member + if should_update: + self.assertGreater(team.last_activity_at, team_last_activity) + self.assertGreater(team_membership.last_activity_at, team_membership_last_activity) + now = datetime.utcnow().replace(tzinfo=pytz.utc) + self.assertGreater(now, team.last_activity_at) + self.assertGreater(now, team_membership.last_activity_at) + else: + self.assertEqual(team.last_activity_at, team_last_activity) + self.assertEqual(team_membership.last_activity_at, team_membership_last_activity) + + @ddt.data( + *itertools.product( + SIGNALS_LIST, + (('user', True), ('moderator', False)) + ) + ) + @ddt.unpack + def test_signals(self, signal, (user, should_update)): + """Test that `last_activity_at` is correctly updated when team-related + signals are sent. + """ + with self.assert_last_activity_updated(should_update): + user = getattr(self, user) + signal.send(sender=None, user=user, post=self.mock_comment()) + + @ddt.data(thread_voted, comment_voted) + def test_vote_others_post(self, signal): + """Test that voting on another user's post correctly fires a + signal.""" + with self.assert_last_activity_updated(True): + signal.send(sender=None, user=self.user, post=self.mock_comment(user=self.moderator)) + + @ddt.data(*SIGNALS_LIST) + def test_signals_course_context(self, signal): + """Test that `last_activity_at` is not updated when activity takes + place in discussions outside of a team. + """ + with self.assert_last_activity_updated(False): + signal.send(sender=None, user=self.user, post=self.mock_comment(context='course')) diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index 01259363c5..3048e24627 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -33,6 +33,7 @@ from opaque_keys.edx.keys import UsageKey from course_modes.models import CourseMode from course_modes.tests.factories import CourseModeFactory from courseware.url_helpers import get_redirect_url +from common.test.utils import XssTestMixin from commerce.tests import TEST_PAYMENT_DATA, TEST_API_URL, TEST_API_SIGNING_KEY from embargo.test_utils import restrict_course from openedx.core.djangoapps.user_api.accounts.api import get_account_settings @@ -51,7 +52,6 @@ from verify_student.models import ( ) from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from xmodule.modulestore.tests.utils import XssTestMixin from xmodule.modulestore.django import modulestore from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.factories import check_mongo_calls diff --git a/lms/lib/comment_client/comment.py b/lms/lib/comment_client/comment.py index d2c8dcbcf7..cc7c097c19 100644 --- a/lms/lib/comment_client/comment.py +++ b/lms/lib/comment_client/comment.py @@ -30,6 +30,11 @@ class Comment(models.Model): def thread(self): return Thread(id=self.thread_id, type='thread') + @property + def context(self): + """Return the context of the thread which this comment belongs to.""" + return self.thread.context + @classmethod def url_for_comments(cls, params={}): if params.get('parent_id'):