Make the UsersController more RESTful by moving actions that act on the Users resource from LoginController to UsersController.

git-svn-id: http://www.rousette.org.uk/svn/tracks-repos/trunk@410 a4c988fc-2ded-0310-b66e-134b36920a42

tracks/app/controllers/application.rb

@@ -71,6 +71,10 @@ class ApplicationController < ActionController::Base
     end
   end
   
+  def redirect_back_or_home
+    redirect_back_or_default home_url
+  end
+  
   private
         
   def parse_date_per_user_prefs( s )

tracks/app/controllers/login_controller.rb

@@ -18,7 +18,7 @@ class LoginController < ApplicationController
           msg = (should_expire_sessions?) ? "will expire after 1 hour of inactivity." : "will not expire." 
           notify :notice, "Login successful: session #{msg}"
           cookies[:tracks_login] = { :value => @user.login, :expires => Time.now + 1.year }
-          redirect_back_or_default home_url
+          redirect_back_or_home
         else
           @login = params['user_login']
           notify :warning, "Login unsuccessful"
@@ -69,7 +69,7 @@ class LoginController < ApplicationController
         unless (@user.nil?)
           notify :notice, "You have successfully verified #{open_id_response.identity_url} as your identity."
           session['user_id'] = @user.id
-          redirect_back_or_default home_path
+          redirect_back_or_home
         else
           notify :warning, "You have successfully verified #{open_id_response.identity_url} as your identity, but you do not have a Tracks account. Please ask your administrator to sign you up."
         end
@@ -83,47 +83,6 @@ class LoginController < ApplicationController
     redirect_to :action => 'login' unless performed?
   end
 
-  def signup
-    if User.no_users_yet?
-      @page_title = "Sign up as the admin user"
-      @user = get_new_user
-    elsif @user && @user.is_admin?
-      @page_title = "Sign up a new user"
-      @user = get_new_user
-    else # all other situations (i.e. a non-admin is logged in, or no one is logged in, but we have some users)
-      @page_title = "No signups"
-      @admin_email = User.find_admin.preference.admin_email
-      render :action => "nosignup"
-    end        
-  end
-
-  def create
-    user = User.new(params['user'])
-    unless user.valid?
-      session['new_user'] = user
-      redirect_to :controller => 'login', :action => 'signup'
-      return
-    end
-
-    user.is_admin = true if User.no_users_yet?
-    if user.save
-      @user = User.authenticate(user.login, params['user']['password'])
-      @user.create_preference
-      @user.save
-      notify :notice, "Signup successful for user #{@user.login}."
-      redirect_back_or_default home_url
-    end
-  end
-
-  def delete
-    if params['id'] and ( params['id'] == @user.id or @user.is_admin )
-      @user = User.find(params['id'])
-      # TODO: Maybe it would be better to mark deleted. That way user deletes can be reversed.
-      @user.destroy
-    end
-    redirect_back_or_default home_url
-  end
-
   def logout
     session['user_id'] = nil
     reset_session
@@ -150,17 +109,7 @@ class LoginController < ApplicationController
   end
   
   private
-  
-  def get_new_user
-    if session['new_user']
-      user = session['new_user']
-      session['new_user'] = nil
-    else
-      user = User.new
-    end
-    user
-  end
-    
+      
   def should_expire_sessions?
     session['noexpiry'] != "on"
   end

tracks/app/controllers/users_controller.rb

@@ -5,20 +5,33 @@ class UsersController < ApplicationController
      before_filter  :begin_open_id_auth,    :only => :update_auth_type
   end
 
-  before_filter :admin_login_required, :only => [ :index, :create, :destroy ]
+  before_filter :admin_login_required, :only => [ :index, :destroy ]
   
   def index
+    @page_title = "TRACKS::Manage Users"
     @user_pages, @users = paginate :users, :order => 'login ASC', :per_page => 10
     @total_users = User.find(:all).size
-    # When we call login/signup from the admin page
+    # When we call users/signup from the admin page
     # we store the URL so that we get returned here when signup is successful
+    expires_now
     store_location
   end
-  
-  # verify  :method => :post,
-  #         :only => %w( create ),
-  #         :render => { :text => '403 Forbidden: Only POST requests on this resource are allowed.',
-  #                     :status => 403 }
+
+  def new
+    if User.no_users_yet?
+      @page_title = "Sign up as the admin user"
+      @user = get_new_user
+    elsif @user && @user.is_admin?
+      @page_title = "Sign up a new user"
+      @user = get_new_user
+    else # all other situations (i.e. a non-admin is logged in, or no one is logged in, but we have some users)
+      @page_title = "No signups"
+      @admin_email = User.find_admin.preference.admin_email
+      render :action => "nosignup", :layout => "login"
+      return
+    end
+    render :layout => "login"
+  end
   
   # Example usage: curl -H 'Accept: application/xml' -H 'Content-Type: application/xml'
   #               -u admin:up2n0g00d
@@ -26,28 +39,56 @@ class UsersController < ApplicationController
   #               http://our.tracks.host/users
   #
   def create
-     if params['exception']
-       render_failure "Expected post format is valid xml like so: <request><login>username</login><password>abc123</password></request>."
-       return
-     end
+    if params['exception']
+      render_failure "Expected post format is valid xml like so: <request><login>username</login><password>abc123</password></request>."
+      return
+    end
+    respond_to do |format|
+      format.html do
+        unless User.no_users_yet? || (@user && @user.is_admin?)
+          @page_title = "No signups"
+          @admin_email = User.find_admin.preference.admin_email
+          render :action => "nosignup", :layout => "login"
+          return
+        end
+        
+        user = User.new(params['user'])
+        unless user.valid?
+          session['new_user'] = user
+          redirect_to :action => 'new'
+          return
+        end
 
-     admin = User.find_admin
-      unless session["user_id"].to_i == admin.id.to_i
-        access_denied
+        user.is_admin = true if User.no_users_yet?
+        if user.save
+          @user = User.authenticate(user.login, params['user']['password'])
+          @user.create_preference
+          @user.save
+          notify :notice, "Signup successful for user #{@user.login}."
+          redirect_back_or_home
+        end
         return
       end
-      unless check_create_user_params
-        render_failure "Expected post format is valid xml like so: <request><login>username</login><password>abc123</password></request>."
+      format.xml do
+        unless User.find_by_id_and_is_admin(session['user_id'], true)
+          render :text => "401 Unauthorized: Only admin users are allowed access to this function.", :status => 401
+          return
+        end
+        unless check_create_user_params
+          render_failure "Expected post format is valid xml like so: <request><login>username</login><password>abc123</password></request>."
+          return
+        end
+        user = User.new(params[:request])
+        user.password_confirmation = params[:request][:password]
+        if user.save
+          render :text => "User created.", :status => 200
+        else
+          render_failure user.errors.to_xml
+        end
         return
       end
-      user = User.new(params[:request])
-      user.password_confirmation = params[:request][:password]
-      if user.save
-        render :text => "User created.", :status => 200
-      else
-        render_failure user.errors.to_xml
-      end
-  end
+    end
+  end  
   
   def destroy
     @deleted_user = User.find_by_id(params[:id])
@@ -176,6 +217,16 @@ class UsersController < ApplicationController
 
   private
     
+  def get_new_user
+    if session['new_user']
+      user = session['new_user']
+      session['new_user'] = nil
+    else
+      user = User.new
+    end
+    user
+  end
+    
   def check_create_user_params
     return false unless params.has_key?(:request)
     return false unless params[:request].has_key?(:login)

tracks/app/helpers/users_helper.rb

@@ -1,2 +1,2 @@
-module UserHelper
+module UsersHelper
 end

tracks/app/views/login/welcome.rhtml

@@ -1,13 +0,0 @@
-
-<div class="memo">
-    <h3>Welcome</h3>
-
-    <p>You are now logged into the system...</p>
-    <p>
-    Since you are here it's safe to assume the application never called store_location, otherwise
-    you would have been redirected somewhere else after a successful login.
-    </p>
-      
-    <%= link_to "&#171; logout", :action=>"logout"%>  
-      
-</div>

tracks/app/views/users/index.rhtml

@@ -33,4 +33,4 @@
     <%= link_to "Next page &raquo;", { :page => @user_pages.current.next } if @user_pages.current.next %>
   </p>
  
-  <p><%= link_to 'Signup new user', :controller => 'login', :action => 'signup' %></p>
+  <p><%= link_to 'Signup new user', signup_path %></p>

tracks/config/routes.rb

@@ -21,12 +21,14 @@ ActionController::Routing::Routes.draw do |map|
   # Login Routes
   map.connect 'login', :controller => 'login', :action => 'login' 
   map.connect 'logout', :controller => 'login', :action => 'logout'
-  map.connect 'signup', :controller => 'login', :action => 'signup'
 
   map.resources :users,
                 :member => {:change_password => :get, :update_password => :post,
                              :change_auth_type => :get, :update_auth_type => :post,
                              :refresh_token => :post }
+ map.with_options :controller => "users" do |users|
+   users.signup 'signup', :action => "new"
+ end
 
   # ToDo Routes
   map.resources :todos,
@@ -37,7 +39,7 @@ ActionController::Routing::Routes.draw do |map|
     todos.tickler 'tickler', :action => "list_deferred"
     todos.done 'done', :action => "completed"
     todos.done_archive 'done/archive', :action => "completed_archive"
-    todos.tag '/todos/tag/:name', :action => "tag"
+    todos.tag 'todos/tag/:name', :action => "tag"
   end
 
   # Context Routes

tracks/test/functional/login_controller_test.rb

@@ -75,79 +75,15 @@ class LoginControllerTest < Test::Unit::TestCase
     assert_equal "Login unsuccessful", flash[:warning]
     assert_response :success
   end
-    
-  # ============================================
-  # Signup and creation of new users
-  # ============================================
-  
-  # Test signup of a new user by admin
-  # Check that newly created user can log in
-  #
-  def test_create
-    admin = login('admin', 'abracadabra', 'on')
-    assert admin.is_admin
-    newbie = create('newbie', 'newbiepass')
-    assert_equal "Signup successful for user newbie.", flash[:notice]
-    assert_redirected_to home_url
-    assert_valid newbie
-    get :logout # logout the admin user
-    assert_equal newbie.login, "newbie"
-    assert newbie.is_admin == false || newbie.is_admin == 0
-    assert_not_nil newbie.preference # have user preferences been created?
-    user = login('newbie', 'newbiepass', 'on') # log in the new user
-    assert_redirected_to home_url
-    assert_equal 'newbie', user.login
-    assert user.is_admin == false || user.is_admin == 0
-    assert_equal User.count, @num_users_in_fixture + 1
-  end
-  
-  # Test whether signup of new users is denied to a non-admin user
-  # 
-  def test_create_by_non_admin
-    non_admin = login('jane', 'sesame', 'on')
-    assert non_admin.is_admin == false || non_admin.is_admin == 0
-    post :signup, :user => {:login => 'newbie2', :password => 'newbiepass2', :password_confirmation => 'newbiepass2'}
-    assert_template 'login/nosignup'
-    assert_number_of_users_is_unchanged
-  end
-  
-  # ============================================
-  # Test validations
-  # ============================================
-  
-  def test_create_with_invalid_password
-    admin = login('admin', 'abracadabra', 'on')
-    assert admin.is_admin
-    assert_equal admin.id, @response.session['user_id']
-    post :create, :user => {:login => 'newbie', :password => '', :password_confirmation => ''}
-    assert_number_of_users_is_unchanged
-    assert_redirected_to :controller => 'login', :action => 'signup'    
-  end
-  
-  def test_create_with_invalid_user
-    admin = login('admin', 'abracadabra', 'on')
-    assert admin.is_admin
-    assert_equal admin.id, @response.session['user_id']
-    post :create, :user => {:login => 'n', :password => 'newbiepass', :password_confirmation => 'newbiepass'}
-    assert_number_of_users_is_unchanged
-    assert_redirected_to :controller => 'login', :action => 'signup'    
-  end
-  
-  # Test uniqueness of login
-  #
-  def test_validate_uniqueness_of_login
-    admin = login('admin', 'abracadabra', 'on')
-    assert admin.is_admin
-    assert_equal admin.id, @response.session['user_id']
-    post :create, :user => {:login => 'jane', :password => 'newbiepass', :password_confirmation => 'newbiepass'}
-    num_users = User.find(:all)
-    assert_number_of_users_is_unchanged
-    assert_redirected_to :controller => 'login', :action => 'signup'
-  end
   
   private
   
