Merge branch '17461-remove-labels-on-issue-close' into 'master'

Remove specified labels on issue close

See merge request gitlab-org/gitlab!61286

app/graphql/mutations/labels/create.rb

@@ -20,6 +20,10 @@ module Mutations
                required: false,
                description: 'Description of the label.'
 
+      argument :remove_on_close, GraphQL::BOOLEAN_TYPE,
+               required: false,
+               description: copy_field_description(Types::LabelType, :remove_on_close)
+
       argument :color, GraphQL::STRING_TYPE,
                required: false,
                default_value: Label::DEFAULT_COLOR,

app/graphql/types/label_type.rb

@@ -23,5 +23,7 @@ module Types
           description: 'When this label was created.'
     field :updated_at, Types::TimeType, null: false,
           description: 'When this label was last updated.'
+    field :remove_on_close, GraphQL::BOOLEAN_TYPE, null: false,
+          description: 'Whether the label should be removed from an issue when the issue is closed.'
   end
 end

app/models/label_link.rb

@@ -11,4 +11,5 @@ class LabelLink < ApplicationRecord
   validates :label, presence: true, unless: :importing?
 
   scope :for_target, -> (target_id, target_type) { where(target_id: target_id, target_type: target_type) }
+  scope :with_remove_on_close_labels, -> { joins(:label).where(labels: { remove_on_close: true }) }
 end

app/services/issues/close_service.rb

@@ -25,6 +25,7 @@ module Issues
       end
 
       if project.issues_enabled? && issue.close(current_user)
+        remove_on_close_labels_from(issue)
         event_service.close_issue(issue, current_user)
         create_note(issue, closed_via) if system_note
 
@@ -51,6 +52,18 @@ module Issues
 
     private
 
+    def remove_on_close_labels_from(issue)
+      old_labels = issue.labels.to_a
+
+      issue.label_links.with_remove_on_close_labels.delete_all
+      issue.labels.reset
+
+      Issuable::CommonSystemNotesService.new(project: project, current_user: current_user).execute(
+        issue,
+        old_labels: old_labels
+      )
+    end
+
     def close_external_issue(issue, closed_via)
       return unless project.external_issue_tracker&.support_close_issue?
 

changelogs/unreleased/17461-remove-labels-on-issue-close.yml

+---
+title: Add option to remove labels on issue close in the REST and GraphQL API
+merge_request: 61286
+author:
+type: added

db/migrate/20210507191949_add_remove_on_issue_close_to_labels.rb

+# frozen_string_literal: true
+
+class AddRemoveOnIssueCloseToLabels < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  def up
+    with_lock_retries do
+      add_column :labels, :remove_on_close, :boolean, null: false, default: false
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :labels, :remove_on_close, :boolean
+    end
+  end
+end

db/schema_migrations/20210507191949

+ab662ff92a4e2a7d324f0652da6f0725e1c38e387f08b89921b43816b5d35834
\ No newline at end of file

db/structure.sql

@@ -14273,7 +14273,8 @@ CREATE TABLE labels (
     description_html text,
     type character varying,
     group_id integer,
-    cached_markdown_version integer
+    cached_markdown_version integer,
+    remove_on_close boolean DEFAULT false NOT NULL
 );
 
 CREATE SEQUENCE labels_id_seq

doc/api/graphql/reference/index.md

