Automatic merge of gitlab-org/gitlab master

app/graphql/mutations/ci/job_token_scope/remove_project.rb

+# frozen_string_literal: true
+
+module Mutations
+  module Ci
+    module JobTokenScope
+      class RemoveProject < BaseMutation
+        include FindsProject
+
+        graphql_name 'CiJobTokenScopeRemoveProject'
+
+        authorize :admin_project
+
+        argument :project_path, GraphQL::ID_TYPE,
+                 required: true,
+                 description: 'The project that the CI job token scope belongs to.'
+
+        argument :target_project_path, GraphQL::ID_TYPE,
+                 required: true,
+                 description: 'The project to be removed from the CI job token scope.'
+
+        field :ci_job_token_scope,
+          Types::Ci::JobTokenScopeType,
+          null: true,
+          description: "The CI job token's scope of access."
+
+        def resolve(project_path:, target_project_path:)
+          project = authorized_find!(project_path)
+          target_project = Project.find_by_full_path(target_project_path)
+
+          result = ::Ci::JobTokenScope::RemoveProjectService
+            .new(project, current_user)
+            .execute(target_project)
+
+          if result.success?
+            {
+              ci_job_token_scope: ::Ci::JobToken::Scope.new(project),
+              errors: []
+            }
+          else
+            {
+              ci_job_token_scope: nil,
+              errors: [result.message]
+            }
+          end
+        end
+      end
+    end
+  end
+end

app/graphql/types/mutation_type.rb

@@ -100,6 +100,7 @@ module Types
     mount_mutation Mutations::Ci::Job::Play
     mount_mutation Mutations::Ci::Job::Retry
     mount_mutation Mutations::Ci::JobTokenScope::AddProject
+    mount_mutation Mutations::Ci::JobTokenScope::RemoveProject
     mount_mutation Mutations::Ci::Runner::Update, feature_flag: :runner_graphql_query
     mount_mutation Mutations::Ci::Runner::Delete, feature_flag: :runner_graphql_query
     mount_mutation Mutations::Ci::RunnersRegistrationToken::Reset, feature_flag: :runner_graphql_query

app/models/ci/job_token/project_scope_link.rb

@@ -19,6 +19,10 @@ module Ci
       validates :target_project, presence: true
       validate :not_self_referential_link
 
+      def self.for_source_and_target(source_project, target_project)
+        self.find_by(source_project: source_project, target_project: target_project)
+      end
+
       private
 
       def not_self_referential_link

app/models/concerns/issuable.rb

@@ -459,6 +459,7 @@ module Issuable
     if old_associations
       old_labels = old_associations.fetch(:labels, labels)
       old_assignees = old_associations.fetch(:assignees, assignees)
+      old_severity = old_associations.fetch(:severity, severity)
 
       if old_labels != labels
         changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
@@ -468,6 +469,10 @@ module Issuable
         changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
       end
 
+      if supports_severity? && old_severity != severity
+        changes[:severity] = [old_severity, severity]
+      end
+
       if self.respond_to?(:total_time_spent)
         old_total_time_spent = old_associations.fetch(:total_time_spent, total_time_spent)
         old_time_change = old_associations.fetch(:time_change, time_change)

app/models/issue.rb

@@ -80,6 +80,7 @@ class Issue < ApplicationRecord
   has_and_belongs_to_many :prometheus_alert_events, join_table: :issues_prometheus_alert_events # rubocop: disable Rails/HasAndBelongsToMany
   has_many :prometheus_alerts, through: :prometheus_alert_events
 
+  accepts_nested_attributes_for :issuable_severity, update_only: true
   accepts_nested_attributes_for :sentry_issue
 
   validates :project, presence: true

app/services/ci/job_token_scope/add_project_service.rb

@@ -3,13 +3,10 @@
 module Ci
   module JobTokenScope
     class AddProjectService < ::BaseService
-      TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND = "The target_project that you are attempting to access does " \
-          "not exist or you don't have permission to perform this action"
+      include EditScopeValidations
 
       def execute(target_project)
-        if error_response = validation_error(target_project)
-          return error_response
-        end
+        validate_edit!(project, target_project, current_user)
 
         link = add_project!(target_project)
         ServiceResponse.success(payload: { project_link: link })
@@ -18,28 +15,8 @@ module Ci
         ServiceResponse.error(message: "Target project is already in the job token scope")
       rescue ActiveRecord::RecordInvalid => e
         ServiceResponse.error(message: e.message)
-      end
-
-      private
-
-      def validation_error(target_project)
-        unless project.ci_job_token_scope_enabled?
-          return ServiceResponse.error(message: "Job token scope is disabled for this project")
-        end
-
-        unless can?(current_user, :admin_project, project)
-          return ServiceResponse.error(message: "Insufficient permissions to modify the job token scope")
-        end
-
-        unless target_project
-          return ServiceResponse.error(message: TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND)
-        end
-
-        unless can?(current_user, :read_project, target_project)
-          return ServiceResponse.error(message: TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND)
-        end
-
-        nil
+      rescue EditScopeValidations::ValidationError => e
+        ServiceResponse.error(message: e.message)
       end
 
       def add_project!(target_project)

app/services/ci/job_token_scope/remove_project_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module JobTokenScope
+    class RemoveProjectService < ::BaseService
+      include EditScopeValidations
+
+      def execute(target_project)
+        validate_edit!(project, target_project, current_user)
+
+        if project == target_project
+          return ServiceResponse.error(message: "Source project cannot be removed from the job token scope")
+        end
+
+        link = ::Ci::JobToken::ProjectScopeLink.for_source_and_target(project, target_project)
+
+        unless link
+          return ServiceResponse.error(message: "Target project is not in the job token scope")
+        end
+
+        if link.destroy
+          ServiceResponse.success
+        else
+          ServiceResponse.error(message: link.errors.full_messages.to_sentence, payload: { project_link: link })
+        end
+      rescue EditScopeValidations::ValidationError => e
+        ServiceResponse.error(message: e.message)
+      end
+    end
+  end
+end

app/services/concerns/ci/job_token_scope/edit_scope_validations.rb

+# frozen_string_literal: true
+
+module Ci
+  module JobTokenScope
+    module EditScopeValidations
+      ValidationError = Class.new(StandardError)
+
+      TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND = "The target_project that you are attempting to access does " \
+          "not exist or you don't have permission to perform this action"
+
+      def validate_edit!(source_project, target_project, current_user)
+        unless source_project.ci_job_token_scope_enabled?
+          raise ValidationError, "Job token scope is disabled for this project"
+        end
+
+        unless can?(current_user, :admin_project, source_project)
+          raise ValidationError, "Insufficient permissions to modify the job token scope"
+        end
+
+        unless can?(current_user, :read_project, target_project)
+          raise ValidationError, TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND
+        end
+      end
+    end
+  end
+end

app/services/incident_management/incidents/create_service.rb

@@ -20,15 +20,14 @@ module IncidentManagement
           params: {
             title: title,
             description: description,
-            issue_type: ISSUE_TYPE
+            issue_type: ISSUE_TYPE,
+            severity: severity
           },
           spam_params: nil
         ).execute
 
         return error(issue.errors.full_messages.to_sentence, issue) unless issue.valid?
 
-        update_severity_for(issue)
-
         success(issue)
       end
 
@@ -43,10 +42,6 @@ module IncidentManagement
       def error(message, issue = nil)
         ServiceResponse.error(payload: { issue: issue }, message: message)
       end
-
-      def update_severity_for(issue)
-        ::IncidentManagement::Incidents::UpdateSeverityService.new(issue, current_user, severity).execute
-      end
     end
   end
 end

app/services/incident_management/incidents/update_severity_service.rb

-# frozen_string_literal: true
-
-module IncidentManagement
-  module Incidents
-    class UpdateSeverityService < BaseService
-      def initialize(issuable, current_user, severity)
-        super(issuable.project, current_user)
-
-        @issuable = issuable
-        @severity = severity.to_s.downcase
-        @severity = IssuableSeverity::DEFAULT unless IssuableSeverity.severities.key?(@severity)
-      end
-
-      def execute
-        return unless issuable.supports_severity?
-
-        update_severity!
-        add_system_note
-      end
-
-      private
-
-      attr_reader :issuable, :severity
-
-      def issuable_severity
-        issuable.issuable_severity || issuable.build_issuable_severity(issue_id: issuable.id)
-      end
-
-      def update_severity!
-        issuable_severity.update!(severity: severity)
-      end
-
-      def add_system_note
-        ::IncidentManagement::AddSeveritySystemNoteWorker.perform_async(issuable.id, current_user.id)
-      end
-    end
-  end
-end

app/services/issuable_base_service.rb

@@ -57,6 +57,7 @@ class IssuableBaseService < ::BaseProjectService
     filter_assignees(issuable)
     filter_milestone
     filter_labels
+    filter_severity(issuable)
   end
 
   def filter_assignees(issuable)
@@ -135,6 +136,16 @@ class IssuableBaseService < ::BaseProjectService
     @labels_service ||= ::Labels::AvailableLabelsService.new(current_user, parent, params)
   end
 
+  def filter_severity(issuable)
+    severity = params.delete(:severity)
+    return unless severity && issuable.supports_severity?
+
+    severity = IssuableSeverity::DEFAULT unless IssuableSeverity.severities.key?(severity)
+    return if severity == issuable.severity
+
+    params[:issuable_severity_attributes] = { severity: severity }
+  end
+
   def process_label_ids(attributes, existing_label_ids: nil, extra_label_ids: [])
     label_ids = attributes.delete(:label_ids)
     add_label_ids = attributes.delete(:add_label_ids)
@@ -352,7 +363,6 @@ class IssuableBaseService < ::BaseProjectService
 
   def change_additional_attributes(issuable)
     change_state(issuable)
-    change_severity(issuable)
     change_subscription(issuable)
     change_todo(issuable)
     toggle_award(issuable)
@@ -371,12 +381,6 @@ class IssuableBaseService < ::BaseProjectService
     end
   end
 
-  def change_severity(issuable)
-    if severity = params.delete(:severity)
-      ::IncidentManagement::Incidents::UpdateSeverityService.new(issuable, current_user, severity).execute
-    end
-  end
-
   def change_subscription(issuable)
     case params.delete(:subscription_event)
     when 'subscribe'
@@ -443,6 +447,7 @@ class IssuableBaseService < ::BaseProjectService
     associations[:time_change] = issuable.time_change if issuable.respond_to?(:time_change)
     associations[:description] = issuable.description
     associations[:reviewers] = issuable.reviewers.to_a if issuable.allows_reviewers?
+    associations[:severity] = issuable.severity if issuable.supports_severity?
 
     associations
   end

app/services/issues/update_service.rb

@@ -42,6 +42,7 @@ module Issues
       old_labels = old_associations.fetch(:labels, [])
       old_mentioned_users = old_associations.fetch(:mentioned_users, [])
       old_assignees = old_associations.fetch(:assignees, [])
