[Ready for Review] Feature: Replace v1 comment routes with v2 routes #4702

samanehsan · 2015-12-07T22:55:04Z

Purpose

Replace existing API v1 routes with API v2 routes for comments.

Changes

In addition to changing the routes...

CommentSerialzer:

Add can_edit, has_children and is_abuse fields to the comment serializer
Add max_length to comment content field
Do not allow whitespace comments

Comment model:

create method sends a signal to send the notification event, since that logic was happening in the v1 create view function which is no longer being used.

Side Effects

Instead of escaping html in comments and making it a string, it gets removed entirely by DRF. E.g. the comment "<em>hi</em>" would be "hi" instead.

Note: This pull request cannot be merged until the v2 API supports anonymous view only links

Add can_edit, has_children and is_abuse fields to the API v2 comment serializer.

Since the v2 comment serializer does not include serializied information about the commenter (and embedded requests have not yet been implemented), iterate through the serialized comments, make a request to get commenter info and then create a CommentModel based on the updated response.

Because the v1 route is being removed, move the logic for calling notify when a comment is added to a separate function that receives the 'comment-added' signal sent from Comment.create.

Handle case where auth is None or the user is an anonymous user.

Also, add a test to ensure that html tags are removed from the content.

Remove query string since it is already passed in as a query parameter.

Ensure that when a comment is edited, it cannot exceed the max length or be empty.

Ensure self.abuseText() defaults to an empty string. Also fix checking whether a user is anoymous in the is_abuse serializer method field.

Check if auth or if user is anonymous before comparing the commenter and user by id

Remove v1 routes for create, edit, delete and undelete for comments and tests.

When finding unread comments for a user on a node, look for comments created since comments_viewed_timestamp OR edited since then (instead of AND). Also exclude any deleted comments and add tests for the model method now that the view method tests have been removed. Fixes [#OSF-5193]

Every time comment replies are fetched instead of pushing the comment model to the array (that already contains the comment) re-assign the value of self.comments to the result of the fetch.

Change updateCommentsUserData to handle a single comment (from when a comment is added) or a list of comments (from fetching comment data).

into feature/v2-comments

Instead of making a separate request to get the data for every comment's user, embed that information in the request for the list of nodes.

into feature/v2-comments

Specify the comment target and type in the POST request

into feature/v2-comments

Instead of making a request to get the current user's information, get the info off of contextVars and pass it in to the model.

samanehsan · 2015-12-16T18:10:12Z

Done making those changes @sloria! 😎

sloria · 2015-12-17T14:54:11Z

api_tests/comments/views/test_comment_detail.py

It would also be useful to test content that only has whitespace characters in it.

Ah, but I see you've done that in TestNodeCommentsList#test_create_comment_trims_whitespace.

sloria · 2015-12-18T19:38:09Z

Pass finished.

Instead of checking that the value is not empty in the create and update methods, use a field validator. Note: the trim_whitespace parameter gets applied after validation, allowing whitespace comments so a custom field validator is needed.

Since empty comments are not allowed, add validation to the comment model as well. Add tests to check that a validation error is raised when trying to create an empty or whitespace comment. Also test that comments with content=None throw a validation error.

samanehsan · 2015-12-20T21:17:19Z

We should also have validation at the model layer. If content is not allowed to be blank, then a validator should be added to the content field.

I added an ODM validator for checking that a field is not empty: cos-archives/modular-odm#126

But then I found a string_required validator in validators.py: https://github.com/CenterForOpenScience/osf.io/blob/develop/framework/mongo/validators.py#L8

Should I use the string_required validator since it's already there?

samanehsan · 2015-12-21T14:20:54Z

Do you mean that these methods should check that the user is able to edit, delete, etc.?

For consistency with the reset of the codebase, comment.edit, comment.delete etc. should do permissions checks, and ValidationError should be handled here.

sloria · 2015-12-21T15:17:53Z

Should I use the string_required validator since it's already there?

Yes.

Do you mean that these methods should check that the user is able to edit, delete, etc.?

Yes, although I now see that my original suggestion may be incorrect; a PermissionsError--not a ValidationError--should be raised.

Also make the string_required validator trim whitespace and then check that the string is empty.

sloria · 2015-12-21T15:31:52Z

website/static/js/comment.js

Minor: It's safer to check self.id() ? 'comments' : 'nodes'; that will ensure you get the correct value if self.id() is either undefined or null.

These lines get changed when I add in file comments: https://github.com/samanehsan/osf.io/blob/feature/file-comments-v2-merge/website/static/js/comment.js#L231-L232

Handle PermissionsErrors in comment serializer methods.

samanehsan · 2015-12-21T17:34:24Z

Done making those changes @sloria! ⭐

If a user is viewing a project through a VOL, pass the private_key to the API request to get comments.

sloria · 2015-12-22T16:49:43Z

Closing in favor of #4804

samanehsan added 29 commits November 9, 2015 14:24

Add missing info to comment serializer

736a205

Add can_edit, has_children and is_abuse fields to the API v2 comment serializer.

Make creating comments/replies use v2 API

db0809c

Use api v2 route to edit comment

125db9f

Use v2 API to delete/undelete comments

bf351ad

Use v2 API to report/unreport comments

0bd2354

Add max_length to comment content field

5f868c8

Send signal to call notify when commenting

515c5b1

Because the v1 route is being removed, move the logic for calling notify when a comment is added to a separate function that receives the 'comment-added' signal sent from Comment.create.

Get unread comments using v2 API

477c1d2

Fix get_is_abuse

52a4129

Handle case where auth is None or the user is an anonymous user.

Do not allow whitespace comments

316e860

Also, add a test to ensure that html tags are removed from the content.

Remove query string in apiv2 node url

5ce7747

Remove query string since it is already passed in as a query parameter.

Add tests for updating comments

2e2739b

Ensure that when a comment is edited, it cannot exceed the max length or be empty.

Fix is_abuse and creating reports

2e23b2f

Ensure self.abuseText() defaults to an empty string. Also fix checking whether a user is anoymous in the is_abuse serializer method field.

Correct/update documentation

0b6d65e

Fix get_can_edit

b529574

Check if auth or if user is anonymous before comparing the commenter and user by id

Remove v1 comment list route

5f791e3

Remove remaining comment CRUD routes

b171e07

Remove v1 routes for create, edit, delete and undelete for comments and tests.

Remove comment (un)report abuse routes

04ea63c

Fix flake8 error

943e352

Fix failing test

40c9a27

Fix getting comment reply lists

348520d

Every time comment replies are fetched instead of pushing the comment model to the array (that already contains the comment) re-assign the value of self.comments to the result of the fetch.

Fix logic for creating comment

9d2d57c

Change updateCommentsUserData to handle a single comment (from when a comment is added) or a list of comments (from fetching comment data).

Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io

54cd0bc

into feature/v2-comments

Embed user data in api requests for comments

4270db0

Instead of making a separate request to get the data for every comment's user, embed that information in the request for the list of nodes.

Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io

4e1f075

into feature/v2-comments

Update creating comments

9daab5a

Specify the comment target and type in the POST request

Merge branch 'develop' of https://github.com/CenterForOpenScience/osf.io

c42e096

into feature/v2-comments

samanehsan changed the title ~~Feature/v2 comments~~ [WIP] Feature/v2 comments Dec 8, 2015

samanehsan added 2 commits December 15, 2015 16:31

Add help text to new comment serializer fields

7f9acec

Pass in user info to the comment model

e222205

Instead of making a request to get the current user's information, get the info off of contextVars and pass it in to the model.

sloria reviewed Dec 17, 2015
View reviewed changes

samanehsan added 4 commits December 18, 2015 16:50

Validate content max_length in Comment model

f54790d

Make content required in model & serializer

84d05a7

Use field validator for content

af8a143

Instead of checking that the value is not empty in the create and update methods, use a field validator. Note: the trim_whitespace parameter gets applied after validation, allowing whitespace comments so a custom field validator is needed.

Use string_required validator for content field

8db0c06

Also make the string_required validator trim whitespace and then check that the string is empty.

sloria reviewed Dec 21, 2015
View reviewed changes

samanehsan force-pushed the feature/v2-comments branch 3 times, most recently from 9a1ea99 to 8063686 Compare December 21, 2015 16:04

Add permissions checks to comment model methods

37d6cdf

Handle PermissionsErrors in comment serializer methods.

samanehsan force-pushed the feature/v2-comments branch from 8063686 to 37d6cdf Compare December 21, 2015 16:58

samanehsan added 2 commits December 22, 2015 10:26

Handle anonymous view only links for comments

a84919c

If a user is viewing a project through a VOL, pass the private_key to the API request to get comments.

Only get unread count if not priviate link

3009537

samanehsan force-pushed the feature/v2-comments branch from 822c038 to 3009537 Compare December 22, 2015 15:36

sloria closed this Dec 22, 2015

sloria mentioned this pull request Dec 22, 2015

Feature: Replace v1 comment routes with v2 routes #4804

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Ready for Review] Feature: Replace v1 comment routes with v2 routes #4702

