In stead of destroying messages when their notifiables

are destroyed (which could lead to messages becoming 
invisible), we nullify the notifiable references.

Since AR's dependent => nullify only nullifies the
id, not the type, a before_destroy filter is introduced.

6 files changed, 51 insertions, 2 deletions

diff --git a/app/models/committership.rb b/app/models/committership.rb
index a3d2907..b6f50b5 100644
--- a/app/models/committership.rb
+++ b/app/models/committership.rb
@@ -30,6 +30,8 @@ class Committership < ActiveRecord::Base
   after_create :notify_repository_owners
   after_create :add_new_committer_event
   after_destroy :add_removed_committer_event
+  has_many :messages, :as => :notifiable
+  before_destroy :nullify_messages
   
   named_scope :groups, :conditions => { :committer_type => "Group" }
   named_scope :users,  :conditions => { :committer_type => "User" }  
@@ -84,4 +86,8 @@ class Committership < ActiveRecord::Base
       repository.project.create_event(Action::REMOVE_COMMITTER, repository, 
                                       creator, committer.title)
     end
+    
+    def nullify_messages
+      messages.update_all({:notifiable_id => nil, :notifiable_type => nil})
+    end
 end
diff --git a/app/models/membership.rb b/app/models/membership.rb
index 0db3088..d24ed8d 100644
--- a/app/models/membership.rb
+++ b/app/models/membership.rb
@@ -20,9 +20,10 @@ class Membership < ActiveRecord::Base
   belongs_to :group
   belongs_to :user
   belongs_to :role
-  has_many :messages, :as => :notifiable, :dependent => :destroy
+  has_many :messages, :as => :notifiable
   before_validation_on_update :dont_demote_group_creator
   before_destroy :dont_delete_group_creator
+  before_destroy :nullify_messages
 
   after_create :send_notification_if_invited
   attr_accessor :inviter
@@ -77,4 +78,8 @@ class Membership < ActiveRecord::Base
     def dont_delete_group_creator
       return user != group.creator
     end
+    
+    def nullify_messages
+      messages.update_all({:notifiable_id => nil, :notifiable_type => nil})
+    end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 4462215..f524e16 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -25,9 +25,11 @@ class MergeRequest < ActiveRecord::Base
   belongs_to :source_repository, :class_name => 'Repository'
   belongs_to :target_repository, :class_name => 'Repository'
   has_many   :events, :as => :target, :dependent => :destroy
-  has_many :messages, :as => :notifiable, :dependent => :destroy
+  has_many :messages, :as => :notifiable
   has_many :comments, :as => :target, :dependent => :destroy
   
+  before_destroy :nullify_messages
+  
   is_indexed :fields => ["proposal"], :include => [{
       :association_name => "user",
       :field => "login",
@@ -360,4 +362,9 @@ class MergeRequest < ActiveRecord::Base
     response = access_token.get("/")
     return Net::HTTPSuccess === response
   end
+  
+  def nullify_messages
+    messages.update_all({:notifiable_id => nil, :notifiable_type => nil})
+  end
+  
 end
diff --git a/test/unit/committership_test.rb b/test/unit/committership_test.rb
index 3bb662b..3ef4867 100644
--- a/test/unit/committership_test.rb
+++ b/test/unit/committership_test.rb
@@ -51,6 +51,18 @@ class CommittershipTest < ActiveSupport::TestCase
     end
   end
   
+  should 'nullify notifiable_type and notifiable_id when destroyed' do
+    owner = users(:johan)
+    c = new_committership(:committer => groups(:team_thunderbird))
+    c.save
+    message = owner.received_messages.last
+    assert_equal c, message.notifiable
+    c.destroy
+    message.reload
+    assert_nil message.notifiable_type
+    assert_nil message.notifiable_id
+  end
+  
   should "have a members attribute that's the group members if the committer is a Group" do
     c = new_committership(:committer => groups(:team_thunderbird))
     assert_equal groups(:team_thunderbird).members, c.members
diff --git a/test/unit/membership_test.rb b/test/unit/membership_test.rb
index ebd54d2..973f200 100644
--- a/test/unit/membership_test.rb
+++ b/test/unit/membership_test.rb
@@ -42,6 +42,17 @@ class MembershipTest < ActiveSupport::TestCase
       assert_equal(membership, message.notifiable)
     end
     
+    should 'nullify messages when deleted' do
+      @invitee = Factory.create(:user)
+      @user.received_messages.destroy_all
+      membership = Membership.build_invitation(@inviter, :user => @invitee, :group => @group, :role => Role.member)
+      membership.save
+      message = membership.messages.first
+      assert membership.destroy
+      assert_nil message.reload.notifiable_type
+      assert_nil message.notifiable_id
+    end
+    
     should 'not send a notification if no inviter is set' do
       membership = Membership.new(:user => @user, :group => @group, :role => roles(:member))
       membership.expects(:send_notification).never
diff --git a/test/unit/merge_request_test.rb b/test/unit/merge_request_test.rb
index 5af1c39..279649a 100644
--- a/test/unit/merge_request_test.rb
+++ b/test/unit/merge_request_test.rb
@@ -281,6 +281,14 @@ class MergeRequestTest < ActiveSupport::TestCase
       end
     end
     
+    should 'nullify associated messages when deleted' do
+      @merge_request.deliver_status_update(users(:moe))
+      message = @merge_request.user.received_messages.last
+      @merge_request.destroy
+      message.reload
+      assert_nil message.notifiable
+    end
+    
     should 'provide a hash of labels and values for possible next states' do
       @merge_request.status = MergeRequest::STATUS_VERIFYING
       assert_equal({'Merged' => 'merge', 'Rejected' => 'reject'}, @merge_request.possible_next_states_hash)