db/migrate/20090319185505_add_disabled_to_user.rb @@ -1,6 +1,9 @@ edge +* Users cannot be directly deleted anymore. They are disabled in order to not break references and keep the correct history. + If you really want to delete a user you have to resort to the script/console or SQL for now. Fixes ticket #110. + * Move gems to vendor/gems and update Capistrano to 2.5.5 and Net::SSH to 2.0.11 * Update Rails to 2.2.2 CHANGELOG.txt @@ -1,5 +1,5 @@ class UsersController < ApplicationController - before_filter :ensure_admin, :only => [:new, :destroy, :create] + before_filter :ensure_admin, :only => [:new, :destroy, :create, :enable] before_filter :ensure_admin_or_my_entry, :only => [:edit, :update] # GET /users @@ -80,10 +80,10 @@ class UsersController < ApplicationController @user = User.find(params[:id]) if @user.admin? && User.admin_count == 1 - message = 'Can not delete last admin user.' + message = 'Can not disable last admin user.' else - @user.destroy - message = 'User was successfully deleted.' + @user.disable + message = 'User was successfully disabled.' end respond_to do |format| @@ -92,6 +92,17 @@ class UsersController < ApplicationController format.xml { head :ok } end end + + def enable + @user = User.find(params[:id]) + @user.enable + flash[:notice] = "The user was enabled" + + respond_to do |format| + format.html { redirect_to users_path } + format.xml { head :ok } + end + end # GET /users/1/deployments # GET /users/1/deployments.xml app/controllers/users_controller.rb @@ -16,6 +16,9 @@ class User < ActiveRecord::Base validates_length_of :email, :within => 3..100 validates_uniqueness_of :login, :email, :case_sensitive => false before_save :encrypt_password + + named_scope :enabled, :conditions => {:disabled => nil} + named_scope :disabled, :conditions => "disabled IS NOT NULL" def validate_on_update if User.find(self.id).admin? && !self.admin? @@ -25,7 +28,7 @@ class User < ActiveRecord::Base # Authenticates a user by their login name and unencrypted password. Returns the user or nil. def self.authenticate(login, password) - u = find_by_login(login) # need to get the salt + u = find_by_login_and_disabled(login, nil) # need to get the salt u && u.authenticated?(password) ? u : nil end @@ -83,12 +86,24 @@ class User < ActiveRecord::Base end def self.admin_count - count(:id, :conditions => ['admin = 1']) + count(:id, :conditions => ['admin = 1 AND disabled IS NULL']) end def recent_deployments(limit=3) self.deployments.find(:all, :limit => limit, :order => 'created_at DESC') end + + def disabled? + !self.disabled.blank? + end + + def disable + self.update_attribute(:disabled, Time.now) + end + + def enable + self.update_attribute(:disabled, nil) + end protected # before filter app/models/user.rb @@ -9,8 +9,9 @@ <table class="sortable"> <tr> <th width="1%">Login</th> - <th width="97%">Email</th> + <th width="96%">Email</th> <th width="1%">Admin</th> + <th width="1%">Disabled</th> <th width="1%">Created At</th> </tr> <% for user in @users %> @@ -18,6 +19,11 @@ <td nowrap><%= link_to h(user.login), user_path(user) %></td> <td><%=h user.email %></td> <td><%=h user.admin? ? 'Admin' : '' %></td> + <td> + <% if user.disabled? %> + <span style="color:red">disabled</span> + <% end %> + </td> <td><%=h user.created_at.to_s(:date_with_day) %></td> <% if current_user.admin? || current_user == user -%> <td><%= link_to 'Edit', edit_user_path(user) %></td> app/views/users/index.html.erb @@ -31,8 +31,12 @@ </p> <br/> <% if current_user.admin? || current_user == @user %> - <%= link_to 'Edit & change password', edit_user_path(@user), :class => 'arrow_link' %> | - <%= link_to 'Delete', user_path(@user), :confirm => 'Are you sure?', :method => :delete, :class => 'arrow_link' %> + <%= link_to 'Edit & change password', edit_user_path(@user), :class => 'arrow_link' %> | + <% if @user.disabled? %> + <%= link_to 'Enable', enable_user_path(@user), :confirm => 'Are you sure?', :method => :post, :class => 'arrow_link' %> + <% else %> + <%= link_to 'Disable', user_path(@user), :confirm => 'Are you sure? Disabled users can no longer login.', :method => :delete, :class => 'arrow_link' %> + <% end %> <% end %> </div> <div class="box_bottom"></div> app/views/users/show.html.erb @@ -30,7 +30,7 @@ ActionController::Routing::Routes.draw do |map| end # RESTful auth - map.resources :users,:member => {:deployments => :get} + map.resources :users,:member => {:deployments => :get, :enable => :post} map.resources :sessions, :collection => {:version => :get} map.signup '/signup', :controller => 'users', :action => 'new' map.login '/login', :controller => 'sessions', :action => 'new' config/routes.rb @@ -9,7 +9,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20090223081938) do +ActiveRecord::Schema.define(:version => 20090319185505) do create_table "configuration_parameters", :force => true do |t| t.string "name" @@ -119,6 +119,9 @@ ActiveRecord::Schema.define(:version => 20090223081938) do t.datetime "remember_token_expires_at" t.integer "admin", :default => 0 t.string "time_zone", :default => "UTC" + t.datetime "disabled" end + add_index "users", ["disabled"], :name => "index_users_on_disabled" + end db/schema.rb @@ -22,6 +22,14 @@ class SessionsControllerTest < Test::Unit::TestCase assert session[:user] assert_response :redirect end + + def test_should_not_login_if_disabled + User.find_by_login('quentin').disable + + post :create, :login => 'quentin', :password => 'test' + assert_nil session[:user] + assert_response :success + end def test_should_fail_login_and_not_redirect post :create, :login => 'quentin', :password => 'bad password' test/functional/sessions_controller_test.rb @@ -99,7 +99,7 @@ class UsersControllerTest < Test::Unit::TestCase assert admin.admin? login(admin) delete :destroy, :id => user_2.id - assert_equal 3, User.count + assert_equal 3, User.enabled.count end def test_admin_status_can_not_be_set_by_non_admins @@ -147,15 +147,15 @@ class UsersControllerTest < Test::Unit::TestCase # delete the user delete :destroy, :id => user.id - assert_equal 2, User.count + assert_equal 2, User.enabled.count # delete the other admin delete :destroy, :id => admin_2.id - assert_equal 1, User.count + assert_equal 1, User.enabled.count # last admin can not be deleted delete :destroy, :id => admin.id - assert_equal 1, User.count + assert_equal 1, User.enabled.count end # basic non-exception test @@ -191,6 +191,44 @@ class UsersControllerTest < Test::Unit::TestCase assert_not_equal 'foobarrr', other.login end + def test_destroy_should_only_mark_as_disabled + user = admin_login + other = create_new_user + assert !other.disabled? + + assert_difference "User.disabled.count" do + assert_no_difference "User.count" do + post :destroy, :id => other.id + assert_response :redirect + end + end + + end + + def test_enable + user = admin_login + other = create_new_user + other.disable + + post :enable, :id => other.id + assert_response :redirect + + other.reload + assert !other.disabled? + end + + def test_enable_only_admin + user = login + other = create_new_user + other.disable + + post :enable, :id => other.id + assert_response :redirect + + other.reload + assert other.disabled? + end + protected def create_user(options = {}) test/functional/users_controller_test.rb @@ -40,6 +40,12 @@ class UserTest < Test::Unit::TestCase assert u.errors.on(:email) end end + + def test_should_not_authenticate_if_disabled + assert_equal users(:quentin), User.authenticate('quentin', 'test') + User.find_by_login("quentin").disable + assert_equal nil, User.authenticate('quentin', 'test') + end def test_should_reset_password users(:quentin).update_attributes(:password => 'new password', :password_confirmation => 'new password') @@ -136,6 +142,35 @@ class UserTest < Test::Unit::TestCase assert_equal 3, user.recent_deployments.size assert_equal 2, user.recent_deployments(2).size end + + def test_disable + user = create_new_user + assert !user.disabled? + + user.disable + + assert user.disabled? + + user.enable + + assert !user.disabled? + end + + def test_enabled_named_scope + User.destroy_all + assert_equal [], User.enabled + assert_equal [], User.disabled + + user = create_new_user + + assert_equal [user], User.enabled + assert_equal [], User.disabled + + user.disable + + assert_equal [], User.enabled + assert_equal [user], User.disabled + end protected def create_user(options = {}) test/unit/user_test.rb cdbff3924bf11832a0e6e458ff8cca769933ca46 Jonathan Weiss jw@innerewut.de http://github.com/peritor/webistrano/commit/519b8dab7b88102bc818afc0352012a9a1b87a62 519b8dab7b88102bc818afc0352012a9a1b87a62 2009-03-19T12:24:46-07:00 2009-03-19T12:24:46-07:00 Users cannot be directly deleted anymore. They are disabled in order to not break references and keep the correct history. If you really want to delete a user you have to resort to the script/console or SQL for now. Fixes ticket #110 ac106ec76a9c1f4a6cb086cd17d6b8c3ba659080 Jonathan Weiss jw@innerewut.de