diff --git a/app/models/concerns/versioned_description.rb b/app/models/concerns/versioned_description.rb index dff7983477a2ad09ec9669f835e420c9ce948826..1270529fdfe56709e99e84785b0bb06e931798a7 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 71c16621c9e240225a7892c4e9f4bccda80b01aa..54dea117a9c6225b7e50b4e6e49c02d82b124e57 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 cb47449d29812152df81b107c1063d20eaf3a7c1..5d5a8e3071c77d480abfe6f00fb6ad2b6b8c7025 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 34b6495634bfb00dc7d05fdd378017e7c4181a7f..c551e1cae472aed48369af26f7ac0e0f55626b37 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 08dc4c437669386d13d727281191db166e0320e8..b24566710f835702c3a15fa5745561bb1bb3fd38 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 0000000000000000000000000000000000000000..a35db912c6732380cfdbddaf087a5ba39e1c2767 --- /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 131d9fc7d1cee1601b2d9bd348c5ac3f87fec352..fb4a723001d392da8f6ee4ec7a9ec58c3e5e1e16 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 d1aa81e192e35267a188c9c68a0aca2174c3fcc6..282b24b32c2b58c92e2762dbe4304f7287d89546 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 f21f2132b7403e30049d0583393390cd4db3a370..3190ab17c24ce5564e784ec42bb41c55932efce7 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 8693ce58f8a1f46b3d1e00ea3c9803545f5eba49..2329c5b7b2126d20107629f76eca86f82276350c 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 cb88eb6ca29de24048ac98b45317542b71ae7ef9..5272b22e4dea77f1ecc24f1579df1db16d0b4112 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 2cb27fad6e520c0f98ca4b325572e0919e760511..3104b9fc034f30ab9469c135cb70819fe1536aad 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 a01de234f4ff888c63ccf201bccf77c0b1c1796d..112b48774dcb457214775d2e38fc7f8dc8f43a70 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 0000000000000000000000000000000000000000..edf24eebbd3065c6f5cf10d64987ffda554ae34c --- /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 6ab635446277e9c49bb57958bc5d428049a835e1..a716f308faa48e1d2a8c19993b4607d931cb0ecb 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 23caa6bab04eadbf9b920cb66cd78f92c4bc8a25..abec43b90b844cf3f1bff59503c90dc10c831229 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 0c3a7920d633abb9279b0ae91d29061d48ebeadf..d374481797341ded6f4a7b1ad6ffbdc4e50d1fd5 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 b7f2b99fda7e6a5df03bd7ff6316109d40625120..29374b9e492e7c9450e7cfb76d7c1729cc8a01d4 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 521203e31d9fedbaa406bf6c59f50ace86b4ccb2..7050d40255ea229c05423694e66f647c725b5986 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 1e62e075ceebe8f29c53cb0a52ab8938947d5b22..5352decf67c1fbc1b51b98f6ae45bb3a0d89641b 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 53ecbfae8dca14359058336752bc2e8e672f0b48..2dfc3dc5d669028c1f33b6743ae5127e09f4f742 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 639052e7ab9648d2eb2c03d7e3ca4388d70adc38..a47133e0f16122262a057e1426e1534bffc0bacc 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 7ac1eb79ce2d37e904d3a0e782fefc75af6c5cc6..0cd5c6599ce68d1ef7e07acb346ab3ce31987cd7 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 dc6307fdf17eb40f2fbe8d50c54be30d0640c76f..6268e1ab9854bf66582e746d51e3613f8f896a2c 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 67b18f513d8bb4ec1e381a8282d4be20e1130831..6272ea6f36eeff51af916739287cd69f2561a487 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 0d273406f310e5f8fceacb4afdd5708f1a300db5..6e6ab1381fee0d41e1debf96c8f1c68e72dd2029 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 31484dd1a279f54d90e41f00077c4dff2aae65d3..b993f933b48738b5e24b05d099e6890910cbc8bf 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