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

Implement #clicks_count to count a link's clicks #9

Open
wants to merge 9 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
2 changes: 2 additions & 0 deletions app/assets/javascripts/sessions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Place all the behaviors and hooks related to the matching controller here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't commit empty files.

// All this logic will automatically be available in application.js.
2 changes: 2 additions & 0 deletions app/assets/javascripts/users.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Place all the behaviors and hooks related to the matching controller here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't commit empty files.

// All this logic will automatically be available in application.js.
4 changes: 4 additions & 0 deletions app/assets/stylesheets/sessions.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't commit empty files.

Place all the styles related to the matching controller here.
They will automatically be included in application.css.
*/
4 changes: 4 additions & 0 deletions app/assets/stylesheets/users.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/*
Place all the styles related to the matching controller here.
They will automatically be included in application.css.
*/
30 changes: 30 additions & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,34 @@ class ApplicationController < ActionController::Base
# Prevent CSRF attacks by raising an exception.
# For APIs, you may want to use :null_session instead.
protect_from_forgery with: :exception

private

def current_user
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for extra indentation here. Rubocop should tell you.

@current_user ||= User.find(session[:user_id]) if session[:user_id]
end

def user_signed_in?
!current_user.nil?
end

def user_signed_out?
current_user.nil?
end

def sign_in!(user)
session[:user_id] = user.id
@current_user = user
end

def sign_out!
@current_user = nil
session.delete(:user_id)
end

def authorize
redirect_to login_url, alert: 'Please log in to access that.' if user_signed_out?
end

helper_method :current_user, :user_signed_in?, :user_signed_out?, :sign_in!, :sign_out!, :authorize
end
27 changes: 21 additions & 6 deletions app/controllers/links_controller.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
class LinksController < ApplicationController
before_filter :authorize, only: [:destroy]

# GET /links
def index
@links = Link.order('created_at DESC')
if current_user
Copy link
Contributor

Choose a reason for hiding this comment

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

Use user_signed_in? method if that's what you mean

@links = current_user.links
else
@links = Link.where(user_id: nil).order('created_at DESC')
end
end

# GET /l/:short_name
Expand All @@ -10,9 +16,10 @@ def show
@link = Link.find_by_short_name(params[:short_name])

if @link
@link.clicked!
redirect_to @link.url
else
render text: "No such link.", status: 404
render text: 'No such link.', status: 404
end
end

Expand All @@ -24,6 +31,7 @@ def new
# POST /links
def create
@link = Link.new(link_params)
@link.user_id = current_user.id if user_signed_in?

if @link.save
redirect_to root_url, notice: 'Link was successfully created.'
Expand All @@ -32,9 +40,16 @@ def create
end
end

private
# Only allow a trusted parameter "white list" through.
def link_params
params.require(:link).permit(:url)
def destroy
@link = Link.find_by_short_name(params[:short_name])
@link.destroy

redirect_to action: :index, notice: 'Link deleted!'
end

private
# Only allow a trusted parameter "white list" through.
def link_params
params.require(:link).permit(:url, :shortname)
end
end
38 changes: 38 additions & 0 deletions app/controllers/sessions_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
class SessionsController < ApplicationController
def show
end

def new
@user = User.new
end

def edit
end

def create
@user = User.find_by_email(params[:email])

