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

Media Ranker Revisited - Angela - Octos #34

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,8 @@
!/log/.keep
!/tmp/.keep

#ignore env file
.env

# Ignore Byebug command history file.
.byebug_history
8 changes: 8 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ gem 'jbuilder', '~> 2.5'
# Use Capistrano for deployment
# gem 'capistrano-rails', group: :development


gem 'omniauth'
gem'omniauth-github'
# Use the Foundation CSS framework
gem 'foundation-rails'
gem 'autoprefixer-rails'
Expand All @@ -49,6 +52,9 @@ group :development, :test do

# Use pry for rails console
gem 'pry-rails'

gem 'binding_of_caller'

end

group :test do
Expand All @@ -65,6 +71,8 @@ group :development do
# Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring
gem 'spring'
gem 'spring-watcher-listen', '~> 2.0.0'

gem 'dotenv-rails'
end

# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
Expand Down
32 changes: 32 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ GEM
erubi (>= 1.0.0)
rack (>= 0.9.0)
bindex (0.5.0)
binding_of_caller (0.8.0)
debug_inspector (>= 0.0.1)
builder (3.2.3)
byebug (10.0.2)
coderay (1.1.2)
Expand All @@ -63,16 +65,24 @@ GEM
coffee-script-source (1.12.2)
concurrent-ruby (1.0.5)
crass (1.0.3)
debug_inspector (0.0.3)
dotenv (2.2.2)
dotenv-rails (2.2.2)
dotenv (= 2.2.2)
railties (>= 3.2, < 6.0)
erubi (1.7.1)
erubis (2.7.0)
execjs (2.7.0)
faraday (0.12.2)
multipart-post (>= 1.2, < 3)
ffi (1.9.23)
foundation-rails (6.4.3.0)
railties (>= 3.1.0)
sass (>= 3.3.0, < 3.5)
sprockets-es6 (>= 0.9.0)
globalid (0.4.1)
activesupport (>= 4.2.0)
hashie (3.5.7)
i18n (1.0.0)
concurrent-ruby (~> 1.0)
jbuilder (2.7.0)
Expand All @@ -82,6 +92,7 @@ GEM
rails-dom-testing (>= 1, < 3)
railties (>= 4.2.0)
thor (>= 0.14, < 2.0)
jwt (1.5.6)
listen (3.0.8)
rb-fsevent (~> 0.9, >= 0.9.4)
rb-inotify (~> 0.9, >= 0.9.7)
Expand All @@ -108,9 +119,26 @@ GEM
minitest (~> 5.0)
rails (>= 4.1)
multi_json (1.13.1)
multi_xml (0.6.0)
multipart-post (2.0.0)
nio4r (2.3.0)
nokogiri (1.8.2)
mini_portile2 (~> 2.3.0)
oauth2 (1.4.0)
faraday (>= 0.8, < 0.13)
jwt (~> 1.0)
multi_json (~> 1.3)
multi_xml (~> 0.5)
rack (>= 1.2, < 3)
omniauth (1.8.1)
hashie (>= 3.4.6, < 3.6.0)
rack (>= 1.6.2, < 3)
omniauth-github (1.3.0)
omniauth (~> 1.5)
omniauth-oauth2 (>= 1.4.0, < 2.0)
omniauth-oauth2 (1.5.0)
oauth2 (~> 1.1)
omniauth (~> 1.2)
pg (0.21.0)
pry (0.11.3)
coderay (~> 1.1.0)
Expand Down Expand Up @@ -197,8 +225,10 @@ PLATFORMS
DEPENDENCIES
autoprefixer-rails
better_errors
binding_of_caller
byebug
coffee-rails (~> 4.2)
dotenv-rails
foundation-rails
jbuilder (~> 2.5)
jquery-rails
Expand All @@ -207,6 +237,8 @@ DEPENDENCIES
minitest-reporters
minitest-skip
minitest-spec-rails
omniauth
omniauth-github
pg (~> 0.18)
pry-rails
puma (~> 3.0)
Expand Down
9 changes: 9 additions & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ def render_404
raise ActionController::RoutingError.new('Not Found')
end

def require_login
if @login_user.nil?
flash[:status] = :error
flash[:result_text] = "You must be logged in to view this section"
redirect_to root_path
end
end


private
def find_user
if session[:user_id]
Expand Down
42 changes: 42 additions & 0 deletions app/controllers/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,48 @@ class SessionsController < ApplicationController
def login_form
end

def create
auth_hash = request.env['omniauth.auth']

if auth_hash['uid']
@user = User.find_by(uid: auth_hash[:uid], provider: 'github')
if @user.nil?
# User doesn't match anything in the DB
# Attempt to create a new user
# @user = User.new(
# username: auth_hash['info']['name'],
# email: auth_hash['info']['email'],
# uid: auth_hash['uid'],
# provider: auth_hash['provider'])
user = User.build_from_github(auth_hash)
if @user.save
session[:user_id] = @user.id

Choose a reason for hiding this comment

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

Line 18 should be @user =.

flash[:success] = "Logged in successfully"
redirect_to root_path
else
flash[:error] = "Could not log in"
redirect_to root_path
end
else
session[:user_id] = @user.id
flash[:error] = "Could not log in"
redirect_to root_path
end
end
end

def index
@user = User.find(session[:user_id]) # < recalls the value set in a previous request
end


# def destroy
# session[:user_id] = nil
# flash[:success] = "Successfully logged out!"
#
# redirect_to root_path
# end

def login
username = params[:username]
if username and user = User.find_by(username: username)
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class UsersController < ApplicationController
before_action :require_login, except: [:root]

def index
@users = User.all
end
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/works_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ class WorksController < ApplicationController
# 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]
before_action :require_login, except: [:root]

