Fixed that Member#destroy triggers after_destroy callbacks twice (#16795).

git-svn-id: http://svn.redmine.org/redmine/trunk@13140 e93f8b46-1217-0410-a6f0-8f06a7374b81

2 files changed, 29 insertions, 0 deletions

diff --git a/app/models/member.rb b/app/models/member.rb
index 76c91febf..c5d943bae 100644
--- a/app/models/member.rb
+++ b/app/models/member.rb
@@ -73,6 +73,18 @@ class Member < ActiveRecord::Base
     member_roles.detect {|mr| mr.inherited_from}.nil?
   end
 
+  def destroy
+    if member_roles.reload.present?
+      # destroying the last role will destroy another instance
+      # of the same Member record, #super would then trigger callbacks twice
+      member_roles.destroy_all
+      @destroyed = true
+  		freeze
+    else
+      super
+    end
+  end
+
   def include?(user)
     if principal.is_a?(Group)
       !user.nil? && user.groups.include?(principal)
diff --git a/test/unit/member_test.rb b/test/unit/member_test.rb
index 586964b52..6ba9717c4 100644
--- a/test/unit/member_test.rb
+++ b/test/unit/member_test.rb
@@ -107,6 +107,23 @@ class MemberTest < ActiveSupport::TestCase
     assert_nil category1.assigned_to_id
   end
 
+  def test_destroy_should_trigger_callbacks_only_once
+    Member.class_eval { def destroy_test_callback; end}
+    Member.after_destroy :destroy_test_callback
+
+    m = Member.create!(:user_id => 1, :project_id => 1, :role_ids => [1,3])
+
+    Member.any_instance.expects(:destroy_test_callback).once
+    assert_difference 'Member.count', -1 do
+      assert_difference 'MemberRole.count', -2 do
+        m.destroy
+      end
+    end
+    assert m.destroyed?
+  ensure
+    Member._destroy_callbacks.reject! {|c| c.filter==:destroy_test_callback}
+  end
+
   def test_sort_without_roles
     a = Member.new(:roles => [Role.first])
     b = Member.new