From f132225b2f3e65960a40cb86ae05096fb90b11a2 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Fri, 6 Nov 2020 10:46:48 +0100 Subject: [PATCH] Fixes #2347 - Merging two Tickets with link to same other Ticket fails. --- app/models/link.rb | 28 +++++++++++ app/models/ticket.rb | 13 ++++- spec/factories/link.rb | 9 ++-- spec/models/ticket_spec.rb | 99 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 5 deletions(-) diff --git a/app/models/link.rb b/app/models/link.rb index 29577623e..d03b5b159 100644 --- a/app/models/link.rb +++ b/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 diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 2879d21d0..9d91e91f0 100644 --- a/app/models/ticket.rb +++ b/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 diff --git a/spec/factories/link.rb b/spec/factories/link.rb index bfa888cc7..02cc87f29 100644 --- a/spec/factories/link.rb +++ b/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 diff --git a/spec/models/ticket_spec.rb b/spec/models/ticket_spec.rb index a08bcd646..8ad3328d8 100644 --- a/spec/models/ticket_spec.rb +++ b/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) }