-
Notifications
You must be signed in to change notification settings - Fork 351
[Ready for Review] Feature: Replace v1 comment routes with v2 routes #4702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 37 commits
736a205
5e0f5b1
db0809c
125db9f
bf351ad
0bd2354
5f868c8
515c5b1
477c1d2
52a4129
316e860
5ce7747
2e2739b
2e23b2f
0b6d65e
b529574
5f791e3
b171e07
04ea63c
943e352
40c9a27
59240d0
348520d
9d2d57c
54cd0bc
4270db0
4e1f075
9daab5a
c42e096
44c3424
75a1773
0044469
da0f8e3
d6a0423
f3ca37e
7f9acec
e222205
f54790d
84d05a7
af8a143
3a34bb9
8db0c06
37d6cdf
a84919c
3009537
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| from rest_framework.exceptions import ValidationError, PermissionDenied | ||
| from api.base.exceptions import InvalidModelValueError, Conflict | ||
| from api.base.utils import absolute_reverse | ||
| from api.base.settings import osf_settings | ||
| from api.base.serializers import (JSONAPISerializer, | ||
| TargetField, | ||
| RelationshipField, | ||
|
|
@@ -29,7 +30,7 @@ class CommentSerializer(JSONAPISerializer): | |
|
|
||
| id = IDField(source='_id', read_only=True) | ||
| type = TypeField() | ||
| content = AuthorizedCharField(source='get_content') | ||
| content = AuthorizedCharField(source='get_content', max_length=osf_settings.COMMENT_MAXLENGTH) | ||
|
|
||
| target = TargetField(link_type='related', meta={'type': 'get_target_type'}) | ||
| user = RelationshipField(related_view='users:user-detail', related_view_kwargs={'user_id': '<user._id>'}) | ||
|
|
@@ -41,19 +42,41 @@ class CommentSerializer(JSONAPISerializer): | |
| date_modified = ser.DateTimeField(read_only=True) | ||
| modified = ser.BooleanField(read_only=True, default=False) | ||
| deleted = ser.BooleanField(read_only=True, source='is_deleted', default=False) | ||
| is_abuse = ser.SerializerMethodField(help_text='Whether the current user reported this comment.') | ||
| has_children = ser.SerializerMethodField(help_text='Whether this comment has any replies.') | ||
| can_edit = ser.SerializerMethodField(help_text='Whether the current user can edit this comment.') | ||
|
|
||
| # LinksField.to_representation adds link to "self" | ||
| links = LinksField({}) | ||
|
|
||
| class Meta: | ||
| type_ = 'comments' | ||
|
|
||
| def get_is_abuse(self, obj): | ||
| user = self.context['request'].user | ||
| if user.is_anonymous(): | ||
| return False | ||
| return user._id in obj.reports | ||
|
|
||
| def get_can_edit(self, obj): | ||
| user = self.context['request'].user | ||
| if user.is_anonymous(): | ||
| return False | ||
| return obj.user._id == user._id | ||
|
|
||
| def get_has_children(self, obj): | ||
| return bool(getattr(obj, 'commented', [])) | ||
|
|
||
| def update(self, comment, validated_data): | ||
| assert isinstance(comment, Comment), 'comment must be a Comment' | ||
| auth = Auth(self.context['request'].user) | ||
| if validated_data: | ||
| if 'get_content' in validated_data: | ||
| comment.edit(validated_data['get_content'], auth=auth, save=True) | ||
| content = validated_data.pop('get_content') | ||
| content = content.strip() | ||
| if not content: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you set
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also have validation at the model layer. If
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember setting |
||
| raise ValidationError('Comment cannot be empty.') | ||
| comment.edit(content, auth=auth, save=True) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency with the reset of the codebase, |
||
| if validated_data.get('is_deleted', None) is True: | ||
| comment.delete(auth, save=True) | ||
| elif comment.is_deleted: | ||
|
|
@@ -111,7 +134,10 @@ def create(self, validated_data): | |
| detail='Invalid comment target \'{}\'.'.format(target_id) | ||
| ) | ||
| validated_data['target'] = target | ||
| validated_data['content'] = validated_data.pop('get_content') | ||
| content = validated_data.pop('get_content') | ||
| validated_data['content'] = content.strip() | ||
| if not validated_data['content']: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| raise ValidationError('Comment cannot be empty.') | ||
| if node and node.can_comment(auth): | ||
| comment = Comment.create(auth=auth, **validated_data) | ||
| else: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| from nose.tools import * # flake8: noqa | ||
|
|
||
| from api.base.settings.defaults import API_BASE | ||
| from api.base.settings import osf_settings | ||
| from tests.base import ApiTestCase | ||
| from tests.factories import ProjectFactory, AuthUserFactory, CommentFactory, RegistrationFactory | ||
|
|
||
|
|
@@ -193,6 +194,39 @@ def test_public_node_non_contributor_commenter_can_update_comment(self): | |
| assert_equal(res.status_code, 200) | ||
| assert_equal(payload['data']['attributes']['content'], res.json['data']['attributes']['content']) | ||
|
|
||
| def test_update_comment_cannot_exceed_max_length(self): | ||
| self._set_up_private_project_with_comment() | ||
| payload = { | ||
| 'data': { | ||
| 'id': self.comment._id, | ||
| 'type': 'comments', | ||
| 'attributes': { | ||
| 'content': ''.join(['c' for c in range(osf_settings.COMMENT_MAXLENGTH + 1)]), | ||
| 'deleted': False | ||
| } | ||
| } | ||
| } | ||
| res = self.app.put_json_api(self.private_url, payload, auth=self.user.auth, expect_errors=True) | ||
| assert_equal(res.status_code, 400) | ||
| assert_equal(res.json['errors'][0]['detail'], | ||
| 'Ensure this field has no more than {} characters.'.format(str(osf_settings.COMMENT_MAXLENGTH))) | ||
|
|
||
| def test_update_comment_cannot_be_empty(self): | ||
| self._set_up_private_project_with_comment() | ||
| payload = { | ||
| 'data': { | ||
| 'id': self.comment._id, | ||
| 'type': 'comments', | ||
| 'attributes': { | ||
| 'content': '', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would also be useful to test content that only has whitespace characters in it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, but I see you've done that in |
||
| 'deleted': False | ||
| } | ||
| } | ||
| } | ||
| res = self.app.put_json_api(self.private_url, payload, auth=self.user.auth, expect_errors=True) | ||
| assert_equal(res.status_code, 400) | ||
| assert_equal(res.json['errors'][0]['detail'], 'This field may not be blank.') | ||
|
|
||
| def test_private_node_only_logged_in_contributor_commenter_can_delete_comment(self): | ||
| self._set_up_private_project_with_comment() | ||
| comment = CommentFactory(node=self.private_project, target=self.private_project, user=self.user) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ | |
| from website import filters, language, settings, mailchimp_utils | ||
| from website.exceptions import NodeStateError | ||
| from website.profile.utils import serialize_user | ||
| from website.project.signals import contributor_added | ||
| from website.project.signals import contributor_added, comment_added | ||
| from website.project.model import ( | ||
| Comment, Node, NodeLog, Pointer, ensure_schemas, has_anonymous_link, | ||
| get_pointer_parent, Embargo, MetaSchema, DraftRegistration | ||
|
|
@@ -4126,6 +4126,17 @@ def test_create(self): | |
| assert_equal(len(comment.node.logs), 2) | ||
| assert_equal(comment.node.logs[-1].action, NodeLog.COMMENT_ADDED) | ||
|
|
||
| def test_create_sends_comment_added_signal(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
| with capture_signals() as mock_signals: | ||
| comment = Comment.create( | ||
| auth=self.auth, | ||
| user=self.comment.user, | ||
| node=self.comment.node, | ||
| target=self.comment.target, | ||
| is_public=True, | ||
| ) | ||
| assert_equal(mock_signals.signals_sent(), set([comment_added])) | ||
|
|
||
| def test_edit(self): | ||
| self.comment.edit( | ||
| auth=self.auth, | ||
|
|
@@ -4231,6 +4242,61 @@ def test_get_content_private_project_throws_permissions_error_for_logged_out_use | |
| with assert_raises(PermissionsError): | ||
| comment.get_content(auth=None) | ||
|
|
||
| def test_find_unread_is_zero_when_no_comments(self): | ||
| n_unread = Comment.find_unread(user=UserFactory(), node=ProjectFactory()) | ||
| assert_equal(n_unread, 0) | ||
|
|
||
| def test_find_unread_new_comments(self): | ||
| project = ProjectFactory() | ||
| user = UserFactory() | ||
| project.add_contributor(user) | ||
| project.save() | ||
| comment = CommentFactory(node=project, user=project.creator) | ||
| n_unread = Comment.find_unread(user=user, node=project) | ||
| assert_equal(n_unread, 1) | ||
|
|
||
| def test_find_unread_includes_comment_replies(self): | ||
| project = ProjectFactory() | ||
| user = UserFactory() | ||
| project.add_contributor(user) | ||
| project.save() | ||
| comment = CommentFactory(node=project, user=user) | ||
| reply = CommentFactory(node=project, target=comment, user=project.creator) | ||
| n_unread = Comment.find_unread(user=user, node=project) | ||
| assert_equal(n_unread, 1) | ||
|
|
||
| # Regression test for https://openscience.atlassian.net/browse/OSF-5193 | ||
| def test_find_unread_includes_edited_comments(self): | ||
| project = ProjectFactory() | ||
| user = AuthUserFactory() | ||
| project.add_contributor(user) | ||
| project.save() | ||
| comment = CommentFactory(node=project, user=project.creator) | ||
|
|
||
| url = project.api_url_for('update_comments_timestamp') | ||
| res = self.app.put_json(url, auth=user.auth) | ||
| user.reload() | ||
| n_unread = Comment.find_unread(user=user, node=project) | ||
| assert_equal(n_unread, 0) | ||
|
|
||
| # Edit previously read comment | ||
| comment.edit( | ||
| auth=Auth(project.creator), | ||
| content='edited', | ||
| save=True | ||
| ) | ||
| n_unread = Comment.find_unread(user=user, node=project) | ||
| assert_equal(n_unread, 1) | ||
|
|
||
| def test_find_unread_does_not_include_deleted_comments(self): | ||
| project = ProjectFactory() | ||
| user = AuthUserFactory() | ||
| project.add_contributor(user) | ||
| project.save() | ||
| comment = CommentFactory(node=project, user=project.creator, is_deleted=True) | ||
| n_unread = Comment.find_unread(user=user, node=project) | ||
| assert_equal(n_unread, 0) | ||
|
|
||
|
|
||
| class TestPrivateLink(OsfTestCase): | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested in my other comments, this validation should also happen at the model layer.