From ddc6d57c1751e2ba35f149d3c6a87ce39b6b67d0 Mon Sep 17 00:00:00 2001 From: lukemelia Date: Sat, 27 Jan 2007 08:28:52 +0000 Subject: [PATCH] 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 | 4 + tracks/app/controllers/login_controller.rb | 57 +---------- tracks/app/controllers/users_controller.rb | 99 ++++++++++++++----- tracks/app/helpers/users_helper.rb | 2 +- tracks/app/views/login/welcome.rhtml | 13 --- tracks/app/views/users/index.rhtml | 2 +- .../{login/signup.rhtml => users/new.rhtml} | 0 .../app/views/{login => users}/nosignup.rhtml | 0 tracks/config/routes.rb | 6 +- .../test/functional/login_controller_test.rb | 78 ++------------- .../test/functional/users_controller_test.rb | 94 ++++++++++++++++-- tracks/test/integration/stories_test.rb | 6 +- tracks/test/test_helper.rb | 16 --- 13 files changed, 184 insertions(+), 193 deletions(-) delete mode 100644 tracks/app/views/login/welcome.rhtml rename tracks/app/views/{login/signup.rhtml => users/new.rhtml} (100%) rename tracks/app/views/{login => users}/nosignup.rhtml (100%) diff --git a/tracks/app/controllers/application.rb b/tracks/app/controllers/application.rb index db8bcee4..6477d384 100644 --- a/tracks/app/controllers/application.rb +++ b/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 ) diff --git a/tracks/app/controllers/login_controller.rb b/tracks/app/controllers/login_controller.rb index 2aa68589..3675cb02 100644 --- a/tracks/app/controllers/login_controller.rb +++ b/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 diff --git a/tracks/app/controllers/users_controller.rb b/tracks/app/controllers/users_controller.rb index 64ba72d8..82f9693f 100644 --- a/tracks/app/controllers/users_controller.rb +++ b/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: usernameabc123." - return - end + if params['exception'] + render_failure "Expected post format is valid xml like so: usernameabc123." + 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: usernameabc123." + 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: usernameabc123." + 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) diff --git a/tracks/app/helpers/users_helper.rb b/tracks/app/helpers/users_helper.rb index 0147c3fe..2310a240 100644 --- a/tracks/app/helpers/users_helper.rb +++ b/tracks/app/helpers/users_helper.rb @@ -1,2 +1,2 @@ -module UserHelper +module UsersHelper end diff --git a/tracks/app/views/login/welcome.rhtml b/tracks/app/views/login/welcome.rhtml deleted file mode 100644 index ff9cc713..00000000 --- a/tracks/app/views/login/welcome.rhtml +++ /dev/null @@ -1,13 +0,0 @@ - -
-

Welcome

- -

You are now logged into the system...

-

- 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. -

- - <%= link_to "« logout", :action=>"logout"%> - -
diff --git a/tracks/app/views/users/index.rhtml b/tracks/app/views/users/index.rhtml index af797613..b7b7b688 100644 --- a/tracks/app/views/users/index.rhtml +++ b/tracks/app/views/users/index.rhtml @@ -33,4 +33,4 @@ <%= link_to "Next page »", { :page => @user_pages.current.next } if @user_pages.current.next %>

-

<%= link_to 'Signup new user', :controller => 'login', :action => 'signup' %>

\ No newline at end of file +

<%= link_to 'Signup new user', signup_path %>

\ No newline at end of file diff --git a/tracks/app/views/login/signup.rhtml b/tracks/app/views/users/new.rhtml similarity index 100% rename from tracks/app/views/login/signup.rhtml rename to tracks/app/views/users/new.rhtml diff --git a/tracks/app/views/login/nosignup.rhtml b/tracks/app/views/users/nosignup.rhtml similarity index 100% rename from tracks/app/views/login/nosignup.rhtml rename to tracks/app/views/users/nosignup.rhtml diff --git a/tracks/config/routes.rb b/tracks/config/routes.rb index 62608d87..8a11edb1 100644 --- a/tracks/config/routes.rb +++ b/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 diff --git a/tracks/test/functional/login_controller_test.rb b/tracks/test/functional/login_controller_test.rb index dba99187..4171fc84 100644 --- a/tracks/test/functional/login_controller_test.rb +++ b/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 diff --git a/tracks/test/functional/users_controller_test.rb b/tracks/test/functional/users_controller_test.rb index 6a040f93..286747f9 100644 --- a/tracks/test/functional/users_controller_test.rb +++ b/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 diff --git a/tracks/test/integration/stories_test.rb b/tracks/test/integration/stories_test.rb index 80b918b4..4cdb1482 100644 --- a/tracks/test/integration/stories_test.rb +++ b/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 diff --git a/tracks/test/test_helper.rb b/tracks/test/test_helper.rb index 801d59df..f2d21701 100644 --- a/tracks/test/test_helper.rb +++ b/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