[Ready for Review] Feature: Replace v1 comment routes with v2 routes #4702

Uh oh!

samanehsan commented Dec 7, 2015

Uh oh!

samanehsan commented Dec 16, 2015

Uh oh!

sloria Dec 17, 2015

Uh oh!

sloria Dec 17, 2015

Uh oh!

sloria commented Dec 18, 2015

Uh oh!

samanehsan commented Dec 20, 2015

Uh oh!

samanehsan commented Dec 21, 2015

Uh oh!

sloria commented Dec 21, 2015

Uh oh!

sloria Dec 21, 2015

Uh oh!

samanehsan Dec 21, 2015

Uh oh!

samanehsan commented Dec 21, 2015

Uh oh!

sloria commented Dec 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Ready for Review] Feature: Replace v1 comment routes with v2 routes #4702

[Ready for Review] Feature: Replace v1 comment routes with v2 routes #4702

Uh oh!

Conversation

samanehsan commented Dec 7, 2015

Purpose

Changes

Side Effects

Uh oh!

samanehsan commented Dec 16, 2015

Uh oh!

sloria Dec 17, 2015

Choose a reason for hiding this comment

Uh oh!

sloria Dec 17, 2015

Choose a reason for hiding this comment

Uh oh!

sloria commented Dec 18, 2015

Uh oh!

samanehsan commented Dec 20, 2015

Uh oh!

samanehsan commented Dec 21, 2015

Uh oh!

sloria commented Dec 21, 2015

Uh oh!

sloria Dec 21, 2015

Choose a reason for hiding this comment

Uh oh!

samanehsan Dec 21, 2015

Choose a reason for hiding this comment

Uh oh!

samanehsan commented Dec 21, 2015

Uh oh!

sloria commented Dec 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants