Skip to content

Instantly share code, notes, and snippets.

Show Gist options
  • Save philsof/c06d603030b72a15ba49 to your computer and use it in GitHub Desktop.
Save philsof/c06d603030b72a15ba49 to your computer and use it in GitHub Desktop.
Code review for branch pair-edwardgemson,kerryimai challenge build-a-rails-blog 2_10_16

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment