Code review for branch pair-edwardgemson,kerryimai challenge build-a-rails-blog 2_10_16

code-review-pair-edwardgemson,kerryimai-build-a-rails-blog.md

Good work, here are my notes on your code:

• Watch out: your post controller is allowing user_id to be passed as a param. This means a client could easily spoof themselves as another user and create posts as someone else (by passing someone else's user_id in the params, perhaps via a command-line http tool like curl). You are going to want to remove user_id from your post_params method and instead hard code the adding of user_id to the post via session.user_id. You always want to get your user id from the session.user_id, since session cannot be spoofed.
• Instead on using find_by when searching by a unique id number (like you do here) it would be better to utilize find since it is designed specifically to search by a unique id number. Here are the docs on find for your reference.
• Watch out: Your update and destroy methods in your post controller do not check if the current user is the owner of the post that is being deleted/updated. You should add logic that confirms the post is owned by session.user_id and only delete/update if it is.
• It looks like you do not have any validations on your User model, which means someone could create a user with the same username as an existing user. This is bad news since your login looks for the first matching username. Thus, the first person that creates username X will have no issue logging in, but the second person with username X will never be able to log in. Uh oh!
• It looks like each user has_many sites, and each site has_many users, but there is no many-to-many join table between them. Do these assocations work properly?
• You don't want any logic happening in your views. But what constitutes logic? Here is a general rule: you should not chain two methods on each other in your views; that would constitute logic, and should happen only in your models or controllers (preferably in your models). For example, here you are pulling a username from a post by chaining @post.creator.username. It would be better to create a creator_username method on your Post model, so that all you would need to do in your view is @post.creator_username. This way, the logic happens in the model, not in the view.

Good work, you are getting this! Keep at it! Any questions let me know.

Phil