Skip to content
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

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

Merged
merged 6 commits into from
Sep 14, 2015

Conversation

armish
Copy link
Member

@armish armish commented Sep 3, 2015

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

@@ -0,0 +1,28 @@
"""Start associating comments with registered users and use user information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd frame these migration notes as 1) describing the change on the DB (e.g. "Add a user_id column to user_comments and drop author_name.") 2) then describe why (e.g. "This allows us to associate comments with users in the database.")

@ihodes
Copy link
Member

ihodes commented Sep 3, 2015

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
Copy link
Member Author

armish commented Sep 3, 2015

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.

userify_comment(comment)
return comments

def userify_comment(comment):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call this something like add_user_to_comment to make it a little clearer that's going on (I see the parallels with epochify_comment, but it's doing something a bit different).

"""
if 'userId' in comment:
with tables(db.engine, 'users') as (con, users):
q = select(users.c).where(users.c.id == comment['userId'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q = select([users.c.username, users.c.email, users.c.id])...; generally better to whitelist than blacklist (which also lets you get rid of "desentisize_user" (if keeping it, spelling -> desensitize).

@ihodes
Copy link
Member

ihodes commented Sep 3, 2015

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

@armish
Copy link
Member Author

armish commented Sep 3, 2015

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

@ihodes
Copy link
Member

ihodes commented Sep 4, 2015

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

Mind squashing those commits down a bit, first?

@armish armish force-pushed the comments-with-users branch 3 times, most recently from d6f94e4 to 6b2e0fd Compare September 8, 2015 20:22
armish added a commit that referenced this pull request Sep 14, 2015
Associate comments with users and use username for author name
@armish armish merged commit 40db63e into master Sep 14, 2015
@armish armish deleted the comments-with-users branch September 14, 2015 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants