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(a)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(a)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.