Merge branch...

Merge branch '198562-merge-request-user-interface-encourages-accidentally-closing-the-request' into 'master'

Remove "Comment & close" buttons on MR/issues

See merge request gitlab-org/gitlab!49614

app/assets/javascripts/notes/components/comment_form.vue

@@ -143,6 +143,9 @@ export default {
     trackingLabel() {
       return slugifyWithUnderscore(`${this.commentButtonTitle} button`);
     },
+    hasCloseAndCommentButton() {
+      return !this.glFeatures.removeCommentCloseReopen;
+    },
   },
   watch: {
     note(newNote) {
@@ -405,7 +408,7 @@ export default {
               </div>
 
               <gl-button
-                v-if="canToggleIssueState"
+                v-if="hasCloseAndCommentButton && canToggleIssueState"
                 :loading="isToggleStateButtonLoading"
                 category="secondary"
                 :variant="buttonVariant"

app/controllers/concerns/comment_and_close_flag.rb

+# frozen_string_literal: true
+
+module CommentAndCloseFlag
+  extend ActiveSupport::Concern
+
+  included do
+    before_action do
+      push_frontend_feature_flag(:remove_comment_close_reopen, @group)
+    end
+  end
+end

app/controllers/projects/issues_controller.rb

@@ -9,6 +9,7 @@ class Projects::IssuesController < Projects::ApplicationController
   include IssuesCalendar
   include SpammableActions
   include RecordUserLastActivity
+  include CommentAndCloseFlag
 
   ISSUES_EXCEPT_ACTIONS = %i[index calendar new create bulk_update import_csv export_csv service_desk].freeze
   SET_ISSUEABLES_INDEX_ONLY_ACTIONS = %i[index calendar service_desk].freeze

app/controllers/projects/merge_requests_controller.rb

@@ -11,6 +11,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   include RecordUserLastActivity
   include SourcegraphDecorator
   include DiffHelper
+  include CommentAndCloseFlag
 
   skip_before_action :merge_request, only: [:index, :bulk_update, :export_csv]
   before_action :apply_diff_view_cookie!, only: [:show]

config/feature_flags/development/remove_comment_close_reopen.yml

+---
+name: remove_comment_close_reopen
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49614
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/198562
+milestone: '13.9'
+type: development
+group: group::code review
+default_enabled: false

ee/app/controllers/groups/epics_controller.rb

@@ -8,6 +8,7 @@ class Groups::EpicsController < Groups::ApplicationController
   include RendersNotes
   include EpicsActions
   include DescriptionDiffActions
+  include CommentAndCloseFlag
 
   before_action :check_epics_available!
   before_action :epic, except: [:index, :create, :new, :bulk_update]

ee/spec/features/epics/epic_show_spec.rb

@@ -28,6 +28,8 @@ RSpec.describe 'Epic show', :js do
   let_it_be(:child_issue_a) { create(:epic_issue, epic: epic, issue: public_issue, relative_position: 1) }
 
   before do
+    stub_feature_flags(remove_comment_close_reopen: false)
+
     group.add_developer(user)
     stub_licensed_features(epics: true, subepics: true)
     sign_in(user)
@@ -292,9 +294,15 @@ RSpec.describe 'Epic show', :js do
     end
 
     describe 'when open' do
-      context 'when clicking the top `Close epic` button', :aggregate_failures do
-        let(:open_epic) { create(:epic, group: group) }
+      let(:open_epic) { create(:epic, group: group) }
+
+      it_behaves_like 'page with comment and close button', 'Close epic' do
+        def setup
+          visit group_epic_path(group, open_epic)
+        end
+      end
 
+      context 'when clicking the top `Close epic` button', :aggregate_failures do
         before do
           visit group_epic_path(group, open_epic)
         end
@@ -303,8 +311,6 @@ RSpec.describe 'Epic show', :js do
       end
 
       context 'when clicking the bottom `Close epic` button', :aggregate_failures do
-        let(:open_epic) { create(:epic, group: group) }
-
         before do
           visit group_epic_path(group, open_epic)
         end
@@ -314,9 +320,15 @@ RSpec.describe 'Epic show', :js do
     end
 
     describe 'when closed' do
-      context 'when clicking the top `Reopen epic` button', :aggregate_failures do
-        let(:closed_epic) { create(:epic, group: group, state: 'closed') }
+      let(:closed_epic) { create(:epic, group: group, state: 'closed') }
 
+      it_behaves_like 'page with comment and close button', 'Reopen epic' do
+        def setup
+          visit group_epic_path(group, closed_epic)
+        end
+      end
+
+      context 'when clicking the top `Reopen epic` button', :aggregate_failures do
         before do
           visit group_epic_path(group, closed_epic)
         end
@@ -325,8 +337,6 @@ RSpec.describe 'Epic show', :js do
       end
 
       context 'when clicking the bottom `Reopen epic` button', :aggregate_failures do
-        let(:closed_epic) { create(:epic, group: group, state: 'closed') }
-
         before do
           visit group_epic_path(group, closed_epic)
         end

ee/spec/features/issues/related_issues_spec.rb

@@ -123,11 +123,23 @@ RSpec.describe 'Related issues', :js do
           expect(find('.js-related-issues-header-issue-count')).to have_content('1')
         end
 
+        it_behaves_like 'page with comment and close button', 'Close issue' do
+          def setup
+            visit project_issue_path(project, issue_a)
+
+            wait_for_requests
+          end
+        end
+
         context 'when clicking the top `Close issue` button in the issue header', :aggregate_failures do
           it_behaves_like 'issue closed by modal', '.detail-page-header'
         end
 
         context 'when clicking the bottom `Close issue` button below the comment textarea', :aggregate_failures do
+          before do
+            stub_feature_flags(remove_comment_close_reopen: false)
+          end
+
           it_behaves_like 'issue closed by modal', '.new-note'
         end
       end

ee/spec/features/merge_request/user_selects_branches_for_new_mr_spec.rb

@@ -20,15 +20,12 @@ RSpec.describe 'Merge request > User selects branches for new MR', :js do
   context 'create a merge request for the selected branches' do
     before do
       visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: 'feature_conflict' })
+      fill_in 'merge_request_title', with: 'A Test MR'
+      click_button 'Submit merge request'
     end
 
-    context 'saving the MR' do
-      it 'shows the saved MR' do
-        fill_in 'merge_request_title', with: 'Test'
-        click_button 'Submit merge request'
-
-        expect(page).to have_button('Close merge request')
-      end
+    it 'shows the saved MR' do
+      expect(page).to have_content('A Test MR')
     end
   end
 end

spec/features/discussion_comments/issue_spec.rb

@@ -8,6 +8,8 @@ RSpec.describe 'Thread Comments Issue', :js do
   let(:issue) { create(:issue, project: project) }
 
   before do
+    stub_feature_flags(remove_comment_close_reopen: false)
+
     project.add_maintainer(user)
     sign_in(user)
 

spec/features/discussion_comments/merge_request_spec.rb

@@ -9,6 +9,7 @@ RSpec.describe 'Thread Comments Merge Request', :js do
 
   before do
     stub_feature_flags(remove_resolve_note: false)
+    stub_feature_flags(remove_comment_close_reopen: false)
 
     project.add_maintainer(user)
     sign_in(user)

spec/features/issues/issue_state_spec.rb

@@ -42,9 +42,15 @@ RSpec.describe 'issue state', :js do
   end
 
   describe 'when open', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/297348' do
-    context 'when clicking the top `Close issue` button', :aggregate_failures do
-      let(:open_issue) { create(:issue, project: project) }
+    let(:open_issue) { create(:issue, project: project) }
 
+    it_behaves_like 'page with comment and close button', 'Close issue' do
+      def setup
+        visit project_issue_path(project, open_issue)
+      end
+    end
+
+    context 'when clicking the top `Close issue` button', :aggregate_failures do
       before do
         visit project_issue_path(project, open_issue)
       end
@@ -53,9 +59,8 @@ RSpec.describe 'issue state', :js do
     end
 
     context 'when clicking the bottom `Close issue` button', :aggregate_failures do
-      let(:open_issue) { create(:issue, project: project) }
-
       before do
+        stub_feature_flags(remove_comment_close_reopen: false)
         visit project_issue_path(project, open_issue)
       end
 
@@ -64,9 +69,15 @@ RSpec.describe 'issue state', :js do
   end
 
   describe 'when closed', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/297201' do
-    context 'when clicking the top `Reopen issue` button', :aggregate_failures do
-      let(:closed_issue) { create(:issue, project: project, state: 'closed') }
+    let(:closed_issue) { create(:issue, project: project, state: 'closed') }
 
+    it_behaves_like 'page with comment and close button', 'Reopen issue' do
+      def setup
+        visit project_issue_path(project, closed_issue)
+      end
+    end
+
+    context 'when clicking the top `Reopen issue` button', :aggregate_failures do
       before do
         visit project_issue_path(project, closed_issue)
       end
@@ -75,9 +86,8 @@ RSpec.describe 'issue state', :js do
     end
 
     context 'when clicking the bottom `Reopen issue` button', :aggregate_failures do
-      let(:closed_issue) { create(:issue, project: project, state: 'closed') }
-
       before do
+        stub_feature_flags(remove_comment_close_reopen: false)
         visit project_issue_path(project, closed_issue)
       end
 

spec/features/merge_request/user_closes_reopens_merge_request_state_spec.rb

@@ -12,9 +12,15 @@ RSpec.describe 'User closes/reopens a merge request', :js, quarantine: 'https://
   end
 
   describe 'when open' do
-    context 'when clicking the top `Close merge request` link', :aggregate_failures do
-      let(:open_merge_request) { create(:merge_request, source_project: project, target_project: project) }
+    let(:open_merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+    it_behaves_like 'page with comment and close button', 'Close merge request' do
+      def setup
+        visit merge_request_path(open_merge_request)
+      end
+    end
 
+    context 'when clicking the top `Close merge request` link', :aggregate_failures do
       before do
         visit merge_request_path(open_merge_request)
       end
@@ -34,9 +40,8 @@ RSpec.describe 'User closes/reopens a merge request', :js, quarantine: 'https://
     end
 
     context 'when clicking the bottom `Close merge request` button', :aggregate_failures do
-      let(:open_merge_request) { create(:merge_request, source_project: project, target_project: project) }
-
       before do
+        stub_feature_flags(remove_comment_close_reopen: false)
         visit merge_request_path(open_merge_request)
       end
 
@@ -56,9 +61,22 @@ RSpec.describe 'User closes/reopens a merge request', :js, quarantine: 'https://
   end
 
   describe 'when closed' do
-    context 'when clicking the top `Reopen merge request` link', :aggregate_failures do
-      let(:closed_merge_request) { create(:merge_request, source_project: project, target_project: project, state: 'closed') }
+    let(:closed_merge_request) { create(:merge_request, source_project: project, target_project: project, state: 'closed') }
 
+    it_behaves_like 'page with comment and close button', 'Close merge request' do
+      def setup
+        visit merge_request_path(closed_merge_request)
+
+        within '.detail-page-header' do
+          click_button 'Toggle dropdown'
+          click_link 'Reopen merge request'
+        end
+
+        wait_for_requests
+      end
+    end
+
+    context 'when clicking the top `Reopen merge request` link', :aggregate_failures do
       before do
         visit merge_request_path(closed_merge_request)
       end
@@ -78,9 +96,8 @@ RSpec.describe 'User closes/reopens a merge request', :js, quarantine: 'https://
     end
 
     context 'when clicking the bottom `Reopen merge request` button', :aggregate_failures do
-      let(:closed_merge_request) { create(:merge_request, source_project: project, target_project: project, state: 'closed') }
-
       before do
+        stub_feature_flags(remove_comment_close_reopen: false)
         visit merge_request_path(closed_merge_request)
       end
 

spec/features/task_lists_spec.rb

@@ -69,7 +69,13 @@ RSpec.describe 'Task Lists', :js do
         wait_for_requests
 
         expect(page).to have_selector(".md .task-list .task-list-item .task-list-item-checkbox")
-        expect(page).to have_selector('.btn-close')
+      end
+
+      it_behaves_like 'page with comment and close button', 'Close issue' do
+        def setup
+          visit_issue(project, issue)
+          wait_for_requests
+        end
       end
 
       it 'is only editable by author' do

spec/support/shared_examples/features/comment_and_close_button_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'page with comment and close button' do |button_text|
+  context 'when remove_comment_close_reopen feature flag is enabled' do
+    before do
+      stub_feature_flags(remove_comment_close_reopen: true)
+      setup
+    end
+
+    it "does not show #{button_text} button" do
+      within '.note-form-actions' do
+        expect(page).not_to have_button(button_text)
+      end
+    end
+  end
+
+  context 'when remove_comment_close_reopen feature flag is disabled' do
+    before do
+      stub_feature_flags(remove_comment_close_reopen: false)
+      setup
+    end
+
+    it "shows #{button_text} button" do
+      within '.note-form-actions' do
+        expect(page).to have_button(button_text)
+      end
+    end
+  end
+end