diff --git a/common/static/coffee/src/discussion/utils.coffee b/common/static/coffee/src/discussion/utils.coffee index 8f3b5e2ced..8b90f04662 100644 --- a/common/static/coffee/src/discussion/utils.coffee +++ b/common/static/coffee/src/discussion/utils.coffee @@ -51,7 +51,6 @@ class @DiscussionUtil follow_discussion : "/courses/#{$$course_id}/discussion/#{param}/follow" unfollow_discussion : "/courses/#{$$course_id}/discussion/#{param}/unfollow" create_thread : "/courses/#{$$course_id}/discussion/#{param}/threads/create" - search_similar_threads : "/courses/#{$$course_id}/discussion/#{param}/threads/search_similar" update_thread : "/courses/#{$$course_id}/discussion/threads/#{param}/update" create_comment : "/courses/#{$$course_id}/discussion/threads/#{param}/reply" delete_thread : "/courses/#{$$course_id}/discussion/threads/#{param}/delete" diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 4495e648d9..8739c54edb 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -23,9 +23,16 @@ log = logging.getLogger(__name__) CS_PREFIX = "http://localhost:4567/api/v1" + +class MockRequestSetupMixin(object): + def _set_mock_request_data(self, mock_request, data): + mock_request.return_value.text = json.dumps(data) + mock_request.return_value.json.return_value = data + + @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @patch('lms.lib.comment_client.utils.requests.request') -class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): +class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): @@ -63,26 +70,35 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): def test_create_thread(self, mock_request): mock_request.return_value.status_code = 200 - mock_request.return_value.text = u'{"title":"Hello",\ - "body":"this is a post",\ - "course_id":"MITx/999/Robot_Super_Course",\ - "anonymous":false,\ - "anonymous_to_peers":false,\ - "commentable_id":"i4x-MITx-999-course-Robot_Super_Course",\ - "created_at":"2013-05-10T18:53:43Z",\ - "updated_at":"2013-05-10T18:53:43Z",\ - "at_position_list":[],\ - "closed":false,\ - "id":"518d4237b023791dca00000d",\ - "user_id":"1","username":"robot",\ - "votes":{"count":0,"up_count":0,\ - "down_count":0,"point":0},\ - "abuse_flaggers":[],\ - "type":"thread","group_id":null,\ - "pinned":false,\ - "endorsed":false,\ - "unread_comments_count":0,\ - "read":false,"comments_count":0}' + self._set_mock_request_data(mock_request, { + "title": "Hello", + "body": "this is a post", + "course_id": "MITx/999/Robot_Super_Course", + "anonymous": False, + "anonymous_to_peers": False, + "commentable_id": "i4x-MITx-999-course-Robot_Super_Course", + "created_at": "2013-05-10T18:53:43Z", + "updated_at": "2013-05-10T18:53:43Z", + "at_position_list": [], + "closed": False, + "id": "518d4237b023791dca00000d", + "user_id": "1", + "username": "robot", + "votes": { + "count": 0, + "up_count": 0, + "down_count": 0, + "point": 0 + }, + "abuse_flaggers": [], + "type": "thread", + "group_id": None, + "pinned": False, + "endorsed": False, + "unread_comments_count": 0, + "read": False, + "comments_count": 0, + }) thread = {"body": ["this is a post"], "anonymous_to_peers": ["false"], "auto_subscribe": ["false"], @@ -110,7 +126,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): assert_equal(response.status_code, 200) def test_delete_comment(self, mock_request): - mock_request.return_value.text = json.dumps({ + self._set_mock_request_data(mock_request, { "user_id": str(self.student.id), "closed": False, }) @@ -138,7 +154,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): } if include_depth: data["depth"] = 0 - mock_request.return_value.text = json.dumps(data) + self._set_mock_request_data(mock_request, data) def _test_request_error(self, view_name, view_kwargs, data, mock_request): """ @@ -290,26 +306,34 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): def test_flag_thread(self, mock_request): mock_request.return_value.status_code = 200 - mock_request.return_value.text = u'{"title":"Hello",\ - "body":"this is a post",\ - "course_id":"MITx/999/Robot_Super_Course",\ - "anonymous":false,\ - "anonymous_to_peers":false,\ - "commentable_id":"i4x-MITx-999-course-Robot_Super_Course",\ - "created_at":"2013-05-10T18:53:43Z",\ - "updated_at":"2013-05-10T18:53:43Z",\ - "at_position_list":[],\ - "closed":false,\ - "id":"518d4237b023791dca00000d",\ - "user_id":"1","username":"robot",\ - "votes":{"count":0,"up_count":0,\ - "down_count":0,"point":0},\ - "abuse_flaggers":[1],\ - "type":"thread","group_id":null,\ - "pinned":false,\ - "endorsed":false,\ - "unread_comments_count":0,\ - "read":false,"comments_count":0}' + self._set_mock_request_data(mock_request, { + "title": "Hello", + "body": "this is a post", + "course_id": "MITx/999/Robot_Super_Course", + "anonymous": False, + "anonymous_to_peers": False, + "commentable_id": "i4x-MITx-999-course-Robot_Super_Course", + "created_at": "2013-05-10T18:53:43Z", + "updated_at": "2013-05-10T18:53:43Z", + "at_position_list": [], + "closed": False, + "id": "518d4237b023791dca00000d", + "user_id": "1","username": "robot", + "votes": { + "count": 0, + "up_count": 0, + "down_count": 0, + "point": 0 + }, + "abuse_flaggers": [1], + "type": "thread", + "group_id": None, + "pinned": False, + "endorsed": False, + "unread_comments_count": 0, + "read": False, + "comments_count": 0, + }) url = reverse('flag_abuse_for_thread', kwargs={'thread_id': '518d4237b023791dca00000d', 'course_id': self.course_id}) response = self.client.post(url) assert_true(mock_request.called) @@ -350,26 +374,35 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): def test_un_flag_thread(self, mock_request): mock_request.return_value.status_code = 200 - mock_request.return_value.text = u'{"title":"Hello",\ - "body":"this is a post",\ - "course_id":"MITx/999/Robot_Super_Course",\ - "anonymous":false,\ - "anonymous_to_peers":false,\ - "commentable_id":"i4x-MITx-999-course-Robot_Super_Course",\ - "created_at":"2013-05-10T18:53:43Z",\ - "updated_at":"2013-05-10T18:53:43Z",\ - "at_position_list":[],\ - "closed":false,\ - "id":"518d4237b023791dca00000d",\ - "user_id":"1","username":"robot",\ - "votes":{"count":0,"up_count":0,\ - "down_count":0,"point":0},\ - "abuse_flaggers":[],\ - "type":"thread","group_id":null,\ - "pinned":false,\ - "endorsed":false,\ - "unread_comments_count":0,\ - "read":false,"comments_count":0}' + self._set_mock_request_data(mock_request, { + "title": "Hello", + "body": "this is a post", + "course_id": "MITx/999/Robot_Super_Course", + "anonymous": False, + "anonymous_to_peers": False, + "commentable_id": "i4x-MITx-999-course-Robot_Super_Course", + "created_at": "2013-05-10T18:53:43Z", + "updated_at": "2013-05-10T18:53:43Z", + "at_position_list": [], + "closed": False, + "id": "518d4237b023791dca00000d", + "user_id": "1", + "username": "robot", + "votes": { + "count": 0, + "up_count": 0, + "down_count": 0, + "point": 0 + }, + "abuse_flaggers": [], + "type": "thread", + "group_id": None, + "pinned": False, + "endorsed": False, + "unread_comments_count": 0, + "read": False, + "comments_count": 0 + }) url = reverse('un_flag_abuse_for_thread', kwargs={'thread_id': '518d4237b023791dca00000d', 'course_id': self.course_id}) response = self.client.post(url) assert_true(mock_request.called) @@ -410,22 +443,29 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): def test_flag_comment(self, mock_request): mock_request.return_value.status_code = 200 - mock_request.return_value.text = u'{"body":"this is a comment",\ - "course_id":"MITx/999/Robot_Super_Course",\ - "anonymous":false,\ - "anonymous_to_peers":false,\ - "commentable_id":"i4x-MITx-999-course-Robot_Super_Course",\ - "created_at":"2013-05-10T18:53:43Z",\ - "updated_at":"2013-05-10T18:53:43Z",\ - "at_position_list":[],\ - "closed":false,\ - "id":"518d4237b023791dca00000d",\ - "user_id":"1","username":"robot",\ - "votes":{"count":0,"up_count":0,\ - "down_count":0,"point":0},\ - "abuse_flaggers":[1],\ - "type":"comment",\ - "endorsed":false}' + self._set_mock_request_data(mock_request, { + "body": "this is a comment", + "course_id": "MITx/999/Robot_Super_Course", + "anonymous": False, + "anonymous_to_peers": False, + "commentable_id": "i4x-MITx-999-course-Robot_Super_Course", + "created_at": "2013-05-10T18:53:43Z", + "updated_at": "2013-05-10T18:53:43Z", + "at_position_list": [], + "closed": False, + "id": "518d4237b023791dca00000d", + "user_id": "1", + "username": "robot", + "votes": { + "count": 0, + "up_count": 0, + "down_count": 0, + "point": 0 + }, + "abuse_flaggers": [1], + "type": "comment", + "endorsed": False + }) url = reverse('flag_abuse_for_comment', kwargs={'comment_id': '518d4237b023791dca00000d', 'course_id': self.course_id}) response = self.client.post(url) assert_true(mock_request.called) @@ -466,22 +506,29 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): def test_un_flag_comment(self, mock_request): mock_request.return_value.status_code = 200 - mock_request.return_value.text = u'{"body":"this is a comment",\ - "course_id":"MITx/999/Robot_Super_Course",\ - "anonymous":false,\ - "anonymous_to_peers":false,\ - "commentable_id":"i4x-MITx-999-course-Robot_Super_Course",\ - "created_at":"2013-05-10T18:53:43Z",\ - "updated_at":"2013-05-10T18:53:43Z",\ - "at_position_list":[],\ - "closed":false,\ - "id":"518d4237b023791dca00000d",\ - "user_id":"1","username":"robot",\ - "votes":{"count":0,"up_count":0,\ - "down_count":0,"point":0},\ - "abuse_flaggers":[],\ - "type":"comment",\ - "endorsed":false}' + self._set_mock_request_data(mock_request, { + "body": "this is a comment", + "course_id": "MITx/999/Robot_Super_Course", + "anonymous": False, + "anonymous_to_peers": False, + "commentable_id": "i4x-MITx-999-course-Robot_Super_Course", + "created_at": "2013-05-10T18:53:43Z", + "updated_at": "2013-05-10T18:53:43Z", + "at_position_list": [], + "closed": False, + "id": "518d4237b023791dca00000d", + "user_id": "1", + "username": "robot", + "votes": { + "count": 0, + "up_count": 0, + "down_count": 0, + "point": 0 + }, + "abuse_flaggers": [], + "type": "comment", + "endorsed": False + }) url = reverse('un_flag_abuse_for_comment', kwargs={'comment_id': '518d4237b023791dca00000d', 'course_id': self.course_id}) response = self.client.post(url) assert_true(mock_request.called) @@ -522,7 +569,7 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase): @patch("lms.lib.comment_client.utils.requests.request") @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase): +class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): super(ViewPermissionsTestCase, self).setUp() @@ -536,7 +583,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase): self.moderator.roles.add(Role.objects.get(name="Moderator", course_id=self.course.id)) def test_pin_thread_as_student(self, mock_request): - mock_request.return_value.text = "{}" + 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, "thread_id": "dummy"}) @@ -544,7 +591,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(response.status_code, 401) def test_pin_thread_as_moderator(self, mock_request): - mock_request.return_value.text = "{}" + 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, "thread_id": "dummy"}) @@ -552,7 +599,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(response.status_code, 200) def test_un_pin_thread_as_student(self, mock_request): - mock_request.return_value.text = "{}" + 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, "thread_id": "dummy"}) @@ -560,7 +607,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase): self.assertEqual(response.status_code, 401) def test_un_pin_thread_as_moderator(self, mock_request): - mock_request.return_value.text = "{}" + 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, "thread_id": "dummy"}) @@ -569,7 +616,7 @@ class ViewPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -class CreateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): +class CreateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): def setUp(self): self.course = CourseFactory.create() seed_permissions_roles(self.course.id) @@ -578,7 +625,7 @@ class CreateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.return_value.text = "{}" + self._set_mock_request_data(mock_request, {}) request = RequestFactory().post("dummy_url", {"body": text, "title": text}) request.user = self.student request.view_name = "create_thread" @@ -591,7 +638,7 @@ class CreateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): +class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): def setUp(self): self.course = CourseFactory.create() seed_permissions_roles(self.course.id) @@ -600,7 +647,7 @@ class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.return_value.text = json.dumps({ + self._set_mock_request_data(mock_request, { "user_id": str(self.student.id), "closed": False, }) @@ -616,7 +663,7 @@ class UpdateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -class CreateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): +class CreateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): def setUp(self): self.course = CourseFactory.create() seed_permissions_roles(self.course.id) @@ -625,7 +672,7 @@ class CreateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.return_value.text = json.dumps({ + self._set_mock_request_data(mock_request, { "closed": False, }) request = RequestFactory().post("dummy_url", {"body": text}) @@ -639,7 +686,7 @@ class CreateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -class UpdateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): +class UpdateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): def setUp(self): self.course = CourseFactory.create() seed_permissions_roles(self.course.id) @@ -648,7 +695,7 @@ class UpdateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.return_value.text = json.dumps({ + self._set_mock_request_data(mock_request, { "user_id": str(self.student.id), "closed": False, }) @@ -663,7 +710,7 @@ class UpdateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) -class CreateSubCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): +class CreateSubCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): def setUp(self): self.course = CourseFactory.create() seed_permissions_roles(self.course.id) @@ -672,7 +719,7 @@ class CreateSubCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin): @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): - mock_request.return_value.text = json.dumps({ + self._set_mock_request_data(mock_request, { "closed": False, "depth": 1, }) diff --git a/lms/djangoapps/django_comment_client/base/urls.py b/lms/djangoapps/django_comment_client/base/urls.py index 972c2ee23e..486cf5a93f 100644 --- a/lms/djangoapps/django_comment_client/base/urls.py +++ b/lms/djangoapps/django_comment_client/base/urls.py @@ -25,8 +25,6 @@ urlpatterns = patterns('django_comment_client.base.views', # nopep8 url(r'comments/(?P[\w\-]+)/flagAbuse$', 'flag_abuse_for_comment', name='flag_abuse_for_comment'), url(r'comments/(?P[\w\-]+)/unFlagAbuse$', 'un_flag_abuse_for_comment', name='un_flag_abuse_for_comment'), url(r'^(?P[\w\-.]+)/threads/create$', 'create_thread', name='create_thread'), - # TODO should we search within the board? - url(r'^(?P[\w\-.]+)/threads/search_similar$', 'search_similar_threads', name='search_similar_threads'), url(r'^(?P[\w\-.]+)/follow$', 'follow_commentable', name='follow_commentable'), url(r'^(?P[\w\-.]+)/unfollow$', 'unfollow_commentable', name='unfollow_commentable'), ) diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index c479e6f65a..cca662ab9e 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -521,27 +521,6 @@ def unfollow_user(request, course_id, followed_user_id): return JsonResponse({}) -@require_GET -def search_similar_threads(request, course_id, commentable_id): - """ - given a course id and commentable id, run query given in text get param - of request - """ - text = request.GET.get('text', None) - if text: - query_params = { - 'text': text, - 'commentable_id': commentable_id, - } - threads = cc.search_similar_threads(course_id, recursive=False, query_params=query_params) - else: - theads = [] - context = {'threads': map(utils.extend_content, threads)} - return JsonResponse({ - 'html': render_to_string('discussion/_similar_posts.html', context) - }) - - @require_POST @login_required @csrf.csrf_exempt diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 0bfeded536..188aba479e 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -109,26 +109,18 @@ def make_mock_request_impl(text, thread_id=None): def mock_request_impl(*args, **kwargs): url = args[1] if url.endswith("threads"): - return Mock( - status_code=200, - text=json.dumps({ - "collection": [make_mock_thread_data(text, "dummy_thread_id", False)] - }) - ) + data = { + "collection": [make_mock_thread_data(text, "dummy_thread_id", False)] + } elif thread_id and url.endswith(thread_id): - return Mock( - status_code=200, - text=json.dumps(make_mock_thread_data(text, thread_id, True)) - ) + data = make_mock_thread_data(text, thread_id, True) else: # user query - return Mock( - status_code=200, - text=json.dumps({ - "upvoted_ids": [], - "downvoted_ids": [], - "subscribed_thread_ids": [], - }) - ) + data = { + "upvoted_ids": [], + "downvoted_ids": [], + "subscribed_thread_ids": [], + } + return Mock(status_code=200, text=json.dumps(data), json=Mock(return_value=data)) return mock_request_impl diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index a8668b957d..407d0715a9 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -196,12 +196,6 @@ def forum_form_discussion(request, course_id): 'page': query_params['page'], }) else: - #recent_active_threads = cc.search_recent_active_threads( - # course_id, - # recursive=False, - # query_params={'follower_id': request.user.id}, - #) - with newrelic.agent.FunctionTrace(nr_transaction, "get_cohort_info"): cohorts = get_course_cohorts(course_id) cohorted_commentables = get_cohorted_commentables(course_id) @@ -283,12 +277,6 @@ def single_thread(request, course_id, discussion_id, thread_id): threads = [utils.safe_content(thread) for thread in threads] - #recent_active_threads = cc.search_recent_active_threads( - # course_id, - # recursive=False, - # query_params={'follower_id': request.user.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) diff --git a/lms/lib/comment_client/comment.py b/lms/lib/comment_client/comment.py index 2b0d77b15f..c09c7d9c98 100644 --- a/lms/lib/comment_client/comment.py +++ b/lms/lib/comment_client/comment.py @@ -21,6 +21,8 @@ class Comment(models.Model): initializable_fields = updatable_fields + metrics_tag_fields = ['course_id', 'endorsed', 'closed'] + base_url = "{prefix}/comments".format(prefix=settings.PREFIX) type = 'comment' @@ -50,7 +52,13 @@ class Comment(models.Model): else: raise CommentClientRequestError("Can only flag/unflag threads or comments") params = {'user_id': user.id} - request = perform_request('put', url, params) + request = perform_request( + 'put', + url, + params, + metric_tags=self._metric_tags, + metric_action='comment.abuse.flagged' + ) voteable.update_attributes(request) def unFlagAbuse(self, user, voteable, removeAll): @@ -65,7 +73,13 @@ class Comment(models.Model): if removeAll: params['all'] = True - request = perform_request('put', url, params) + request = perform_request( + 'put', + url, + params, + metric_tags=self._metric_tags, + metric_action='comment.abuse.unflagged' + ) voteable.update_attributes(request) diff --git a/lms/lib/comment_client/comment_client.py b/lms/lib/comment_client/comment_client.py index d6a14deeff..f747d7e294 100644 --- a/lms/lib/comment_client/comment_client.py +++ b/lms/lib/comment_client/comment_client.py @@ -4,27 +4,3 @@ from .comment import Comment from .thread import Thread from .user import User from .commentable import Commentable - -from .utils import perform_request - -import settings - - -def search_similar_threads(course_id, recursive=False, query_params={}, *args, **kwargs): - default_params = {'course_id': course_id, 'recursive': recursive} - attributes = dict(default_params.items() + query_params.items()) - return perform_request('get', _url_for_search_similar_threads(), attributes, *args, **kwargs) - - -def search_recent_active_threads(course_id, recursive=False, query_params={}, *args, **kwargs): - default_params = {'course_id': course_id, 'recursive': recursive} - attributes = dict(default_params.items() + query_params.items()) - return perform_request('get', _url_for_search_recent_active_threads(), attributes, *args, **kwargs) - - -def _url_for_search_similar_threads(): - return "{prefix}/search/threads/more_like_this".format(prefix=settings.PREFIX) - - -def _url_for_search_recent_active_threads(): - return "{prefix}/search/threads/recent_active".format(prefix=settings.PREFIX) diff --git a/lms/lib/comment_client/models.py b/lms/lib/comment_client/models.py index 367b9abd10..5bb3205641 100644 --- a/lms/lib/comment_client/models.py +++ b/lms/lib/comment_client/models.py @@ -8,6 +8,7 @@ class Model(object): initializable_fields = ['id'] base_url = None default_retrieve_params = {} + metric_tag_fields = [] DEFAULT_ACTIONS_WITH_ID = ['get', 'put', 'delete'] DEFAULT_ACTIONS_WITHOUT_ID = ['get_all', 'post'] @@ -62,9 +63,32 @@ class Model(object): def _retrieve(self, *args, **kwargs): url = self.url(action='get', params=self.attributes) - response = perform_request('get', url, self.default_retrieve_params) + response = perform_request( + 'get', + url, + self.default_retrieve_params, + metric_tags=self._metric_tags, + metric_action='model.retrieve' + ) self.update_attributes(**response) + @property + def _metric_tags(self): + """ + Returns a list of tags to be used when recording metrics about this model. + + Each field named in ``self.metric_tag_fields`` is used as a tag value, + under the key ``.``. The tag model_class is used to + record the class name of the model. + """ + tags = [ + u'{}.{}:{}'.format(self.__class__.__name__, attr, self[attr]) + for attr in self.metric_tag_fields + if attr in self.attributes + ] + tags.append(u'model_class:{}'.format(self.__class__.__name__)) + return tags + @classmethod def find(cls, id): return cls(id=id) @@ -94,17 +118,29 @@ class Model(object): self.before_save(self) if self.id: # if we have id already, treat this as an update url = self.url(action='put', params=self.attributes) - response = perform_request('put', url, self.updatable_attributes()) + response = perform_request( + 'put', + url, + self.updatable_attributes(), + metric_tags=self._metric_tags, + metric_action='model.update' + ) else: # otherwise, treat this as an insert url = self.url(action='post', params=self.attributes) - response = perform_request('post', url, self.initializable_attributes()) + response = perform_request( + 'post', + url, + self.initializable_attributes(), + metric_tags=self._metric_tags, + metric_action='model.insert' + ) self.retrieved = True self.update_attributes(**response) self.after_save(self) def delete(self): url = self.url(action='delete', params=self.attributes) - response = perform_request('delete', url) + response = perform_request('delete', url, metric_tags=self._metric_tags, metric_action='model.delete') self.retrieved = True self.update_attributes(**response) diff --git a/lms/lib/comment_client/thread.py b/lms/lib/comment_client/thread.py index 768cc3aa1b..335f24c9c5 100644 --- a/lms/lib/comment_client/thread.py +++ b/lms/lib/comment_client/thread.py @@ -20,6 +20,11 @@ class Thread(models.Model): 'closed', 'user_id', 'commentable_id', 'group_id', 'group_name', 'pinned' ] + metric_tag_fields = [ + 'course_id', 'group_id', 'pinned', 'closed', 'anonymous', 'anonymous_to_peers', + 'endorsed', 'read' + ] + initializable_fields = updatable_fields base_url = "{prefix}/threads".format(prefix=settings.PREFIX) @@ -27,7 +32,7 @@ class Thread(models.Model): type = 'thread' @classmethod - def search(cls, query_params, *args, **kwargs): + def search(cls, query_params): default_params = {'page': 1, 'per_page': 20, @@ -41,7 +46,14 @@ class Thread(models.Model): url = cls.url(action='get_all', params=extract(params, 'commentable_id')) if params.get('commentable_id'): del params['commentable_id'] - response = perform_request('get', url, params, *args, **kwargs) + response = perform_request( + 'get', + url, + params, + metric_tags=[u'course_id:{}'.format(query_params['course_id'])], + metric_action='thread.search', + paged_results=True + ) return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1) @classmethod @@ -79,7 +91,13 @@ class Thread(models.Model): } request_params = strip_none(request_params) - response = perform_request('get', url, request_params) + response = perform_request( + 'get', + url, + request_params, + metric_action='model.retrieve', + metric_tags=self._metric_tags + ) self.update_attributes(**response) def flagAbuse(self, user, voteable): @@ -90,7 +108,13 @@ class Thread(models.Model): else: raise CommentClientRequestError("Can only flag/unflag threads or comments") params = {'user_id': user.id} - request = perform_request('put', url, params) + request = perform_request( + 'put', + url, + params, + metric_action='thread.abuse.flagged', + metric_tags=self._metric_tags + ) voteable.update_attributes(request) def unFlagAbuse(self, user, voteable, removeAll): @@ -105,19 +129,37 @@ class Thread(models.Model): if removeAll: params['all'] = True - request = perform_request('put', url, params) + request = perform_request( + 'put', + url, + params, + metric_tags=self._metric_tags, + metric_action='thread.abuse.unflagged' + ) voteable.update_attributes(request) def pin(self, user, thread_id): url = _url_for_pin_thread(thread_id) params = {'user_id': user.id} - request = perform_request('put', url, params) + request = perform_request( + 'put', + url, + params, + metric_tags=self._metric_tags, + metric_action='thread.pin' + ) self.update_attributes(request) def un_pin(self, user, thread_id): url = _url_for_un_pin_thread(thread_id) params = {'user_id': user.id} - request = perform_request('put', url, params) + request = perform_request( + 'put', + url, + params, + metric_tags=self._metric_tags, + metric_action='thread.unpin' + ) self.update_attributes(request) diff --git a/lms/lib/comment_client/user.py b/lms/lib/comment_client/user.py index 77e8b0d011..fdea00062f 100644 --- a/lms/lib/comment_client/user.py +++ b/lms/lib/comment_client/user.py @@ -16,6 +16,8 @@ class User(models.Model): updatable_fields = ['username', 'external_id', 'email', 'default_sort_key'] initializable_fields = updatable_fields + metric_tag_fields = ['course_id'] + base_url = "{prefix}/users".format(prefix=settings.PREFIX) default_retrieve_params = {'complete': True} type = 'user' @@ -29,11 +31,23 @@ class User(models.Model): def follow(self, source): params = {'source_type': source.type, 'source_id': source.id} - response = perform_request('post', _url_for_subscription(self.id), params) + response = perform_request( + 'post', + _url_for_subscription(self.id), + params, + metric_action='user.follow', + metric_tags=self._metric_tags + ['target.type:{}'.format(source.type)], + ) def unfollow(self, source): params = {'source_type': source.type, 'source_id': source.id} - response = perform_request('delete', _url_for_subscription(self.id), params) + response = perform_request( + 'delete', + _url_for_subscription(self.id), + params, + metric_action='user.unfollow', + metric_tags=self._metric_tags + ['target.type:{}'.format(source.type)], + ) def vote(self, voteable, value): if voteable.type == 'thread': @@ -43,7 +57,13 @@ class User(models.Model): else: raise CommentClientRequestError("Can only vote / unvote for threads or comments") params = {'user_id': self.id, 'value': value} - request = perform_request('put', url, params) + request = perform_request( + 'put', + url, + params, + metric_action='user.vote', + metric_tags=self._metric_tags + ['target.type:{}'.format(voteable.type)], + ) voteable.update_attributes(request) def unvote(self, voteable): @@ -54,7 +74,13 @@ class User(models.Model): else: raise CommentClientRequestError("Can only vote / unvote for threads or comments") params = {'user_id': self.id} - request = perform_request('delete', url, params) + request = perform_request( + 'delete', + url, + params, + metric_action='user.unvote', + metric_tags=self._metric_tags + ['target.type:{}'.format(voteable.type)], + ) voteable.update_attributes(request) def active_threads(self, query_params={}): @@ -63,7 +89,14 @@ class User(models.Model): url = _url_for_user_active_threads(self.id) params = {'course_id': self.course_id} params = merge_dict(params, query_params) - response = perform_request('get', url, params) + response = perform_request( + 'get', + url, + params, + metric_action='user.active_threads', + metric_tags=self._metric_tags, + paged_results=True, + ) return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1) def subscribed_threads(self, query_params={}): @@ -72,7 +105,14 @@ class User(models.Model): url = _url_for_user_subscribed_threads(self.id) params = {'course_id': self.course_id} params = merge_dict(params, query_params) - response = perform_request('get', url, params) + response = perform_request( + 'get', + url, + params, + metric_action='user.subscribed_threads', + metric_tags=self._metric_tags, + paged_results=True + ) return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1) def _retrieve(self, *args, **kwargs): @@ -81,13 +121,25 @@ class User(models.Model): if self.attributes.get('course_id'): retrieve_params['course_id'] = self.course_id try: - response = perform_request('get', url, retrieve_params) + response = perform_request( + 'get', + url, + retrieve_params, + metric_action='model.retrieve', + metric_tags=self._metric_tags, + ) except CommentClientRequestError as e: if e.status_code == 404: # attempt to gracefully recover from a previous failure # to sync this user to the comments service. self.save() - response = perform_request('get', url, retrieve_params) + response = perform_request( + 'get', + url, + retrieve_params, + metric_action='model.retrieve', + metric_tags=self._metric_tags, + ) else: raise self.update_attributes(**response) diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index 5e1eb88ebf..17fc198973 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -33,12 +33,13 @@ def merge_dict(dic1, dic2): @contextmanager -def request_timer(request_id, method, url): +def request_timer(request_id, method, url, tags=None): start = time() - yield + with dog_stats_api.timer('comment_client.request.time', tags=tags): + yield end = time() duration = end - start - dog_stats_api.histogram('comment_client.request.time', duration, end) + log.info( "comment_client_request_log: request_id={request_id}, method={method}, " "url={url}, duration={duration}".format( @@ -50,7 +51,16 @@ def request_timer(request_id, method, url): ) -def perform_request(method, url, data_or_params=None, *args, **kwargs): +def perform_request(method, url, data_or_params=None, raw=False, + metric_action=None, metric_tags=None, paged_results=False): + + if metric_tags is None: + metric_tags = [] + + metric_tags.append(u'method:{}'.format(method)) + if metric_action: + metric_tags.append(u'action:{}'.format(metric_action)) + if data_or_params is None: data_or_params = {} headers = { @@ -66,7 +76,7 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs): else: data = None params = merge_dict(data_or_params, request_id_dict) - with request_timer(request_id, method, url): + with request_timer(request_id, method, url, metric_tags): response = requests.request( method, url, @@ -76,6 +86,14 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs): timeout=5 ) + metric_tags.append(u'status_code:{}'.format(response.status_code)) + if response.status_code > 200: + metric_tags.append(u'result:failure') + else: + metric_tags.append(u'result:success') + + dog_stats_api.increment('comment_client.request.count', tags=metric_tags) + if 200 < response.status_code < 500: raise CommentClientRequestError(response.text, response.status_code) # Heroku returns a 503 when an application is in maintenance mode @@ -84,10 +102,27 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs): elif response.status_code == 500: raise CommentClient500Error(response.text) else: - if kwargs.get("raw", False): + if raw: return response.text else: - return json.loads(response.text) + data = response.json() + if paged_results: + dog_stats_api.histogram( + 'comment_client.request.paged.result_count', + value=len(data.get('collection', [])), + tags=metric_tags + ) + dog_stats_api.histogram( + 'comment_client.request.paged.page', + value=data.get('page', 1), + tags=metric_tags + ) + dog_stats_api.histogram( + 'comment_client.request.paged.num_pages', + value=data.get('num_pages', 1), + tags=metric_tags + ) + return data class CommentClientError(Exception):