From ca829cdfdbc96e4c8509da0188313f3c11c33156 Mon Sep 17 00:00:00 2001 From: Sawy Date: Tue, 25 Feb 2025 11:09:40 +0200 Subject: [PATCH 1/3] CV2-6141: update opotions if relationship already exists --- app/models/bot/alegre.rb | 2 +- app/models/relationship.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/bot/alegre.rb b/app/models/bot/alegre.rb index 43abff02e..4d25df1b5 100644 --- a/app/models/bot/alegre.rb +++ b/app/models/bot/alegre.rb @@ -693,7 +693,7 @@ def self.relationship_model_not_allowed(relationship_model) def self.report_exception_if_bad_relationship(relationship, pm_id_scores, relationship_type) if relationship.model.nil? || relationship.weight.nil? || relationship.source_field.nil? || relationship.target_field.nil? || self.relationship_model_not_allowed(relationship.model) - CheckSentry.notify(Bot::Alegre::Error.new("[Alegre] Bad relationship was stored without required metadata"), **{trace: Thread.current.backtrace.join("\n"), relationship: relationship.attributes, relationship_type: relationship_type, pm_id_scores: pm_id_scores}) + CheckSentry.notify(Bot::Alegre::Error.new("[Alegre] Bad relationship with ID [#{relationship.id}] was stored without required metadata"), **{trace: Thread.current.backtrace.join("\n"), relationship: relationship.attributes, relationship_type: relationship_type, pm_id_scores: pm_id_scores}) end end diff --git a/app/models/relationship.rb b/app/models/relationship.rb index 5cef94695..a5aae46da 100644 --- a/app/models/relationship.rb +++ b/app/models/relationship.rb @@ -159,6 +159,13 @@ def create_or_update_parent_id def self.create_unless_exists(source_id, target_id, relationship_type, options = {}) # Verify that the target is not part of another source; in this case, we should return the existing relationship. r = Relationship.where(target_id: target_id).where.not(source_id: source_id).last + # should update options if exists + unless r.nil? || !options.blank? + options.each do |key, value| + r.send("#{key}=", value) if r.respond_to?("#{key}=") + end + r.save! + end return r unless r.nil? # otherwise we should get the existing relationship based on source, target and type r = Relationship.where(source_id: source_id, target_id: target_id).where('relationship_type = ?', relationship_type.to_yaml).last From 2274247e156c1b5f26cfadaab77299011dab22eb Mon Sep 17 00:00:00 2001 From: Sawy Date: Tue, 25 Feb 2025 19:19:59 +0200 Subject: [PATCH 2/3] CV2-6141: update options in case existing model is blank --- app/models/relationship.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/relationship.rb b/app/models/relationship.rb index a5aae46da..5f8f614a1 100644 --- a/app/models/relationship.rb +++ b/app/models/relationship.rb @@ -159,8 +159,8 @@ def create_or_update_parent_id def self.create_unless_exists(source_id, target_id, relationship_type, options = {}) # Verify that the target is not part of another source; in this case, we should return the existing relationship. r = Relationship.where(target_id: target_id).where.not(source_id: source_id).last - # should update options if exists - unless r.nil? || !options.blank? + # should update options if exists and at least on field is blank + if !r.nil? && !options.blank? && r.model.blank? options.each do |key, value| r.send("#{key}=", value) if r.respond_to?("#{key}=") end From 4a43e2cecdda337cf68704f23896717ee2ac61ba Mon Sep 17 00:00:00 2001 From: Sawy Date: Wed, 26 Feb 2025 08:08:20 +0200 Subject: [PATCH 3/3] CV2-6141: apply PR comments --- app/models/relationship.rb | 20 +++++++++++++------- test/models/relationship_test.rb | 12 ++++++++++-- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/app/models/relationship.rb b/app/models/relationship.rb index 5f8f614a1..f6b28a50e 100644 --- a/app/models/relationship.rb +++ b/app/models/relationship.rb @@ -159,13 +159,7 @@ def create_or_update_parent_id def self.create_unless_exists(source_id, target_id, relationship_type, options = {}) # Verify that the target is not part of another source; in this case, we should return the existing relationship. r = Relationship.where(target_id: target_id).where.not(source_id: source_id).last - # should update options if exists and at least on field is blank - if !r.nil? && !options.blank? && r.model.blank? - options.each do |key, value| - r.send("#{key}=", value) if r.respond_to?("#{key}=") - end - r.save! - end + r.update_options(options) unless r.nil? return r unless r.nil? # otherwise we should get the existing relationship based on source, target and type r = Relationship.where(source_id: source_id, target_id: target_id).where('relationship_type = ?', relationship_type.to_yaml).last @@ -203,6 +197,8 @@ def self.create_unless_exists(source_id, target_id, relationship_type, options = exception_message = e.message exception_class = e.class.name end + else + r.update_options(options) end if r.nil? Rails.logger.error("[Relationship::create_unless_exists] returning nil: source_id #{source_id}, target_id #{target_id}, relationship_type #{relationship_type}.") @@ -234,6 +230,16 @@ def self.sync_statuses(item, status) end end + def update_options(options) + # should update options if exists and at least on field has a value + if self.model.blank? && !options.values.compact.empty? + options.each do |key, value| + self.send("#{key}=", value) if self.respond_to?("#{key}=") + end + self.save! + end + end + protected def update_elasticsearch_parent(action = 'create_or_update') diff --git a/test/models/relationship_test.rb b/test/models/relationship_test.rb index 92b49e047..9ff193312 100644 --- a/test/models/relationship_test.rb +++ b/test/models/relationship_test.rb @@ -326,9 +326,17 @@ def setup target = create_project_media team: t r = nil assert_difference 'Relationship.count' do - r = Relationship.create_unless_exists(source.id, target.id, Relationship.confirmed_type, { user_id: u.id }) + r = Relationship.create_unless_exists(source.id, target.id, Relationship.confirmed_type) end - assert_equal u.id, r.user_id + # should update options if relationship already exists + assert_nil r.model + another_source = create_project_media team: t + r2 = nil + assert_no_difference 'Relationship.count' do + r2 = Relationship.create_unless_exists(another_source.id, target.id, Relationship.confirmed_type, { model: 'elasticsearch' }) + end + assert_equal r, r2 + assert_equal 'elasticsearch', r2.model r2 = nil assert_no_difference 'Relationship.count' do r2 = Relationship.create_unless_exists(source.id, target.id, Relationship.confirmed_type)