Sorry for the delay, had to read up on authlogic to understand what you were doing before I could really review. Mostly good, but NACK because of a few issues (with fixes) and suggestions inline. Snipped parts with no comments. One here that doesn't fit anywhere else though. Since we have no rpm for the gem currently, this will kill rpm builds, so I think we should include an update to the README in this patch explaining that.
On Thu, Sep 24, 2009 at 1:56 PM, Scott Seago sseago@redhat.com wrote:
At the moment you've got to install the authlogic gem -- we will eventually need to package it as an RPM and add it as a dependency in the specfile.
Right now any user can create an account and access all pages. We aren't doing any access control beyond requiring a login account.
Signed-off-by: Scott Seago sseago@redhat.com diff --git a/src/app/views/layouts/_header.rhtml b/src/app/views/layouts/_header.rhtml index 76f9e73..f85510f 100644 --- a/src/app/views/layouts/_header.rhtml +++ b/src/app/views/layouts/_header.rhtml @@ -1,10 +1,10 @@ <div class="header_logo"><%= image_tag "dcloud.png" %></div>
<div class="header_info">
- <div id="hi-username">Hi, <%= @user %></div>
- <div id="hi-username"><%= "Hi, " + @current_user.login if defined? @current_user %></div>
I ran into a problem with the above line (clicking register would blow up), because @current_user is sometimes defined when you would think it should not be, so the template tries to render nil.login. Changing this check to @current_user.login unless @current_user.nil? fixed this issue for me. Also, jp may have another idea of where to put this, but for now, it would be nice to be able to get to the user page (and subsequently the edit page). I was thinking an easy way for now would be to have username be a link to account/
<form method="POST" id="search-form" action="<%= url_for :controller => "search", :action => 'results' %>"> <input id="textfield_effect" name="terms" value="Search" onkeypress="" onfocus="if( this.value == this.defaultValue ) this.value='';" type="text"> <input id="searchbox-button" src="<%= image_path "icon_search.png"%>" title="Search" type="image"> | </form>
- <%= link_to 'Log out', { :controller => "login", :action => "logout"}%>
- <%= link_to 'Log out', { :controller => "user_sessions", :action => "logout"}%>
The above link should toggle between login and logout, as appropriate.
</div> \ No newline at end of file diff --git a/src/app/views/layouts/dcloud.rhtml b/src/app/views/layouts/dcloud.rhtml index 725f630..53dfc67 100644 --- a/src/app/views/layouts/dcloud.rhtml +++ b/src/app/views/layouts/dcloud.rhtml @@ -69,7 +69,7 @@ </div>
<div id="side">
- <%= render :partial => '/layouts/main_nav' %>
- <%= render :partial => '/layouts/main_nav' if defined? @current_user %>
Same change as above, for check to work properly (can see this by the 'add provider' link disappearing), this check should be 'unless @current_user.nil? '
</div>
<div id="tabs-and-content-container"> diff --git a/src/app/views/user_sessions/login.html.erb b/src/app/views/user_sessions/login.html.erb new file mode 100644 index 0000000..1322fa2 --- /dev/null +++ b/src/app/views/user_sessions/login.html.erb @@ -0,0 +1,19 @@ +<div class="dcloud_form">
- <%= error_messages_for 'user_session' %>
This probably isn't strictly in the view template, but in the controller, but testing this is what made me notice- if you have errors, you are not rerouted properly to see them. Instead you get a server error, with the following log:
ActionView::MissingTemplate (Missing template user_sessions/new.erb in view path app/views): /usr/lib/ruby/gems/1.8/gems/locale_rails-2.0.4/lib/locale_rails/action_view.rb:46:in `find_template' app/controllers/user_sessions_controller.rb:15:in `create'
Line 15 of that file is: 'render :action => :new'. It looks like this should go to UserController::new instead, since there is none in UserSessions.
- <h2>Login</h2>
- <% form_for @user_session, :url => user_session_path do |f| %>
- <%= f.error_messages %>
- <%= f.label :login %><br />
- <%= f.text_field :login %><br />
- <br />
- <%= f.label :password %><br />
- <%= f.password_field :password %><br />
- <br />
- <%= f.check_box :remember_me %><%= f.label :remember_me %><br />
- <br />
- <%= f.submit "Login" %>
- <% end %>
- <%= link_to 'Register', {:controller => 'users', :action => 'new'}, :class => 'actionlink' %>
+</div> diff --git a/src/app/views/users/edit.html.erb b/src/app/views/users/edit.html.erb new file mode 100644 index 0000000..7c9db32 --- /dev/null +++ b/src/app/views/users/edit.html.erb @@ -0,0 +1,12 @@ +<div class="dcloud_form">
- <%= error_messages_for 'user' %>
Either this (^^) or the one below needs to go, otherwise summary message is listed twice.
- <h2>Edit My Profile</h2>
- <% form_for @user, :url => account_path do |f| %>
- <%= f.error_messages %>
- <%= render :partial => "form", :object => f %>
- <%= f.submit "Update" %>
- <% end %>
- <br /><%= link_to "My Profile", account_path %>
+</div> diff --git a/src/app/views/users/new.html.erb b/src/app/views/users/new.html.erb new file mode 100644 index 0000000..681d9b0 --- /dev/null +++ b/src/app/views/users/new.html.erb @@ -0,0 +1,10 @@ +<div class="dcloud_form">
- <%= error_messages_for 'user' %>
Both the error dup issue and probably the same failure as noted in login view comments.
- <h2>Register</h2>
- <% form_for @user, :url => account_path do |f| %>
- <%= f.error_messages %>
- <%= render :partial => "form", :object => f %>
- <%= f.submit "Register" %>
- <% end %>
+</div> diff --git a/src/app/views/users/show.html.erb b/src/app/views/users/show.html.erb new file mode 100644 index 0000000..e4e6e67 --- /dev/null +++ b/src/app/views/users/show.html.erb @@ -0,0 +1,37 @@
This is just an opinion, but from a UX standpoint, I think this would look nicer if it were wrapped in the same dcloud_form block as the actual forms, with a heading something like 'My Profile' (at least as first pass). Also, I think it would be more consistent to give the link the 'actionlink' class, as in the forms and nav (we might think about flipping this usage, as 'actionlink currently seems more common than not).
+<p>
- <b>Login:</b>
- <%=h @user.login %>
+</p>
+<p>
- <b>Login count:</b>
- <%=h @user.login_count %>
+</p>
+<p>
- <b>Last request at:</b>
- <%=h @user.last_request_at %>
+</p>
+<p>
- <b>Last login at:</b>
- <%=h @user.last_login_at %>
+</p>
+<p>
- <b>Current login at:</b>
- <%=h @user.current_login_at %>
+</p>
+<p>
- <b>Last login ip:</b>
- <%=h @user.last_login_ip %>
+</p>
+<p>
- <b>Current login ip:</b>
- <%=h @user.current_login_ip %>
+</p>
+<%= link_to 'Edit', edit_account_path %> diff --git a/src/config/routes.rb b/src/config/routes.rb index ae9bc75..d2a171b 100644 --- a/src/config/routes.rb +++ b/src/config/routes.rb @@ -33,8 +33,12 @@ ActionController::Routing::Routes.draw do |map| # -- just remember to delete public/index.html. map.default '', :controller => 'provider'
- map.login '/login', :controller => 'resources', :action => 'list'
- map.logout '/logout', :controller => 'resources', :action => 'list'
- map.resource :user_session
- map.root :controller => "user_sessions", :action => "login"
- map.login 'login', :controller => "user_sessions", :action => "login"
why not map logout the same way, so it doesn't have 'user_sessions' in the url?
- map.resource :account, :controller => "users"
- map.resources :users
# Allow downloading Web Service WSDL as a file with an extension # instead of a file named 'wsdl' diff --git a/src/db/migrate/20090917192602_create_users.rb b/src/db/migrate/20090917192602_create_users.rb new file mode 100644 index 0000000..1bcf0d4 --- /dev/null +++ b/src/db/migrate/20090917192602_create_users.rb @@ -0,0 +1,30 @@ +class CreateUsers < ActiveRecord::Migration
- def self.up
- create_table :users do |t|
- t.string :login, :null => false
- t.string :email, :null => false
- t.string :crypted_password, :null => false
- t.string :password_salt, :null => false
Are salted passwords strong enough encryption? Or is this just temporary? While reading about authlogic, I saw there are several other mechanisms that work with it available, but this could certainly be in a subsequent patch
- t.string :persistence_token, :null => false
- t.string :single_access_token, :null => false
- t.string :perishable_token, :null => false
- # Magic columns, just like ActiveRecord's created_at and updated_at.
- # These are automatically maintained by Authlogic if they are present.
- t.integer :login_count, :null => false, :default => 0
- t.integer :failed_login_count, :null => false, :default => 0
- t.datetime :last_request_at
- t.datetime :current_login_at
- t.datetime :last_login_at
- t.string :current_login_ip
- t.string :last_login_ip
- t.timestamps
- end
- add_index :users, :login
- add_index :users, :persistence_token
- add_index :users, :last_request_at
- end
- def self.down
- drop_table :users
- end
+end diff --git a/src/test/fixtures/users.yml b/src/test/fixtures/users.yml new file mode 100644 index 0000000..5bf0293 --- /dev/null +++ b/src/test/fixtures/users.yml @@ -0,0 +1,7 @@
No tests/fixtures! :( <soapbox> /me thinks it would be good to start building these from the start on this project while things are simple, my errors in testing were a good example of things we could catch with tests </soapbox>
+# Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
+# one: +# column: value +# +# two: +# column: value diff --git a/src/test/functional/user_sessions_controller_test.rb b/src/test/functional/user_sessions_controller_test.rb new file mode 100644 index 0000000..5024b23 --- /dev/null +++ b/src/test/functional/user_sessions_controller_test.rb @@ -0,0 +1,8 @@ +require 'test_helper'
+class UserSessionsControllerTest < ActionController::TestCase
- # Replace this with your real tests.
- test "the truth" do
- assert true
- end
+end
<snip/>
Hope that didn't come off too harsh, didn't expect to have so much to say when I started this review. Even with all that though, this is already far superior, imho, to the path we took with ovirt, would love to see us retrofit this in there at some point.