diff --git a/app/helpers/integrations_helper.rb b/app/helpers/integrations_helper.rb index ee373452100da7ee84cca58f4ba9e0f125051556..795ef3098444ee11575610540903259af424a66a 100644 --- a/app/helpers/integrations_helper.rb +++ b/app/helpers/integrations_helper.rb @@ -205,6 +205,7 @@ def integration_webhook_event_human_name(event) project_events: s_('Webhooks|Project events'), push_events: _('Push events'), releases_events: s_('Webhooks|Releases events'), + milestone_events: s_('Webhooks|Milestone events'), repository_update_events: _('Repository update events'), resource_access_token_events: s_('Webhooks|Project or group access token events'), subgroup_events: s_('Webhooks|Subgroup events'), diff --git a/app/models/group.rb b/app/models/group.rb index ea17559d073399b23c8cde0c5b8d5345f0a4bff0..74ad36562a8a4415c52ef92bd95e89020aecc993 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -1132,6 +1132,11 @@ def supports_group_work_items? false end + # overriden in EE + def has_active_hooks?(hooks_scope = :push_hooks) + false + end + # overriden in EE def enterprise_user_settings_available?(user = nil) false diff --git a/app/services/milestones/base_service.rb b/app/services/milestones/base_service.rb index ecfe7db325e51436c3c0b570de8f195ac40a4155..6c5451d7fd8da4ebdd8067de6fd996da7e705628 100644 --- a/app/services/milestones/base_service.rb +++ b/app/services/milestones/base_service.rb @@ -16,8 +16,6 @@ def initialize(parent, user, params = {}) private def execute_hooks(milestone, action) - # At the moment, only project milestones support webhooks, not group milestones - return unless milestone.project_milestone? return unless milestone.parent.has_active_hooks?(:milestone_hooks) payload = Gitlab::DataBuilder::Milestone.build(milestone, action) diff --git a/app/services/milestones/close_service.rb b/app/services/milestones/close_service.rb index 616c6d85b7c55a1cbebaa7ec5cb423933a02e5f7..968494eeaac54867c32c33dea555cc32f146cdda 100644 --- a/app/services/milestones/close_service.rb +++ b/app/services/milestones/close_service.rb @@ -3,8 +3,8 @@ module Milestones class CloseService < Milestones::BaseService def execute(milestone) - if milestone.close && milestone.project_milestone? - event_service.close_milestone(milestone, current_user) + if milestone.close + event_service.close_milestone(milestone, current_user) if milestone.project_milestone? execute_hooks(milestone, 'close') end diff --git a/app/services/milestones/create_service.rb b/app/services/milestones/create_service.rb index d8a9be6aafebd451d58d26b0e4dac489caba9118..a8d43cbcf1563e73efaa149b0987b035e4dd1ffd 100644 --- a/app/services/milestones/create_service.rb +++ b/app/services/milestones/create_service.rb @@ -7,8 +7,8 @@ def execute before_create(milestone) - if milestone.save && milestone.project_milestone? - event_service.open_milestone(milestone, current_user) + if milestone.save + event_service.open_milestone(milestone, current_user) if milestone.project_milestone? execute_hooks(milestone, 'create') end diff --git a/app/services/milestones/destroy_service.rb b/app/services/milestones/destroy_service.rb index dc605472e37bf0b10b820376c31c9baef1a6a0fe..39d32709ec622af696f595112b4a4a0928456852 100644 --- a/app/services/milestones/destroy_service.rb +++ b/app/services/milestones/destroy_service.rb @@ -14,7 +14,7 @@ def execute(milestone) return unless milestone.destroyed? - execute_hooks(milestone, 'delete') if milestone.project_milestone? + execute_hooks(milestone, 'delete') milestone end diff --git a/app/services/milestones/reopen_service.rb b/app/services/milestones/reopen_service.rb index a4a9ccfd4011dc5371cea76c465bbabd59f6cebb..13e138bd5615de9c5bc6fc56dcb8fccf99b24b28 100644 --- a/app/services/milestones/reopen_service.rb +++ b/app/services/milestones/reopen_service.rb @@ -3,8 +3,8 @@ module Milestones class ReopenService < Milestones::BaseService def execute(milestone) - if milestone.activate && milestone.project_milestone? - event_service.reopen_milestone(milestone, current_user) + if milestone.activate + event_service.reopen_milestone(milestone, current_user) if milestone.project_milestone? execute_hooks(milestone, 'reopen') end diff --git a/doc/api/group_webhooks.md b/doc/api/group_webhooks.md index 28021ac2e0b16374e719b763466845e81260f8c8..cbb57e2ac3b58e5814b0c6c201f01b216e876c1a 100644 --- a/doc/api/group_webhooks.md +++ b/doc/api/group_webhooks.md @@ -70,6 +70,7 @@ Example response: "deployment_events": false, "feature_flag_events": false, "releases_events": false, + "milestone_events": false, "subgroup_events": false, "emoji_events": false, "resource_access_token_events": false, @@ -130,6 +131,7 @@ Example response: "deployment_events": true, "feature_flag_events": false, "releases_events": true, + "milestone_events": false, "subgroup_events": true, "member_events": true, "project_events": true, @@ -500,6 +502,7 @@ Supported attributes: | `deployment_events` | boolean | no | Trigger hook on deployment events. | | `feature_flag_events` | boolean | no | Trigger hook on feature flag events. | | `releases_events` | boolean | no | Trigger hook on release events. | +| `milestone_events` | boolean | no | Trigger hook on milestone events. | | `subgroup_events` | boolean | no | Trigger hook on subgroup events. | | `member_events` | boolean | no | Trigger hook on member events. | | `project_events` | boolean | no | Trigger hook on project events. | @@ -543,6 +546,7 @@ Example response: "deployment_events": true, "feature_flag_events": true, "releases_events": true, + "milestone_events": true, "subgroup_events": true, "member_events": true, "project_events": true, @@ -589,6 +593,7 @@ Supported attributes: | `deployment_events` | boolean | no | Trigger hook on deployment events. | | `feature_flag_events` | boolean | no | Trigger hook on feature flag events. | | `releases_events` | boolean | no | Trigger hook on release events. | +| `milestone_events` | boolean | no | Trigger hook on milestone events. | | `subgroup_events` | boolean | no | Trigger hook on subgroup events. | | `member_events` | boolean | no | Trigger hook on member events. | | `project_events` | boolean | no | Trigger hook on project events. | @@ -633,6 +638,7 @@ Example response: "deployment_events": true, "feature_flag_events": true, "releases_events": true, + "milestone_events": true, "subgroup_events": true, "member_events": true, "project_events": true, @@ -699,7 +705,7 @@ POST /groups/:id/hooks/:hook_id/test/:trigger |-----------|-------------------|----------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `hook_id` | integer | Yes | The ID of the group hook. | | `id` | integer or string | Yes | The ID or [URL-encoded path of the group](rest/_index.md#namespaced-paths). | -| `trigger` | string | Yes | One of `push_events`, `tag_push_events`, `issues_events`, `confidential_issues_events`, `note_events`, `merge_requests_events`, `job_events`, `pipeline_events`, `wiki_page_events`, `releases_events`, `emoji_events`, or `resource_access_token_events`. | +| `trigger` | string | Yes | One of `push_events`, `tag_push_events`, `issues_events`, `confidential_issues_events`, `note_events`, `merge_requests_events`, `job_events`, `pipeline_events`, `wiki_page_events`, `releases_events`, `milestone_events`, `emoji_events`, or `resource_access_token_events`. | Example request: diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 60a317fc0a381cf34c85f9083b03211c62d16021..c9ff98c994475e5fffc35c23fe9cf4537f5c1ad9 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -812,6 +812,12 @@ def execute_hooks(data, hooks_scope) execute_async_hooks(group_hooks, hooks_scope, data) end + override :has_active_hooks? + def has_active_hooks?(hooks_scope = :push_hooks) + feature_available?(:group_webhooks) && + GroupHook.where(group_id: self_and_ancestors).hooks_for(hooks_scope).any? + end + override :git_transfer_in_progress? def git_transfer_in_progress? reference_counter(type: ::Gitlab::GlRepository::WIKI).value > 0 diff --git a/ee/app/models/hooks/group_hook.rb b/ee/app/models/hooks/group_hook.rb index 2026d1ec685d96e2abbd855195a074857b723dd7..f87861ee3adc1ae3e4095509684ad9f667c91905 100644 --- a/ee/app/models/hooks/group_hook.rb +++ b/ee/app/models/hooks/group_hook.rb @@ -28,6 +28,7 @@ class GroupHook < WebHook :project_hooks, :push_hooks, :release_hooks, + :milestone_hooks, :resource_access_token_hooks, :subgroup_hooks, :tag_push_hooks, diff --git a/ee/lib/api/group_hooks.rb b/ee/lib/api/group_hooks.rb index 81059504e03a2688c612f0cdfdb4dcf96adb0154..f58e5fe684394b8cf637fc63ec2610c0fc81a2aa 100644 --- a/ee/lib/api/group_hooks.rb +++ b/ee/lib/api/group_hooks.rb @@ -40,6 +40,7 @@ def hook_scope optional :deployment_events, type: Boolean, desc: "Trigger hook on deployment events" optional :feature_flag_events, type: Boolean, desc: "Trigger hook on feature flag events" optional :releases_events, type: Boolean, desc: "Trigger hook on release events" + optional :milestone_events, type: Boolean, desc: "Trigger hook on milestone events" optional :subgroup_events, type: Boolean, desc: "Trigger hook on subgroup events" optional :emoji_events, type: Boolean, desc: "Trigger hook on emoji events" optional :resource_access_token_events, type: Boolean, desc: "Trigger hook on group access token expiry events" diff --git a/ee/lib/ee/api/entities/group_hook.rb b/ee/lib/ee/api/entities/group_hook.rb index b897c30fc3b07e64d8931fedb4aafe1fef5e124f..614be2a453f4946eea1016be0888c32c2cc85add 100644 --- a/ee/lib/ee/api/entities/group_hook.rb +++ b/ee/lib/ee/api/entities/group_hook.rb @@ -15,6 +15,7 @@ class GroupHook < ::API::Entities::Hook expose :deployment_events, documentation: { type: 'boolean' } expose :feature_flag_events, documentation: { type: 'boolean' } expose :releases_events, documentation: { type: 'boolean' } + expose :milestone_events, documentation: { type: 'boolean' } expose :subgroup_events, documentation: { type: 'boolean' } expose :emoji_events, documentation: { type: 'boolean' } expose :resource_access_token_events, documentation: { type: 'boolean' } diff --git a/ee/lib/ee/gitlab/hook_data/milestone_builder.rb b/ee/lib/ee/gitlab/hook_data/milestone_builder.rb new file mode 100644 index 0000000000000000000000000000000000000000..fb77f38984df4e1dda98b386b4e0577c064b2c17 --- /dev/null +++ b/ee/lib/ee/gitlab/hook_data/milestone_builder.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module EE + module Gitlab + module HookData + module MilestoneBuilder + extend ActiveSupport::Concern + + EE_SAFE_HOOK_ATTRIBUTES = %i[ + group_id + ].freeze + + class_methods do + extend ::Gitlab::Utils::Override + + override :safe_hook_attributes + def safe_hook_attributes + super + EE_SAFE_HOOK_ATTRIBUTES + end + end + end + end + end +end diff --git a/ee/spec/controllers/groups/hooks_controller_spec.rb b/ee/spec/controllers/groups/hooks_controller_spec.rb index 7d2a1ad029fc311cde69b965abac8fdd76abaa56..9d44d88aa5ca7a0519b7b1dd68f039a261433a14 100644 --- a/ee/spec/controllers/groups/hooks_controller_spec.rb +++ b/ee/spec/controllers/groups/hooks_controller_spec.rb @@ -141,6 +141,7 @@ def it_renders_correctly wiki_page_events: true, deployment_events: true, releases_events: true, + milestone_events: true, member_events: true, subgroup_events: true, url_variables: [ diff --git a/ee/spec/factories/group_hooks.rb b/ee/spec/factories/group_hooks.rb index 71962ef291cb23359c5bbd72c72df1f0fab69210..53f09c0c50a72061acf0d111538c7c2198ade5b9 100644 --- a/ee/spec/factories/group_hooks.rb +++ b/ee/spec/factories/group_hooks.rb @@ -30,6 +30,7 @@ project_events { true } wiki_page_events { true } releases_events { true } + milestone_events { true } subgroup_events { true } emoji_events { true } resource_access_token_events { true } diff --git a/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json b/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json index bc13fbd35d3ec224a87e6e9808a0a3e358c2c160..0300624eab1f83e60355eda0ae51a0cce5eff068 100644 --- a/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json +++ b/ee/spec/fixtures/api/schemas/public_api/v4/group_hook.json @@ -20,6 +20,7 @@ "job_events", "deployment_events", "releases_events", + "milestone_events", "subgroup_events", "emoji_events", "feature_flag_events", @@ -115,6 +116,9 @@ "releases_events": { "type": "boolean" }, + "milestone_events": { + "type": "boolean" + }, "subgroup_events": { "type": "boolean" }, diff --git a/ee/spec/lib/ee/gitlab/hook_data/milestone_builder_spec.rb b/ee/spec/lib/ee/gitlab/hook_data/milestone_builder_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..794f30338b8645115515f983cb42caf52a230280 --- /dev/null +++ b/ee/spec/lib/ee/gitlab/hook_data/milestone_builder_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::HookData::MilestoneBuilder, feature_category: :webhooks do + let_it_be(:milestone) { create(:milestone) } + + let(:builder) { described_class.new(milestone) } + + describe '.safe_hook_attributes' do + subject(:safe_attribute_keys) { described_class.safe_hook_attributes } + + it 'includes safe attribute' do + expected_safe_attribute_keys = %i[ + id + iid + title + description + state + created_at + updated_at + due_date + start_date + project_id + group_id + ].freeze + + expect(safe_attribute_keys).to match_array(expected_safe_attribute_keys) + end + end +end diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index 0d3daa53f95c464bf717bc84a726ef4aa960360e..4dc1fdd7f0d1b6ef936988c77c63a5b5a032bbdb 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -2980,6 +2980,71 @@ def webhook_headers end end + describe '#has_active_hooks?' do + let_it_be_with_reload(:parent_group) { create(:group) } + let_it_be_with_reload(:group) { create(:group, parent: parent_group) } + let(:hooks_scope) { :push_hooks } + + subject { group.has_active_hooks?(hooks_scope) } + + context 'when group_webhooks feature is available' do + before do + stub_licensed_features(group_webhooks: true) + end + + context 'when no hooks exist' do + it 'returns false' do + expect(subject).to be false + end + end + + context 'when hook exists for the group' do + let_it_be_with_reload(:group_hook) { create(:group_hook, group: group, push_events: true) } + + it 'returns true for push_hooks scope' do + expect(subject).to be true + end + + context 'with different hook scope' do + let(:hooks_scope) { :issues_hooks } + + it 'returns false' do + expect(subject).to be false + end + end + end + + context 'when hook exists for the ancestor group' do + let!(:parent_hook) { create(:group_hook, group: parent_group, push_events: true) } + + it 'returns true when checking group that inherits hooks from ancestor' do + expect(subject).to be true + end + end + + context 'when hook exists for unrelated group' do + let!(:unrelated_group) { create(:group) } + let!(:unrelated_group_hook) { create(:group_hook, group: unrelated_group, push_events: true) } + + it 'returns false' do + expect(subject).to be false + end + end + end + + context 'when group_webhooks feature is not available' do + before do + stub_licensed_features(group_webhooks: false) + end + + let!(:group_hook) { create(:group_hook, group: group, push_events: true) } + + it 'returns false even when hook exists' do + expect(subject).to be false + end + end + end + context 'when resource access token hooks for expiry notification' do let(:group) { create(:group) } let(:group_hook) { create(:group_hook, group: group, resource_access_token_events: true) } diff --git a/ee/spec/requests/api/group_hooks_spec.rb b/ee/spec/requests/api/group_hooks_spec.rb index 2e40485306fddc7959dbd7696fdda54cfe2e5cec..f15d0749ef128f98bdbf36d97cbfb80cc4fe3c27 100644 --- a/ee/spec/requests/api/group_hooks_spec.rb +++ b/ee/spec/requests/api/group_hooks_spec.rb @@ -53,6 +53,7 @@ def event_names wiki_page_events deployment_events releases_events + milestone_events subgroup_events feature_flag_events emoji_events diff --git a/ee/spec/services/milestones/close_service_spec.rb b/ee/spec/services/milestones/close_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d423ddad6090e22f3c7d9b5f6e0bf98b444618fa --- /dev/null +++ b/ee/spec/services/milestones/close_service_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Milestones::CloseService, feature_category: :team_planning do + let_it_be(:user) { create(:user) } + + subject(:service) { described_class.new(container, user, {}) } + + describe '#execute' do + context 'on group milestones' do + let_it_be(:container) { create(:group, maintainers: user) } + + let(:milestone) { create(:milestone, title: 'Milestone v1.0', group: container) } + + context 'when group webhooks are available' do + before do + stub_licensed_features(group_webhooks: true) + end + + context 'when group has active milestone hooks' do + before do + allow(container).to receive(:has_active_hooks?).with(:milestone_hooks).and_return(true) + end + + it_behaves_like 'closes the milestone', with_hooks: true, with_event: false + end + + context 'when group has no active milestone hooks' do + it_behaves_like 'closes the milestone', with_hooks: false, with_event: false + end + end + + context 'when group webhooks are not available' do + it_behaves_like 'closes the milestone', with_hooks: false, with_event: false + end + end + end +end diff --git a/ee/spec/services/milestones/create_service_spec.rb b/ee/spec/services/milestones/create_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..449d3cf23c68f4e980729cc0b93d754e9fd6b877 --- /dev/null +++ b/ee/spec/services/milestones/create_service_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Milestones::CreateService, feature_category: :team_planning do + let_it_be(:params) { { title: 'New Milestone', description: 'Description' } } + let_it_be(:user) { create(:user) } + + subject(:service) { described_class.new(container, user, params) } + + describe '#execute' do + context 'on group milestones' do + let_it_be(:container) { create(:group, maintainers: user) } + + context 'when group webhooks are available' do + before do + stub_licensed_features(group_webhooks: true) + end + + context 'when group has active milestone hooks' do + before do + allow(container).to receive(:has_active_hooks?).with(:milestone_hooks).and_return(true) + end + + it_behaves_like 'creates the milestone', with_hooks: true, with_event: false + end + + context 'when group has no active milestone hooks' do + it_behaves_like 'creates the milestone', with_hooks: false, with_event: false + end + end + + context 'when group webhooks are not available' do + it_behaves_like 'creates the milestone', with_hooks: false, with_event: false + end + end + end +end diff --git a/ee/spec/services/milestones/destroy_service_spec.rb b/ee/spec/services/milestones/destroy_service_spec.rb index 2b93a9bcea6ce39ed9abe0b24fc92fccff5f0903..c31b181513fab2e186e02fd9a09755559e75df57 100644 --- a/ee/spec/services/milestones/destroy_service_spec.rb +++ b/ee/spec/services/milestones/destroy_service_spec.rb @@ -3,27 +3,67 @@ require 'spec_helper' RSpec.describe Milestones::DestroyService, feature_category: :team_planning do - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } - let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) } + let_it_be(:user) { create(:user) } - before do - project.add_maintainer(user) - end - - def service - described_class.new(project, user, {}) - end + subject(:service) { described_class.new(container, user, {}) } describe '#execute' do - context 'with an existing merge request' do - let!(:issue) { create(:issue, project: project, milestone: milestone) } - let!(:merge_request) { create(:merge_request, source_project: project, milestone: milestone) } + context 'on project milestones' do + let_it_be(:container) { create(:project, :repository, maintainers: user) } + + let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: container) } + + context 'with an existing merge request' do + let!(:issue) { create(:issue, project: container, milestone: milestone) } + let!(:merge_request) { create(:merge_request, source_project: container, milestone: milestone) } + + it 'manually queues MergeRequests::SyncCodeOwnerApprovalRulesWorker jobs' do + expect(::MergeRequests::SyncCodeOwnerApprovalRulesWorker).to receive(:perform_async).with(merge_request.id) + + service.execute(milestone) + end + end + end + + context 'on group milestones' do + let_it_be(:container) { create(:group, maintainers: user) } - it 'manually queues MergeRequests::SyncCodeOwnerApprovalRulesWorker jobs' do - expect(::MergeRequests::SyncCodeOwnerApprovalRulesWorker).to receive(:perform_async).with(merge_request.id) + let(:milestone) { create(:milestone, title: 'Milestone v1.0', group: container) } + + shared_examples 'deletes group-milestone' do |with_hooks:| + it 'conditionally executes webhooks and does not create a new event' do + if with_hooks + expect(milestone.parent).to receive(:execute_hooks).with(a_hash_including(action: 'delete'), + :milestone_hooks) + end + + expect(Event).not_to receive(:new) + + service.execute(milestone) + expect { milestone.reload }.to raise_error ActiveRecord::RecordNotFound + end + end + + context 'when group webhooks are available' do + before do + stub_licensed_features(group_webhooks: true) + end + + context 'when group has active milestone hooks' do + before do + allow(container).to receive(:has_active_hooks?).with(:milestone_hooks).and_return(true) + end + + it_behaves_like 'deletes group-milestone', with_hooks: true + end + + context 'when group has no active milestone hooks' do + it_behaves_like 'deletes group-milestone', with_hooks: false + end + end - service.execute(milestone) + context 'when group webhooks are not available' do + it_behaves_like 'deletes group-milestone', with_hooks: false end end end diff --git a/ee/spec/services/milestones/reopen_service_spec.rb b/ee/spec/services/milestones/reopen_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..184e3cc8571c2b4a0802649490ff5548725d9254 --- /dev/null +++ b/ee/spec/services/milestones/reopen_service_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Milestones::ReopenService, feature_category: :team_planning do + let_it_be(:user) { create(:user) } + + subject(:service) { described_class.new(container, user, {}) } + + describe '#execute' do + context 'on group milestones' do + let_it_be(:container) { create(:group, maintainers: user) } + + let(:milestone) { create(:milestone, :closed, title: 'Milestone v1.0', group: container) } + + context 'when group webhooks are available' do + before do + stub_licensed_features(group_webhooks: true) + end + + context 'when group has active milestone hooks' do + before do + allow(container).to receive(:has_active_hooks?).with(:milestone_hooks).and_return(true) + end + + it_behaves_like 'reopens the milestone', with_hooks: true, with_event: false + end + + context 'when group has no active milestone hooks' do + it_behaves_like 'reopens the milestone', with_hooks: false, with_event: false + end + end + + context 'when group webhooks are not available' do + it_behaves_like 'reopens the milestone', with_hooks: false, with_event: false + end + end + end +end diff --git a/lib/gitlab/data_builder/milestone.rb b/lib/gitlab/data_builder/milestone.rb index 6f6e269c082513e6703b85d72261c9b2827ab0c2..ce9c017973e378985e2eb50022ab5c3f17501e3b 100644 --- a/lib/gitlab/data_builder/milestone.rb +++ b/lib/gitlab/data_builder/milestone.rb @@ -22,6 +22,7 @@ def build(milestone, action) object_kind: 'milestone', event_type: 'milestone', project: milestone.project&.hook_attrs, + group: milestone.group&.hook_attrs, object_attributes: milestone.hook_attrs, action: action } diff --git a/lib/gitlab/hook_data/milestone_builder.rb b/lib/gitlab/hook_data/milestone_builder.rb index f2022a956eb25b685cf5a794ed87c340b7c5dda2..3e523e3d4f71daaa1545e408f1c0042d6f662bcc 100644 --- a/lib/gitlab/hook_data/milestone_builder.rb +++ b/lib/gitlab/hook_data/milestone_builder.rb @@ -3,18 +3,20 @@ module Gitlab module HookData class MilestoneBuilder < BaseBuilder - SAFE_HOOK_ATTRIBUTES = %i[ - id - iid - title - description - state - created_at - updated_at - due_date - start_date - project_id - ].freeze + def self.safe_hook_attributes + %i[ + id + iid + title + description + state + created_at + updated_at + due_date + start_date + project_id + ].freeze + end alias_method :milestone, :object @@ -22,8 +24,10 @@ def build milestone .attributes .with_indifferent_access - .slice(*SAFE_HOOK_ATTRIBUTES) + .slice(*self.class.safe_hook_attributes) end end end end + +Gitlab::HookData::MilestoneBuilder.prepend_mod diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 70b2265af9e8e812151c38860e2fe29a8ecc15d2..54d686a3d684eab1ded73e9f5f65a3335c8df530 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -70420,6 +70420,9 @@ msgstr "" msgid "Webhooks|Merge request events" msgstr "" +msgid "Webhooks|Milestone events" +msgstr "" + msgid "Webhooks|Must match part of URL" msgstr "" diff --git a/spec/helpers/integrations_helper_spec.rb b/spec/helpers/integrations_helper_spec.rb index cd11007dc77d83e1a4d43a50ba83a93937a57d09..1e79bdeb3b1231cbc5fd78d1da1966c4c60b59f9 100644 --- a/spec/helpers/integrations_helper_spec.rb +++ b/spec/helpers/integrations_helper_spec.rb @@ -405,6 +405,7 @@ def relation :deployment_events | 'Deployment events' :feature_flag_events | 'Feature flag events' :releases_events | 'Releases events' + :milestone_events | 'Milestone events' :resource_access_token_events | 'Project or group access token events' :vulnerability_events | 'Vulnerability events' end diff --git a/spec/lib/gitlab/hook_data/milestone_builder_spec.rb b/spec/lib/gitlab/hook_data/milestone_builder_spec.rb index ef26dc9dd5d339bd58a03c59b67bab931298da52..7043428a17c8870474b911e51b2dbb39fc6901de 100644 --- a/spec/lib/gitlab/hook_data/milestone_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/milestone_builder_spec.rb @@ -11,7 +11,7 @@ subject(:data) { builder.build } it 'includes safe attributes' do - expect(data.keys).to match_array(described_class::SAFE_HOOK_ATTRIBUTES.map(&:to_s)) + expect(data.keys).to match_array(described_class.safe_hook_attributes.map(&:to_s)) end it 'returns indifferent access hash' do diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index b55ee1607bf69abb241585e102fad54cff372a9e..3840fde252d59b29d1229095561d7b9c1429f05b 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -4373,7 +4373,21 @@ def define_cache_expectations(cache_key) subject { group.supports_group_work_items? } - it { is_expected.to be false } + it 'returns false' do + expect(subject).to be false + end + end + + describe '#has_active_hooks?' do + let(:group) { build(:group) } + + subject { group.has_active_hooks? } + + # Would work in plain CE-version since it's implementation always returns false. + # Would work in EE-version since licensed features are disabled for specs in specs/ by deafult. + it 'returns false' do + expect(subject).to be false + end end describe '#glql_load_on_click_feature_flag_enabled?' do diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb index 16f7e07ede920b0273c0c5f472d7f5c6af8987de..f055cdea268031240c887f1e34623d46d9fc148c 100644 --- a/spec/requests/api/project_hooks_spec.rb +++ b/spec/requests/api/project_hooks_spec.rb @@ -58,6 +58,7 @@ def event_names deployment_events feature_flag_events releases_events + milestone_events emoji_events resource_access_token_events ] diff --git a/spec/services/milestones/close_service_spec.rb b/spec/services/milestones/close_service_spec.rb index 2c0caa38bb6ec3436612cffb0af59fd218f654da..0eee799f09f14e2c1a8aecb71ff3a0552686eb13 100644 --- a/spec/services/milestones/close_service_spec.rb +++ b/spec/services/milestones/close_service_spec.rb @@ -3,16 +3,13 @@ require 'spec_helper' RSpec.describe Milestones::CloseService, feature_category: :team_planning do - let(:user) { create(:user) } - let(:project) { create(:project) } - let(:milestone) { create(:milestone, title: "Milestone v1.2", project: project) } + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be_with_reload(:project) { create(:project, maintainers: user) } - before do - project.add_maintainer(user) - end + let(:milestone) { create(:milestone, title: 'Milestone v1.2', project: project) } - describe '#execute' do - let(:service) { described_class.new(project, user, {}) } + subject(:service) { described_class.new(project, user, {}) } context 'when service is called before test suite' do before do @@ -31,36 +28,17 @@ end end - shared_examples 'closes the milestone' do |with_project_hooks:| - it 'executes hooks with close action and creates new event' do - expect(service).to receive(:execute_hooks).with(milestone, 'close').and_call_original - expect(project).to receive(:execute_hooks).with(kind_of(Hash), :milestone_hooks) if with_project_hooks - - expect { service.execute(milestone) }.to change { Event.count }.by(1) - end - end - - shared_examples 'does not close the milestone' do - it 'does not execute hooks and does not create new event' do - expect(service).not_to receive(:execute_hooks) - - expect { service.execute(milestone) }.not_to change { Event.count } - end - end - context 'when milestone is successfully closed' do context 'when project has active milestone hooks' do - let(:project) do - create(:project).tap do |project| - create(:project_hook, project: project, milestone_events: true) - end + before do + allow(project).to receive(:has_active_hooks?).with(:milestone_hooks).and_return(true) end - it_behaves_like 'closes the milestone', with_project_hooks: true + it_behaves_like 'closes the milestone', with_hooks: true, with_event: true end context 'when project has no active milestone hooks' do - it_behaves_like 'closes the milestone', with_project_hooks: false + it_behaves_like 'closes the milestone', with_hooks: false, with_event: true end end @@ -68,14 +46,12 @@ context 'when milestone is already closed' do let(:milestone) { create(:milestone, :closed, project: project) } - it_behaves_like 'does not close the milestone' - end - - context 'when milestone is a group milestone' do - let(:group) { create(:group) } - let(:milestone) { create(:milestone, group: group) } + it 'does not execute hooks and does not create new event' do + expect(service).not_to receive(:execute_hooks) + expect(Event).not_to receive(:new) - it_behaves_like 'does not close the milestone' + expect { service.execute(milestone) }.not_to change { milestone.state } + end end end end diff --git a/spec/services/milestones/create_service_spec.rb b/spec/services/milestones/create_service_spec.rb index 1999b1d40392bb7befdea7614f188a0194150a80..c871433384ff050b05db838a21381f6433e66c40 100644 --- a/spec/services/milestones/create_service_spec.rb +++ b/spec/services/milestones/create_service_spec.rb @@ -4,15 +4,15 @@ RSpec.describe Milestones::CreateService, feature_category: :team_planning do let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project) } + let_it_be(:container) { create(:project) } let_it_be(:params) { { title: 'New Milestone', description: 'Description' } } - subject(:create_milestone) { described_class.new(project, user, params) } + subject(:service) { described_class.new(container, user, params) } describe '#execute' do context 'when milestone is saved successfully' do it 'creates a new milestone' do - expect { create_milestone.execute }.to change { Milestone.count }.by(1) + expect { service.execute }.to change { Milestone.count }.by(1) end it 'opens the milestone if it is a project milestone' do @@ -20,37 +20,26 @@ expect(instance).to receive(:open_milestone) end - create_milestone.execute + service.execute end it 'returns the created milestone' do - milestone = create_milestone.execute + milestone = service.execute expect(milestone).to be_a(Milestone) expect(milestone.title).to eq('New Milestone') expect(milestone.description).to eq('Description') end - shared_examples 'creates the milestone' do |with_project_hooks:| - it 'executes hooks with create action and creates new event' do - expect(create_milestone).to receive(:execute_hooks).with(kind_of(Milestone), 'create').and_call_original - expect(project).to receive(:execute_hooks).with(kind_of(Hash), :milestone_hooks) if with_project_hooks - - expect { create_milestone.execute }.to change { Event.count }.by(1) - end - end - context 'when project has active milestone hooks' do - let(:project) do - create(:project).tap do |project| - create(:project_hook, project: project, milestone_events: true) - end + before do + allow(container).to receive(:has_active_hooks?).with(:milestone_hooks).and_return(true) end - it_behaves_like 'creates the milestone', with_project_hooks: true + it_behaves_like 'creates the milestone', with_hooks: true, with_event: true end context 'when project has no active milestone hooks' do - it_behaves_like 'creates the milestone', with_project_hooks: false + it_behaves_like 'creates the milestone', with_hooks: false, with_event: true end end @@ -62,47 +51,32 @@ end it 'does not create a new milestone' do - expect { create_milestone.execute }.not_to change { Milestone.count } + expect { service.execute }.not_to change { Milestone.count } end it 'does not open the milestone' do expect(EventCreateService).not_to receive(:open_milestone) - create_milestone.execute + service.execute end it 'does not execute hooks and does not create new event' do - expect(create_milestone).not_to receive(:execute_hooks) + expect(service).not_to receive(:execute_hooks) - expect { create_milestone.execute }.not_to change { Event.count } + expect { service.execute }.not_to change { Event.count } end it 'returns the unsaved milestone' do - milestone = create_milestone.execute + milestone = service.execute expect(milestone).to be_a(Milestone) expect(milestone.title).to eq('New Milestone') expect(milestone.persisted?).to be_falsey end end - context 'when milestone is a group milestone' do - let(:group) { create(:group) } - let(:group_service) { described_class.new(group, user, params) } - - it 'does not execute hooks for group milestones' do - milestone = build(:milestone, group: group) - allow(milestone).to receive(:save).and_return(true) - allow(group_service).to receive(:build_milestone).and_return(milestone) - - expect(group).not_to receive(:execute_hooks) - - group_service.execute - end - end - it 'calls before_create method' do - expect(create_milestone).to receive(:before_create) - create_milestone.execute + expect(service).to receive(:before_create) + service.execute end end diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index c76edd2648c361276265637c8326268b6233e839..1e83fa4c5701ed80bad6b53e5a029f364b09384d 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -27,7 +27,8 @@ updated_attributes: %w[milestone_id] ) - milestone.reload + expect { milestone.reload }.to raise_error ActiveRecord::RecordNotFound + issues.each do |issue| expect(issue.reload.milestone).to be_nil end @@ -36,7 +37,7 @@ end context 'on project milestones' do - let_it_be_with_reload(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) } + let(:milestone) { create(:milestone, title: 'Milestone v1.0', project: project) } let(:container) { project } @@ -74,8 +75,7 @@ end context 'on group milestones' do - let_it_be_with_reload(:milestone) { create(:milestone, group: group) } - + let(:milestone) { create(:milestone, group: group) } let(:container) { group } it 'deletes milestone' do @@ -87,8 +87,6 @@ it_behaves_like 'deletes milestone id from issuables' it 'does not log destroy event and does not run on-delete webhook' do - expect(service).not_to receive(:execute_hooks).with(milestone, 'delete') - expect { service.execute(milestone) }.not_to change { Event.count } end end diff --git a/spec/services/milestones/reopen_service_spec.rb b/spec/services/milestones/reopen_service_spec.rb index fc7874cbc84fb5a74965806bc0821a6dd65faec4..2dafd66eb2e1a0d3c7199cd494fc88be4bc0cb7e 100644 --- a/spec/services/milestones/reopen_service_spec.rb +++ b/spec/services/milestones/reopen_service_spec.rb @@ -32,53 +32,34 @@ end end - shared_examples 'reopens the milestone' do |with_project_hooks:| - it 'executes hooks with reopen action and creates new event' do - expect(service).to receive(:execute_hooks).with(milestone, 'reopen').and_call_original - expect(project).to receive(:execute_hooks).with(kind_of(Hash), :milestone_hooks) if with_project_hooks - - expect { service.execute(milestone) }.to change { Event.count }.by(1) - end - end - - shared_examples 'does not reopen the milestone' do - it 'does not execute hooks and does not create new event' do - expect(service).not_to receive(:execute_hooks) - - expect { service.execute(milestone) }.not_to change { Event.count } - end - end - context 'when milestone is successfully reopened' do - let(:milestone) { create(:milestone, :closed, project: project) } + context 'when milestone is a project milestone' do + let(:milestone) { create(:milestone, :closed, project: project) } - context 'when project has active milestone hooks' do - let(:project) do - create(:project).tap do |project| - create(:project_hook, project: project, milestone_events: true) + context 'when project has active milestone hooks' do + before do + allow(project).to receive(:has_active_hooks?).with(:milestone_hooks).and_return(true) end - end - it_behaves_like 'reopens the milestone', with_project_hooks: true - end - - context 'when project has no active milestone hooks' do - it_behaves_like 'reopens the milestone', with_project_hooks: false - end - end + it_behaves_like 'reopens the milestone', with_hooks: true, with_event: true + end - context 'when milestone fails to reopen' do - context 'when milestone is already active' do - let(:milestone) { create(:milestone, project: project) } + context 'when project has no active milestone hooks' do + it_behaves_like 'reopens the milestone', with_hooks: false, with_event: true + end - it_behaves_like 'does not reopen the milestone' - end + context 'when milestone fails to reopen' do + context 'when milestone is already active' do + let(:milestone) { create(:milestone, project: project) } - context 'when milestone is a group milestone' do - let(:group) { create(:group) } - let(:milestone) { create(:milestone, :closed, group: group) } + it 'does not execute hooks and does not create new event' do + expect(service).not_to receive(:execute_hooks) + expect { service.execute(milestone) }.not_to change { Event.count } - it_behaves_like 'does not reopen the milestone' + expect { service.execute(milestone) }.not_to change { milestone.state } + end + end + end end end end diff --git a/spec/support/shared_examples/services/milestones_shared_examples.rb b/spec/support/shared_examples/services/milestones_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..41403d132be7b03553ab2d02550d46d07e178682 --- /dev/null +++ b/spec/support/shared_examples/services/milestones_shared_examples.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'creates the milestone' do |with_hooks:, with_event:| + it 'conditionally executes hooks with create action and creates new event' do + if with_hooks + expect(container).to receive(:execute_hooks).with(a_hash_including(action: 'create'), + :milestone_hooks) + end + + expect(Event).to receive(:new).exactly(1).time.and_call_original if with_event + + expect { service.execute }.to change { Milestone.count }.by(1) + end +end + +RSpec.shared_examples 'reopens the milestone' do |with_hooks:, with_event:| + it 'conditionally executes hooks with reopen action and creates new event' do + if with_hooks + expect(milestone.parent).to receive(:execute_hooks).with(a_hash_including(action: 'reopen'), + :milestone_hooks) + end + + expect(Event).to receive(:new).exactly(1).time.and_call_original if with_event + + expect { service.execute(milestone) }.to change { milestone.state }.to('active') + end +end + +RSpec.shared_examples 'closes the milestone' do |with_hooks:, with_event:| + it 'conditionally executes hooks with close action and creates new event' do + if with_hooks + expect(milestone.parent).to receive(:execute_hooks).with(a_hash_including(action: 'close'), + :milestone_hooks) + end + + expect(Event).to receive(:new).exactly(1).time.and_call_original if with_event + + expect { service.execute(milestone) }.to change { milestone.state }.to('closed') + end +end