diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 6060e129298abac60faeca959ec46a9274227f08..8b872d8120c4d5e93f928e398b5b942228891f59 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -15,8 +15,8 @@ class UsersController < ApplicationController
layout 'admin'
before_filter :require_admin, :except => :show
- before_filter :find_user, :only => [:show, :edit, :update, :edit_membership, :destroy_membership]
- accept_key_auth :index, :show, :create, :update
+ before_filter :find_user, :only => [:show, :edit, :update, :destroy, :edit_membership, :destroy_membership]
+ accept_key_auth :index, :show, :create, :update, :destroy
include SortHelper
include CustomFieldsHelper
@@ -177,6 +177,24 @@ class UsersController < ApplicationController
redirect_to :controller => 'users', :action => 'edit', :id => @user
end
+ verify :method => :delete, :only => :destroy, :render => {:nothing => true, :status => :method_not_allowed }
+ def destroy
+ # Only allow to delete users with STATUS_REGISTERED for now
+ # It is assumed that these users are not yet references in any way
+ # from other objects.
+ return render_403 unless @user.deletable?
+
+ @user.destroy
+ respond_to do |format|
+ format.html {
+ flash[:notice] = l(:notice_successful_delete)
+ redirect_back_or_default(:action => 'index')
+ }
+ format.api { head :ok }
+ end
+ end
+
+
def edit_membership
@membership = Member.edit_membership(params[:membership_id], params[:membership], @user)
@membership.save if request.post?
diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb
index 876175bff6c90ab0ca9d32325e76655eff731d7a..6c99877dc1b60ba710e954c35c40d85e02bf2610 100644
--- a/app/helpers/users_helper.rb
+++ b/app/helpers/users_helper.rb
@@ -36,13 +36,26 @@ module UsersHelper
def change_status_link(user)
url = {:controller => 'users', :action => 'update', :id => user, :page => params[:page], :status => params[:status], :tab => nil}
+ links = []
if user.locked?
- link_to l(:button_unlock), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock'
+ links << link_to(l(:button_unlock), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock')
elsif user.registered?
- link_to l(:button_activate), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock'
+ links << link_to(l(:button_activate), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock')
elsif user != User.current
- link_to l(:button_lock), url.merge(:user => {:status => User::STATUS_LOCKED}), :method => :put, :class => 'icon icon-lock'
+ links << link_to(l(:button_lock), url.merge(:user => {:status => User::STATUS_LOCKED}), :method => :put, :class => 'icon icon-lock')
end
+
+ if user.deletable?
+ links << link_to(
+ l(:button_delete), {:controller => 'users', :action => 'destroy', :id => user},
+ :method => :delete,
+ :confirm => l(:text_are_you_sure),
+ :title => l(:button_delete),
+ :class => 'icon icon-del'
+ )
+ end
+
+ links.join(" ")
end
def user_settings_tabs
diff --git a/app/models/user.rb b/app/models/user.rb
index 6f46a31e1d1bf08fe4a78e77d0368389d0202cb1..784f2529f2992543e569e66cf9029a1116be0216 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -207,6 +207,11 @@ class User < Principal
update_attribute(:status, STATUS_LOCKED)
end
+ def deletable?
+ registered? && last_login_on.nil?
+ end
+
+
# Returns true if +clear_password+ is the correct user's password, otherwise false
def check_password?(clear_password)
if auth_source_id.present?
diff --git a/config/routes.rb b/config/routes.rb
index 971966ac16aa0f2d1c136ff63e6d57e682da979a..dae8069db5b0c4f52a0ae02705bd5676fbf2bc47 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -136,8 +136,7 @@ ActionController::Routing::Routes.draw do |map|
map.resources :users, :member => {
:edit_membership => :post,
:destroy_membership => :post
- },
- :except => [:destroy]
+ }
# For nice "roadmap" in the url for the index action
map.connect 'projects/:project_id/roadmap', :controller => 'versions', :action => 'index'
diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb
index bff9119236b439b6a1c1ec3236f957ac0636bf8b..41345765be3f07b40a1beca7d33ada0ceff534dc 100644
--- a/test/functional/users_controller_test.rb
+++ b/test/functional/users_controller_test.rb
@@ -269,6 +269,31 @@ class UsersControllerTest < ActionController::TestCase
assert u.check_password?('newpass')
end
+ def test_destroy
+ u = User.new(:firstname => 'Death', :lastname => 'Row', :mail => 'death.row@example.com', :language => 'en')
+ u.login = 'death.row'
+ u.status = User::STATUS_REGISTERED
+ u.save!
+
+ delete :destroy, :id => u.id
+ assert_redirected_to :action => 'index'
+ # make sure that the user was actually destroyed
+ assert_raises(ActiveRecord::RecordNotFound) { u.reload }
+ end
+
+ def test_failing_destroy
+ u = User.new(:firstname => 'Surviving', :lastname => 'Patient', :mail => 'surviving.patient@example.com', :language => 'en')
+ u.login = 'surviving.patient'
+ u.status = User::STATUS_ACTIVE
+ u.save!
+
+ delete :destroy, :id => u.id
+ assert_response :forbidden
+ # make sure the user is still around
+ assert !u.reload.destroyed?
+ end
+
+
def test_edit_membership
post :edit_membership, :id => 2, :membership_id => 1,
:membership => { :role_ids => [2]}
diff --git a/test/integration/api_test/users_test.rb b/test/integration/api_test/users_test.rb
index 6196d2ce47909ceea551fea0787cbb1def983fb4..56085d21ecb8a22b0bbd3ef2109c7009c417b08d 100644
--- a/test/integration/api_test/users_test.rb
+++ b/test/integration/api_test/users_test.rb
@@ -240,26 +240,52 @@ class ApiTest::UsersTest < ActionController::IntegrationTest
end
end
end
+ end
- context "DELETE /users/2" do
- context ".xml" do
- should "not be allowed" do
- assert_no_difference('User.count') do
- delete '/users/2.xml'
- end
+ context "DELETE /users/:temp:" do
+ context ".xml" do
+ should "delete the user" do
+ u = User.new(:firstname => 'Death', :lastname => 'Row', :mail => 'death.row@example.com', :language => 'en')
+ u.login = 'death.row'
+ u.status = User::STATUS_REGISTERED
+ u.save!
+
+ assert_difference('User.count',-1) do
+ delete "/users/#{u.id}.xml", {}, :authorization => credentials('admin')
+ end
+
+ assert_response :success
+ assert_nil User.find_by_id(u.id)
+ end
- assert_response :method_not_allowed
+ should "not delete active user" do
+ assert_difference('User.count',0) do
+ delete "/users/2.xml", {}, :authorization => credentials('jsmith')
end
+ assert_response :forbidden
end
+ end
- context ".json" do
- should "not be allowed" do
- assert_no_difference('User.count') do
- delete '/users/2.json'
- end
+ context ".json" do
+ should "delete the user" do
+ u = User.new(:firstname => 'Death', :lastname => 'Row', :mail => 'death.row@example.com', :language => 'en')
+ u.login = 'death.row'
+ u.status = User::STATUS_REGISTERED
+ u.save!
+
+ assert_difference('User.count',-1) do
+ delete "/users/#{u.id}.json", {}, :authorization => credentials('admin')
+ end
+
+ assert_response :success
+ assert_nil User.find_by_id(u.id)
+ end
- assert_response :method_not_allowed
+ should "not delete active user" do
+ assert_difference('User.count',0) do
+ delete "/users/2.json", {}, :authorization => credentials('jsmith')
end
+ assert_response :forbidden
end
end
end