From ebffd39ec360b3baa1c7b0a49b950d8f52d7f8af Mon Sep 17 00:00:00 2001
From: Alexandru Croitor <acroitor@gitlab.com>
Date: Fri, 28 Jun 2024 14:58:53 +0000
Subject: [PATCH] Add description version unification for epic and epic work
item
This allows handling epic and epic work item description_versions
transparently between epic and its corresponding epic work item.
This will also allow us to easier unify epic and epic work item
system notes related todescription version changes and later on
drop legacy epic support while keeping the data intergrity.
re https://gitlab.com/gitlab-org/gitlab/-/issues/443540
---
app/models/concerns/versioned_description.rb | 7 +-
app/models/description_version.rb | 2 +
app/models/note.rb | 7 +-
.../ee/types/description_version_type.rb | 7 +-
.../concerns/work_items/epic_as_work_item.rb | 5 +
.../description_versions.rb | 43 ++++++
ee/app/models/ee/description_version.rb | 12 +-
ee/app/models/ee/group.rb | 4 +
ee/app/models/ee/note.rb | 4 +
ee/app/models/ee/work_item.rb | 5 -
ee/app/policies/epic_policy.rb | 1 +
.../concerns/epics/sync_as_work_item.rb | 4 +-
.../concerns/work_items/sync_as_epic.rb | 2 +-
.../epic_and_work_item_notes_unification.yml | 9 ++
.../groups/epics_controller_spec.rb | 14 ++
.../groups/work_items_controller_spec.rb | 12 ++
.../projects/issues_controller_spec.rb | 6 +
.../merge_requests_controller_spec.rb | 3 +
ee/spec/models/ee/description_version_spec.rb | 3 +-
ee/spec/models/epic_spec.rb | 89 +++++++++----
ee/spec/models/work_item_spec.rb | 11 +-
ee/spec/policies/epic_policy_spec.rb | 16 +--
.../ee/issuable/destroy_service_spec.rb | 124 +++++++++++++-----
ee/spec/services/epics/update_service_spec.rb | 9 +-
.../work_items/update_service_spec.rb | 14 ++
...escription_diff_actions_shared_examples.rb | 4 -
spec/lib/gitlab/import_export/all_models.yml | 2 +
27 files changed, 323 insertions(+), 96 deletions(-)
create mode 100644 ee/app/models/concerns/work_items/unified_associations/description_versions.rb
create mode 100644 ee/config/feature_flags/wip/epic_and_work_item_notes_unification.yml
diff --git a/app/models/concerns/versioned_description.rb b/app/models/concerns/versioned_description.rb
index dff7983477a2a..1270529fdfe56 100644
--- a/app/models/concerns/versioned_description.rb
+++ b/app/models/concerns/versioned_description.rb
@@ -5,10 +5,11 @@ module VersionedDescription
included do
attr_accessor :saved_description_version
+ attr_accessor :skip_description_version
has_many :description_versions
- after_update :save_description_version, unless: :skip_description_version?
+ after_update :save_description_version, unless: :skip_description_version
end
private
@@ -27,8 +28,4 @@ def save_description_version
self.saved_description_version = description_versions.create!(description: description)
end
-
- def skip_description_version?
- false
- end
end
diff --git a/app/models/description_version.rb b/app/models/description_version.rb
index 71c16621c9e24..54dea117a9c62 100644
--- a/app/models/description_version.rb
+++ b/app/models/description_version.rb
@@ -1,6 +1,8 @@
# frozen_string_literal: true
class DescriptionVersion < ApplicationRecord
+ include FromUnion
+
belongs_to :issue
belongs_to :merge_request
diff --git a/app/models/note.rb b/app/models/note.rb
index cb47449d29812..5d5a8e3071c77 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -133,8 +133,11 @@ class Note < ApplicationRecord
scope :inc_note_diff_file, -> { includes(:note_diff_file) }
scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) }
scope :inc_relations_for_view, ->(noteable = nil) do
- relations = [{ project: :group }, { author: :status }, :updated_by, :resolved_by,
- :award_emoji, :note_metadata, { system_note_metadata: :description_version }, :suggestions]
+ relations = [
+ { project: :group }, { author: :status }, :updated_by, :resolved_by,
+ :award_emoji, :note_metadata, :suggestions,
+ { system_note_metadata: { description_version: [:issue, :merge_request] } }
+ ]
if noteable.nil? || DiffNote.noteable_types.include?(noteable.class.name)
relations += [:note_diff_file, :diff_note_positions]
diff --git a/ee/app/graphql/ee/types/description_version_type.rb b/ee/app/graphql/ee/types/description_version_type.rb
index 34b6495634bfb..c551e1cae472a 100644
--- a/ee/app/graphql/ee/types/description_version_type.rb
+++ b/ee/app/graphql/ee/types/description_version_type.rb
@@ -33,7 +33,6 @@ module DescriptionVersionType
deprecated: { reason: DEPRECATION_REASON, milestone: '15.7' }
field :deleted, GraphQL::Types::Boolean,
null: true,
- method: :deleted?,
description: 'Whether description version associated to the note metadata is deleted.',
deprecated: { reason: DEPRECATION_REASON, milestone: '15.7' }
@@ -53,7 +52,11 @@ def can_delete
return unless object.issuable.present? && description_diff_available?
rule = "admin_#{object.issuable.class.to_ability_name}"
- Ability.allowed?(current_user, rule, object.issuable.resource_parent)
+ Ability.allowed?(current_user, rule, object.issuable)
+ end
+
+ def deleted
+ object.deleted_at?
end
end
diff --git a/ee/app/models/concerns/work_items/epic_as_work_item.rb b/ee/app/models/concerns/work_items/epic_as_work_item.rb
index 08dc4c4376693..b24566710f835 100644
--- a/ee/app/models/concerns/work_items/epic_as_work_item.rb
+++ b/ee/app/models/concerns/work_items/epic_as_work_item.rb
@@ -8,6 +8,7 @@ module EpicAsWorkItem
include Gitlab::Utils::StrongMemoize
include ::WorkItems::UnifiedAssociations::Labels
include ::WorkItems::UnifiedAssociations::AwardEmoji
+ include ::WorkItems::UnifiedAssociations::DescriptionVersions
# this overrides the scope in Issuable by removing the labels association from it as labels are now preloaded
# by loading labels for epic and for epic work item
@@ -37,6 +38,10 @@ def unified_associations?
def labels_unification_enabled?
unified_associations? && container&.epic_and_work_item_labels_unification_enabled?
end
+
+ def notes_unification_enabled?
+ unified_associations? && container&.epic_and_work_item_notes_unification_enabled?
+ end
end
end
end
diff --git a/ee/app/models/concerns/work_items/unified_associations/description_versions.rb b/ee/app/models/concerns/work_items/unified_associations/description_versions.rb
new file mode 100644
index 0000000000000..a35db912c6732
--- /dev/null
+++ b/ee/app/models/concerns/work_items/unified_associations/description_versions.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module WorkItems
+ module UnifiedAssociations
+ module DescriptionVersions
+ extend ActiveSupport::Concern
+
+ included do
+ # rubocop:disable Rails/InverseOf -- this is temporary and polymorphic so no inverse for now
+ has_many :own_description_versions, class_name: 'DescriptionVersion',
+ foreign_key: "#{base_class.name.underscore}_id"
+ has_many :description_versions, foreign_key: "#{base_class.name.underscore}_id" do
+ def load_target
+ return super unless proxy_association.owner.notes_unification_enabled?
+
+ proxy_association.target = scope.to_a unless proxy_association.loaded?
+
+ proxy_association.loaded!
+ proxy_association.target
+ end
+
+ def scope
+ DescriptionVersion.from_union(
+ [
+ proxy_association.owner.sync_object&.own_description_versions || DescriptionVersion.none,
+ proxy_association.owner.own_description_versions
+ ],
+ remove_duplicates: true
+ )
+ end
+
+ def find(*args)
+ return super unless proxy_association.owner.notes_unification_enabled?
+ return super if block_given?
+
+ scope.find(*args)
+ end
+ end
+ # rubocop:enable Rails/InverseOf
+ end
+ end
+ end
+end
diff --git a/ee/app/models/ee/description_version.rb b/ee/app/models/ee/description_version.rb
index 131d9fc7d1cee..fb4a723001d39 100644
--- a/ee/app/models/ee/description_version.rb
+++ b/ee/app/models/ee/description_version.rb
@@ -6,6 +6,7 @@ module DescriptionVersion
prepended do
belongs_to :epic
+ belongs_to :work_item, class_name: 'WorkItem', foreign_key: :issue_id # rubocop:disable Rails/InverseOf -- temporary
# This scope is using `deleted_at` column which is not indexed.
# Prevent using it in not scoped contexts.
@@ -19,7 +20,7 @@ def issuable_attrs
end
def issuable
- epic || super
+ epic || work_item || super
end
def previous_version
@@ -38,9 +39,10 @@ def delete!(start_id: nil)
description_versions =
issuable_description_versions.where('id BETWEEN ? AND ?', start_id, self.id)
- description_versions.update_all(deleted_at: Time.current)
+ ::DescriptionVersion.id_in(description_versions).update_all(deleted_at: Time.current)
issuable&.broadcast_notes_changed
+ issuable.sync_object.broadcast_notes_changed if issuable&.try(:sync_object).present?
end
def deleted?
@@ -50,11 +52,7 @@ def deleted?
private
def issuable_description_versions
- self.class.where(
- issue_id: issue_id,
- merge_request_id: merge_request_id,
- epic_id: epic_id
- )
+ issuable.description_versions
end
end
end
diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb
index d1aa81e192e35..282b24b32c2b5 100644
--- a/ee/app/models/ee/group.rb
+++ b/ee/app/models/ee/group.rb
@@ -265,6 +265,10 @@ def epic_and_work_item_associations_unification_enabled?
::Feature.enabled?(:epic_and_work_item_associations_unification, self, type: :wip)
end
+ def epic_and_work_item_notes_unification_enabled?
+ ::Feature.enabled?(:epic_and_work_item_notes_unification, self, type: :wip)
+ end
+
def epic_and_work_item_labels_unification_enabled?
::Feature.enabled?(:epic_and_work_item_labels_unification, self, type: :wip)
end
diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb
index f21f2132b7403..3190ab17c24ce 100644
--- a/ee/app/models/ee/note.rb
+++ b/ee/app/models/ee/note.rb
@@ -31,6 +31,10 @@ def use_separate_indices?
def with_web_entity_associations
super.preload(project: [:group, { namespace: :route }])
end
+
+ def inc_relations_for_view(noteable = nil)
+ super.preload({ system_note_metadata: { description_version: [:epic] } })
+ end
end
def search_index
diff --git a/ee/app/models/ee/work_item.rb b/ee/app/models/ee/work_item.rb
index 8693ce58f8a1f..2329c5b7b2126 100644
--- a/ee/app/models/ee/work_item.rb
+++ b/ee/app/models/ee/work_item.rb
@@ -60,11 +60,6 @@ def average_progress_of_children
(::WorkItems::Progress.where(work_item: children).sum(:progress).to_i / child_count).to_i
end
- override :skip_description_version?
- def skip_description_version?
- super || epic_work_item?
- end
-
override :skip_metrics?
def skip_metrics?
super || epic_work_item?
diff --git a/ee/app/policies/epic_policy.rb b/ee/app/policies/epic_policy.rb
index cb88eb6ca29de..5272b22e4dea7 100644
--- a/ee/app/policies/epic_policy.rb
+++ b/ee/app/policies/epic_policy.rb
@@ -37,6 +37,7 @@ class EpicPolicy < BasePolicy
enable :read_epic_iid
enable :read_note
enable :read_issuable_participables
+ enable :read_issuable
end
rule { can?(:read_epic) & ~anonymous }.policy do
diff --git a/ee/app/services/concerns/epics/sync_as_work_item.rb b/ee/app/services/concerns/epics/sync_as_work_item.rb
index 2cb27fad6e520..3104b9fc034f3 100644
--- a/ee/app/services/concerns/epics/sync_as_work_item.rb
+++ b/ee/app/services/concerns/epics/sync_as_work_item.rb
@@ -56,7 +56,9 @@ def update_params(epic)
.intersection(ALLOWED_PARAMS + %i[title_html description_html])
.index_with { |attr| epic[attr] }
- filtered_attributes.merge({ updated_by: epic.updated_by, updated_at: epic.updated_at })
+ filtered_attributes.merge(
+ { updated_by: epic.updated_by, updated_at: epic.updated_at, skip_description_version: true }
+ )
end
def sync_color(epic, work_item)
diff --git a/ee/app/services/concerns/work_items/sync_as_epic.rb b/ee/app/services/concerns/work_items/sync_as_epic.rb
index a01de234f4ff8..112b48774dcb4 100644
--- a/ee/app/services/concerns/work_items/sync_as_epic.rb
+++ b/ee/app/services/concerns/work_items/sync_as_epic.rb
@@ -28,7 +28,7 @@ def update_epic_for!(work_item)
return true unless epic
return true unless epic.group.work_item_sync_to_epic_enabled?
- epic.assign_attributes(update_params(work_item))
+ epic.assign_attributes(update_params(work_item).merge(skip_description_version: true))
epic.save!(touch: false)
rescue StandardError => error
handle_error!(:update, error, work_item)
diff --git a/ee/config/feature_flags/wip/epic_and_work_item_notes_unification.yml b/ee/config/feature_flags/wip/epic_and_work_item_notes_unification.yml
new file mode 100644
index 0000000000000..edf24eebbd306
--- /dev/null
+++ b/ee/config/feature_flags/wip/epic_and_work_item_notes_unification.yml
@@ -0,0 +1,9 @@
+---
+name: epic_and_work_item_notes_unification
+feature_issue_url:
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/157161
+rollout_issue_url:
+milestone: '17.2'
+group: group::product planning
+type: wip
+default_enabled: false
diff --git a/ee/spec/controllers/groups/epics_controller_spec.rb b/ee/spec/controllers/groups/epics_controller_spec.rb
index 6ab635446277e..a716f308faa48 100644
--- a/ee/spec/controllers/groups/epics_controller_spec.rb
+++ b/ee/spec/controllers/groups/epics_controller_spec.rb
@@ -472,6 +472,20 @@ def update_epic(epic, params)
it_behaves_like DescriptionDiffActions do
let_it_be(:group) { create(:group, :public) }
let_it_be(:issuable) { create(:epic, group: group) }
+ let_it_be(:version_1) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_3) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+
+ let(:base_params) { { group_id: group, id: issuable } }
+ end
+
+ it_behaves_like DescriptionDiffActions do
+ let_it_be(:group) { create(:group, :public) }
+ let_it_be(:epic) { create(:epic, group: group) }
+ let_it_be(:issuable) { epic.work_item }
+ let_it_be(:version_1) { create(:description_version, epic.base_class_name.underscore.to_sym => epic) }
+ let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_3) { create(:description_version, epic.base_class_name.underscore.to_sym => epic) }
let(:base_params) { { group_id: group, id: issuable } }
end
diff --git a/ee/spec/controllers/groups/work_items_controller_spec.rb b/ee/spec/controllers/groups/work_items_controller_spec.rb
index 23caa6bab04ea..abec43b90b844 100644
--- a/ee/spec/controllers/groups/work_items_controller_spec.rb
+++ b/ee/spec/controllers/groups/work_items_controller_spec.rb
@@ -10,6 +10,10 @@
context 'when issuable is an issue type issue' do
it_behaves_like DescriptionDiffActions do
let_it_be(:issuable) { create(:issue, :group_level, namespace: group) }
+ let_it_be(:version_1) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_3) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+
let(:base_params) { group_params }
end
end
@@ -17,6 +21,10 @@
context 'when work item is an issue type issue' do
it_behaves_like DescriptionDiffActions do
let_it_be(:issuable) { create(:work_item, :group_level, namespace: group) }
+ let_it_be(:version_1) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_3) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+
let(:base_params) { group_params }
end
end
@@ -24,6 +32,10 @@
context 'when issuable is a task/work_item' do
it_behaves_like DescriptionDiffActions do
let_it_be(:issuable) { create(:work_item, :group_level, :task, namespace: group) }
+ let_it_be(:version_1) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_3) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+
let(:base_params) { group_params }
end
end
diff --git a/ee/spec/controllers/projects/issues_controller_spec.rb b/ee/spec/controllers/projects/issues_controller_spec.rb
index 0c3a7920d633a..d374481797341 100644
--- a/ee/spec/controllers/projects/issues_controller_spec.rb
+++ b/ee/spec/controllers/projects/issues_controller_spec.rb
@@ -415,12 +415,18 @@ def update_issue(issue_params: {}, additional_params: {}, id: nil)
context 'when issuable is an issue type issue' do
it_behaves_like DescriptionDiffActions do
let_it_be(:issuable) { create(:issue, project: project) }
+ let_it_be(:version_1) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_3) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
end
end
context 'when issuable is a task/work_item' do
it_behaves_like DescriptionDiffActions do
let_it_be(:issuable) { create(:issue, :task, project: project) }
+ let_it_be(:version_1) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_3) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
end
end
end
diff --git a/ee/spec/controllers/projects/merge_requests_controller_spec.rb b/ee/spec/controllers/projects/merge_requests_controller_spec.rb
index b7f2b99fda7e6..29374b9e492e7 100644
--- a/ee/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/ee/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -869,5 +869,8 @@ def expect_rebase_worker_for(user)
it_behaves_like DescriptionDiffActions do
let_it_be(:project) { create(:project, :repository, :public) }
let_it_be(:issuable) { create(:merge_request, source_project: project) }
+ let_it_be(:version_1) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
+ let_it_be(:version_3) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
end
end
diff --git a/ee/spec/models/ee/description_version_spec.rb b/ee/spec/models/ee/description_version_spec.rb
index 521203e31d9fe..7050d40255ea2 100644
--- a/ee/spec/models/ee/description_version_spec.rb
+++ b/ee/spec/models/ee/description_version_spec.rb
@@ -56,7 +56,8 @@ def deleted_count
it 'broadcasts notes update' do
version = epic.description_versions.last
- expect(epic).to receive(:broadcast_notes_changed)
+ expect(version.issuable).to receive(:broadcast_notes_changed)
+ expect(version.issuable.sync_object).to receive(:broadcast_notes_changed)
version.delete!
end
diff --git a/ee/spec/models/epic_spec.rb b/ee/spec/models/epic_spec.rb
index 1e62e075ceebe..5352decf67c1f 100644
--- a/ee/spec/models/epic_spec.rb
+++ b/ee/spec/models/epic_spec.rb
@@ -1723,40 +1723,81 @@ def as_item(item)
end
end
- context 'with labels' do
- let_it_be(:label1) { create(:group_label, group: group, title: 'epic-label-1') }
- let_it_be(:label2) { create(:group_label, group: group, title: 'epic-label-2') }
- let_it_be(:epic) { create(:epic, group: group, labels: [label1]) }
- let_it_be(:work_item) { epic.work_item }
+ context 'with unified associations' do
+ context 'with labels' do
+ let_it_be(:label1) { create(:group_label, group: group, title: 'epic-label-1') }
+ let_it_be(:label2) { create(:group_label, group: group, title: 'epic-label-2') }
+ let_it_be(:epic) { create(:epic, group: group, labels: [label1]) }
+ let_it_be(:work_item) { epic.work_item }
- before do
- work_item.labels << label2
- end
-
- context 'when labels are fetched just from the epic itself' do
before do
- stub_feature_flags(epic_and_work_item_labels_unification: false)
+ work_item.labels << label2
end
- it 'returns only epic labels' do
- expect(epic.reload.labels).to match_array([label1])
- expect(work_item.reload.labels).to match_array([label2])
+ context 'when labels are fetched just from the epic itself' do
+ before do
+ stub_feature_flags(epic_and_work_item_labels_unification: false)
+ end
+
+ it 'returns only epic labels' do
+ expect(epic.reload.labels).to match_array([label1])
+ expect(work_item.reload.labels).to match_array([label2])
+ end
end
- end
- context 'when labels are fetched from the epic and epic work item' do
- before do
- stub_feature_flags(epic_and_work_item_labels_unification: true)
+ context 'when labels are fetched from the epic and epic work item' do
+ before do
+ stub_feature_flags(epic_and_work_item_labels_unification: true)
+ end
+
+ it 'returns epic and epic work item labels' do
+ expect(epic.reload.labels).to match_array([label1, label2])
+ expect(work_item.reload.labels).to match_array([label1, label2])
+ end
+
+ it 'returns epic and epic work item labels queried by id' do
+ expect(epic.reload.labels.find(label1.id)).to eq(label1)
+ expect(work_item.reload.labels.find(label1.id)).to eq(label1)
+ end
end
+ end
- it 'returns only epic labels' do
- expect(epic.reload.labels).to match_array([label1, label2])
- expect(work_item.reload.labels).to match_array([label1, label2])
+ context 'with description versions' do
+ let_it_be(:epic) { create(:epic, group: group) }
+ let_it_be(:work_item) { epic.work_item }
+ let(:version1) { create(:description_version, epic: epic) }
+ let(:version2) { create(:description_version, issue: work_item) }
+
+ context 'when description versions are fetched just from the epic itself' do
+ before do
+ stub_feature_flags(epic_and_work_item_notes_unification: false)
+ end
+
+ it 'returns only epic notes' do
+ expect(epic.reload.description_versions).to match_array([version1])
+ expect(work_item.reload.description_versions).to match_array([version2])
+ expect(epic.reload.own_description_versions).to match_array([version1])
+ expect(work_item.reload.own_description_versions).to match_array([version2])
+ end
end
- it 'returns only epic labels queried by id' do
- expect(epic.reload.labels.find(label1.id)).to eq(label1)
- expect(work_item.reload.labels.find(label1.id)).to eq(label1)
+ context 'when notes are fetched from the epic and epic work item' do
+ before do
+ stub_feature_flags(epic_and_work_item_notes_unification: true)
+ end
+
+ it 'returns epic and epic work item notes' do
+ expect(epic.reload.description_versions).to match_array([version1, version2])
+ expect(work_item.reload.description_versions).to match_array([version1, version2])
+
+ expect(epic.reload.own_description_versions).to match_array([version1])
+ expect(work_item.reload.own_description_versions).to match_array([version2])
+ end
+
+ it 'returns epic and epic work item notes queried by id' do
+ expect(epic.reload.description_versions.find(version1.id)).to eq(version1)
+ expect(work_item.reload.description_versions.find(version1.id)).to eq(version1)
+ end
end
end
end
diff --git a/ee/spec/models/work_item_spec.rb b/ee/spec/models/work_item_spec.rb
index 53ecbfae8dca1..2dfc3dc5d6690 100644
--- a/ee/spec/models/work_item_spec.rb
+++ b/ee/spec/models/work_item_spec.rb
@@ -464,9 +464,16 @@
create(:work_item, description: 'Original description', work_item_type: type, project: reusable_project)
end
- it 'does not create a versioned description' do
+ it 'creates a versioned description on epic work item' do
expect { work_item.update!(description: 'Another description') }
- .not_to change { work_item.description_versions.count }
+ .to change { work_item.own_description_versions.count }
+ end
+
+ context 'when set to skip description version' do
+ it 'does not create a versioned description on epic work item' do
+ expect { work_item.update!(description: 'Another description', skip_description_version: true) }
+ .not_to change { work_item.own_description_versions.count }
+ end
end
end
end
diff --git a/ee/spec/policies/epic_policy_spec.rb b/ee/spec/policies/epic_policy_spec.rb
index 639052e7ab964..a47133e0f1612 100644
--- a/ee/spec/policies/epic_policy_spec.rb
+++ b/ee/spec/policies/epic_policy_spec.rb
@@ -50,7 +50,7 @@
shared_examples 'can only read epics' do
it 'matches expected permissions' do
is_expected.to be_allowed(
- :read_epic, :read_epic_iid, :read_note,
+ :read_epic, :read_issuable, :read_epic_iid, :read_note,
:create_todo, :read_issuable_participables
)
is_expected.to be_disallowed(
@@ -64,7 +64,7 @@
shared_examples 'can manage epics' do
it 'matches expected permissions' do
is_expected.to be_allowed(
- :read_epic, :read_epic_iid, :read_note,
+ :read_epic, :read_issuable, :read_epic_iid, :read_note,
:read_issuable_participables, :read_internal_note,
:update_epic, :admin_epic, :create_epic, :admin_epic_relation,
:create_todo, :admin_epic_link_relation, :set_epic_metadata,
@@ -77,7 +77,7 @@
shared_examples 'all epic permissions disabled' do
it 'matches expected permissions' do
is_expected.to be_disallowed(
- :read_epic, :read_epic_iid, :update_epic,
+ :read_epic, :read_issuable, :read_epic_iid, :update_epic,
:destroy_epic, :admin_epic, :create_epic,
:create_note, :award_emoji, :read_note,
:read_issuable_participables,
@@ -93,7 +93,7 @@
shared_examples 'all reporter epic permissions enabled' do
it 'matches expected permissions' do
is_expected.to be_allowed(
- :read_epic, :read_epic_iid, :update_epic,
+ :read_epic, :read_issuable, :read_epic_iid, :update_epic,
:admin_epic, :create_epic, :create_note,
:award_emoji, :read_note, :create_todo,
:read_issuable_participables, :read_internal_note,
@@ -230,7 +230,7 @@
it 'matches expected permissions' do
is_expected.to be_allowed(
- :read_epic, :read_epic_iid, :read_note, :read_issuable_participables
+ :read_epic, :read_issuable, :read_epic_iid, :read_note, :read_issuable_participables
)
is_expected.to be_disallowed(
@@ -339,7 +339,7 @@
end
it 'matches expected permissions' do
- is_expected.to be_allowed(:read_epic, :read_epic_iid)
+ is_expected.to be_allowed(:read_epic, :read_issuable, :read_epic_iid)
is_expected.to be_disallowed(
:update_epic, :destroy_epic, :admin_epic,
:create_epic, :set_epic_metadata, :set_confidentiality,
@@ -369,7 +369,7 @@
it 'matches expected permissions' do
is_expected.to be_allowed(
- :read_epic, :read_epic_iid, :update_epic,
+ :read_epic, :read_issuable, :read_epic_iid, :update_epic,
:admin_epic, :create_epic, :create_note,
:award_emoji, :read_note, :create_todo,
:read_issuable_participables, :admin_epic_relation,
@@ -389,7 +389,7 @@
it 'matches expected permissions' do
is_expected.to be_allowed(
- :read_epic, :read_epic_iid, :update_epic,
+ :read_epic, :read_issuable, :read_epic_iid, :update_epic,
:admin_epic, :create_epic, :create_note,
:award_emoji, :read_note, :create_todo,
:read_issuable_participables, :admin_epic_relation,
diff --git a/ee/spec/services/ee/issuable/destroy_service_spec.rb b/ee/spec/services/ee/issuable/destroy_service_spec.rb
index 7ac1eb79ce2d3..0cd5c6599ce68 100644
--- a/ee/spec/services/ee/issuable/destroy_service_spec.rb
+++ b/ee/spec/services/ee/issuable/destroy_service_spec.rb
@@ -10,57 +10,115 @@
describe '#execute' do
context 'when destroying an epic' do
let_it_be(:group) { create(:group) }
- let_it_be(:label1) { create(:group_label, group: group, title: 'epic-label-1') }
- let_it_be(:label2) { create(:group_label, group: group, title: 'epic-label-2') }
- let_it_be(:epic) { create(:epic, group: group, labels: [label1]) }
- let(:work_item) { epic.sync_object }
- let(:issuable) { epic }
- let(:sync_object) { work_item }
+ context 'when deleting the epic' do
+ let_it_be(:label1) { create(:group_label, group: group) }
+ let_it_be(:label2) { create(:group_label, group: group) }
+ let_it_be(:epic) { create(:epic, group: group, labels: [label1]) }
+ let_it_be(:work_item) { epic.sync_object }
- before do
- sync_object.labels << label2
- end
+ let_it_be(:issuable) { epic }
+ let_it_be(:sync_object) { work_item }
- it 'deletes the epic and the epic work item' do
- # making sure we have this work item
- expect(WorkItem.find_by_id(sync_object.id)).to be_present
+ before do
+ sync_object.labels << label2
+ end
- expect { subject.execute(issuable) }.to change { Epic.count }.by(-1).and(
- change { WorkItem.count }.by(-1)
- )
+ it 'deletes the epic and the epic work item' do
+ epic_id = epic.id
+ epic_work_item_id = epic.issue_id
- expect(sync_object.id).not_to be_nil
- expect(WorkItem.find_by_id(sync_object.id)).to be_nil
- end
+ expect { subject.execute(issuable) }.to change { Epic.count }.by(-1).and(change { WorkItem.count }.by(-1))
- it 'records usage ping epic destroy event' do
- expect(Gitlab::UsageDataCounters::EpicActivityUniqueCounter).to receive(
- :track_epic_destroyed).with(author: user, namespace: group)
+ expect(Epic.find_by_id(epic_id)).to be_nil
+ expect(WorkItem.find_by_id(epic_work_item_id)).to be_nil
+ end
- subject.execute(issuable)
+ it 'records usage ping epic destroy event' do
+ expect(Gitlab::UsageDataCounters::EpicActivityUniqueCounter).to receive(
+ :track_epic_destroyed).with(author: user, namespace: group)
+
+ subject.execute(issuable)
+ end
+
+ it_behaves_like 'service deleting todos'
+ it_behaves_like 'service deleting label links'
end
- it_behaves_like 'service deleting todos'
- it_behaves_like 'service deleting label links'
+ context 'when deleting the epic work item' do
+ let_it_be(:label1) { create(:group_label, group: group) }
+ let_it_be(:label2) { create(:group_label, group: group) }
+ let_it_be(:epic) { create(:epic, group: group, labels: [label1]) }
+ let_it_be(:work_item) { epic.sync_object }
- context 'when destroying an epic work item' do
- let(:issuable) { work_item }
- let(:sync_object) { epic }
+ let_it_be(:issuable) { work_item.reload }
+ let_it_be(:sync_object) { epic.reload }
+
+ before do
+ sync_object.labels << label2
+ end
it 'deletes the epic and the epic work item' do
- # making sure we have this work item
- expect { subject.execute(issuable) }.to change { Epic.count }.by(-1).and(
- change { WorkItem.count }.by(-1)
- )
+ epic_id = epic.id
+ epic_work_item_id = epic.issue_id
+
+ expect { subject.execute(issuable) }.to change { Epic.count }.by(-1).and(change { WorkItem.count }.by(-1))
- expect(sync_object.id).not_to be_nil
- expect(Epic.find_by_id(sync_object.id)).to be_nil
+ expect(Epic.find_by_id(epic_id)).to be_nil
+ expect(WorkItem.find_by_id(epic_work_item_id)).to be_nil
end
it_behaves_like 'service deleting todos'
it_behaves_like 'service deleting label links'
end
+
+ context 'with unified description_versions' do
+ shared_examples 'deletes description versions on both epic and epic work item' do
+ it 'deletes the epic, epic work item and all notes' do
+ epic_id = epic.id
+ epic_work_item_id = epic.issue_id
+
+ expect(DescriptionVersion.where(epic_id: epic_id).count).to eq(1)
+ expect(DescriptionVersion.where(issue_id: epic_work_item_id).count).to eq(1)
+
+ expect { subject.execute(issuable) }.to change { Epic.count }.by(-1).and(
+ change { WorkItem.count }.by(-1)).and(change { DescriptionVersion.count }.by(-2))
+
+ expect(Epic.find_by_id(epic_id)).to be_nil
+ expect(WorkItem.find_by_id(epic_work_item_id)).to be_nil
+ expect(DescriptionVersion.where(epic_id: epic_id).count).to eq(0)
+ expect(DescriptionVersion.where(issue_id: epic_work_item_id).count).to eq(0)
+ end
+ end
+
+ context 'when deleting the epic' do
+ let_it_be(:epic) { create(:epic, group: group) }
+ let_it_be(:work_item) { epic.sync_object }
+ let_it_be(:label1) { create(:group_label, group: group) }
+ let_it_be(:label2) { create(:group_label, group: group) }
+ let_it_be(:version1) { create(:description_version, epic: epic) }
+ let_it_be(:version2) { create(:description_version, issue: work_item) }
+
+ let_it_be(:issuable) { epic }
+ let_it_be(:sync_object) { work_item }
+
+ it_behaves_like 'deletes description versions on both epic and epic work item'
+ end
+
+ context 'when deleting the epic work item' do
+ let_it_be(:epic) { create(:epic, group: group) }
+ let_it_be(:work_item) { epic.sync_object }
+ let_it_be(:label1) { create(:group_label, group: group) }
+ let_it_be(:label2) { create(:group_label, group: group) }
+ let_it_be(:version1) { create(:description_version, epic: epic) }
+ let_it_be(:version2) { create(:description_version, issue: work_item) }
+
+ let_it_be(:issuable) { work_item }
+ let_it_be(:sync_object) { epic }
+
+ it_behaves_like 'deletes description versions on both epic and epic work item'
+ end
+ end
end
context 'when destroying other issuable type' do
diff --git a/ee/spec/services/epics/update_service_spec.rb b/ee/spec/services/epics/update_service_spec.rb
index dc6307fdf17eb..6268e1ab9854b 100644
--- a/ee/spec/services/epics/update_service_spec.rb
+++ b/ee/spec/services/epics/update_service_spec.rb
@@ -107,9 +107,16 @@ def update_epic(opts)
update_epic(description: 'updated description')
end
+
+ it 'creates description version for epic only' do
+ update_epic(description: 'New description')
+
+ expect(epic.reload.own_description_versions.count).to eq(2)
+ expect(epic.sync_object.reload.own_description_versions.count).to eq(0)
+ end
end
- context 'when decription is not changed' do
+ context 'when description is not changed' do
it 'does not trigger GraphQL description updated subscription' do
expect(GraphqlTriggers).not_to receive(:issuable_description_updated)
diff --git a/ee/spec/services/work_items/update_service_spec.rb b/ee/spec/services/work_items/update_service_spec.rb
index 67b18f513d8bb..6272ea6f36eef 100644
--- a/ee/spec/services/work_items/update_service_spec.rb
+++ b/ee/spec/services/work_items/update_service_spec.rb
@@ -379,6 +379,20 @@
end
it_behaves_like 'syncs all data from a work_item to an epic'
+
+ context 'and description version created for work item only' do
+ before do
+ work_item.update_columns(description: "some older description")
+ work_item.sync_object.update_columns(description: "some older description")
+ end
+
+ it 'creates description version on epic work item only' do
+ subject
+
+ expect(work_item.reload.own_description_versions.count).to eq(2)
+ expect(work_item.sync_object.reload.own_description_versions.count).to eq(0)
+ end
+ end
end
it 'syncs the data to the epic', :aggregate_failures do
diff --git a/ee/spec/support/shared_examples/controllers/concerns/description_diff_actions_shared_examples.rb b/ee/spec/support/shared_examples/controllers/concerns/description_diff_actions_shared_examples.rb
index 0d273406f310e..6e6ab1381fee0 100644
--- a/ee/spec/support/shared_examples/controllers/concerns/description_diff_actions_shared_examples.rb
+++ b/ee/spec/support/shared_examples/controllers/concerns/description_diff_actions_shared_examples.rb
@@ -4,10 +4,6 @@
let(:base_params) { { namespace_id: project.namespace, project_id: project, id: issuable } }
describe do
- let_it_be(:version_1) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
- let_it_be(:version_2) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
- let_it_be(:version_3) { create(:description_version, issuable.base_class_name.underscore.to_sym => issuable) }
-
def get_description_diff(extra_params = {})
get :description_diff, params: base_params.merge(extra_params)
end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index 31484dd1a279f..b993f933b4873 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -23,6 +23,7 @@ issues:
- own_label_links
- labels
- own_labels
+- own_description_versions
- sync_object
- last_edited_by
- todos
@@ -1025,6 +1026,7 @@ epic:
- own_label_links
- labels
- own_labels
+- own_description_versions
- sync_object
- todos
- metrics
--
GitLab