@@ -2700,6 +2700,7 @@ Input type: `LabelCreateInput`
 | <a id="mutationlabelcreatedescription"></a>`description` | [`String`](#string) | Description of the label. |
 | <a id="mutationlabelcreategrouppath"></a>`groupPath` | [`ID`](#id) | Full path of the group with which the resource is associated. |
 | <a id="mutationlabelcreateprojectpath"></a>`projectPath` | [`ID`](#id) | Full path of the project with which the resource is associated. |
+| <a id="mutationlabelcreateremoveonclose"></a>`removeOnClose` | [`Boolean`](#boolean) | Whether the label should be removed from an issue when the issue is closed. |
 | <a id="mutationlabelcreatetitle"></a>`title` | [`String!`](#string) | Title of the label. |
 
 #### Fields
@@ -9742,6 +9743,7 @@ four standard [pagination arguments](#connection-pagination-arguments):
 | <a id="labeldescription"></a>`description` | [`String`](#string) | Description of the label (Markdown rendered as HTML for caching). |
 | <a id="labeldescriptionhtml"></a>`descriptionHtml` | [`String`](#string) | The GitLab Flavored Markdown rendering of `description`. |
 | <a id="labelid"></a>`id` | [`ID!`](#id) | Label ID. |
+| <a id="labelremoveonclose"></a>`removeOnClose` | [`Boolean!`](#boolean) | Whether the label should be removed from an issue when the issue is closed. |
 | <a id="labeltextcolor"></a>`textColor` | [`String!`](#string) | Text color of the label. |
 | <a id="labeltitle"></a>`title` | [`String!`](#string) | Content of the label. |
 | <a id="labelupdatedat"></a>`updatedAt` | [`Time!`](#time) | When this label was last updated. |

doc/api/labels.md

@@ -46,7 +46,8 @@ Example response:
     "open_merge_requests_count": 1,
     "subscribed": false,
     "priority": 10,
-    "is_project_label": true
+    "is_project_label": true,
+    "remove_on_close": false
   },
   {
     "id" : 4,
@@ -60,7 +61,8 @@ Example response:
     "open_merge_requests_count": 0,
     "subscribed": false,
     "priority": null,
-    "is_project_label": true
+    "is_project_label": true,
+    "remove_on_close": false
   },
   {
     "id" : 7,
@@ -74,7 +76,8 @@ Example response:
     "open_merge_requests_count": 1,
     "subscribed": false,
     "priority": null,
-    "is_project_label": true
+    "is_project_label": true,
+    "remove_on_close": true
   },
   {
     "id" : 8,
@@ -88,7 +91,8 @@ Example response:
     "open_merge_requests_count": 2,
     "subscribed": false,
     "priority": null,
-    "is_project_label": false
+    "is_project_label": false,
+    "remove_on_close": false
   },
   {
     "id" : 9,
@@ -102,7 +106,8 @@ Example response:
     "open_merge_requests_count": 1,
     "subscribed": true,
     "priority": null,
-    "is_project_label": true
+    "is_project_label": true,
+    "remove_on_close": false
   }
 ]
 ```
@@ -140,7 +145,8 @@ Example response:
   "open_merge_requests_count": 1,
   "subscribed": false,
   "priority": 10,
-  "is_project_label": true
+  "is_project_label": true,
+  "remove_on_close": true
 }
 ```
 
@@ -159,6 +165,7 @@ POST /projects/:id/labels
 | `color`       | string  | yes      | The color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the [CSS color names](https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#Color_keywords) |
 | `description` | string  | no       | The description of the label |
 | `priority`    | integer | no       | The priority of the label. Must be greater or equal than zero or `null` to remove the priority. |
+| `remove_on_close` | boolean | no | Whether the label should be removed from an issue when the issue is closed. _([Introduced in GitLab 13.12](https://gitlab.com/gitlab-org/gitlab/-/issues/17461))_ |
 
 ```shell
 curl --data "name=feature&color=#5843AD" --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/labels"
@@ -179,7 +186,8 @@ Example response:
   "open_merge_requests_count": 0,
   "subscribed": false,
   "priority": null,
-    "is_project_label": true
+  "is_project_label": true,
+  "remove_on_close": true
 }
 ```
 
@@ -220,6 +228,7 @@ PUT /projects/:id/labels/:label_id
 | `color`         | string  | yes if `new_name` is not provided | The color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the [CSS color names](https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#Color_keywords) |
 | `description`   | string  | no                                | The new description of the label |
 | `priority`    | integer | no       | The new priority of the label. Must be greater or equal than zero or `null` to remove the priority. |
+| `remove_on_close` | boolean | no | Boolean option specifying whether the label should be removed from issues when they are closed. |
 
 ```shell
 curl --request PUT --data "new_name=docs&color=#8E44AD&description=Documentation" --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/labels/documentation"
@@ -240,7 +249,8 @@ Example response:
   "open_merge_requests_count": 2,
   "subscribed": false,
   "priority": null,
-  "is_project_label": true
+  "is_project_label": true,
+  "remove_on_close": true
 }
 ```
 
@@ -281,7 +291,8 @@ Example response:
   "open_issues_count": 1,
   "closed_issues_count": 0,
   "open_merge_requests_count": 2,
-  "subscribed": false
+  "subscribed": false,
+  "remove_on_close": true
 }
 ```
 
@@ -322,7 +333,8 @@ Example response:
   "open_merge_requests_count": 1,
   "subscribed": true,
   "priority": null,
-  "is_project_label": true
+  "is_project_label": true,
+  "remove_on_close": true
 }
 ```
 

lib/api/entities/label_basic.rb

@@ -3,7 +3,7 @@
 module API
   module Entities
     class LabelBasic < Grape::Entity
-      expose :id, :name, :color, :description, :description_html, :text_color
+      expose :id, :name, :color, :description, :description_html, :text_color, :remove_on_close
     end
   end
 end

lib/api/helpers/label_helpers.rb

@@ -5,27 +5,34 @@ module API
     module LabelHelpers
       extend Grape::API::Helpers
 
+      params :optional_label_params do
+        optional :description, type: String, desc: 'The description of the label'
+        optional :remove_on_close, type: Boolean, desc: 'Whether the label should be removed from an issue when the issue is closed'
+      end
+
       params :label_create_params do
         requires :name, type: String, desc: 'The name of the label to be created'
         requires :color, type: String, desc: "The color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the allowed CSS color names"
-        optional :description, type: String, desc: 'The description of label to be created'
+
+        use :optional_label_params
       end
 
       params :label_update_params do
         optional :new_name, type: String, desc: 'The new name of the label'
         optional :color, type: String, desc: "The new color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the allowed CSS color names"
-        optional :description, type: String, desc: 'The new description of label'
+
+        use :optional_label_params
       end
 
       params :project_label_update_params do
         use :label_update_params
         optional :priority, type: Integer, desc: 'The priority of the label', allow_blank: true
-        at_least_one_of :new_name, :color, :description, :priority
+        at_least_one_of :new_name, :color, :description, :priority, :remove_on_close
       end
 
       params :group_label_update_params do
         use :label_update_params
-        at_least_one_of :new_name, :color, :description
+        at_least_one_of :new_name, :color, :description, :remove_on_close
       end
 
       def find_label(parent, id_or_title, params = { include_ancestor_groups: true })

spec/fixtures/api/schemas/public_api/v4/label_basic.json

@@ -6,7 +6,8 @@
     "color",
     "description",
     "description_html",
-    "text_color"
+    "text_color",
+    "remove_on_close"
   ],
   "properties": {
     "id": { "type": "integer" },
@@ -20,7 +21,8 @@
     "text_color": {
       "type": "string",
       "pattern": "^#[0-9A-Fa-f]{3}{1,2}$"
-    }
+    },
+    "remove_on_close": { "type": "boolean" }
   },
   "additionalProperties": false
 }

spec/graphql/mutations/issues/update_spec.rb

@@ -69,17 +69,33 @@ RSpec.describe Mutations::Issues::Update do
       context 'when changing state' do
         let_it_be_with_refind(:issue) { create(:issue, project: project, state: :opened) }
 
-        it 'closes issue' do
-          mutation_params[:state_event] = 'close'
+        before do
+          mutation_params[:state_event] = state_event
+        end
+
+        context 'when state_event is close' do
+          let_it_be(:removable_label) { create(:label, project: project, remove_on_close: true, issues: [issue]) }
 
-          expect { subject }.to change { issue.reload.state }.from('opened').to('closed')
+          let(:state_event) { 'close' }
+
+          it 'closes issue' do
+            expect do
+              subject
+              issue.reload
+            end.to change(issue, :state).from('opened').to('closed').and(
+              change { issue.label_ids }.from([removable_label.id]).to([])
+            )
+          end
         end
 
-        it 'reopens issue' do
-          issue.close
-          mutation_params[:state_event] = 'reopen'
+        context 'when state_event is reopen' do
+          let(:state_event) { 'reopen' }
+
+          it 'reopens issue' do
+            issue.close
 
-          expect { subject }.to change { issue.reload.state }.from('closed').to('opened')
+            expect { subject }.to change { issue.reload.state }.from('closed').to('opened')
+          end
         end
       end
 

spec/graphql/types/label_type_spec.rb

@@ -11,6 +11,7 @@ RSpec.describe GitlabSchema.types['Label'] do
       :color,
       :text_color,
       :created_at,
+      :remove_on_close,
       :updated_at
     ]
 

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -102,6 +102,7 @@ ProjectLabel:
 - template
 - description
 - priority
+- remove_on_close
 Milestone:
 - id
 - title

spec/models/label_link_spec.rb

@@ -13,14 +13,27 @@ RSpec.describe LabelLink do
     let(:invalid_items_for_bulk_insertion) { [] } # class does not have any validations defined
   end
 
-  describe '.for_target' do
-    it 'returns the label links for a given target' do
-      label_link = create(:label_link, target: create(:merge_request))
+  describe 'scopes' do
+    describe '.for_target' do
+      it 'returns the label links for a given target' do
+        label_link = create(:label_link, target: create(:merge_request))
 
-      create(:label_link, target: create(:issue))
+        create(:label_link, target: create(:issue))
 
-      expect(described_class.for_target(label_link.target_id, label_link.target_type))
-        .to contain_exactly(label_link)
+        expect(described_class.for_target(label_link.target_id, label_link.target_type))
+          .to contain_exactly(label_link)
+      end
+    end
+
+    describe '.with_remove_on_close_labels' do
+      it 'responds with label_links that can be removed when an issue is closed' do
+        issue = create(:issue)
+        removable_label = create(:label, project: issue.project, remove_on_close: true)
+        create(:label_link, target: issue)
+        removable_issue_label_link = create(:label_link, label: removable_label, target: issue)
+
+        expect(described_class.with_remove_on_close_labels).to contain_exactly(removable_issue_label_link)
+      end
     end
   end
 end

spec/requests/api/graphql/mutations/labels/create_spec.rb

@@ -11,7 +11,8 @@ RSpec.describe Mutations::Labels::Create do
     {
       'title' => 'foo',
       'description' => 'some description',
-      'color' => '#FF0000'
+      'color' => '#FF0000',
+      'removeOnClose' => true
     }
   end
 

spec/requests/api/group_labels_spec.rb

@@ -290,7 +290,7 @@ RSpec.describe API::GroupLabels do
       put api("/groups/#{group.id}/labels", user), params: { name: group_label1.name }
 
       expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response['error']).to eq('new_name, color, description are missing, '\
+      expect(json_response['error']).to eq('new_name, color, description, remove_on_close are missing, '\
                                            'at least one parameter must be provided')
     end
   end
@@ -337,7 +337,7 @@ RSpec.describe API::GroupLabels do
       put api("/groups/#{group.id}/labels/#{valid_group_label_title_1_esc}", user)
 
       expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response['error']).to eq('new_name, color, description are missing, '\
+      expect(json_response['error']).to eq('new_name, color, description, remove_on_close are missing, '\
                                            'at least one parameter must be provided')
     end
   end

spec/requests/api/issues/put_projects_issues_spec.rb

@@ -402,6 +402,17 @@ RSpec.describe API::Issues do
       expect(response).to have_gitlab_http_status(:ok)
       expect(json_response['state']).to eq 'opened'
     end
+
+    it 'removes labels marked to be removed on issue closed' do
+      removable_label = create(:label, project: project, remove_on_close: true)
+      create(:label_link, target: issue, label: removable_label)
+
+      put api_for_user, params: { state_event: 'close' }
+
+      expect(issue.reload.label_ids).not_to include(removable_label.id)
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response['state']).to eq 'closed'
+    end
   end
 
   describe 'PUT /projects/:id/issues/:issue_iid to update updated_at param' do

spec/requests/api/labels_spec.rb

@@ -57,7 +57,7 @@ RSpec.describe API::Labels do
         put_labels_api(route_type, user, spec_params)
 
         expect(response).to have_gitlab_http_status(:bad_request)
-        expect(json_response['error']).to eq('new_name, color, description, priority are missing, '\
+        expect(json_response['error']).to eq('new_name, color, description, priority, remove_on_close are missing, '\
                                              'at least one parameter must be provided')
       end
 
@@ -112,6 +112,14 @@ RSpec.describe API::Labels do
         expect(json_response['id']).to eq(expected_response_label_id)
         expect(json_response['priority']).to eq(10)
       end
+
+      it "returns 200 if remove_on_close is changed (#{route_type} route)" do
+        put_labels_api(route_type, user, spec_params, remove_on_close: true)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['id']).to eq(expected_response_label_id)
+        expect(json_response['remove_on_close']).to eq(true)
+      end
     end
 
     it 'returns 200 if a priority is removed (deprecated route)' do
@@ -301,7 +309,8 @@ RSpec.describe API::Labels do
              name: valid_label_title_2,
              color: '#FFAABB',
              description: 'test',
-             priority: 2
+             priority: 2,
+             remove_on_close: true
            }
 
       expect(response).to have_gitlab_http_status(:created)
@@ -309,6 +318,7 @@ RSpec.describe API::Labels do
       expect(json_response['color']).to eq('#FFAABB')
       expect(json_response['description']).to eq('test')
       expect(json_response['priority']).to eq(2)
+      expect(json_response['remove_on_close']).to eq(true)
     end
 
     it 'returns created label when only required params' do

spec/services/issues/close_service_spec.rb

@@ -3,22 +3,50 @@
 require 'spec_helper'
 
 RSpec.describe Issues::CloseService do
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user, email: "user@example.com") }
-  let(:user2) { create(:user, email: "user2@example.com") }
-  let(:guest) { create(:user) }
-  let(:issue) { create(:issue, title: "My issue", project: project, assignees: [user2], author: create(:user)) }
+  subject(:close_issue) { described_class.new(project: project, current_user: user).close_issue(issue) }
+
+  let_it_be(:project, refind: true) { create(:project, :repository) }
+  let_it_be(:label1) { create(:label, project: project) }
+  let_it_be(:label2) { create(:label, project: project, remove_on_close: true) }
+  let_it_be(:author) { create(:user) }
+  let_it_be(:user) { create(:user, email: "user@example.com") }
+  let_it_be(:user2) { create(:user, email: "user2@example.com") }
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:closing_merge_request) { create(:merge_request, source_project: project) }
+
   let(:external_issue) { ExternalIssue.new('JIRA-123', project) }
-  let(:closing_merge_request) { create(:merge_request, source_project: project) }
-  let(:closing_commit) { create(:commit, project: project) }
-  let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
+  let!(:issue) { create(:issue, title: "My issue", project: project, assignees: [user2], author: author) }
 
-  before do
+  before_all do
     project.add_maintainer(user)
     project.add_developer(user2)
     project.add_guest(guest)
   end
 
+  shared_examples 'removes labels marked for removal from issue when closed' do
+    before do
+      issue.update!(label_ids: [label1.id, label2.id])
+    end
+
+    it 'removes labels marked for removal' do
+      expect do
+        close_issue
+      end.to change { issue.reload.label_ids }.from(containing_exactly(label1.id, label2.id)).to(containing_exactly(label1.id))
+    end
+
+    it 'creates system notes for the removed labels' do
+      expect do
+        close_issue
+      end.to change(ResourceLabelEvent, :count).by(1)
+
+      expect(ResourceLabelEvent.last.slice(:action, :issue_id, :label_id)).to eq(
+        'action' => 'remove',
+        'issue_id' => issue.id,
+        'label_id' => label2.id
+      )
+    end
+  end
+
   describe '#execute' do
     let(:service) { described_class.new(project: project, current_user: user) }
 
@@ -121,6 +149,8 @@ RSpec.describe Issues::CloseService do
         end
       end
 
+      it_behaves_like 'removes labels marked for removal from issue when closed'
+
       it 'mentions closure via a merge request' do
         close_issue
 
@@ -184,10 +214,18 @@ RSpec.describe Issues::CloseService do
     end
 
     context "closed by a commit", :sidekiq_might_not_need_inline do
-      it 'mentions closure via a commit' do
+      subject(:close_issue) do
         perform_enqueued_jobs do
           described_class.new(project: project, current_user: user).close_issue(issue, closed_via: closing_commit)
         end
+      end
+
+      let(:closing_commit) { create(:commit, project: project) }
+
+      it_behaves_like 'removes labels marked for removal from issue when closed'
+
+      it 'mentions closure via a commit' do
+        close_issue
 
         email = ActionMailer::Base.deliveries.last
 
@@ -199,9 +237,8 @@ RSpec.describe Issues::CloseService do
       context 'when user cannot read the commit' do
         it 'does not mention the commit id' do
           project.project_feature.update_attribute(:repository_access_level, ProjectFeature::DISABLED)
-          perform_enqueued_jobs do
-            described_class.new(project: project, current_user: user).close_issue(issue, closed_via: closing_commit)
-          end
+
+          close_issue
 
           email = ActionMailer::Base.deliveries.last
           body_text = email.body.parts.map(&:body).join(" ")
@@ -222,12 +259,14 @@ RSpec.describe Issues::CloseService do
 
       it 'verifies the number of queries' do
         recorded = ActiveRecord::QueryRecorder.new { close_issue }
-        expected_queries = 23
+        expected_queries = 32
 
         expect(recorded.count).to be <= expected_queries
         expect(recorded.cached_count).to eq(0)
       end
 
+      it_behaves_like 'removes labels marked for removal from issue when closed'
+
       it 'closes the issue' do
         close_issue
 
@@ -257,6 +296,8 @@ RSpec.describe Issues::CloseService do
       end
 
       it 'marks todos as done' do
+        todo = create(:todo, :assigned, user: user, project: project, target: issue, author: user2)
+
         close_issue
 
         expect(todo.reload).to be_done
@@ -321,26 +362,32 @@ RSpec.describe Issues::CloseService do
     end
 
     context 'when issue is not confidential' do
+      it_behaves_like 'removes labels marked for removal from issue when closed'
+
       it 'executes issue hooks' do
         expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
         expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks)
 
-        described_class.new(project: project, current_user: user).close_issue(issue)
+        close_issue
       end
     end
 
     context 'when issue is confidential' do
-      it 'executes confidential issue hooks' do
-        issue = create(:issue, :confidential, project: project)
+      let(:issue) { create(:issue, :confidential, project: project) }
 
+      it_behaves_like 'removes labels marked for removal from issue when closed'
+
+      it 'executes confidential issue hooks' do
         expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks)
         expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks)
 
-        described_class.new(project: project, current_user: user).close_issue(issue)
+        close_issue
       end
     end
 
     context 'internal issues disabled' do
+      let!(:todo) { create(:todo, :assigned, user: user, project: project, target: issue, author: user2) }
+
       before do
         project.issues_enabled = false
         project.save!