Merge pull request #90 from meineerde/issues/master/124-delete-registered-users

[124] delete registered users

Merge pull request #90 from meineerde/issues/master/124-delete-registered-users
[124] delete registered users
03d956c3 · Felix Schäfer · a80f8224 · 54b4fdf1 · 03d956c3 · 03d956c3
Commit 03d956c3 authored Nov 25, 2011 by Felix Schäfer
7 changed files
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -16,8 +16,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
@@ -178,6 +178,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?

--- a/app/helpers/users_helper.rb
+++ b/app/helpers/users_helper.rb
@@ -37,13 +37,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

--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -70,6 +70,7 @@ class User < Principal
  validates_length_of :mail, :maximum => 60, :allow_nil => true
  validates_confirmation_of :password, :allow_nil => true
  validates_inclusion_of :mail_notification, :in => MAIL_NOTIFICATION_OPTIONS.collect(&:first), :allow_blank => true
+  validates_inclusion_of :status, :in => [STATUS_ANONYMOUS, STATUS_ACTIVE, STATUS_REGISTERED, STATUS_LOCKED]

  named_scope :in_group, lambda {|group|
    group_id = group.is_a?(Group) ? group.id : group.to_i
@@ -207,6 +208,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?
@@ -526,6 +532,24 @@ class User < Principal
    if !password.nil? && password.size < Setting.password_min_length.to_i
      errors.add(:password, :too_short, :count => Setting.password_min_length.to_i)
    end
+
+    # Status
+    if !new_record? && status_changed?
+      case status_was
+      when nil
+        # initial setting is always save
+        true
+      when STATUS_ANONYMOUS
+        # never allow a state change of the anonymous user
+        false
+      when STATUS_REGISTERED
+        [STATUS_ACTIVE, STATUS_LOCKED].include? status
+      when STATUS_ACTIVE
+        [STATUS_LOCKED].include? status
+      when STATUS_LOCKED
+        [STATUS_ACTIVE].include? status
+      end || errors.add(:status, :inclusion)
+    end
  end

  private

--- a/config/routes.rb
+++ b/config/routes.rb
@@ -137,8 +137,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'

--- a/test/functional/users_controller_test.rb
+++ b/test/functional/users_controller_test.rb
@@ -270,6 +270,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]}

--- a/test/integration/api_test/users_test.rb
+++ b/test/integration/api_test/users_test.rb
@@ -241,26 +241,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

--- a/test/unit/user_test.rb
+++ b/test/unit/user_test.rb
@@ -174,6 +174,14 @@ class UserTest < ActiveSupport::TestCase
    assert_equal nil, user
  end

+  def test_error_on_active_to_registered
+    user = User.try_to_login("jsmith", "jsmith")
+    assert_equal @jsmith, user
+
+    @jsmith.status = User::STATUS_REGISTERED
+    assert !@jsmith.save
+  end
+
  context ".try_to_login" do
    context "with good credentials" do
      should "return the user" do