if @user && @user.authenticate(params[:password])
sign_in!(@user)
redirect_to root_url, notice: 'Logged in!'
else
### IS THERE A BETTER WAY TO SHOW A LOGIN ERROR?
@user = User.new
@user.errors.add(:base, 'That email and password were not valid! Please try again.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Encapsulate this logic in the mode. The controller should never, ever be reaching into the guts of a model like this.

Copy link
Author

Choose a reason for hiding this comment

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

@jfarmer Hey, just to confirm, do you mean that a controller should not be adding errors to a model like @user.errors.add? That instead the User model should have a method for adding an error? e.g. @user.add_error(:field, message)?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the controller shouldn't be thinking in terms of "add error to model" at all.

Jesse Farmer (via iPhone)

http://codeunion.io · @jfarmer

[email protected] · 773.319.9355

On Thu, Apr 16, 2015 at 8:28 PM, Mark G. [email protected] wrote:

  • def edit
  • end
  • def create
  • @user = User.find_by_email(params[:email])
  • if @user && @user.authenticate(params[:password])
  •  sign_in!(@user)
    
  •  redirect_to root_url, notice: 'Logged in!'
    
  • else
  •  ### IS THERE A BETTER WAY TO SHOW A LOGIN ERROR?
    
  •  @user = User.new
    
  •  @user.errors.add(:base, 'That email and password were not valid! Please try again.')
    

    @jfarmer Hey, just to confirm, do you mean that a controller should not be adding errors to a model like @user.errors.add? That instead the User model should have a method for adding an error? e.g. @user.add_error(:field, message)?

    Reply to this email directly or view it on GitHub:
    https://github.com/codeunion/url-shortener/pull/9/files#r28567872

render :new
end
end

def update
end

def destroy
session[:user_id] = nil
redirect_to root_url, notice: 'Logged out!'
end

private
def session_params
# params.require(:session).permit(???)
Copy link
Contributor

Choose a reason for hiding this comment

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

don't commit commented out code

end
end
36 changes: 36 additions & 0 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
class UsersController < ApplicationController
def index
end

def show
end

def new
@user = User.new
end

def edit
end

def create
@user = User.new(user_params)

if @user.save
sign_in!(@user)
redirect_to root_url, notice: 'Welcome to url-shortener!'
else
render :new
end
end

def update
end

def destroy
end

private
def user_params
params.require(:user).permit(:email, :password, :password_confirmation)
end
end
2 changes: 2 additions & 0 deletions app/helpers/sessions_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module SessionsHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't commit empty files.

end
2 changes: 2 additions & 0 deletions app/helpers/users_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module UsersHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't commit empty files.

end
49 changes: 35 additions & 14 deletions app/models/link.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
class Link < ActiveRecord::Base
before_create :set_short_name
before_validation :set_short_name, :validate_url_and_prepend_scheme_if_none!

validates :url, :presence => true

validates :clicks_count,
:numericality => {
:only_integer => true,
:greater_than_or_equal_to => 0
},
:presence => true

belongs_to :user

def clicked!
self.clicks_count += 1
self.save
end

# This controls how an ActiveRecord object is displayed in a URL context.
# This way, if we do link_path(@link), Rails will generate a path like
# "/l/#{@link.short_name}" vs. "/l/#{@link.id}".
Expand All @@ -12,19 +26,26 @@ def to_param
end

private
def set_short_name
# Generate and assign a random short_name unless one has already been set.
return self.short_name if self.short_name.present?

# See: http://www.ruby-doc.org/stdlib-2.1.2/libdoc/securerandom/rdoc/SecureRandom.html#method-c-urlsafe_base64
# We do this to ensure we're not creating two links with the same short_name
# Since it's randomly generated and not user-supplied, we can't rely on
# validations to do this for us.
try_short_name = SecureRandom.urlsafe_base64(6)
while Link.where(:short_name => try_short_name).any?
try_short_name = SecureRandom.urlsafe_base64(6)
def validate_url_and_prepend_scheme_if_none!
uri = URI.parse(url)
url.prepend("http://") unless uri.kind_of?(URI::HTTP) || uri.kind_of?(URI::HTTPS)
rescue URI::BadURIError, URI::InvalidURIError
self.errors.add(:url, 'is not a valid URL')
end

self.short_name = try_short_name
end
def set_short_name
# Generate and assign a random short_name unless one has already been set.
return self.short_name if self.short_name.present?

# See: http://www.ruby-doc.org/stdlib-2.1.2/libdoc/securerandom/rdoc/SecureRandom.html#method-c-urlsafe_base64
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comments

# We do this to ensure we're not creating two links with the same short_name
# Since it's randomly generated and not user-supplied, we can't rely on
# validations to do this for us.
try_short_name = SecureRandom.urlsafe_base64(6)
while Link.where(:short_name => try_short_name).any?
try_short_name = SecureRandom.urlsafe_base64(6)
end

self.short_name = try_short_name
end
end
6 changes: 6 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class User < ActiveRecord::Base
has_secure_password
validates :email, :presence => true, :uniqueness => true

has_many :links
end
1 change: 1 addition & 0 deletions app/views/layouts/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<title>UrlShortener</title>
<%= stylesheet_link_tag 'application', media: 'all' %>
<%= csrf_meta_tags %>
<%= javascript_include_tag "application" %>
</head>
<body>

Expand Down
4 changes: 2 additions & 2 deletions app/views/links/_form.html.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<%= form_for(@link) do |f| %>
<%= form_for @link do |f| %>
<% if @link.errors.any? %>
<div id="error_explanation">
<h2><%= pluralize(@link.errors.count, "error") %> prohibited this link from being saved:</h2>
<h2><%= pluralize(@link.errors.count, "error") %> found:</h2>

<ul>
<% @link.errors.full_messages.each do |message| %>
Expand Down
17 changes: 15 additions & 2 deletions app/views/links/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,22 @@
</div>
<% end %>

<p/><% if user_signed_in? %>
Copy link
Contributor

Choose a reason for hiding this comment

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

<p/> isn't a valid HTML tag

Logged in as <%= current_user.email %>.
<%= link_to 'Log Out', logout_path, :method => :delete %>
<% else %>
<%= link_to 'Log In', login_path %> or
<%= link_to 'Sign Up', new_user_path %>
<% end %>
<p/>
Copy link
Contributor

Choose a reason for hiding this comment

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

<p/> isn't a valid HTML tag


<table>
<thead>
<tr>
<th>Added</th>
<th>Short URL</th>
<th>URL</th>
<th colspan="3"></th>
<th>Clicks</th>
</tr>
</thead>

Expand All @@ -22,7 +31,11 @@
<td><%= time_ago_in_words(link.created_at) %> ago</td>
<td><%= link_url(link) %></td>
<td><%= link.url %></td>
<td><%= link_to 'Visit link', link %></td>
<td><%= link.clicks_count %></td>
<td><%= link_to 'Visit Link', link, :target => '_blank' %></td>
<% if user_signed_out? || (user_signed_in? && link.user == current_user) %>
<td><%= link_to 'Delete Link', link, :method => :delete %></td>
<% end %>
</tr>
<% end %>
</tbody>
Expand Down
2 changes: 1 addition & 1 deletion app/views/links/new.html.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<h1>New link</h1>
<h1>New Link</h1>

<%= render 'form' %>

Expand Down
26 changes: 26 additions & 0 deletions app/views/sessions/_form.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra empty line

<%= form_tag login_path do %>
<% if @user.errors.any? %>
<div id="error_explanation">
<h2><%= pluralize(@user.errors.count, "error") %> found:</h2>

<ul>
<% @user.errors.full_messages.each do |message| %>
<li><%= message %></li>
<% end %>
</ul>
</div>
<% end %>

<div class="field">
<%= label_tag :email %><br>
<%= text_field_tag :email, params[:email] %>
</div>
<div class="field">
<%= label_tag :password %><br>
<%= password_field_tag :password %>
</div>
<div class="actions">
<%= submit_tag "Log In" %>
</div>
<% end %>
3 changes: 3 additions & 0 deletions app/views/sessions/new.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<h1>Log In</h1>

<%= render 'form' %>
29 changes: 29 additions & 0 deletions app/views/users/_form.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<%= form_for @user do |f| %>
<% if @user.errors.any? %>
<div id="error_explanation">
<h2><%= pluralize(@user.errors.count, "error") %> found:</h2>

<ul>
<% @user.errors.full_messages.each do |message| %>
<li><%= message %></li>
<% end %>
</ul>
</div>
<% end %>

<div class="field">
<%= f.label :email %><br>
<%= f.text_field :email %>
</div>
<div class="field">
<%= f.label :password %><br>
<%= f.password_field :password %>
</div>
<div class="field">
<%= f.label :password_confirmation %><br>
<%= f.password_field :password_confirmation %>
</div>
<div class="actions">
<%= f.submit %>
</div>
<% end %>
6 changes: 6 additions & 0 deletions app/views/users/new.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra empty line

<h1>Sign Up</h1>

<%= render 'form' %>

<%= link_to 'Back', links_path %>
Loading