From 5c6626a2d72843f5aa5ab21ba5a14ca4a47e39a1 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Fri, 4 Nov 2022 15:59:32 +0530
Subject: [PATCH 01/17] Add update logic for timeline event
---
.../timeline_event/update.rb | 4 ++
.../timeline_events/update_service.rb | 46 +++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/app/graphql/mutations/incident_management/timeline_event/update.rb b/app/graphql/mutations/incident_management/timeline_event/update.rb
index 1f53bdc19cb2e7..b35feed308249d 100644
--- a/app/graphql/mutations/incident_management/timeline_event/update.rb
+++ b/app/graphql/mutations/incident_management/timeline_event/update.rb
@@ -18,6 +18,10 @@ class Update < Base
required: false,
description: 'Timestamp when the event occurred.'
+ argument :timeline_event_tag_names, [GraphQL::Types::String],
+ required: false,
+ description: copy_field_description(Types::IncidentManagement::TimelineEventType, :timeline_event_tags)
+
def resolve(id:, **args)
timeline_event = authorized_find!(id: id)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index 8d4e29c6857485..86c337caf53dd2 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -17,6 +17,7 @@ def initialize(timeline_event, user, params)
@note = params[:note]
@occurred_at = params[:occurred_at]
@validation_context = VALIDATION_CONTEXT
+ @timeline_event_tags = params[:timeline_event_tag_names]
end
def execute
@@ -61,6 +62,51 @@ def was_changed(timeline_event)
:none
end
+ def update_timeline_event_tags(timeline_event, tag_updates)
+ return unless tag_updates.any?
+
+ tag_updates = tag_updates.map(&:downcase)
+ already_assigned_tags = timeline_event.timeline_event_tags.pluck_names.map(&:downcase)
+ tags_defined_on_project = timeline_event.project.incident_management_timeline_event_tags.pluck_names.map(&:downcase)
+
+ tags_to_remove = already_assigned_tags - tag_updates
+ tags_to_add = tag_updates - already_assigned_tags
+
+ validate_tags(tags_to_add, tags_defined_on_project)
+
+ remove_tag_links(timeline_event, tags_to_remove)
+ create_tag_links(timeline_event, tags_to_add)
+ end
+
+ def remove_tag_links(timeline_event, tags_to_remove_names)
+ tags_to_remove_ids = timeline_event.timeline_event_tags.by_names(tags_to_remove_names).ids
+
+ timeline_event.timeline_event_tag_links.where(timeline_event_tag_id: tag_to_remove_ids).delete_all
+ end
+
+ def create_tag_links(timeline_event, tags_to_add_names)
+ tags_to_add_ids = timeline_event.project.incident_management_timeline_event_tags.by_names(tags_to_add_names).ids
+
+ tag_links = tags_to_add_ids.map do |tag_id|
+ {
+ timeline_even_id: timeline_event.id
+ timeline_event_tag_id: tag_id
+ created_at: DateTime.current
+ }
+ end
+
+ IncidentManagement::TimelineEventTagLink.insert_all(tag_links) if tag_links.any?
+ end
+
+ def validate_tags(tags_to_add, defined_tags)
+ non_existing_tags = tags_to_add - defined_tags
+
+ return if non_existing_tags.empty?
+
+ raise Gitlab::Graphql::Errors::ArgumentError,
+ "Following tags don't exist: #{non_existing_tags}"
+ end
+
def allowed?
user&.can?(:edit_incident_management_timeline_event, timeline_event)
end
--
GitLab
From f4b98a94f4cbd78c4fc19a4c6651e406b4712a66 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Fri, 4 Nov 2022 16:01:11 +0530
Subject: [PATCH 02/17] Add update logic for timeline event
---
.../incident_management/timeline_events/update_service.rb | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index 86c337caf53dd2..e0f26875b1f062 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -89,8 +89,8 @@ def create_tag_links(timeline_event, tags_to_add_names)
tag_links = tags_to_add_ids.map do |tag_id|
{
- timeline_even_id: timeline_event.id
- timeline_event_tag_id: tag_id
+ timeline_even_id: timeline_event.id,
+ timeline_event_tag_id: tag_id,
created_at: DateTime.current
}
end
--
GitLab
From 15bf25c6eccf820ea16cc112cfbec051b16aef81 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Fri, 4 Nov 2022 17:17:04 +0530
Subject: [PATCH 03/17] Update gql docs
---
.../timeline_events/update_service.rb | 9 +++++----
doc/api/graphql/reference/index.md | 1 +
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index e0f26875b1f062..5d9c695f14bc60 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -24,6 +24,7 @@ def execute
return error_no_permissions unless allowed?
timeline_event.assign_attributes(update_params)
+ update_timeline_event_tags(timeline_event, timeline_event_tags) if timeline_event_tags
if timeline_event.save(context: validation_context)
add_system_note(timeline_event)
@@ -37,7 +38,7 @@ def execute
private
- attr_reader :timeline_event, :incident, :user, :note, :occurred_at, :validation_context
+ attr_reader :timeline_event, :incident, :user, :note, :occurred_at, :validation_context, :timeline_event_tags
def update_params
{ updated_by_user: user, note: note, occurred_at: occurred_at }.compact
@@ -74,14 +75,14 @@ def update_timeline_event_tags(timeline_event, tag_updates)
validate_tags(tags_to_add, tags_defined_on_project)
- remove_tag_links(timeline_event, tags_to_remove)
- create_tag_links(timeline_event, tags_to_add)
+ remove_tag_links(timeline_event, tags_to_remove) if tags_to_remove.any?
+ create_tag_links(timeline_event, tags_to_add) if tags_to_add.any?
end
def remove_tag_links(timeline_event, tags_to_remove_names)
tags_to_remove_ids = timeline_event.timeline_event_tags.by_names(tags_to_remove_names).ids
- timeline_event.timeline_event_tag_links.where(timeline_event_tag_id: tag_to_remove_ids).delete_all
+ timeline_event.timeline_event_tag_links.where(timeline_event_tag_id: tags_to_remove_ids).delete_all
end
def create_tag_links(timeline_event, tags_to_add_names)
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index 0d04223d96e439..d1583cbdc8ac3a 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -5067,6 +5067,7 @@ Input type: `TimelineEventUpdateInput`
| <a id="mutationtimelineeventupdateid"></a>`id` | [`IncidentManagementTimelineEventID!`](#incidentmanagementtimelineeventid) | ID of the timeline event to update. |
| <a id="mutationtimelineeventupdatenote"></a>`note` | [`String`](#string) | Text note of the timeline event. |
| <a id="mutationtimelineeventupdateoccurredat"></a>`occurredAt` | [`Time`](#time) | Timestamp when the event occurred. |
+| <a id="mutationtimelineeventupdatetimelineeventtagnames"></a>`timelineEventTagNames` | [`[String!]`](#string) | Tags for the incident timeline event. |
#### Fields
--
GitLab
From 5580c59d16da4e64ffeb2e67dbb0e497ce004998 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Fri, 4 Nov 2022 21:03:03 +0530
Subject: [PATCH 04/17] Always execute tag updates unless nil
---
.../incident_management/timeline_events/update_service.rb | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index 5d9c695f14bc60..972add03618313 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -24,7 +24,7 @@ def execute
return error_no_permissions unless allowed?
timeline_event.assign_attributes(update_params)
- update_timeline_event_tags(timeline_event, timeline_event_tags) if timeline_event_tags
+ update_timeline_event_tags(timeline_event, timeline_event_tags) unless timeline_event_tags.nil?
if timeline_event.save(context: validation_context)
add_system_note(timeline_event)
@@ -64,8 +64,6 @@ def was_changed(timeline_event)
end
def update_timeline_event_tags(timeline_event, tag_updates)
- return unless tag_updates.any?
-
tag_updates = tag_updates.map(&:downcase)
already_assigned_tags = timeline_event.timeline_event_tags.pluck_names.map(&:downcase)
tags_defined_on_project = timeline_event.project.incident_management_timeline_event_tags.pluck_names.map(&:downcase)
@@ -90,7 +88,7 @@ def create_tag_links(timeline_event, tags_to_add_names)
tag_links = tags_to_add_ids.map do |tag_id|
{
- timeline_even_id: timeline_event.id,
+ timeline_event_id: timeline_event.id,
timeline_event_tag_id: tag_id,
created_at: DateTime.current
}
--
GitLab
From 9145208ca26685c5fded05506fadc61f22549ae6 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Wed, 16 Nov 2022 19:34:37 +0530
Subject: [PATCH 05/17] Rebase to master and refactor validate tags
---
.../incident_management/timeline_events/update_service.rb | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index 972add03618313..b855dbc9087191 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -66,12 +66,11 @@ def was_changed(timeline_event)
def update_timeline_event_tags(timeline_event, tag_updates)
tag_updates = tag_updates.map(&:downcase)
already_assigned_tags = timeline_event.timeline_event_tags.pluck_names.map(&:downcase)
- tags_defined_on_project = timeline_event.project.incident_management_timeline_event_tags.pluck_names.map(&:downcase)
tags_to_remove = already_assigned_tags - tag_updates
tags_to_add = tag_updates - already_assigned_tags
- validate_tags(tags_to_add, tags_defined_on_project)
+ validate_tags(tags_to_add)
remove_tag_links(timeline_event, tags_to_remove) if tags_to_remove.any?
create_tag_links(timeline_event, tags_to_add) if tags_to_add.any?
@@ -97,7 +96,9 @@ def create_tag_links(timeline_event, tags_to_add_names)
IncidentManagement::TimelineEventTagLink.insert_all(tag_links) if tag_links.any?
end
- def validate_tags(tags_to_add, defined_tags)
+ def validate_tags(tags_to_add)
+ defined_tags = timeline_event.project.incident_management_timeline_event_tags.by_names(tags_to_add)
+
non_existing_tags = tags_to_add - defined_tags
return if non_existing_tags.empty?
--
GitLab
From 6d527e61d5f6fdffbc2567f00a82445d982852b9 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Wed, 16 Nov 2022 19:44:01 +0530
Subject: [PATCH 06/17] Fix cop offense for spec
---
app/models/incident_management/timeline_event_tag.rb | 4 ++++
.../timeline_events/update_service.rb | 11 ++++++++---
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/app/models/incident_management/timeline_event_tag.rb b/app/models/incident_management/timeline_event_tag.rb
index d1e3fbc2a6a8b8..59a3ed2d9f9b0d 100644
--- a/app/models/incident_management/timeline_event_tag.rb
+++ b/app/models/incident_management/timeline_event_tag.rb
@@ -25,5 +25,9 @@ class TimelineEventTag < ApplicationRecord
def self.pluck_names
pluck(:name)
end
+
+ def self.tag_ids
+ pluck(:id)
+ end
end
end
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index b855dbc9087191..e480316dea960e 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -77,13 +77,18 @@ def update_timeline_event_tags(timeline_event, tag_updates)
end
def remove_tag_links(timeline_event, tags_to_remove_names)
- tags_to_remove_ids = timeline_event.timeline_event_tags.by_names(tags_to_remove_names).ids
+ tags_to_remove_ids = timeline_event.timeline_event_tags.by_names(tags_to_remove_names).tag_ids
- timeline_event.timeline_event_tag_links.where(timeline_event_tag_id: tags_to_remove_ids).delete_all
+ timeline_event
+ .timeline_event_tag_links
+ .find_by_timeline_event_tag_id(timeline_event_tag_id: tags_to_remove_ids).delete_all
end
def create_tag_links(timeline_event, tags_to_add_names)
- tags_to_add_ids = timeline_event.project.incident_management_timeline_event_tags.by_names(tags_to_add_names).ids
+ tags_to_add_ids = timeline_event
+ .project
+ .incident_management_timeline_event_tags
+ .by_names(tags_to_add_names).tag_ids
tag_links = tags_to_add_ids.map do |tag_id|
{
--
GitLab
From 35c16f5f1f479e751ad53e7e4849a73bf81dfcda Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Thu, 17 Nov 2022 14:46:20 +0530
Subject: [PATCH 07/17] Update service for update event
Update link modal with a scope
Add specs for the graphql, api and service
---
.../timeline_event_tag_link.rb | 2 +
.../timeline_events/update_service.rb | 36 ++++++---
.../timeline_event/update_spec.rb | 44 ++++++++++-
.../timeline_event_tag_link_spec.rb | 29 +++++++
.../timeline_event/update_spec.rb | 41 +++++++++-
.../timeline_events/update_service_spec.rb | 77 +++++++++++++++++++
6 files changed, 215 insertions(+), 14 deletions(-)
diff --git a/app/models/incident_management/timeline_event_tag_link.rb b/app/models/incident_management/timeline_event_tag_link.rb
index 912339717a848f..23a2bd65a7f6ca 100644
--- a/app/models/incident_management/timeline_event_tag_link.rb
+++ b/app/models/incident_management/timeline_event_tag_link.rb
@@ -7,5 +7,7 @@ class TimelineEventTagLink < ApplicationRecord
belongs_to :timeline_event_tag, class_name: 'IncidentManagement::TimelineEventTag'
belongs_to :timeline_event, class_name: 'IncidentManagement::TimelineEvent'
+
+ scope :by_tag_ids, -> (ids) { where(timeline_event_tag_id: ids) }
end
end
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index e480316dea960e..0023655c4b814e 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -23,8 +23,18 @@ def initialize(timeline_event, user, params)
def execute
return error_no_permissions unless allowed?
+ # We first update the tags, if any, as they return error for non-existing tags.
+ # And then we update the other attributes of timeline event.
+ unless timeline_event_tags.nil?
+ tags_to_remove, tags_to_add = compute_tag_updates(timeline_event, timeline_event_tags)
+ non_existing_tags = validate_tags(tags_to_add)
+
+ return error("#{_("Following tags don't exist")}: #{non_existing_tags}") unless non_existing_tags.empty?
+
+ update_timeline_event_tags(timeline_event, tags_to_add, tags_to_remove)
+ end
+
timeline_event.assign_attributes(update_params)
- update_timeline_event_tags(timeline_event, timeline_event_tags) unless timeline_event_tags.nil?
if timeline_event.save(context: validation_context)
add_system_note(timeline_event)
@@ -63,15 +73,17 @@ def was_changed(timeline_event)
:none
end
- def update_timeline_event_tags(timeline_event, tag_updates)
+ def compute_tag_updates(timeline_event, tag_updates)
tag_updates = tag_updates.map(&:downcase)
already_assigned_tags = timeline_event.timeline_event_tags.pluck_names.map(&:downcase)
tags_to_remove = already_assigned_tags - tag_updates
tags_to_add = tag_updates - already_assigned_tags
- validate_tags(tags_to_add)
+ [tags_to_remove, tags_to_add]
+ end
+ def update_timeline_event_tags(timeline_event, tags_to_add, tags_to_remove)
remove_tag_links(timeline_event, tags_to_remove) if tags_to_remove.any?
create_tag_links(timeline_event, tags_to_add) if tags_to_add.any?
end
@@ -81,7 +93,7 @@ def remove_tag_links(timeline_event, tags_to_remove_names)
timeline_event
.timeline_event_tag_links
- .find_by_timeline_event_tag_id(timeline_event_tag_id: tags_to_remove_ids).delete_all
+ .by_tag_ids(tags_to_remove_ids).delete_all
end
def create_tag_links(timeline_event, tags_to_add_names)
@@ -102,14 +114,14 @@ def create_tag_links(timeline_event, tags_to_add_names)
end
def validate_tags(tags_to_add)
- defined_tags = timeline_event.project.incident_management_timeline_event_tags.by_names(tags_to_add)
-
- non_existing_tags = tags_to_add - defined_tags
-
- return if non_existing_tags.empty?
-
- raise Gitlab::Graphql::Errors::ArgumentError,
- "Following tags don't exist: #{non_existing_tags}"
+ defined_tags = timeline_event
+ .project
+ .incident_management_timeline_event_tags
+ .by_names(tags_to_add)
+ .pluck_names
+ .map(&:downcase)
+
+ tags_to_add - defined_tags
end
def allowed?
diff --git a/spec/graphql/mutations/incident_management/timeline_event/update_spec.rb b/spec/graphql/mutations/incident_management/timeline_event/update_spec.rb
index 7081fb7117e26b..317e8f5fcb6d0a 100644
--- a/spec/graphql/mutations/incident_management/timeline_event/update_spec.rb
+++ b/spec/graphql/mutations/incident_management/timeline_event/update_spec.rb
@@ -7,21 +7,33 @@
let_it_be(:reporter) { create(:user) }
let_it_be(:project) { create(:project) }
let_it_be(:incident) { create(:incident, project: project) }
+ let_it_be(:tag1) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 1') }
+ let_it_be(:tag2) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 2') }
let_it_be_with_reload(:timeline_event) do
create(:incident_management_timeline_event, project: project, incident: incident)
end
+ # Pre-attach a tag to the event
+ let_it_be(:tag_link1) do
+ create(:incident_management_timeline_event_tag_link,
+ timeline_event: timeline_event,
+ timeline_event_tag: tag1
+ )
+ end
+
let(:args) do
{
id: timeline_event_id,
note: note,
- occurred_at: occurred_at
+ occurred_at: occurred_at,
+ timeline_event_tag_names: tag_names
}
end
let(:note) { 'Updated Note' }
let(:timeline_event_id) { GitlabSchema.id_from_object(timeline_event).to_s }
let(:occurred_at) { 1.minute.ago }
+ let(:tag_names) { [] }
before do
project.add_developer(developer)
@@ -92,6 +104,36 @@
expect(resolve).to eq(timeline_event: nil, errors: ["Occurred at can't be blank"])
end
end
+
+ context 'when timeline event tag do not exist' do
+ let(:tag_names) { ['some other tag'] }
+
+ it 'does not update the timeline event' do
+ expect { resolve }.not_to change { timeline_event.reload.updated_at }
+ end
+
+ it 'responds with error' do
+ expect(resolve).to eq(timeline_event: nil, errors: ["Following tags don't exist: [\"some other tag\"]"])
+ end
+ end
+ end
+
+ context 'when timeline event tags are passed' do
+ let(:tag_names) { [tag2.name] }
+
+ it 'returns updated timeline event' do
+ expect(resolve).to eq(
+ timeline_event: timeline_event.reload,
+ errors: []
+ )
+ end
+
+ it 'removes tag1 and assigns tag2 to the event' do
+ response = resolve
+ timeline_event = response[:timeline_event]
+
+ expect(timeline_event.timeline_event_tags).to contain_exactly(tag2)
+ end
end
end
diff --git a/spec/models/incident_management/timeline_event_tag_link_spec.rb b/spec/models/incident_management/timeline_event_tag_link_spec.rb
index fe31a6604c14f7..65225337f037f4 100644
--- a/spec/models/incident_management/timeline_event_tag_link_spec.rb
+++ b/spec/models/incident_management/timeline_event_tag_link_spec.rb
@@ -7,4 +7,33 @@
it { is_expected.to belong_to(:timeline_event) }
it { is_expected.to belong_to(:timeline_event_tag) }
end
+
+ describe '#by_tag_ids scope' do
+ let_it_be(:project) { create(:project) }
+ let_it_be(:tag1) { create(:incident_management_timeline_event_tag, name: 'Test tag 1', project: project) }
+ let_it_be(:tag2) { create(:incident_management_timeline_event_tag, name: 'Test tag 2', project: project) }
+ let_it_be(:incident) { create(:incident, project: project) }
+
+ let_it_be(:timeline_event) do
+ create(:incident_management_timeline_event, project: project, incident: incident)
+ end
+
+ let_it_be(:tag_link1) do
+ create(:incident_management_timeline_event_tag_link,
+ timeline_event: timeline_event,
+ timeline_event_tag: tag1
+ )
+ end
+
+ let_it_be(:tag_link2) do
+ create(:incident_management_timeline_event_tag_link,
+ timeline_event: timeline_event,
+ timeline_event_tag: tag2
+ )
+ end
+
+ it 'returns the tag requested' do
+ expect(described_class.by_tag_ids(tag1.id)).to contain_exactly(tag_link1)
+ end
+ end
end
diff --git a/spec/requests/api/graphql/mutations/incident_management/timeline_event/update_spec.rb b/spec/requests/api/graphql/mutations/incident_management/timeline_event/update_spec.rb
index 542d51b990f674..207dd55d527011 100644
--- a/spec/requests/api/graphql/mutations/incident_management/timeline_event/update_spec.rb
+++ b/spec/requests/api/graphql/mutations/incident_management/timeline_event/update_spec.rb
@@ -8,18 +8,30 @@
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project) }
let_it_be(:incident) { create(:incident, project: project) }
+ let_it_be(:tag1) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 1') }
+ let_it_be(:tag2) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 2') }
let_it_be_with_reload(:timeline_event) do
create(:incident_management_timeline_event, incident: incident, project: project)
end
+ # Pre-attach a tag to the event
+ let_it_be(:tag_link1) do
+ create(:incident_management_timeline_event_tag_link,
+ timeline_event: timeline_event,
+ timeline_event_tag: tag1
+ )
+ end
+
let(:occurred_at) { 1.minute.ago.iso8601 }
let(:note) { 'Updated note' }
+ let(:tag_names) { [] }
let(:variables) do
{
id: timeline_event.to_global_id.to_s,
note: note,
- occurred_at: occurred_at
+ occurred_at: occurred_at,
+ timeline_event_tag_names: tag_names
}
end
@@ -33,6 +45,7 @@
author { id username }
updatedByUser { id username }
incident { id title }
+ timelineEventTags { nodes { name } }
note
noteHtml
occurredAt
@@ -71,6 +84,9 @@
'id' => incident.to_global_id.to_s,
'title' => incident.title
},
+ 'timelineEventTags' => {
+ 'nodes' => []
+ },
'note' => note,
'noteHtml' => timeline_event.note_html,
'occurredAt' => occurred_at,
@@ -85,4 +101,27 @@
it_behaves_like 'timeline event mutation responds with validation error',
error_message: 'Timeline text is too long (maximum is 280 characters)'
end
+
+ context 'when timeline event tag names are passed' do
+ context 'when tags exist' do
+ let(:tag_names) { [tag2.name] }
+
+ it 'removes tag1 and adds tag2' do
+ post_graphql_mutation(mutation, current_user: user)
+
+ timeline_event_response = mutation_response['timelineEvent']
+ tag_names = timeline_event_response['timelineEventTags']['nodes']
+
+ expect(response).to have_gitlab_http_status(:success)
+ expect(tag_names).to contain_exactly({ "name" => tag2.name })
+ end
+ end
+
+ context 'when tags do not exist' do
+ let(:tag_names) { ['some other tag'] }
+
+ it_behaves_like 'timeline event mutation responds with validation error',
+ error_message: "Following tags don't exist: [\"some other tag\"]"
+ end
+ end
end
diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb
index 2373a73e108673..f87a632731f2f7 100644
--- a/spec/services/incident_management/timeline_events/update_service_spec.rb
+++ b/spec/services/incident_management/timeline_events/update_service_spec.rb
@@ -142,6 +142,83 @@
it_behaves_like 'error response',
'You have insufficient permissions to manage timeline events for this incident'
end
+
+ context 'when timeline event tags are passed' do
+ context 'when they exist' do
+ let_it_be(:tag1) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 1') }
+ let_it_be(:tag2) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 2') }
+ let_it_be(:tag3) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 3') }
+
+ let!(:tag_link1) do
+ create(:incident_management_timeline_event_tag_link,
+ timeline_event: timeline_event,
+ timeline_event_tag: tag3
+ )
+ end
+
+ let(:params) do
+ {
+ note: 'Updated note',
+ occurred_at: occurred_at,
+ timeline_event_tag_names: [tag3.name, tag1.name]
+ }
+ end
+
+ it_behaves_like 'successful response'
+
+ it 'adds the new tag' do
+ expect { execute }.to change(timeline_event.timeline_event_tags, :count).by(1)
+ end
+
+ it 'adds the new tag link' do
+ expect { execute }.to change(IncidentManagement::TimelineEventTagLink, :count).by(1)
+ end
+
+ it 'returns the new tag in response' do
+ timeline_event = execute.payload[:timeline_event]
+
+ expect(timeline_event.timeline_event_tags.pluck_names).to contain_exactly(tag1.name, tag3.name)
+ end
+
+ context 'when tag is removed' do
+ let(:params) { { note: 'Updated note', occurred_at: occurred_at, timeline_event_tag_names: [tag2.name] } }
+
+ it_behaves_like 'successful response'
+
+ it 'adds the new tag and removes the old tag' do
+ # Since it adds a tag (+1) and removes old tag (-1) so next change in count in 0
+ expect { execute }.to change(timeline_event.timeline_event_tags, :count).by(0)
+ end
+
+ it 'adds the new tag link and removes the old tag link' do
+ # Since it adds a tag link (+1) and removes old tag link (-1) so next change in count in 0
+ expect { execute }.to change(IncidentManagement::TimelineEventTagLink, :count).by(0)
+ end
+
+ it 'returns the new tag and does not contain the old tag in response' do
+ timeline_event = execute.payload[:timeline_event]
+
+ expect(timeline_event.timeline_event_tags.pluck_names).to contain_exactly(tag2.name)
+ end
+ end
+ end
+
+ context 'when they do not exist' do
+ let(:params) do
+ {
+ note: 'Updated note 2',
+ occurred_at: occurred_at,
+ timeline_event_tag_names: ['non existing tag']
+ }
+ end
+
+ it_behaves_like 'error response', "Following tags don't exist: [\"non existing tag\"]"
+
+ it 'does not update the note' do
+ expect { execute }.not_to change { timeline_event.reload.note }
+ end
+ end
+ end
end
context 'when user does not have permissions' do
--
GitLab
From a7958c1b088e4bc6acb0820dbac3563ab3d8794d Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Thu, 17 Nov 2022 14:56:41 +0530
Subject: [PATCH 08/17] Remove passing event around in the service
---
.../timeline_events/update_service.rb | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index 0023655c4b814e..213c1c76c7453f 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -26,12 +26,12 @@ def execute
# We first update the tags, if any, as they return error for non-existing tags.
# And then we update the other attributes of timeline event.
unless timeline_event_tags.nil?
- tags_to_remove, tags_to_add = compute_tag_updates(timeline_event, timeline_event_tags)
+ tags_to_remove, tags_to_add = compute_tag_updates
non_existing_tags = validate_tags(tags_to_add)
return error("#{_("Following tags don't exist")}: #{non_existing_tags}") unless non_existing_tags.empty?
- update_timeline_event_tags(timeline_event, tags_to_add, tags_to_remove)
+ update_timeline_event_tags(tags_to_add, tags_to_remove)
end
timeline_event.assign_attributes(update_params)
@@ -73,8 +73,8 @@ def was_changed(timeline_event)
:none
end
- def compute_tag_updates(timeline_event, tag_updates)
- tag_updates = tag_updates.map(&:downcase)
+ def compute_tag_updates
+ tag_updates = timeline_event_tags.map(&:downcase)
already_assigned_tags = timeline_event.timeline_event_tags.pluck_names.map(&:downcase)
tags_to_remove = already_assigned_tags - tag_updates
@@ -83,12 +83,12 @@ def compute_tag_updates(timeline_event, tag_updates)
[tags_to_remove, tags_to_add]
end
- def update_timeline_event_tags(timeline_event, tags_to_add, tags_to_remove)
- remove_tag_links(timeline_event, tags_to_remove) if tags_to_remove.any?
- create_tag_links(timeline_event, tags_to_add) if tags_to_add.any?
+ def update_timeline_event_tags(tags_to_add, tags_to_remove)
+ remove_tag_links(tags_to_remove) if tags_to_remove.any?
+ create_tag_links(tags_to_add) if tags_to_add.any?
end
- def remove_tag_links(timeline_event, tags_to_remove_names)
+ def remove_tag_links(tags_to_remove_names)
tags_to_remove_ids = timeline_event.timeline_event_tags.by_names(tags_to_remove_names).tag_ids
timeline_event
@@ -96,7 +96,7 @@ def remove_tag_links(timeline_event, tags_to_remove_names)
.by_tag_ids(tags_to_remove_ids).delete_all
end
- def create_tag_links(timeline_event, tags_to_add_names)
+ def create_tag_links(tags_to_add_names)
tags_to_add_ids = timeline_event
.project
.incident_management_timeline_event_tags
--
GitLab
From 1c0a2b3166cfff34ba57863892253ebefe2f8ced Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Fri, 18 Nov 2022 12:16:21 +0530
Subject: [PATCH 09/17] Add transaction to the service
Add random case to the tag inputs in spec
---
.../timeline_events/update_service.rb | 16 +++++---
.../timeline_events/update_service_spec.rb | 40 +++++++++++++------
2 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index 213c1c76c7453f..bf264e3e45dbaa 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -23,18 +23,14 @@ def initialize(timeline_event, user, params)
def execute
return error_no_permissions unless allowed?
- # We first update the tags, if any, as they return error for non-existing tags.
- # And then we update the other attributes of timeline event.
unless timeline_event_tags.nil?
tags_to_remove, tags_to_add = compute_tag_updates
non_existing_tags = validate_tags(tags_to_add)
return error("#{_("Following tags don't exist")}: #{non_existing_tags}") unless non_existing_tags.empty?
-
- update_timeline_event_tags(tags_to_add, tags_to_remove)
end
- timeline_event.assign_attributes(update_params)
+ update_timeline_event_and_event_tags(tags_to_add, tags_to_remove)
if timeline_event.save(context: validation_context)
add_system_note(timeline_event)
@@ -50,6 +46,16 @@ def execute
attr_reader :timeline_event, :incident, :user, :note, :occurred_at, :validation_context, :timeline_event_tags
+ def update_timeline_event_and_event_tags(tags_to_add, tags_to_remove)
+ IncidentManagement::TimelineEvent.transaction do
+ IncidentManagement::TimelineEventTag.transaction do
+ update_timeline_event_tags(tags_to_add, tags_to_remove) unless timeline_event_tags.nil?
+
+ timeline_event.assign_attributes(update_params)
+ end
+ end
+ end
+
def update_params
{ updated_by_user: user, note: note, occurred_at: occurred_at }.compact
end
diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb
index f87a632731f2f7..8dc644006a77d9 100644
--- a/spec/services/incident_management/timeline_events/update_service_spec.rb
+++ b/spec/services/incident_management/timeline_events/update_service_spec.rb
@@ -13,6 +13,24 @@
let(:current_user) { user }
describe '#execute' do
+ shared_examples 'successful tag response' do
+ it_behaves_like 'successful response'
+
+ it 'adds the new tag' do
+ expect { execute }.to change(timeline_event.timeline_event_tags, :count).by(1)
+ end
+
+ it 'adds the new tag link' do
+ expect { execute }.to change(IncidentManagement::TimelineEventTagLink, :count).by(1)
+ end
+
+ it 'returns the new tag in response' do
+ timeline_event = execute.payload[:timeline_event]
+
+ expect(timeline_event.timeline_event_tags.pluck_names).to contain_exactly(tag1.name, tag3.name)
+ end
+ end
+
shared_examples 'successful response' do
it 'responds with success', :aggregate_failures do
expect(execute).to be_success
@@ -164,20 +182,18 @@
}
end
- it_behaves_like 'successful response'
-
- it 'adds the new tag' do
- expect { execute }.to change(timeline_event.timeline_event_tags, :count).by(1)
- end
+ it_behaves_like 'successful tag response'
- it 'adds the new tag link' do
- expect { execute }.to change(IncidentManagement::TimelineEventTagLink, :count).by(1)
- end
-
- it 'returns the new tag in response' do
- timeline_event = execute.payload[:timeline_event]
+ context 'when tag name is of random case' do
+ let(:params) do
+ {
+ note: 'Updated note',
+ occurred_at: occurred_at,
+ timeline_event_tag_names: ['tAg 3', 'TaG 1']
+ }
+ end
- expect(timeline_event.timeline_event_tags.pluck_names).to contain_exactly(tag1.name, tag3.name)
+ it_behaves_like 'successful tag response'
end
context 'when tag is removed' do
--
GitLab
From 54caba302b7127a649107ddd4ea2edebd6d5d11d Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Fri, 18 Nov 2022 13:57:27 +0530
Subject: [PATCH 10/17] Add more specs, make update as a transaction
---
.../timeline_events/update_service.rb | 39 ++++++++-----------
.../timeline_events/update_service_spec.rb | 8 ++--
2 files changed, 21 insertions(+), 26 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index bf264e3e45dbaa..e789a1317858f6 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -25,12 +25,17 @@ def execute
unless timeline_event_tags.nil?
tags_to_remove, tags_to_add = compute_tag_updates
- non_existing_tags = validate_tags(tags_to_add)
+ defined_tags = timeline_event
+ .project
+ .incident_management_timeline_event_tags
+ .by_names(tags_to_add)
+
+ non_existing_tags = validate_tags(tags_to_add, defined_tags)
return error("#{_("Following tags don't exist")}: #{non_existing_tags}") unless non_existing_tags.empty?
end
- update_timeline_event_and_event_tags(tags_to_add, tags_to_remove)
+ update_timeline_event_and_event_tags(tags_to_add, tags_to_remove, defined_tags)
if timeline_event.save(context: validation_context)
add_system_note(timeline_event)
@@ -46,13 +51,11 @@ def execute
attr_reader :timeline_event, :incident, :user, :note, :occurred_at, :validation_context, :timeline_event_tags
- def update_timeline_event_and_event_tags(tags_to_add, tags_to_remove)
- IncidentManagement::TimelineEvent.transaction do
- IncidentManagement::TimelineEventTag.transaction do
- update_timeline_event_tags(tags_to_add, tags_to_remove) unless timeline_event_tags.nil?
+ def update_timeline_event_and_event_tags(tags_to_add, tags_to_remove, defined_tags)
+ ApplicationRecord.transaction do
+ update_timeline_event_tags(tags_to_add, tags_to_remove, defined_tags) unless timeline_event_tags.nil?
- timeline_event.assign_attributes(update_params)
- end
+ timeline_event.assign_attributes(update_params)
end
end
@@ -89,9 +92,9 @@ def compute_tag_updates
[tags_to_remove, tags_to_add]
end
- def update_timeline_event_tags(tags_to_add, tags_to_remove)
+ def update_timeline_event_tags(tags_to_add, tags_to_remove, defined_tags)
remove_tag_links(tags_to_remove) if tags_to_remove.any?
- create_tag_links(tags_to_add) if tags_to_add.any?
+ create_tag_links(tags_to_add, defined_tags) if tags_to_add.any?
end
def remove_tag_links(tags_to_remove_names)
@@ -102,11 +105,8 @@ def remove_tag_links(tags_to_remove_names)
.by_tag_ids(tags_to_remove_ids).delete_all
end
- def create_tag_links(tags_to_add_names)
- tags_to_add_ids = timeline_event
- .project
- .incident_management_timeline_event_tags
- .by_names(tags_to_add_names).tag_ids
+ def create_tag_links(tags_to_add_names, defined_tags)
+ tags_to_add_ids = defined_tags.tag_ids
tag_links = tags_to_add_ids.map do |tag_id|
{
@@ -119,13 +119,8 @@ def create_tag_links(tags_to_add_names)
IncidentManagement::TimelineEventTagLink.insert_all(tag_links) if tag_links.any?
end
- def validate_tags(tags_to_add)
- defined_tags = timeline_event
- .project
- .incident_management_timeline_event_tags
- .by_names(tags_to_add)
- .pluck_names
- .map(&:downcase)
+ def validate_tags(tags_to_add, defined_tags)
+ defined_tags = defined_tags.pluck_names.map(&:downcase)
tags_to_add - defined_tags
end
diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb
index 8dc644006a77d9..ff4105f5ded20a 100644
--- a/spec/services/incident_management/timeline_events/update_service_spec.rb
+++ b/spec/services/incident_management/timeline_events/update_service_spec.rb
@@ -17,11 +17,11 @@
it_behaves_like 'successful response'
it 'adds the new tag' do
- expect { execute }.to change(timeline_event.timeline_event_tags, :count).by(1)
+ expect { execute }.to change { timeline_event.timeline_event_tags.count }.by(1)
end
it 'adds the new tag link' do
- expect { execute }.to change(IncidentManagement::TimelineEventTagLink, :count).by(1)
+ expect { execute }.to change { IncidentManagement::TimelineEventTagLink.count }.by(1)
end
it 'returns the new tag in response' do
@@ -203,12 +203,12 @@
it 'adds the new tag and removes the old tag' do
# Since it adds a tag (+1) and removes old tag (-1) so next change in count in 0
- expect { execute }.to change(timeline_event.timeline_event_tags, :count).by(0)
+ expect { execute }.to change { timeline_event.timeline_event_tags.count }.by(0)
end
it 'adds the new tag link and removes the old tag link' do
# Since it adds a tag link (+1) and removes old tag link (-1) so next change in count in 0
- expect { execute }.to change(IncidentManagement::TimelineEventTagLink, :count).by(0)
+ expect { execute }.to change { IncidentManagement::TimelineEventTagLink.count }.by(0)
end
it 'returns the new tag and does not contain the old tag in response' do
--
GitLab
From fb2fe66182cacf081f096377af836966ba69a31c Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Fri, 18 Nov 2022 14:31:24 +0530
Subject: [PATCH 11/17] Add more specs to check the transaction
---
.../timeline_events/update_service.rb | 10 ++++-
.../timeline_events/update_service_spec.rb | 45 ++++++++++++-------
2 files changed, 38 insertions(+), 17 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index e789a1317858f6..583697937d4c6e 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -35,9 +35,13 @@ def execute
return error("#{_("Following tags don't exist")}: #{non_existing_tags}") unless non_existing_tags.empty?
end
- update_timeline_event_and_event_tags(tags_to_add, tags_to_remove, defined_tags)
+ begin
+ timeline_event_saved = update_timeline_event_and_event_tags(tags_to_add, tags_to_remove, defined_tags)
+ rescue ActiveRecord::RecordInvalid
+ error_in_save(timeline_event)
+ end
- if timeline_event.save(context: validation_context)
+ if timeline_event_saved
add_system_note(timeline_event)
track_usage_event(:incident_management_timeline_event_edited, user.id)
@@ -56,6 +60,8 @@ def update_timeline_event_and_event_tags(tags_to_add, tags_to_remove, defined_ta
update_timeline_event_tags(tags_to_add, tags_to_remove, defined_tags) unless timeline_event_tags.nil?
timeline_event.assign_attributes(update_params)
+
+ timeline_event.save!(context: validation_context)
end
end
diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb
index ff4105f5ded20a..96d8f2f6143206 100644
--- a/spec/services/incident_management/timeline_events/update_service_spec.rb
+++ b/spec/services/incident_management/timeline_events/update_service_spec.rb
@@ -6,6 +6,16 @@
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project) }
let_it_be(:incident) { create(:incident, project: project) }
+ let_it_be(:tag1) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 1') }
+ let_it_be(:tag2) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 2') }
+ let_it_be(:tag3) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 3') }
+
+ let!(:tag_link1) do
+ create(:incident_management_timeline_event_tag_link,
+ timeline_event: timeline_event,
+ timeline_event_tag: tag3
+ )
+ end
let!(:timeline_event) { create(:incident_management_timeline_event, project: project, incident: incident) }
let(:occurred_at) { 1.minute.ago }
@@ -85,7 +95,7 @@
it_behaves_like 'passing the correct was_changed value', :occurred_at_and_note
context 'when note is nil' do
- let(:params) { { occurred_at: occurred_at } }
+ let(:params) { { occurred_at: occurred_at, timeline_event_tag_names: [tag3.name, tag2.name] } }
it_behaves_like 'successful response'
it_behaves_like 'passing the correct was_changed value', :occurred_at
@@ -97,18 +107,30 @@
it 'updates occurred_at' do
expect { execute }.to change { timeline_event.occurred_at }.to(params[:occurred_at])
end
+
+ it 'updates the tags' do
+ expect { execute }.to change { timeline_event.timeline_event_tags.count }.by(1)
+ end
end
context 'when note is blank' do
- let(:params) { { note: '', occurred_at: occurred_at } }
+ let(:params) { { note: '', occurred_at: occurred_at, timeline_event_tag_names: [tag3.name, tag2.name] } }
it_behaves_like 'error response', "Timeline text can't be blank"
+
+ it 'does not add the tags as it rollsback the transaction' do
+ expect { execute }.not_to change { timeline_event.timeline_event_tags.count }
+ end
end
context 'when note is more than 280 characters long' do
- let(:params) { { note: 'n' * 281, occurred_at: occurred_at } }
+ let(:params) { { note: 'n' * 281, occurred_at: occurred_at, timeline_event_tag_names: [tag3.name, tag2.name] } }
it_behaves_like 'error response', 'Timeline text is too long (maximum is 280 characters)'
+
+ it 'does not add the tags as it rollsback the transaction' do
+ expect { execute }.not_to change { timeline_event.timeline_event_tags.count }
+ end
end
context 'when occurred_at is nil' do
@@ -127,9 +149,13 @@
end
context 'when occurred_at is blank' do
- let(:params) { { note: 'Updated note', occurred_at: '' } }
+ let(:params) { { note: 'Updated note', occurred_at: '', timeline_event_tag_names: [tag3.name, tag2.name] } }
it_behaves_like 'error response', "Occurred at can't be blank"
+
+ it 'does not add the tags as it rollsback the transaction' do
+ expect { execute }.not_to change { timeline_event.timeline_event_tags.count }
+ end
end
context 'when both occurred_at and note is nil' do
@@ -163,17 +189,6 @@
context 'when timeline event tags are passed' do
context 'when they exist' do
- let_it_be(:tag1) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 1') }
- let_it_be(:tag2) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 2') }
- let_it_be(:tag3) { create(:incident_management_timeline_event_tag, project: project, name: 'Tag 3') }
-
- let!(:tag_link1) do
- create(:incident_management_timeline_event_tag_link,
- timeline_event: timeline_event,
- timeline_event_tag: tag3
- )
- end
-
let(:params) do
{
note: 'Updated note',
--
GitLab
From 8eb01bf74daeee758a4f2545d77f77b0f3c3cc50 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Wed, 23 Nov 2022 18:09:17 +0530
Subject: [PATCH 12/17] Add db reviewer and backend suggestions
---
.../timeline_events/update_service.rb | 53 +++++--------------
.../timeline_event_tag_spec.rb | 18 +++++--
2 files changed, 27 insertions(+), 44 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index 583697937d4c6e..3996d76c4e63c1 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -24,19 +24,18 @@ def execute
return error_no_permissions unless allowed?
unless timeline_event_tags.nil?
- tags_to_remove, tags_to_add = compute_tag_updates
+ tags_to_add = compute_tags_to_add
defined_tags = timeline_event
.project
.incident_management_timeline_event_tags
- .by_names(tags_to_add)
non_existing_tags = validate_tags(tags_to_add, defined_tags)
- return error("#{_("Following tags don't exist")}: #{non_existing_tags}") unless non_existing_tags.empty?
+ return error("#{_("Following tags don't exist")}: #{non_existing_tags}") if non_existing_tags.any?
end
begin
- timeline_event_saved = update_timeline_event_and_event_tags(tags_to_add, tags_to_remove, defined_tags)
+ timeline_event_saved = update_timeline_event_and_event_tags(defined_tags)
rescue ActiveRecord::RecordInvalid
error_in_save(timeline_event)
end
@@ -55,9 +54,12 @@ def execute
attr_reader :timeline_event, :incident, :user, :note, :occurred_at, :validation_context, :timeline_event_tags
- def update_timeline_event_and_event_tags(tags_to_add, tags_to_remove, defined_tags)
+ def update_timeline_event_and_event_tags(defined_tags)
ApplicationRecord.transaction do
- update_timeline_event_tags(tags_to_add, tags_to_remove, defined_tags) unless timeline_event_tags.nil?
+ unless timeline_event_tags.nil?
+ tags = defined_tags.by_names(timeline_event_tags.map(&:downcase))
+ timeline_event.timeline_event_tags = tags
+ end
timeline_event.assign_attributes(update_params)
@@ -88,45 +90,18 @@ def was_changed(timeline_event)
:none
end
- def compute_tag_updates
+ def compute_tags_to_add
tag_updates = timeline_event_tags.map(&:downcase)
already_assigned_tags = timeline_event.timeline_event_tags.pluck_names.map(&:downcase)
- tags_to_remove = already_assigned_tags - tag_updates
- tags_to_add = tag_updates - already_assigned_tags
-
- [tags_to_remove, tags_to_add]
- end
-
- def update_timeline_event_tags(tags_to_add, tags_to_remove, defined_tags)
- remove_tag_links(tags_to_remove) if tags_to_remove.any?
- create_tag_links(tags_to_add, defined_tags) if tags_to_add.any?
- end
-
- def remove_tag_links(tags_to_remove_names)
- tags_to_remove_ids = timeline_event.timeline_event_tags.by_names(tags_to_remove_names).tag_ids
-
- timeline_event
- .timeline_event_tag_links
- .by_tag_ids(tags_to_remove_ids).delete_all
- end
-
- def create_tag_links(tags_to_add_names, defined_tags)
- tags_to_add_ids = defined_tags.tag_ids
-
- tag_links = tags_to_add_ids.map do |tag_id|
- {
- timeline_event_id: timeline_event.id,
- timeline_event_tag_id: tag_id,
- created_at: DateTime.current
- }
- end
-
- IncidentManagement::TimelineEventTagLink.insert_all(tag_links) if tag_links.any?
+ tag_updates - already_assigned_tags
end
def validate_tags(tags_to_add, defined_tags)
- defined_tags = defined_tags.pluck_names.map(&:downcase)
+ defined_tags = defined_tags
+ .by_names(tags_to_add)
+ .pluck_names
+ .map(&:downcase)
tags_to_add - defined_tags
end
diff --git a/spec/models/incident_management/timeline_event_tag_spec.rb b/spec/models/incident_management/timeline_event_tag_spec.rb
index 1ec4fa30fb5f4a..0bc198eeec55c7 100644
--- a/spec/models/incident_management/timeline_event_tag_spec.rb
+++ b/spec/models/incident_management/timeline_event_tag_spec.rb
@@ -26,12 +26,20 @@
it { is_expected.not_to allow_value('s' * 256).for(:name) }
end
- describe '.pluck_names' do
- it 'returns the names of the tags' do
- tag1 = create(:incident_management_timeline_event_tag)
- tag2 = create(:incident_management_timeline_event_tag)
+ describe 'model methods' do
+ let_it_be(:tag1) { create(:incident_management_timeline_event_tag) }
+ let_it_be(:tag2) { create(:incident_management_timeline_event_tag) }
- expect(described_class.pluck_names).to contain_exactly(tag1.name, tag2.name)
+ context 'when .pluck_names is called' do
+ it 'returns the names of the tags' do
+ expect(described_class.pluck_names).to contain_exactly(tag1.name, tag2.name)
+ end
+ end
+
+ context 'when .tag_ids is called' do
+ it 'returns the ids of the tags' do
+ expect(described_class.tag_ids).to contain_exactly(tag1.id, tag2.id)
+ end
end
end
--
GitLab
From 4215083f6d7786db2cc9a94e6e6e32e9151af6eb Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Wed, 23 Nov 2022 18:18:37 +0530
Subject: [PATCH 13/17] Add one spec for removing all tags
---
.../timeline_events/update_service_spec.rb | 20 +++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/spec/services/incident_management/timeline_events/update_service_spec.rb b/spec/services/incident_management/timeline_events/update_service_spec.rb
index 96d8f2f6143206..8ccc6ded6d05e8 100644
--- a/spec/services/incident_management/timeline_events/update_service_spec.rb
+++ b/spec/services/incident_management/timeline_events/update_service_spec.rb
@@ -232,6 +232,26 @@
expect(timeline_event.timeline_event_tags.pluck_names).to contain_exactly(tag2.name)
end
end
+
+ context 'when all assigned tags are removed' do
+ let(:params) { { note: 'Updated note', occurred_at: occurred_at, timeline_event_tag_names: [] } }
+
+ it_behaves_like 'successful response'
+
+ it 'removes all the assigned tags' do
+ expect { execute }.to change { timeline_event.timeline_event_tags.count }.by(-1)
+ end
+
+ it 'removes all the assigned tag links' do
+ expect { execute }.to change { IncidentManagement::TimelineEventTagLink.count }.by(-1)
+ end
+
+ it 'does not contain any tags in response' do
+ timeline_event = execute.payload[:timeline_event]
+
+ expect(timeline_event.timeline_event_tags.pluck_names).to be_empty
+ end
+ end
end
context 'when they do not exist' do
--
GitLab
From 57cd330008b86b5867f1194694c82828d034fe12 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Wed, 23 Nov 2022 18:26:57 +0530
Subject: [PATCH 14/17] Remove redandunt method in model
---
.../incident_management/timeline_event_tag.rb | 4 ----
.../timeline_event_tag_spec.rb | 18 +++++-------------
2 files changed, 5 insertions(+), 17 deletions(-)
diff --git a/app/models/incident_management/timeline_event_tag.rb b/app/models/incident_management/timeline_event_tag.rb
index 59a3ed2d9f9b0d..d1e3fbc2a6a8b8 100644
--- a/app/models/incident_management/timeline_event_tag.rb
+++ b/app/models/incident_management/timeline_event_tag.rb
@@ -25,9 +25,5 @@ class TimelineEventTag < ApplicationRecord
def self.pluck_names
pluck(:name)
end
-
- def self.tag_ids
- pluck(:id)
- end
end
end
diff --git a/spec/models/incident_management/timeline_event_tag_spec.rb b/spec/models/incident_management/timeline_event_tag_spec.rb
index 0bc198eeec55c7..1ec4fa30fb5f4a 100644
--- a/spec/models/incident_management/timeline_event_tag_spec.rb
+++ b/spec/models/incident_management/timeline_event_tag_spec.rb
@@ -26,20 +26,12 @@
it { is_expected.not_to allow_value('s' * 256).for(:name) }
end
- describe 'model methods' do
- let_it_be(:tag1) { create(:incident_management_timeline_event_tag) }
- let_it_be(:tag2) { create(:incident_management_timeline_event_tag) }
+ describe '.pluck_names' do
+ it 'returns the names of the tags' do
+ tag1 = create(:incident_management_timeline_event_tag)
+ tag2 = create(:incident_management_timeline_event_tag)
- context 'when .pluck_names is called' do
- it 'returns the names of the tags' do
- expect(described_class.pluck_names).to contain_exactly(tag1.name, tag2.name)
- end
- end
-
- context 'when .tag_ids is called' do
- it 'returns the ids of the tags' do
- expect(described_class.tag_ids).to contain_exactly(tag1.id, tag2.id)
- end
+ expect(described_class.pluck_names).to contain_exactly(tag1.name, tag2.name)
end
end
--
GitLab
From ce26c6a044dc1921f2944d2bf32da3c8c9e59943 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Wed, 23 Nov 2022 18:31:33 +0530
Subject: [PATCH 15/17] Remove tag ids scope for tag links model
---
app/models/incident_management/timeline_event_tag_link.rb | 2 --
1 file changed, 2 deletions(-)
diff --git a/app/models/incident_management/timeline_event_tag_link.rb b/app/models/incident_management/timeline_event_tag_link.rb
index 23a2bd65a7f6ca..912339717a848f 100644
--- a/app/models/incident_management/timeline_event_tag_link.rb
+++ b/app/models/incident_management/timeline_event_tag_link.rb
@@ -7,7 +7,5 @@ class TimelineEventTagLink < ApplicationRecord
belongs_to :timeline_event_tag, class_name: 'IncidentManagement::TimelineEventTag'
belongs_to :timeline_event, class_name: 'IncidentManagement::TimelineEvent'
-
- scope :by_tag_ids, -> (ids) { where(timeline_event_tag_id: ids) }
end
end
--
GitLab
From fa73922299922dc1b3eee2e41ada3e9b453c4aca Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Wed, 23 Nov 2022 19:06:07 +0530
Subject: [PATCH 16/17] Remove tag link specs for tag ids scope
---
.../timeline_event_tag_link_spec.rb | 29 -------------------
1 file changed, 29 deletions(-)
diff --git a/spec/models/incident_management/timeline_event_tag_link_spec.rb b/spec/models/incident_management/timeline_event_tag_link_spec.rb
index 65225337f037f4..fe31a6604c14f7 100644
--- a/spec/models/incident_management/timeline_event_tag_link_spec.rb
+++ b/spec/models/incident_management/timeline_event_tag_link_spec.rb
@@ -7,33 +7,4 @@
it { is_expected.to belong_to(:timeline_event) }
it { is_expected.to belong_to(:timeline_event_tag) }
end
-
- describe '#by_tag_ids scope' do
- let_it_be(:project) { create(:project) }
- let_it_be(:tag1) { create(:incident_management_timeline_event_tag, name: 'Test tag 1', project: project) }
- let_it_be(:tag2) { create(:incident_management_timeline_event_tag, name: 'Test tag 2', project: project) }
- let_it_be(:incident) { create(:incident, project: project) }
-
- let_it_be(:timeline_event) do
- create(:incident_management_timeline_event, project: project, incident: incident)
- end
-
- let_it_be(:tag_link1) do
- create(:incident_management_timeline_event_tag_link,
- timeline_event: timeline_event,
- timeline_event_tag: tag1
- )
- end
-
- let_it_be(:tag_link2) do
- create(:incident_management_timeline_event_tag_link,
- timeline_event: timeline_event,
- timeline_event_tag: tag2
- )
- end
-
- it 'returns the tag requested' do
- expect(described_class.by_tag_ids(tag1.id)).to contain_exactly(tag_link1)
- end
- end
end
--
GitLab
From 49f22442fbb2a4d5e79bfc10daa6e73f44da86d6 Mon Sep 17 00:00:00 2001
From: Rajendra Kadam <rkadam@gitlab.com>
Date: Thu, 24 Nov 2022 10:01:28 +0530
Subject: [PATCH 17/17] Apply maintainer suggestions - 2
---
.../timeline_events/update_service.rb | 35 ++++++-------------
1 file changed, 10 insertions(+), 25 deletions(-)
diff --git a/app/services/incident_management/timeline_events/update_service.rb b/app/services/incident_management/timeline_events/update_service.rb
index 3996d76c4e63c1..0e424f86e1a5ee 100644
--- a/app/services/incident_management/timeline_events/update_service.rb
+++ b/app/services/incident_management/timeline_events/update_service.rb
@@ -24,18 +24,18 @@ def execute
return error_no_permissions unless allowed?
unless timeline_event_tags.nil?
- tags_to_add = compute_tags_to_add
- defined_tags = timeline_event
- .project
- .incident_management_timeline_event_tags
+ new_tags = timeline_event
+ .project
+ .incident_management_timeline_event_tags
+ .by_names(timeline_event_tags)
- non_existing_tags = validate_tags(tags_to_add, defined_tags)
+ non_existing_tags = validate_tags(new_tags)
return error("#{_("Following tags don't exist")}: #{non_existing_tags}") if non_existing_tags.any?
end
begin
- timeline_event_saved = update_timeline_event_and_event_tags(defined_tags)
+ timeline_event_saved = update_timeline_event_and_event_tags(new_tags)
rescue ActiveRecord::RecordInvalid
error_in_save(timeline_event)
end
@@ -54,12 +54,9 @@ def execute
attr_reader :timeline_event, :incident, :user, :note, :occurred_at, :validation_context, :timeline_event_tags
- def update_timeline_event_and_event_tags(defined_tags)
+ def update_timeline_event_and_event_tags(new_tags)
ApplicationRecord.transaction do
- unless timeline_event_tags.nil?
- tags = defined_tags.by_names(timeline_event_tags.map(&:downcase))
- timeline_event.timeline_event_tags = tags
- end
+ timeline_event.timeline_event_tags = new_tags unless timeline_event_tags.nil?
timeline_event.assign_attributes(update_params)
@@ -90,20 +87,8 @@ def was_changed(timeline_event)
:none
end
- def compute_tags_to_add
- tag_updates = timeline_event_tags.map(&:downcase)
- already_assigned_tags = timeline_event.timeline_event_tags.pluck_names.map(&:downcase)
-
- tag_updates - already_assigned_tags
- end
-
- def validate_tags(tags_to_add, defined_tags)
- defined_tags = defined_tags
- .by_names(tags_to_add)
- .pluck_names
- .map(&:downcase)
-
- tags_to_add - defined_tags
+ def validate_tags(new_tags)
+ timeline_event_tags.map(&:downcase) - new_tags.map(&:name).map(&:downcase)
end
def allowed?
--
GitLab