def root
@albums = Work.best_albums
Expand All @@ -10,7 +11,7 @@ def root
@best_work = Work.order(vote_count: :desc).first
end

def index
def index #template method pattern
@works_by_category = Work.to_category_hash
end

Expand Down
4 changes: 4 additions & 0 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,8 @@ module ApplicationHelper
def render_date(date)
date.strftime("%b %e, %Y")
end

def readable_date(date)
("<span class='date'>" + date.strftime("%A, %b %d") + "</span>").html_safe
end
end
10 changes: 9 additions & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,13 @@ class User < ApplicationRecord
has_many :votes
has_many :ranked_works, through: :votes, source: :work

validates :username, uniqueness: true, presence: true
validates :name, uniqueness: true, presence: true

def build_from_github(auth_hash)
return User.new(
name: auth_hash['info']['name'],
email: auth_hash['info']['email'],

Choose a reason for hiding this comment

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

This should be a self method! In your controller you call User.build_from_github, which means logging in as a new user doesn't work.

I'm guessing you made the change after logging in for the first time, so you never hit it in a real workflow. That's why thorough testing is important, it's not always plausible to manually test things.

uid: auth_hash['uid'],
provider: auth_hash['provider'])
end
end
1 change: 1 addition & 0 deletions app/models/work.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,4 @@ def fix_category
end
end
end

4 changes: 2 additions & 2 deletions app/views/layouts/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@
</div>
<div class="user-nav">
<% if @login_user %>
<%= link_to "Logged in as #{@login_user.username}", user_path(@login_user), class: "button" %>
<%= link_to "Logged in as #{@login_user.name}", user_path(@login_user), class: "button" %>
<%= link_to "Log Out", logout_path, method: :post, class: "button" %>
<% else %>
<%= link_to "Log In", login_path, class: "button" %>
<%= link_to "Log In", "/auth/github", class: "button" %>
<% end %>
</div>
</nav>
Expand Down
4 changes: 2 additions & 2 deletions app/views/users/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
<table>
<thead>
<tr>
<th>Username</th>
<th>Name</th>
<th width="100">Votes</th>
<th width="200">Joined</th>
</tr>
</thead>
<tbody>
<% @users.each do |user| %>
<tr>
<td><%= link_to user.username, user_path(user) %></td>
<td><%= link_to user.name, user_path(user) %></td>
<td><%= user.votes.count %></td>
<td><%= render_date user.created_at %></td>
</tr>
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/show.html.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<h3>User Summary: <%= link_to @user.username, user_path(@user) %></h3>
<h3>User Summary: <%= link_to @user.name, user_path(@user) %></h3>
<p>Joined site <%= render_date @user.created_at %></p>

<h4>Votes</h4>
Expand Down
2 changes: 1 addition & 1 deletion app/views/works/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<tbody>
<% @votes.each do |vote| %>
<tr>
<td><%= link_to vote.user.username, user_path(vote.user) %></td>
<td><%= link_to vote.user.name, user_path(vote.user) %></td>
<td><%= render_date vote.created_at %></td>
</li>
<% end %>
Expand Down
3 changes: 3 additions & 0 deletions config/initializers/omniauth.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Rails.application.config.middleware.use OmniAuth::Builder do
provider :github, ENV["GITHUB_CLIENT_ID"], ENV["GITHUB_CLIENT_SECRET"], scope: "user:email"
end
3 changes: 3 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,7 @@
post '/works/:id/upvote', to: 'works#upvote', as: 'upvote'

resources :users, only: [:index, :show]

get "/auth/:provider/callback", to: "sessions#create", as: 'auth_callback'
# delete "/logout", to: "sessions#destroy", as: "logout"
end
7 changes: 7 additions & 0 deletions db/migrate/20180417183001_add_uid.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class AddUid < ActiveRecord::Migration[5.0]
def change
add_column :users, :email, :string
add_column :users, :uid, :integer, null: false # this is the identifier provided by GitHub
add_column :users, :provider, :string, null: false # this tells us who provided the identifier
end
end
5 changes: 5 additions & 0 deletions db/migrate/20180417220630_replace_username_with_name.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class ReplaceUsernameWithName < ActiveRecord::Migration[5.0]
def change
rename_column :users, :username, :name
end
end
7 changes: 5 additions & 2 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,18 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20170407164321) do
ActiveRecord::Schema.define(version: 20180417220630) do

# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"

create_table "users", force: :cascade do |t|
t.string "username"
t.string "name"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.string "email"
t.integer "uid", null: false
t.string "provider", null: false
end

create_table "votes", force: :cascade do |t|
Expand Down
51 changes: 51 additions & 0 deletions test/controllers/sessions_controller_test.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,56 @@
require "test_helper"

describe SessionsController do
describe 'auth_callback' do
# it 'creates a DB entry for a new user' do
# old_user_count = User.count
#
# user = User.new(

Choose a reason for hiding this comment

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

Here is the test that would have caught those two bugs

# provider: 'github',
# uid: 80085,
# email: '[email protected]',
# name: 'test userr'
# )
# user.must_be :valid?
#
# login(user)
#
# User.count.must_equal old_user_count + 1
# session[:user_id].must_equal User.last.id
# end

it 'logs in an existing user' do
#arrange
user = User.first
old_user_count = User.count

login(user)
User.count.must_equal old_user_count
session[:user_id].must_equal user.id
end

it 'does not log in with insufficient data' do
user = User.new(
provider: 'github',
email: '[email protected]',
)
user.wont_be :valid?
old_user_count = User.count

login(user)
User.count.must_equal old_user_count
end

it 'does not log in if the user data is invalid' do
user = User.first
dup_user = User.new(
provider: 'github',
uid: 80085,
email: '[email protected]',
name: user.name
)
end

#what about if the user is already logged in?
end
end
Loading