+      old_severity = old_associations[:severity]
 
       if has_changes?(issue, old_labels: old_labels, old_assignees: old_assignees)
         todo_service.resolve_todos_for_target(issue, current_user)
@@ -74,6 +75,8 @@ module Issues
       if added_mentions.present?
         notification_service.async.new_mentions_in_issue(issue, added_mentions, current_user)
       end
+
+      handle_severity_change(issue, old_severity)
     end
 
     def handle_assignee_changes(issue, old_assignees)
@@ -181,6 +184,12 @@ module Issues
       end
     end
 
+    def handle_severity_change(issue, old_severity)
+      return unless old_severity && issue.severity != old_severity
+
+      ::IncidentManagement::AddSeveritySystemNoteWorker.perform_async(issue.id, current_user.id)
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
     def issuable_for_positioning(id, board_group_id = nil)
       return unless id

doc/api/graphql/reference/index.md

@@ -800,6 +800,26 @@ Input type: `CiJobTokenScopeAddProjectInput`
 | <a id="mutationcijobtokenscopeaddprojectclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
 | <a id="mutationcijobtokenscopeaddprojecterrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
 
+### `Mutation.ciJobTokenScopeRemoveProject`
+
+Input type: `CiJobTokenScopeRemoveProjectInput`
+
+#### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationcijobtokenscoperemoveprojectclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationcijobtokenscoperemoveprojectprojectpath"></a>`projectPath` | [`ID!`](#id) | The project that the CI job token scope belongs to. |
+| <a id="mutationcijobtokenscoperemoveprojecttargetprojectpath"></a>`targetProjectPath` | [`ID!`](#id) | The project to be removed from the CI job token scope. |
+
+#### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationcijobtokenscoperemoveprojectcijobtokenscope"></a>`ciJobTokenScope` | [`CiJobTokenScopeType`](#cijobtokenscopetype) | The CI job token's scope of access. |
+| <a id="mutationcijobtokenscoperemoveprojectclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationcijobtokenscoperemoveprojecterrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
+
 ### `Mutation.clusterAgentDelete`
 
 Input type: `ClusterAgentDeleteInput`

lib/gitlab/hook_data/issue_builder.rb

@@ -8,6 +8,7 @@ module Gitlab
         labels
         total_time_spent
         time_change
+        severity
       ].freeze
 
       def self.safe_hook_attributes
@@ -51,7 +52,8 @@ module Gitlab
             assignee_ids: issue.assignee_ids,
             assignee_id: issue.assignee_ids.first, # This key is deprecated
             labels: issue.labels_hook_attrs,
-            state: issue.state
+            state: issue.state,
+            severity: issue.severity
         }
 
         issue.attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes)

spec/graphql/mutations/ci/job_token_scope/remove_project_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Mutations::Ci::JobTokenScope::RemoveProject do
+  let(:mutation) do
+    described_class.new(object: nil, context: { current_user: current_user }, field: nil)
+  end
+
+  describe '#resolve' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:target_project) { create(:project) }
+
+    let_it_be(:link) do
+      create(:ci_job_token_project_scope_link,
+        source_project: project,
+        target_project: target_project)
+    end
+
+    let(:target_project_path) { target_project.full_path }
+
+    subject do
+      mutation.resolve(project_path: project.full_path, target_project_path: target_project_path)
+    end
+
+    context 'when user is not logged in' do
+      let(:current_user) { nil }
+
+      it 'raises error' do
+        expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+      end
+    end
+
+    context 'when user is logged in' do
+      let(:current_user) { create(:user) }
+
+      context 'when user does not have permissions to admin project' do
+        it 'raises error' do
+          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+        end
+      end
+
+      context 'when user has permissions to admin project and read target project' do
+        before do
+          project.add_maintainer(current_user)
+          target_project.add_guest(current_user)
+        end
+
+        it 'removes target project from the job token scope' do
+          expect do
+            expect(subject).to include(ci_job_token_scope: be_present, errors: be_empty)
+          end.to change { Ci::JobToken::ProjectScopeLink.count }.by(-1)
+        end
+
+        context 'when the service returns an error' do
+          let(:service) { double(:service) }
+
+          it 'returns an error response' do
+            expect(::Ci::JobTokenScope::RemoveProjectService).to receive(:new).with(project, current_user).and_return(service)
+            expect(service).to receive(:execute).with(target_project).and_return(ServiceResponse.error(message: 'The error message'))
+
+            expect(subject.fetch(:ci_job_token_scope)).to be_nil
+            expect(subject.fetch(:errors)).to include("The error message")
+          end
+        end
+      end
+    end
+  end
+end

spec/graphql/mutations/issues/set_severity_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Mutations::Issues::SetSeverity do
   specify { expect(described_class).to require_graphql_authorizations(:update_issue) }
 
   describe '#resolve' do
-    let(:severity) { 'CRITICAL' }
+    let(:severity) { 'critical' }
     let(:mutated_incident) { subject[:issue] }
 
     subject(:resolve) { mutation.resolve(project_path: issue.project.full_path, iid: issue.iid, severity: severity) }

spec/lib/gitlab/hook_data/issue_builder_spec.rb

@@ -48,6 +48,7 @@ RSpec.describe Gitlab::HookData::IssueBuilder do
       expect(data).to include(:human_time_change)
       expect(data).to include(:assignee_ids)
       expect(data).to include(:state)
+      expect(data).to include(:severity)
       expect(data).to include('labels' => [label.hook_attrs])
     end
 