-  def assert_number_of_users_is_unchanged
-    assert_equal User.count, @num_users_in_fixture    
+  # Logs in a user and returns the user object found in the session object
+  def login(login,password,expiry)
+    post :login, {:user_login => login, :user_password => password, :user_noexpiry => expiry}
+    assert_not_nil(session['user_id'])
+    return User.find(session['user_id'])
   end
+  
+    
 end

tracks/test/functional/users_controller_test.rb

@@ -6,7 +6,7 @@ require 'user'
 class UsersController; def rescue_action(e) raise e end; end
 
 class UsersControllerTest < Test::Unit::TestCase
-  fixtures :users
+  fixtures :preferences, :users
   
   def setup
     assert_equal "test", ENV['RAILS_ENV']
@@ -14,6 +14,9 @@ class UsersControllerTest < Test::Unit::TestCase
     @controller = UsersController.new
     @request    = ActionController::TestRequest.new
     @response   = ActionController::TestResponse.new
+    @num_users_in_fixture = User.count
+    @admin_user = users(:admin_user)
+    @nonadmin_user = users(:other_user)
   end
   
   def test_get_index_when_not_logged_in
@@ -22,20 +25,20 @@ class UsersControllerTest < Test::Unit::TestCase
   end
   
   def test_get_index_by_nonadmin
-    @request.session['user_id'] = users(:other_user).id
+    login_as @nonadmin_user
     get :index
-    assert_redirected_to home_path
+    assert_response 401
   end
   
   def test_get_index_by_admin
-    @request.session['user_id'] = users(:admin_user).id
+    login_as @admin_user
     get :index
     assert_response :success
   end
   
   def test_destroy_user
+    login_as @admin_user
     @no_users_before = User.find(:all).size
-    @request.session['user_id'] = users(:admin_user).id
     xhr :post, :destroy, :id => 3
     assert_rjs :page, "user-3", :remove
     assert_equal @no_users_before-1, User.find(:all).size
@@ -44,7 +47,7 @@ class UsersControllerTest < Test::Unit::TestCase
   def test_update_password_successful
     get :change_password # should fail because no login
     assert_redirected_to :controller => 'login', :action => 'login'
-    @request.session['user_id'] = users(:admin_user).id # log in the admin user
+    login_as @admin_user
     @user = @request.session['user_id']
     get :change_password # should now pass because we're logged in
     assert_response :success
@@ -59,7 +62,7 @@ class UsersControllerTest < Test::Unit::TestCase
   def test_update_password_no_confirmation
     post :update_password # should fail because no login
     assert_redirected_to :controller => 'login', :action => 'login'
-    @request.session['user_id'] = users(:admin_user).id # log in the admin user
+    login_as @admin_user
     post :update_password, :updateuser => {:password => 'newpassword', :password_confirmation => 'wrong'}
     assert_redirected_to :controller => 'users', :action => 'change_password'
     assert users(:admin_user).save, false
@@ -69,7 +72,7 @@ class UsersControllerTest < Test::Unit::TestCase
   def test_update_password_validation_errors
     post :update_password # should fail because no login
     assert_redirected_to :controller => 'login', :action => 'login'
-    @request.session['user_id'] = users(:admin_user).id # log in the admin user
+    login_as @admin_user
     post :update_password, :updateuser => {:password => 'ba', :password_confirmation => 'ba'}
     assert_redirected_to :controller => 'users', :action => 'change_password'
     assert users(:admin_user).save, false
@@ -79,4 +82,79 @@ class UsersControllerTest < Test::Unit::TestCase
     assert_equal flash[:warning], 'There was a problem saving the password. Please retry.'
   end
   
+  # ============================================
+  # Signup and creation of new users
+  # ============================================
+  
+  def test_create
+    login_as @admin_user
+    newbie = create('newbie', 'newbiepass')
+    assert_equal "Signup successful for user newbie.", flash[:notice], "expected flash notice not found"
+    assert_redirected_to home_url
+    assert_valid newbie
+    session['user_id'] = nil # logout the admin user
+    assert_equal newbie.login, "newbie"
+    assert newbie.is_admin == false || newbie.is_admin == 0
+    assert_not_nil newbie.preference # have user preferences been created?
+    assert_not_nil User.authenticate('newbie', 'newbiepass')
+    assert_equal User.count, @num_users_in_fixture + 1
+  end
+  
+  # Test whether signup of new users is denied to a non-admin user
+  # 
+  def test_create_by_non_admin
+    non_admin = login_as @nonadmin_user
+    post :create, :user => {:login => 'newbie2', :password => 'newbiepass2', :password_confirmation => 'newbiepass2'}
+    assert_response :success
+    assert_template 'users/nosignup'
+    assert_number_of_users_is_unchanged
+  end
+  
+  # ============================================
+  # Test validations
+  # ============================================
+  
+  def test_create_with_invalid_password
+    login_as @admin_user
+    post :create, :user => {:login => 'newbie', :password => '', :password_confirmation => ''}
+    assert_number_of_users_is_unchanged
+    assert_redirected_to :controller => 'users', :action => 'new'    
+  end
+  
+  def test_create_with_invalid_user
+    login_as @admin_user
+    post :create, :user => {:login => 'n', :password => 'newbiepass', :password_confirmation => 'newbiepass'}
+    assert_number_of_users_is_unchanged
+    assert_redirected_to :controller => 'users', :action => 'new'    
+  end
+  
+  # Test uniqueness of login
+  #
+  def test_validate_uniqueness_of_login
+    login_as @admin_user
+    post :create, :user => {:login => 'jane', :password => 'newbiepass', :password_confirmation => 'newbiepass'}
+    num_users = User.find(:all)
+    assert_number_of_users_is_unchanged
+    assert_redirected_to :controller => 'users', :action => 'new'    
+  end
+  
+  private
+  
+  def login_as(user)
+    returning user do |u|
+      @request.session['user_id'] = u.id
+    end
+  end
+  
+  # Creates a new users with the login and password given
+  def create(login,password)
+    post :create, :user => {:login => login, :password => password, :password_confirmation => password}
+    return User.find_by_login(login)
+  end
+  
+  def assert_number_of_users_is_unchanged
+    assert_equal User.count, @num_users_in_fixture    
+  end
+  
+  
 end

tracks/test/integration/stories_test.rb

@@ -49,17 +49,17 @@ class StoriesTest < ActionController::IntegrationTest
       def goes_to_signup
         get "/signup"
         assert_response :success
-        assert_template "login/signup"
+        assert_template "users/new"
       end
       
       def goes_to_signup_as_nonadmin
         get "/signup"
         assert_response :success
-        assert_template "login/nosignup"
+        assert_template "users/nosignup"
       end
 
       def signs_up_with(options)
-        post "/login/create", options
+        post "/users", options
         assert_response :redirect
         follow_redirect!
         assert_response :success

tracks/test/test_helper.rb

@@ -16,22 +16,6 @@ class Test::Unit::TestCase
   
   # Instantiated fixtures are slow, but give you @david where you otherwise would need people(:david)
   self.use_instantiated_fixtures  = false
-
-  # Add more helper methods to be used by all tests here...
-  # Logs in a user and returns the user object found in the session object
-  #
-  def login(login,password,expiry)
-    post :login, {:user_login => login, :user_password => password, :user_noexpiry => expiry}
-    assert_not_nil(session['user_id'])
-    return User.find(session['user_id'])
-  end
-  
-  # Creates a new users with the login and password given
-  def create(login,password)
-    post :create, :user => {:login => login, :password => password, :password_confirmation => password}
-    return User.find_by_login(login)
-  end
-  
     
   # Generates a random string of ascii characters (a-z, "1 0")
   # of a given length for testing assignment to fields