Skip to content
This repository was archived by the owner on Apr 14, 2018. It is now read-only.

Conversation

@marisamorris
Copy link

No description provided.

@marisamorris marisamorris changed the title media-ranker-oauth - Marisa Morris Marisa Morris - Carets Oct 30, 2017
@CheezItMan
Copy link

Hey Marisa,

I've reviewed your Media Ranker OAuth project and I have a bit of feedback.

The good:

  1. You've got the login functionality working
  2. Users can't vote for works more than once or modify works they didn't create

So you have the basics down.

Things to work on:

  1. Testing... It doesn't look like you did much here besides set up the test_helper.rb.
  2. The Login button on the homepage doesn't recognize if the user is logged in (left a comment in your code).
  3. Good trying to use response codes, but some of them end up with that you are being redirected message.

I'm guessing you ran out of some time here.

So not bad, but there was more to work though. In what little we have left of Rails and the classroom content, more practice with testing would be valuable.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Just a few notes

# We should always be able to tell what category
# of work we're dealing with
before_action :category_from_work, except: [:root, :index, :new, :create]
skip_before_action :find_user, only: [:root]

Choose a reason for hiding this comment

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

By avoiding this filter it shows a log in button on the homepage, even after I'm logged in. You can add a different bit of ocood to the root path to find the user. Or create 2 filters, one to find the user and another to redirect if it's not there.

<!-- --------------------------------- -->
<div class="float-right">
<!-- T0DO -->
<%= @login_user %>

Choose a reason for hiding this comment

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

This is something to remove.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants