Merge branch 'revert-47474d27' into 'master'

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

See merge request gitlab-org/gitlab!62056

app/graphql/mutations/labels/create.rb

@@ -20,10 +20,6 @@ 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,7 +23,5 @@ 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,5 +11,4 @@ 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,7 +25,6 @@ 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
 
@@ -52,18 +51,6 @@ 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
+  # This migration was reverted in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/62056
 
   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/structure.sql

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

doc/api/graphql/reference/index.md

@@ -2700,7 +2700,6 @@ 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
@@ -9743,7 +9742,6 @@ 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,8 +46,7 @@ Example response:
     "open_merge_requests_count": 1,
     "subscribed": false,
     "priority": 10,
-    "is_project_label": true,
-    "remove_on_close": false
+    "is_project_label": true
   },
   {
     "id" : 4,
@@ -61,8 +60,7 @@ Example response:
     "open_merge_requests_count": 0,
     "subscribed": false,
     "priority": null,
-    "is_project_label": true,
-    "remove_on_close": false
+    "is_project_label": true
   },
   {
     "id" : 7,
@@ -76,8 +74,7 @@ Example response:
     "open_merge_requests_count": 1,
     "subscribed": false,
     "priority": null,
-    "is_project_label": true,
-    "remove_on_close": true
+    "is_project_label": true
   },
   {
     "id" : 8,
@@ -91,8 +88,7 @@ Example response:
     "open_merge_requests_count": 2,
     "subscribed": false,
     "priority": null,
-    "is_project_label": false,
-    "remove_on_close": false
+    "is_project_label": false
   },
   {
     "id" : 9,
@@ -106,8 +102,7 @@ Example response:
     "open_merge_requests_count": 1,
     "subscribed": true,
     "priority": null,
-    "is_project_label": true,
-    "remove_on_close": false
+    "is_project_label": true
   }
 ]
 ```
@@ -145,8 +140,7 @@ Example response:
   "open_merge_requests_count": 1,
   "subscribed": false,
   "priority": 10,
-  "is_project_label": true,
-  "remove_on_close": true
+  "is_project_label": true
 }
 ```
 
@@ -165,7 +159,6 @@ 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"
@@ -186,8 +179,7 @@ Example response:
   "open_merge_requests_count": 0,
   "subscribed": false,
   "priority": null,
-  "is_project_label": true,
-  "remove_on_close": true
+    "is_project_label": true
 }
 ```
 
@@ -228,7 +220,6 @@ 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"
@@ -249,8 +240,7 @@ Example response:
   "open_merge_requests_count": 2,
   "subscribed": false,
   "priority": null,
-  "is_project_label": true,
-  "remove_on_close": true
+  "is_project_label": true
 }
 ```
 
@@ -291,8 +281,7 @@ Example response:
   "open_issues_count": 1,
   "closed_issues_count": 0,
   "open_merge_requests_count": 2,
-  "subscribed": false,
-  "remove_on_close": true
+  "subscribed": false
 }
 ```
 
@@ -333,8 +322,7 @@ Example response:
   "open_merge_requests_count": 1,
   "subscribed": true,
   "priority": null,
-  "is_project_label": true,
-  "remove_on_close": true
+  "is_project_label": 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, :remove_on_close
+      expose :id, :name, :color, :description, :description_html, :text_color
     end
   end
 end

lib/api/helpers/label_helpers.rb

@@ -5,34 +5,27 @@ 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"
-
-        use :optional_label_params
+        optional :description, type: String, desc: 'The description of label to be created'
       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"
-
-        use :optional_label_params
+        optional :description, type: String, desc: 'The new description of label'
       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, :remove_on_close
+        at_least_one_of :new_name, :color, :description, :priority
       end
 
       params :group_label_update_params do
         use :label_update_params
-        at_least_one_of :new_name, :color, :description, :remove_on_close
+        at_least_one_of :new_name, :color, :description
       end
 
       def find_label(parent, id_or_title, params = { include_ancestor_groups: true })

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

@@ -6,8 +6,7 @@
     "color",
     "description",
     "description_html",
-    "text_color",
-    "remove_on_close"
+    "text_color"
   ],
   "properties": {
     "id": { "type": "integer" },
@@ -21,8 +20,7 @@
     "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,33 +69,17 @@ RSpec.describe Mutations::Issues::Update do
       context 'when changing state' do
         let_it_be_with_refind(:issue) { create(:issue, project: project, state: :opened) }
 
-        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]) }
+        it 'closes issue' do
+          mutation_params[:state_event] = 'close'
 
-          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
+          expect { subject }.to change { issue.reload.state }.from('opened').to('closed')
         end
 
-        context 'when state_event is reopen' do
-          let(:state_event) { 'reopen' }
-
-          it 'reopens issue' do
-            issue.close
+        it 'reopens issue' do
+          issue.close
+          mutation_params[:state_event] = 'reopen'
 
-            expect { subject }.to change { issue.reload.state }.from('closed').to('opened')
-          end
+          expect { subject }.to change { issue.reload.state }.from('closed').to('opened')
         end
       end
 

spec/graphql/types/label_type_spec.rb

@@ -11,7 +11,6 @@ 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,7 +102,6 @@ ProjectLabel:
 - template
 - description
 - priority
-- remove_on_close
 Milestone:
 - id
 - title

spec/models/label_link_spec.rb

@@ -13,27 +13,14 @@ RSpec.describe LabelLink do
     let(:invalid_items_for_bulk_insertion) { [] } # class does not have any validations defined
   end
 
-  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))
+  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)
-      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
+      expect(described_class.for_target(label_link.target_id, label_link.target_type))
+        .to contain_exactly(label_link)
     end
   end
 end

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

@@ -11,8 +11,7 @@ RSpec.describe Mutations::Labels::Create do
     {
       'title' => 'foo',
       'description' => 'some description',
-      'color' => '#FF0000',
-      'removeOnClose' => true
+      'color' => '#FF0000'
     }
   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, remove_on_close are missing, '\
+      expect(json_response['error']).to eq('new_name, color, description 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, remove_on_close are missing, '\
+      expect(json_response['error']).to eq('new_name, color, description are missing, '\
                                            'at least one parameter must be provided')
     end
   end

spec/requests/api/issues/put_projects_issues_spec.rb

@@ -402,17 +402,6 @@ 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, remove_on_close are missing, '\
+        expect(json_response['error']).to eq('new_name, color, description, priority are missing, '\
                                              'at least one parameter must be provided')
       end
 
@@ -112,14 +112,6 @@ 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
@@ -309,8 +301,7 @@ RSpec.describe API::Labels do
              name: valid_label_title_2,
              color: '#FFAABB',
              description: 'test',
-             priority: 2,
-             remove_on_close: true
+             priority: 2
            }
 
       expect(response).to have_gitlab_http_status(:created)
@@ -318,7 +309,6 @@ 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,50 +3,22 @@
 require 'spec_helper'
 
 RSpec.describe Issues::CloseService do
-  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(: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)) }
   let(:external_issue) { ExternalIssue.new('JIRA-123', project) }
-  let!(:issue) { create(:issue, title: "My issue", project: project, assignees: [user2], author: author) }
+  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) }
 
-  before_all do
+  before 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) }
 
@@ -149,8 +121,6 @@ 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
 
@@ -214,18 +184,10 @@ RSpec.describe Issues::CloseService do
     end
 
     context "closed by a commit", :sidekiq_might_not_need_inline do
-      subject(:close_issue) do
+      it 'mentions closure via a commit' 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
 
@@ -237,8 +199,9 @@ 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)
-
-          close_issue
+          perform_enqueued_jobs do
+            described_class.new(project: project, current_user: user).close_issue(issue, closed_via: closing_commit)
+          end
 
           email = ActionMailer::Base.deliveries.last
           body_text = email.body.parts.map(&:body).join(" ")
@@ -259,14 +222,12 @@ RSpec.describe Issues::CloseService do
 
       it 'verifies the number of queries' do
         recorded = ActiveRecord::QueryRecorder.new { close_issue }
-        expected_queries = 32
+        expected_queries = 23
 
         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
 
@@ -296,8 +257,6 @@ 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
@@ -362,32 +321,26 @@ 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)
 
-        close_issue
+        described_class.new(project: project, current_user: user).close_issue(issue)
       end
     end
 
     context 'when issue is confidential' do
-      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
+        issue = create(:issue, :confidential, project: project)
+
         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)
 
-        close_issue
+        described_class.new(project: project, current_user: user).close_issue(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!