search

hammerlab / cycledash

Variant Caller Analysis Dashboard and Data Management System
Other
35 stars 2 forks source link

Associate comments with users and use username for author name #831

Closed armish closed 9 years ago

armish commented 9 years ago

This PR deprecates #813 and adds support for proper associations between comments and their authors. With this change, we get rid of the Author Name field from the CommentBox and automatically assign user to a new comment: if the user logged in, then the comments belong to that user; if not, it is anonymous. If the user authentication is not enabled, all comments will be anonymous.

This PR requires a minor update to our DB schema and hence features an alembic migration script. During migration, all of our comments will lose their author_name information and therefore will become anonymous.

Here are some screenshots to get a feeling of how this looks now: screen shot 2015-09-03 at 1 38 19 pm

screen shot 2015-09-03 at 1 38 33 pm

screen shot 2015-09-03 at 1 38 45 pm

Review on Reviewable

ihodes commented 9 years ago

Awesome, thanks for doing this. I'm still looking through/commenting, but wanted to ask if the reason for the Anonymous user was for passing seltest tests (the only time I can think of that we have Anonymous users)?

armish commented 9 years ago

Thanks for the comments @ihodes -- keep them coming :)

About the Anonymous user and the need for seltests: yes and no -- seltests are the only place where we set LOGIN_DISABLED=true and it helped uncover a bug: current_user cannot be serialized into JSON when the user is anonymous (see examine.html). I added that class just to make sure that it is serializable and the whole implementation behaves normally when the user anonymous (hence the username and null id).

There might be other workarounds for this, but I thought that this might be good way for us to move forwards where we can customize the fields for Anonymous user and reduce the complexity of the code in other places.

ihodes commented 9 years ago

This looks great; a few mostly minor comments. This really exposed you to the whole Cycledash stack now :) Thanks for handling this!

armish commented 9 years ago

Thanks @ihodes, this was really fun to work on. The latest changes address all your comments/suggestions.

ihodes commented 9 years ago

Awesome, thanks--looks good to merge once you get it to go green!

Mind squashing those commits down a bit, first?