Browse Source

Fixes #2347 - Merging two Tickets with link to same other Ticket fails.

Rolf Schmidt 4 years ago
parent
commit
f132225b2f
4 changed files with 144 additions and 5 deletions
  1. 28 0
      app/models/link.rb
  2. 11 2
      app/models/ticket.rb
  3. 6 3
      spec/factories/link.rb
  4. 99 0
      spec/models/ticket_spec.rb

+ 28 - 0
app/models/link.rb

@@ -244,4 +244,32 @@ class Link < ApplicationModel
     end
   end
 
+  def self.duplicates(object1_id:, object1_value:, object2_value:, object2_id: nil)
+    if !object2_id
+      object2_id = object1_id
+    end
+
+    Link.joins(', links as linksb').where('
+       (
+         links.link_type_id = linksb.link_type_id
+         AND links.link_object_source_id = linksb.link_object_source_id
+         AND links.link_object_source_value = linksb.link_object_source_value
+         AND links.link_object_target_id = ?
+         AND linksb.link_object_target_id = ?
+         AND links.link_object_target_value = ?
+         AND linksb.link_object_target_value = ?
+       )
+       OR
+       (
+         links.link_type_id = linksb.link_type_id
+         AND links.link_object_target_id = linksb.link_object_target_id
+         AND links.link_object_target_value = linksb.link_object_target_value
+         AND links.link_object_source_id = ?
+         AND linksb.link_object_source_id = ?
+         AND links.link_object_source_value = ?
+         AND linksb.link_object_source_value = ?
+       )
+    ', object1_id, object2_id, object1_value, object2_value, object1_id, object2_id, object1_value, object2_value)
+  end
+
 end

+ 11 - 2
app/models/ticket.rb

@@ -357,12 +357,21 @@ returns
 
       # reassign links to the new ticket
       # rubocop:disable Rails/SkipsModelValidations
+      ticket_source_id = Link::Object.find_by(name: 'Ticket').id
+
+      # search for all duplicate source and target links and destroy them
+      # before link merging
+      Link.duplicates(
+        object1_id:    ticket_source_id,
+        object1_value: id,
+        object2_value: data[:ticket_id]
+      ).destroy_all
       Link.where(
-        link_object_source_id:    Link::Object.find_by(name: 'Ticket').id,
+        link_object_source_id:    ticket_source_id,
         link_object_source_value: id,
       ).update_all(link_object_source_value: data[:ticket_id])
       Link.where(
-        link_object_target_id:    Link::Object.find_by(name: 'Ticket').id,
+        link_object_target_id:    ticket_source_id,
         link_object_target_value: id,
       ).update_all(link_object_target_value: data[:ticket_id])
       # rubocop:enable Rails/SkipsModelValidations

+ 6 - 3
spec/factories/link.rb

@@ -1,13 +1,16 @@
 FactoryBot.define do
   factory :link do
     transient do
+      link_type { 'normal' }
+      link_object_source { 'Ticket' }
+      link_object_target { 'Ticket' }
       from { Ticket.first }
       to   { Ticket.last }
     end
 
-    link_type_id             { Link::Type.find_by(name: 'normal').id }
-    link_object_source_id    { Link::Object.find_by(name: 'Ticket').id }
-    link_object_target_id    { Link::Object.find_by(name: 'Ticket').id }
+    link_type_id             { Link::Type.create_if_not_exists(name: link_type, active: true).id }
+    link_object_source_id    { Link::Object.create_if_not_exists(name: link_object_source).id }
+    link_object_target_id    { Link::Object.create_if_not_exists(name: link_object_target).id }
     link_object_source_value { from.id }
     link_object_target_value { to.id }
   end

+ 99 - 0
spec/models/ticket_spec.rb

@@ -83,6 +83,105 @@ RSpec.describe Ticket, type: :model do
         end
       end
 
+      context 'when both tickets are linked with the same parent (parent->child)' do
+        let(:parent) { create(:ticket) }
+
+        before do
+          create(:link,
+                 link_type:                'child',
+                 link_object_source_value: ticket.id,
+                 link_object_target_value: parent.id)
+          create(:link,
+                 link_type:                'child',
+                 link_object_source_value: target_ticket.id,
+                 link_object_target_value: parent.id)
+
+          ticket.merge_to(ticket_id: target_ticket.id, user_id: 1)
+        end
+
+        it 'does remove the link from the merged ticket' do
+          links = Link.list(
+            link_object:       'Ticket',
+            link_object_value: ticket.id
+          )
+          expect(links.count).to eq(1) # one link to the source ticket (no parent link)
+        end
+
+        it 'does not remove the link from the target ticket' do
+          links = Link.list(
+            link_object:       'Ticket',
+            link_object_value: target_ticket.id
+          )
+          expect(links.count).to eq(2) # one link to the merged ticket + parent link
+        end
+      end
+
+      context 'when both tickets are linked with the same parent (child->parent)' do
+        let(:parent) { create(:ticket) }
+
+        before do
+          create(:link,
+                 link_type:                'child',
+                 link_object_source_value: parent.id,
+                 link_object_target_value: ticket.id)
+          create(:link,
+                 link_type:                'child',
+                 link_object_source_value: parent.id,
+                 link_object_target_value: target_ticket.id)
+
+          ticket.merge_to(ticket_id: target_ticket.id, user_id: 1)
+        end
+
+        it 'does remove the link from the merged ticket' do
+          links = Link.list(
+            link_object:       'Ticket',
+            link_object_value: ticket.id
+          )
+          expect(links.count).to eq(1) # one link to the source ticket (no parent link)
+        end
+
+        it 'does not remove the link from the target ticket' do
+          links = Link.list(
+            link_object:       'Ticket',
+            link_object_value: target_ticket.id
+          )
+          expect(links.count).to eq(2) # one link to the merged ticket + parent link
+        end
+      end
+
+      context 'when both tickets are linked with the same parent (different link types)' do
+        let(:parent) { create(:ticket) }
+
+        before do
+          create(:link,
+                 link_type:                'normal',
+                 link_object_source_value: parent.id,
+                 link_object_target_value: ticket.id)
+          create(:link,
+                 link_type:                'child',
+                 link_object_source_value: parent.id,
+                 link_object_target_value: target_ticket.id)
+
+          ticket.merge_to(ticket_id: target_ticket.id, user_id: 1)
+        end
+
+        it 'does remove the link from the merged ticket' do
+          links = Link.list(
+            link_object:       'Ticket',
+            link_object_value: ticket.id
+          )
+          expect(links.count).to eq(1) # one link to the source ticket (no normal link)
+        end
+
+        it 'does not remove the link from the target ticket' do
+          links = Link.list(
+            link_object:       'Ticket',
+            link_object_value: target_ticket.id
+          )
+          expect(links.count).to eq(3) # one lin to the merged ticket + parent link + normal link
+        end
+      end
+
       context 'when merging' do
         let(:merge_user) { create(:user) }