spec/models/ci/job_token/project_scope_link_spec.rb

@@ -65,4 +65,22 @@ RSpec.describe Ci::JobToken::ProjectScopeLink do
       expect(subject).to contain_exactly(target_link)
     end
   end
+
+  describe '.for_source_and_target' do
+    let_it_be(:link) { create(:ci_job_token_project_scope_link, source_project: project) }
+
+    subject { described_class.for_source_and_target(project, target_project) }
+
+    context 'when link is found' do
+      let(:target_project) { link.target_project }
+
+      it { is_expected.to eq(link) }
+    end
+
+    context 'when link is not found' do
+      let(:target_project) { create(:project) }
+
+      it { is_expected.to be_nil }
+    end
+  end
 end

spec/models/concerns/issuable_spec.rb

@@ -535,6 +535,26 @@ RSpec.describe Issuable do
         merge_request.to_hook_data(user, old_associations: { assignees: [user] })
       end
     end
+
+    context 'incident severity is updated' do
+      let(:issue) { create(:incident) }
+
+      before do
+        issue.update!(issuable_severity_attributes: { severity: 'low' })
+        expect(Gitlab::HookData::IssuableBuilder)
+          .to receive(:new).with(issue).and_return(builder)
+      end
+
+      it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
+        expect(builder).to receive(:build).with(
+          user: user,
+          changes: hash_including(
+            'severity' => %w(unknown low)
+          ))
+
+        issue.to_hook_data(user, old_associations: { severity: 'unknown' })
+      end
+    end
   end
 
   describe '#labels_array' do

spec/models/integrations/buildkite_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
 
   let(:project) { create(:project) }
 
-  subject(:service) do
+  subject(:integration) do
     described_class.create!(
       project: project,
       properties: {
@@ -25,17 +25,17 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
   end
 
   describe 'Validations' do
-    context 'when service is active' do
+    context 'when integration is active' do
       before do
         subject.active = true
       end
 
       it { is_expected.to validate_presence_of(:project_url) }
       it { is_expected.to validate_presence_of(:token) }
-      it_behaves_like 'issue tracker service URL attribute', :project_url
+      it_behaves_like 'issue tracker integration URL attribute', :project_url
     end
 
-    context 'when service is inactive' do
+    context 'when integration is inactive' do
       before do
         subject.active = false
       end
@@ -47,7 +47,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
 
   describe '.supported_events' do
     it 'supports push, merge_request, and tag_push events' do
-      expect(service.supported_events).to eq %w(push merge_request tag_push)
+      expect(integration.supported_events).to eq %w(push merge_request tag_push)
     end
   end
 
@@ -57,18 +57,18 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
     end
 
     it 'always activates SSL verification after saved' do
-      service.create_service_hook(enable_ssl_verification: false)
+      integration.create_service_hook(enable_ssl_verification: false)
 
-      service.enable_ssl_verification = false
-      service.active = true
+      integration.enable_ssl_verification = false
+      integration.active = true
 
-      expect { service.save! }
-        .to change { service.service_hook.enable_ssl_verification }.from(false).to(true)
+      expect { integration.save! }
+        .to change { integration.service_hook.enable_ssl_verification }.from(false).to(true)
     end
 
     describe '#webhook_url' do
       it 'returns the webhook url' do
-        expect(service.webhook_url).to eq(
+        expect(integration.webhook_url).to eq(
           'https://webhook.buildkite.com/deliver/secret-sauce-webhook-token'
         )
       end
@@ -76,7 +76,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
 
     describe '#commit_status_path' do
       it 'returns the correct status page' do
-        expect(service.commit_status_path('2ab7834c')).to eq(
+        expect(integration.commit_status_path('2ab7834c')).to eq(
           'https://gitlab.buildkite.com/status/secret-sauce-status-token.json?commit=2ab7834c'
         )
       end
@@ -84,7 +84,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
 
     describe '#build_page' do
       it 'returns the correct build page' do
-        expect(service.build_page('2ab7834c', nil)).to eq(
+        expect(integration.build_page('2ab7834c', nil)).to eq(
           'https://buildkite.com/organization-name/example-pipeline/builds?commit=2ab7834c'
         )
       end
@@ -92,9 +92,9 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
 
     describe '#commit_status' do
       it 'returns the contents of the reactive cache' do
-        stub_reactive_cache(service, { commit_status: 'foo' }, 'sha', 'ref')
+        stub_reactive_cache(integration, { commit_status: 'foo' }, 'sha', 'ref')
 
-        expect(service.commit_status('sha', 'ref')).to eq('foo')
+        expect(integration.commit_status('sha', 'ref')).to eq('foo')
       end
     end
 
@@ -104,7 +104,7 @@ RSpec.describe Integrations::Buildkite, :use_clean_rails_memory_store_caching do
           'https://gitlab.buildkite.com/status/secret-sauce-status-token.json?commit=123'
         end
 
-        subject { service.calculate_reactive_cache('123', 'unused')[:commit_status] }
+        subject { integration.calculate_reactive_cache('123', 'unused')[:commit_status] }
 
         it 'sets commit status to :error when status is 500' do
           stub_request(status: 500)

spec/models/integrations/custom_issue_tracker_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Integrations::CustomIssueTracker do
   end
 
   describe 'Validations' do
-    context 'when service is active' do
+    context 'when integration is active' do
       before do
         subject.active = true
       end
@@ -17,12 +17,12 @@ RSpec.describe Integrations::CustomIssueTracker do
       it { is_expected.to validate_presence_of(:project_url) }
       it { is_expected.to validate_presence_of(:issues_url) }
       it { is_expected.to validate_presence_of(:new_issue_url) }
-      it_behaves_like 'issue tracker service URL attribute', :project_url
-      it_behaves_like 'issue tracker service URL attribute', :issues_url
-      it_behaves_like 'issue tracker service URL attribute', :new_issue_url
+      it_behaves_like 'issue tracker integration URL attribute', :project_url
+      it_behaves_like 'issue tracker integration URL attribute', :issues_url
+      it_behaves_like 'issue tracker integration URL attribute', :new_issue_url
     end
 
-    context 'when service is inactive' do
+    context 'when integration is inactive' do
       before do
         subject.active = false
       end

spec/models/integrations/emails_on_push_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Integrations::EmailsOnPush do
   let_it_be(:project) { create_default(:project).freeze }
 
   describe 'Validations' do
-    context 'when service is active' do
+    context 'when integration is active' do
       before do
         subject.active = true
       end
@@ -14,7 +14,7 @@ RSpec.describe Integrations::EmailsOnPush do
       it { is_expected.to validate_presence_of(:recipients) }
     end
 
-    context 'when service is inactive' do
+    context 'when integration is inactive' do
       before do
         subject.active = false
       end
@@ -27,7 +27,7 @@ RSpec.describe Integrations::EmailsOnPush do
         stub_const("#{described_class}::RECIPIENTS_LIMIT", 2)
       end
 
-      subject(:service) { described_class.new(project: project, recipients: recipients, active: true) }
+      subject(:integration) { described_class.new(project: project, recipients: recipients, active: true) }
 
       context 'valid number of recipients' do
         let(:recipients) { 'foo@bar.com duplicate@example.com Duplicate@example.com invalid-email' }
@@ -43,14 +43,14 @@ RSpec.describe Integrations::EmailsOnPush do
         it { is_expected.not_to be_valid }
 
         it 'adds an error message' do
-          service.valid?
+          integration.valid?
 
-          expect(service.errors).to contain_exactly('Recipients can\'t exceed 2')
+          expect(integration.errors).to contain_exactly('Recipients can\'t exceed 2')
         end
 
-        context 'when service is not active' do
+        context 'when integration is not active' do
           before do
-            service.active = false
+            integration.active = false
           end
 
           it { is_expected.to be_valid }

spec/models/integrations/open_project_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Integrations::OpenProject do
   describe 'Validations' do
-    context 'when service is active' do
+    context 'when integration is active' do
       before do
         subject.active = true
       end
@@ -13,11 +13,11 @@ RSpec.describe Integrations::OpenProject do
       it { is_expected.to validate_presence_of(:token) }
       it { is_expected.to validate_presence_of(:project_identifier_code) }
 
-      it_behaves_like 'issue tracker service URL attribute', :url
-      it_behaves_like 'issue tracker service URL attribute', :api_url
+      it_behaves_like 'issue tracker integration URL attribute', :url
+      it_behaves_like 'issue tracker integration URL attribute', :api_url
     end
 
-    context 'when service is inactive' do
+    context 'when integration is inactive' do
       before do
         subject.active = false
       end

spec/models/integrations/pivotaltracker_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Integrations::Pivotaltracker do
   end
 
   describe 'Validations' do
-    context 'when service is active' do
+    context 'when integration is active' do
       before do
         subject.active = true
       end
@@ -19,7 +19,7 @@ RSpec.describe Integrations::Pivotaltracker do
       it { is_expected.to validate_presence_of(:token) }
     end
 
-    context 'when service is inactive' do
+    context 'when integration is inactive' do
       before do
         subject.active = false
       end
@@ -29,9 +29,9 @@ RSpec.describe Integrations::Pivotaltracker do
   end
 
   describe 'Execute' do
-    let(:service) do
-      described_class.new.tap do |service|
-        service.token = 'secret_api_token'
+    let(:integration) do
+      described_class.new.tap do |integration|
+        integration.token = 'secret_api_token'
       end
     end
 
@@ -59,7 +59,7 @@ RSpec.describe Integrations::Pivotaltracker do
     end
 
     it 'posts correct message' do
-      service.execute(push_data)
+      integration.execute(push_data)
       expect(WebMock).to have_requested(:post, stubbed_hostname(url)).with(
         body: {
           'source_commit' => {
@@ -77,22 +77,22 @@ RSpec.describe Integrations::Pivotaltracker do
     end
 
     context 'when allowed branches is specified' do
-      let(:service) do
-        super().tap do |service|
-          service.restrict_to_branch = 'master,v10'
+      let(:integration) do
+        super().tap do |integration|
+          integration.restrict_to_branch = 'master,v10'
         end
       end
 
       it 'posts message if branch is in the list' do
-        service.execute(push_data(branch: 'master'))
-        service.execute(push_data(branch: 'v10'))
+        integration.execute(push_data(branch: 'master'))
+        integration.execute(push_data(branch: 'v10'))
 
         expect(WebMock).to have_requested(:post, stubbed_hostname(url)).twice
       end
 
       it 'does not post message if branch is not in the list' do
-        service.execute(push_data(branch: 'mas'))
-        service.execute(push_data(branch: 'v11'))
+        integration.execute(push_data(branch: 'mas'))
+        integration.execute(push_data(branch: 'v11'))
 
         expect(WebMock).not_to have_requested(:post, stubbed_hostname(url))
       end

spec/models/integrations/youtrack_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Integrations::Youtrack do
   end
 
   describe 'Validations' do
-    context 'when service is active' do
+    context 'when integration is active' do
       before do
         subject.active = true
       end
@@ -17,11 +17,11 @@ RSpec.describe Integrations::Youtrack do
       it { is_expected.to validate_presence_of(:project_url) }
       it { is_expected.to validate_presence_of(:issues_url) }
 
-      it_behaves_like 'issue tracker service URL attribute', :project_url
-      it_behaves_like 'issue tracker service URL attribute', :issues_url
+      it_behaves_like 'issue tracker integration URL attribute', :project_url
+      it_behaves_like 'issue tracker integration URL attribute', :issues_url
     end
 
-    context 'when service is inactive' do
+    context 'when integration is inactive' do
       before do
         subject.active = false
       end

spec/requests/api/graphql/mutations/ci/job_token_scope/add_project_spec.rb

@@ -71,7 +71,7 @@ RSpec.describe 'CiJobTokenScopeAddProject' do
       it 'has mutation errors' do
         post_graphql_mutation(mutation, current_user: current_user)
 
-        expect(mutation_response['errors']).to contain_exactly(Ci::JobTokenScope::AddProjectService::TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND)
+        expect(mutation_response['errors']).to contain_exactly(Ci::JobTokenScope::EditScopeValidations::TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND)
       end
     end
   end

spec/requests/api/graphql/mutations/ci/job_token_scope/remove_project_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'CiJobTokenScopeRemoveProject' do
+  include GraphqlHelpers
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:target_project) { create(:project) }
+
+  let_it_be(:link) do
+    create(:ci_job_token_project_scope_link,
+      source_project: project,
+      target_project: target_project)
+  end
+
+  let(:variables) do
+    {
+      project_path: project.full_path,
+      target_project_path: target_project.full_path
+    }
+  end
+
+  let(:mutation) do
+    graphql_mutation(:ci_job_token_scope_remove_project, variables) do
+      <<~QL
+        errors
+        ciJobTokenScope {
+          projects {
+            nodes {
+              path
+            }
+          }
+        }
+      QL
+    end
+  end
+
+  let(:mutation_response) { graphql_mutation_response(:ci_job_token_scope_remove_project) }
+
+  context 'when unauthorized' do
+    let(:current_user) { create(:user) }
+
+    context 'when not a maintainer' do
+      before do
+        project.add_developer(current_user)
+      end
+
+      it 'has graphql errors' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(graphql_errors).not_to be_empty
+      end
+    end
+  end
+
+  context 'when authorized' do
+    let_it_be(:current_user) { project.owner }
+
+    before do
+      target_project.add_guest(current_user)
+    end
+
+    it 'removes the target project from the job token scope' do
+      expect do
+        post_graphql_mutation(mutation, current_user: current_user)
+        expect(response).to have_gitlab_http_status(:success)
+        expect(mutation_response.dig('ciJobTokenScope', 'projects', 'nodes')).not_to be_empty
+      end.to change { Ci::JobToken::Scope.new(project).includes?(target_project) }.from(true).to(false)
+    end
+
+    context 'when invalid target project is provided' do
+      before do
+        variables[:target_project_path] = 'unknown/project'
+      end
+
+      it 'has mutation errors' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(mutation_response['errors']).to contain_exactly(Ci::JobTokenScope::EditScopeValidations::TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND)
+      end
+    end
+  end
+end

spec/services/ci/job_token_scope/add_project_service_spec.rb

@@ -11,68 +11,28 @@ RSpec.describe Ci::JobTokenScope::AddProjectService do
   describe '#execute' do
     subject(:result) { service.execute(target_project) }
 
-    shared_examples 'returns error' do |error|
-      it 'returns an error response', :aggregate_failures do
-        expect(result).to be_error
-        expect(result.message).to eq(error)
-      end
-    end
-
-    context 'when job token scope is disabled for the given project' do
-      before do
-        allow(project).to receive(:ci_job_token_scope_enabled?).and_return(false)
-      end
-
-      it_behaves_like 'returns error', 'Job token scope is disabled for this project'
-    end
-
-    context 'when user does not have permissions to edit the job token scope' do
-      it_behaves_like 'returns error', 'Insufficient permissions to modify the job token scope'
-    end
-
-    context 'when user has permissions to edit the job token scope' do
-      before do
-        project.add_maintainer(current_user)
-      end
-
-      context 'when target project is not provided' do
-        let(:target_project) { nil }
-
-        it_behaves_like 'returns error', Ci::JobTokenScope::AddProjectService::TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND
-      end
-
-      context 'when target project is provided' do
-        context 'when user does not have permissions to read the target project' do
-          it_behaves_like 'returns error', Ci::JobTokenScope::AddProjectService::TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND
+    it_behaves_like 'editable job token scope' do
+      context 'when user has permissions on source and target projects' do
+        before do
+          project.add_maintainer(current_user)
+          target_project.add_developer(current_user)
         end
 
-        context 'when user has permissions to read the target project' do
-          before do
-            target_project.add_guest(current_user)
-          end
-
-          it 'adds the project to the scope' do
-            expect do
-              expect(result).to be_success
-            end.to change { Ci::JobToken::ProjectScopeLink.count }.by(1)
-          end
-
-          context 'when target project is already in scope' do
-            before do
-              create(:ci_job_token_project_scope_link,
-                source_project: project,
-                target_project: target_project)
-            end
+        it 'adds the project to the scope' do
+          expect do
+            expect(result).to be_success
+          end.to change { Ci::JobToken::ProjectScopeLink.count }.by(1)
+        end
+      end
 
-            it_behaves_like 'returns error', "Target project is already in the job token scope"
-          end
+      context 'when target project is same as the source project' do
+        before do
+          project.add_maintainer(current_user)
         end
 
-        context 'when target project is same as the source project' do
-          let(:target_project) { project }
+        let(:target_project) { project }
 
-          it_behaves_like 'returns error', "Validation failed: Target project can't be the same as the source project"
-        end
+        it_behaves_like 'returns error', "Validation failed: Target project can't be the same as the source project"
       end
     end
   end

spec/services/ci/job_token_scope/remove_project_service_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Ci::JobTokenScope::RemoveProjectService do
+  let(:service) { described_class.new(project, current_user) }
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:target_project) { create(:project) }
+  let_it_be(:current_user) { create(:user) }
+
+  let_it_be(:link) do
+    create(:ci_job_token_project_scope_link,
+      source_project: project,
+      target_project: target_project)
+  end
+
+  describe '#execute' do
+    subject(:result) { service.execute(target_project) }
+
+    it_behaves_like 'editable job token scope' do
+      context 'when user has permissions on source and target project' do
+        before do
+          project.add_maintainer(current_user)
+          target_project.add_developer(current_user)
+        end
+
+        it 'removes the project from the scope' do
+          expect do
+            expect(result).to be_success
+          end.to change { Ci::JobToken::ProjectScopeLink.count }.by(-1)
+        end
+      end
+
+      context 'when target project is same as the source project' do
+        before do
+          project.add_maintainer(current_user)
+        end
+
+        let(:target_project) { project }
+
+        it_behaves_like 'returns error', "Source project cannot be removed from the job token scope"
+      end
+    end
+  end
+end

spec/services/incident_management/incidents/update_severity_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe IncidentManagement::Incidents::UpdateSeverityService do
-  let_it_be(:user) { create(:user) }
-
-  describe '#execute' do
-    let(:severity) { 'low' }
-    let(:system_note_worker) { ::IncidentManagement::AddSeveritySystemNoteWorker }
-
-    subject(:update_severity) { described_class.new(issuable, user, severity).execute }
-
-    before do
-      allow(system_note_worker).to receive(:perform_async)
-    end
-
-    shared_examples 'adds a system note' do
-      it 'calls AddSeveritySystemNoteWorker' do
-        update_severity
-
-        expect(system_note_worker).to have_received(:perform_async).with(issuable.id, user.id)
-      end
-    end
-
-    context 'when issuable not an incident' do
-      %i(issue merge_request).each do |issuable_type|
-        let(:issuable) { build_stubbed(issuable_type) }
-
-        it { is_expected.to be_nil }
-
-        it 'does not set severity' do
-          expect { update_severity }.not_to change(IssuableSeverity, :count)
-        end
-
-        it 'does not add a system note' do
-          update_severity
-
-          expect(system_note_worker).not_to have_received(:perform_async)
-        end
-      end
-    end
-
-    context 'when issuable is an incident' do
-      let!(:issuable) { create(:incident) }
-
-      context 'when issuable does not have issuable severity yet' do
-        it 'creates new record' do
-          expect { update_severity }.to change { IssuableSeverity.where(issue: issuable).count }.to(1)
-        end
-
-        it 'sets severity to specified value' do
-          expect { update_severity }.to change { issuable.severity }.to('low')
-        end
-
-        it_behaves_like 'adds a system note'
-      end
-
-      context 'when issuable has an issuable severity' do
-        let!(:issuable_severity) { create(:issuable_severity, issue: issuable, severity: 'medium') }
-
-        it 'does not create new record' do
-          expect { update_severity }.not_to change(IssuableSeverity, :count)
-        end
-
-        it 'updates existing issuable severity' do
-          expect { update_severity }.to change { issuable_severity.severity }.to(severity)
-        end
-
-        it_behaves_like 'adds a system note'
-      end
-
-      context 'when severity value is unsupported' do
-        let(:severity) { 'unsupported-severity' }
-
-        it 'sets the severity to default value' do
-          update_severity
-
-          expect(issuable.issuable_severity.severity).to eq(IssuableSeverity::DEFAULT)
-        end
-
-        it_behaves_like 'adds a system note'
-      end
-    end
-  end
-end

spec/services/issues/update_service_spec.rb

@@ -83,16 +83,16 @@ RSpec.describe Issues::UpdateService, :mailer do
       end
 
       context 'when issue type is not incident' do
-        it 'returns default severity' do
+        before do
           update_issue(opts)
-
-          expect(issue.severity).to eq(IssuableSeverity::DEFAULT)
         end
 
-        it_behaves_like 'not an incident issue' do
-          before do
-            update_issue(opts)
-          end
+        it_behaves_like 'not an incident issue'
+
+        context 'when confidentiality is changed' do
+          subject { update_issue(confidential: true) }
+
+          it_behaves_like 'does not track incident management event'
         end
       end
 
@@ -105,12 +105,16 @@ RSpec.describe Issues::UpdateService, :mailer do
 
         it_behaves_like 'incident issue'
 
-        it 'changes updates the severity' do
-          expect(issue.severity).to eq('low')
+        it 'does not add an incident label' do
+          expect(issue.labels).to match_array [label]
         end
 
-        it 'does not apply incident labels' do
-          expect(issue.labels).to match_array [label]
+        context 'when confidentiality is changed' do
+          let(:current_user) { user }
+
+          subject { update_issue(confidential: true) }
+
+          it_behaves_like 'an incident management tracked event', :incident_management_incident_change_confidential
         end
       end
 
@@ -140,24 +144,6 @@ RSpec.describe Issues::UpdateService, :mailer do
         expect(issue.confidential).to be_falsey
       end
 
-      context 'issue in incident type' do
-        let(:current_user) { user }
-
-        before do
-          opts.merge!(issue_type: 'incident', confidential: true)
-        end
-
-        subject { update_issue(opts) }
-
-        it_behaves_like 'an incident management tracked event', :incident_management_incident_change_confidential
-
-        it_behaves_like 'incident issue' do
-          before do
-            subject
-          end
-        end
-      end
-
       context 'changing issue_type' do
         let!(:label_1) { create(:label, project: project, title: 'incident') }
         let!(:label_2) { create(:label, project: project, title: 'missed-sla') }
@@ -167,6 +153,12 @@ RSpec.describe Issues::UpdateService, :mailer do
         end
 
         context 'from issue to incident' do
+          it_behaves_like 'incident issue' do
+            before do
+              update_issue(**opts, issue_type: 'incident')
+            end
+          end
+
           it 'adds a `incident` label if one does not exist' do
             expect { update_issue(issue_type: 'incident') }.to change(issue.labels, :count).by(1)
             expect(issue.labels.pluck(:title)).to eq(['incident'])
@@ -1020,6 +1012,101 @@ RSpec.describe Issues::UpdateService, :mailer do
       include_examples 'updating mentions', described_class
     end
 
+    context 'updating severity' do
+      let(:opts) { { severity: 'low' } }
+
+      shared_examples 'updates the severity' do |expected_severity|
+        it 'has correct value' do
+          update_issue(opts)
+
+          expect(issue.severity).to eq(expected_severity)
+        end
+
+        it 'creates a system note' do
+          expect(::IncidentManagement::AddSeveritySystemNoteWorker).to receive(:perform_async).with(issue.id, user.id)
+
+          update_issue(opts)
+        end
+
+        it 'triggers webhooks' do
+          expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
+          expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks)
+
+          update_issue(opts)
+        end
+      end
+
+      shared_examples 'does not change the severity' do
+        it 'retains the original value' do
+          expected_severity = issue.severity
+
+          update_issue(opts)
+
+          expect(issue.severity).to eq(expected_severity)
+        end
+
+        it 'does not trigger side-effects' do
+          expect(::IncidentManagement::AddSeveritySystemNoteWorker).not_to receive(:perform_async)
+          expect(project).not_to receive(:execute_hooks)
+          expect(project).not_to receive(:execute_integrations)
+
+          expect { update_issue(opts) }.not_to change(IssuableSeverity, :count)
+        end
+      end
+
+      context 'on incidents' do
+        let(:issue) { create(:incident, project: project) }
+
+        context 'when severity has not been set previously' do
+          it_behaves_like 'updates the severity', 'low'
+
+          it 'creates a new record' do
+            expect { update_issue(opts) }.to change(IssuableSeverity, :count).by(1)
+          end
+
+          context 'with unsupported severity value' do
+            let(:opts) { { severity: 'unsupported-severity' } }
+
+            it_behaves_like 'does not change the severity'
+          end
+
+          context 'with severity value defined but unchanged' do
+            let(:opts) { { severity: IssuableSeverity::DEFAULT } }
+
+            it_behaves_like 'does not change the severity'
+          end
+        end
+
+        context 'when severity has been set before' do
+          before do
+            create(:issuable_severity, issue: issue, severity: 'high')
+          end
+
+          it_behaves_like 'updates the severity', 'low'
+
+          it 'does not create a new record' do
+            expect { update_issue(opts) }.not_to change(IssuableSeverity, :count)
+          end
+
+          context 'with unsupported severity value' do
+            let(:opts) { { severity: 'unsupported-severity' } }
+
+            it_behaves_like 'updates the severity', IssuableSeverity::DEFAULT
+          end
+
+          context 'with severity value defined but unchanged' do
+            let(:opts) { { severity: issue.severity } }
+
+            it_behaves_like 'does not change the severity'
+          end
+        end
+      end
+
+      context 'when issue type is not incident' do
+        it_behaves_like 'does not change the severity'
+      end
+    end
+
     context 'duplicate issue' do
       let(:canonical_issue) { create(:issue, project: project) }
 

spec/support/shared_examples/ci/edit_job_token_scope_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'editable job token scope' do
+  shared_examples 'returns error' do |error|
+    it 'returns an error response', :aggregate_failures do
+      expect(result).to be_error
+      expect(result.message).to eq(error)
+    end
+  end
+
+  context 'when job token scope is disabled for the given project' do
+    before do
+      allow(project).to receive(:ci_job_token_scope_enabled?).and_return(false)
+    end
+
+    it_behaves_like 'returns error', 'Job token scope is disabled for this project'
+  end
+
+  context 'when user does not have permissions to edit the job token scope' do
+    it_behaves_like 'returns error', 'Insufficient permissions to modify the job token scope'
+  end
+
+  context 'when user has permissions to edit the job token scope' do
+    before do
+      project.add_maintainer(current_user)
+    end
+
+    context 'when target project is not provided' do
+      let(:target_project) { nil }
+
+      it_behaves_like 'returns error', Ci::JobTokenScope::EditScopeValidations::TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND
+    end
+
+    context 'when target project is provided' do
+      context 'when user does not have permissions to read the target project' do
+        it_behaves_like 'returns error', Ci::JobTokenScope::EditScopeValidations::TARGET_PROJECT_UNAUTHORIZED_OR_UNFOUND
+      end
+    end
+  end
+end