Merge branch '16877-quick-actions-on-description-edit' into 'master'

Support quick actions when editing descriptions

See merge request gitlab-org/gitlab!31186

app/assets/javascripts/gfm_auto_complete.js

@@ -88,7 +88,6 @@ class GfmAutoComplete {
     if (this.enableMap.labels) this.setupLabels($input);
     if (this.enableMap.snippets) this.setupSnippets($input);
 
-    // We don't instantiate the quick actions autocomplete for note and issue/MR edit forms
     $input.filter('[data-supports-quick-actions="true"]').atwho({
       at: '/',
       alias: 'commands',

app/assets/javascripts/issue_show/components/fields/description.vue

@@ -55,7 +55,7 @@ export default {
           class="note-textarea js-gfm-input js-autosize markdown-area
           qa-description-textarea"
           dir="auto"
-          data-supports-quick-actions="false"
+          data-supports-quick-actions="true"
           :aria-label="__('Description')"
           :placeholder="__('Write a comment or drag your files here…')"
           @keydown.meta.enter="updateIssuable"

app/controllers/projects/merge_requests_controller.rb

@@ -179,7 +179,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   end
 
   def update
-    @merge_request = ::MergeRequests::UpdateService.new(project, current_user, merge_request_params).execute(@merge_request)
+    @merge_request = ::MergeRequests::UpdateService.new(project, current_user, merge_request_update_params).execute(@merge_request)
 
     respond_to do |format|
       format.html do
@@ -313,6 +313,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   private
 
+  def merge_request_update_params
+    merge_request_params.merge!(params.permit(:merge_request_diff_head_sha))
+  end
+
   def head_pipeline
     strong_memoize(:head_pipeline) do
       pipeline = @merge_request.head_pipeline

app/services/issuable_base_service.rb

@@ -115,7 +115,7 @@ class IssuableBaseService < BaseService
     new_label_ids.uniq
   end
 
-  def handle_quick_actions_on_create(issuable)
+  def handle_quick_actions(issuable)
     merge_quick_actions_into_params!(issuable)
   end
 
@@ -123,17 +123,21 @@ class IssuableBaseService < BaseService
     original_description = params.fetch(:description, issuable.description)
 
     description, command_params =
-      QuickActions::InterpretService.new(project, current_user)
+      QuickActions::InterpretService.new(project, current_user, quick_action_options)
         .execute(original_description, issuable, only: only)
 
     # Avoid a description already set on an issuable to be overwritten by a nil
-    params[:description] = description if description
+    params[:description] = description if description && description != original_description
 
     params.merge!(command_params)
   end
 
+  def quick_action_options
+    {}
+  end
+
   def create(issuable)
-    handle_quick_actions_on_create(issuable)
+    handle_quick_actions(issuable)
     filter_params(issuable)
 
     params.delete(:state_event)
@@ -177,11 +181,13 @@ class IssuableBaseService < BaseService
   end
 
   def update(issuable)
+    handle_quick_actions(issuable)
+    filter_params(issuable)
+
     change_state(issuable)
     change_subscription(issuable)
     change_todo(issuable)
     toggle_award(issuable)
-    filter_params(issuable)
     old_associations = associations_before_update(issuable)
 
     label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids)

app/services/merge_requests/base_service.rb

@@ -2,6 +2,7 @@
 
 module MergeRequests
   class BaseService < ::IssuableBaseService
+    extend ::Gitlab::Utils::Override
     include MergeRequests::AssignsMergeParams
 
     def create_note(merge_request, state = merge_request.state)
@@ -63,6 +64,12 @@ module MergeRequests
       super
     end
 
+    override :handle_quick_actions
+    def handle_quick_actions(merge_request)
+      super
+      handle_wip_event(merge_request)
+    end
+
     def handle_wip_event(merge_request)
       if wip_event = params.delete(:wip_event)
         # We update the title that is provided in the params or we use the mr title

app/services/merge_requests/create_service.rb

@@ -33,12 +33,6 @@ module MergeRequests
       super
     end
 
-    # Override from IssuableBaseService
-    def handle_quick_actions_on_create(merge_request)
-      super
-      handle_wip_event(merge_request)
-    end
-
     private
 
     def set_projects!

app/services/merge_requests/update_service.rb

@@ -2,6 +2,8 @@
 
 module MergeRequests
   class UpdateService < MergeRequests::BaseService
+    extend ::Gitlab::Utils::Override
+
     def execute(merge_request)
       # We don't allow change of source/target projects and source branch
       # after merge request was created
@@ -9,14 +11,11 @@ module MergeRequests
       params.delete(:target_project_id)
       params.delete(:source_branch)
 
-      merge_from_quick_action(merge_request) if params[:merge]
-
       if merge_request.closed_without_fork?
         params.delete(:target_branch)
         params.delete(:force_remove_source_branch)
       end
 
-      handle_wip_event(merge_request)
       update_task_event(merge_request) || update(merge_request)
     end
 
@@ -77,26 +76,6 @@ module MergeRequests
       todo_service.update_merge_request(merge_request, current_user)
     end
 
-    def merge_from_quick_action(merge_request)
-      last_diff_sha = params.delete(:merge)
-
-      if Feature.enabled?(:merge_orchestration_service, merge_request.project, default_enabled: true)
-        MergeRequests::MergeOrchestrationService
-          .new(project, current_user, { sha: last_diff_sha })
-          .execute(merge_request)
-      else
-        return unless merge_request.mergeable_with_quick_action?(current_user, last_diff_sha: last_diff_sha)
-
-        merge_request.update(merge_error: nil)
-
-        if merge_request.head_pipeline_active?
-          AutoMergeService.new(project, current_user, { sha: last_diff_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
-        else
-          merge_request.merge_async(current_user.id, { sha: last_diff_sha })
-        end
-      end
-    end
-
     def reopen_service
       MergeRequests::ReopenService
     end
@@ -134,6 +113,37 @@ module MergeRequests
         issuable, issuable.project, current_user, branch_type,
         old_branch, new_branch)
     end
+
+    override :handle_quick_actions
+    def handle_quick_actions(merge_request)
+      super
+      merge_from_quick_action(merge_request) if params[:merge]
+    end
+
+    def merge_from_quick_action(merge_request)
+      last_diff_sha = params.delete(:merge)
+
+      if Feature.enabled?(:merge_orchestration_service, merge_request.project, default_enabled: true)
+        MergeRequests::MergeOrchestrationService
+          .new(project, current_user, { sha: last_diff_sha })
+          .execute(merge_request)
+      else
+        return unless merge_request.mergeable_with_quick_action?(current_user, last_diff_sha: last_diff_sha)
+
+        merge_request.update(merge_error: nil)
+
+        if merge_request.head_pipeline_active?
+          AutoMergeService.new(project, current_user, { sha: last_diff_sha }).execute(merge_request, AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
+        else
+          merge_request.merge_async(current_user.id, { sha: last_diff_sha })
+        end
+      end
+    end
+
+    override :quick_action_options
+    def quick_action_options
+      { merge_request_diff_head_sha: params.delete(:merge_request_diff_head_sha) }
+    end
   end
 end
 

app/views/shared/form_elements/_description.html.haml

 - project = local_assigns.fetch(:project)
 - model = local_assigns.fetch(:model)
-
-
-
 - form = local_assigns.fetch(:form)
 - placeholder = model.is_a?(MergeRequest) ? _('Describe the goal of the changes and what reviewers should be aware of.') : _('Write a comment or drag your files here…')
-- supports_quick_actions = model.new_record?
 
-- if supports_quick_actions
-  - preview_url = preview_markdown_path(project, target_type: model.class.name)
-- else
-  - preview_url = preview_markdown_path(project)
+- supports_quick_actions = true
+- preview_url = preview_markdown_path(project, target_type: model.class.name)
 
 .form-group.row.detail-page-description
   = form.label :description, 'Description', class: 'col-form-label col-sm-2'
   .col-sm-10
+    - if model.is_a?(MergeRequest)
+      = hidden_field_tag :merge_request_diff_head_sha, model.diff_head_sha
 
     - if model.is_a?(Issuable)
       = render 'shared/issuable/form/template_selector', issuable: model

changelogs/unreleased/16877-quick-actions-on-description-edit.yml

+---
+title: Support quick actions when editing issue, merge request, and epic descriptions
+merge_request: 31186
+author:
+type: added

doc/user/project/quick_actions.md

@@ -7,14 +7,15 @@ info: To determine the technical writer assigned to the Stage/Group associated w
 
 # GitLab Quick Actions
 
+> - Introduced in [GitLab 12.1](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/26672):
+>   once an action is executed, an alert appears when a quick action is successfully applied.
+> - In [GitLab 13.2](https://gitlab.com/gitlab-org/gitlab/-/issues/16877) and later, you can use
+>   quick actions when updating the description of issues, epics, and merge requests.
+
 Quick actions are textual shortcuts for common actions on issues, epics, merge requests,
 and commits that are usually done by clicking buttons or dropdowns in GitLab's UI.
-You can enter these commands while creating a new issue or merge request, or
-in comments of issues, epics, merge requests, and commits. Each command should be
-on a separate line in order to be properly detected and executed.
-
-> From [GitLab 12.1](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/26672), once an
-> action is executed, an alert appears when a quick action is successfully applied.
+You can enter these commands in the description or in comments of issues, epics, merge requests, and commits.
+Each command should be on a separate line in order to be properly detected and executed.
 
 ## Quick Actions for issues, merge requests and epics
 

ee/spec/features/epics/gfm_autocomplete_spec.rb

@@ -17,6 +17,14 @@ RSpec.describe 'GFM autocomplete', :js do
     wait_for_requests
   end
 
+  it 'opens quick action autocomplete when updating description' do
+    find('.js-issuable-edit').click
+
+    find('#issue-description').native.send_keys('/')
+
+    expect(page).to have_selector('.atwho-container')
+  end
+
   context 'issuables' do
     let(:project) { create(:project, :repository, namespace: group) }
 

ee/spec/services/epics/update_service_spec.rb

@@ -312,5 +312,15 @@ RSpec.describe Epics::UpdateService do
       let(:issuable) { epic }
       let(:parent) { group }
     end
+
+    context 'with quick actions in the description' do
+      let(:label) { create(:group_label, group: group) }
+
+      it 'adds labels to the epic' do
+        update_epic(description: "/label ~#{label.name}")
+
+        expect(epic.label_ids).to contain_exactly(label.id)
+      end
+    end
   end
 end

spec/features/issues/gfm_autocomplete_spec.rb

@@ -3,24 +3,23 @@
 require 'spec_helper'
 
 RSpec.describe 'GFM autocomplete', :js do
-  let(:issue_xss_title) { 'This will execute alert<img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;' }
-  let(:user_xss_title) { 'eve <img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;' }
-  let(:label_xss_title) { 'alert label &lt;img src=x onerror="alert(\'Hello xss\');" a' }
-  let(:milestone_xss_title) { 'alert milestone &lt;img src=x onerror="alert(\'Hello xss\');" a' }
-
-  let(:user_xss) { create(:user, name: user_xss_title, username: 'xss.user') }
-  let(:user) { create(:user, name: '💃speciąl someone💃', username: 'someone.special') }
-  let(:project) { create(:project) }
-  let(:label) { create(:label, project: project, title: 'special+') }
+  let_it_be(:user_xss_title) { 'eve <img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;' }
+  let_it_be(:user_xss) { create(:user, name: user_xss_title, username: 'xss.user') }
+  let_it_be(:user) { create(:user, name: '💃speciąl someone💃', username: 'someone.special') }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:label) { create(:label, project: project, title: 'special+') }
+
   let(:issue) { create(:issue, project: project) }
 
+  before_all do
+    project.add_maintainer(user)
+    project.add_maintainer(user_xss)
+  end
+
   describe 'when tribute_autocomplete feature flag is off' do
     before do
       stub_feature_flags(tribute_autocomplete: false)
 
-      project.add_maintainer(user)
-      project.add_maintainer(user_xss)
-
       sign_in(user)
       visit project_issue_path(project, issue)
 
@@ -45,6 +44,14 @@ RSpec.describe 'GFM autocomplete', :js do
       expect(find('.description')).to have_content(user.to_reference)
     end
 
+    it 'opens quick action autocomplete when updating description' do
+      find('.js-issuable-edit').click
+
+      find('#issue-description').native.send_keys('/')
+
+      expect(page).to have_selector('.atwho-container')
+    end
+
     it 'opens autocomplete menu when field starts with text' do
       page.within '.timeline-content-form' do
         find('#note-body').native.send_keys('@')
@@ -54,6 +61,7 @@ RSpec.describe 'GFM autocomplete', :js do
     end
 
     it 'opens autocomplete menu for Issues when field starts with text with item escaping HTML characters' do
+      issue_xss_title = 'This will execute alert<img src=x onerror=alert(2)&lt;img src=x onerror=alert(1)&gt;'
       create(:issue, project: project, title: issue_xss_title)
 
       page.within '.timeline-content-form' do
@@ -84,6 +92,7 @@ RSpec.describe 'GFM autocomplete', :js do
     end
 
     it 'opens autocomplete menu for Milestone when field starts with text with item escaping HTML characters' do
+      milestone_xss_title = 'alert milestone &lt;img src=x onerror="alert(\'Hello xss\');" a'
       create(:milestone, project: project, title: milestone_xss_title)
 
       page.within '.timeline-content-form' do
@@ -327,6 +336,7 @@ RSpec.describe 'GFM autocomplete', :js do
 
     context 'labels' do
       it 'opens autocomplete menu for Labels when field starts with text with item escaping HTML characters' do
+        label_xss_title = 'alert label &lt;img src=x onerror="alert(\'Hello xss\');" a'
         create(:label, project: project, title: label_xss_title)
 
         note = find('#note-body')
@@ -462,9 +472,6 @@ RSpec.describe 'GFM autocomplete', :js do
     before do
       stub_feature_flags(tribute_autocomplete: true)
 
-      project.add_maintainer(user)
-      project.add_maintainer(user_xss)
-
       sign_in(user)
       visit project_issue_path(project, issue)
 

spec/services/issues/create_service_spec.rb

@@ -284,7 +284,9 @@ RSpec.describe Issues::CreateService do
       end
     end
 
-    it_behaves_like 'new issuable record that supports quick actions'
+    it_behaves_like 'issuable record that supports quick actions' do
+      let(:issuable) { described_class.new(project, user, params).execute }
+    end
 
     context 'Quick actions' do
       context 'with assignee and milestone in params and command' do

spec/services/issues/update_service_spec.rb

@@ -866,5 +866,10 @@ RSpec.describe Issues::UpdateService, :mailer do
         end
       end
     end
+
+    it_behaves_like 'issuable record that supports quick actions' do
+      let(:existing_issue) { create(:issue, project: project) }
+      let(:issuable) { described_class.new(project, user, params).execute(existing_issue) }
+    end
   end
 end

spec/services/merge_requests/create_service_spec.rb

@@ -339,13 +339,14 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
       end
     end
 
-    it_behaves_like 'new issuable record that supports quick actions' do
+    it_behaves_like 'issuable record that supports quick actions' do
       let(:default_params) do
         {
           source_branch: 'feature',
           target_branch: 'master'
         }
       end
+      let(:issuable) { described_class.new(project, user, params).execute }
     end
 
     context 'Quick actions' do

spec/services/merge_requests/update_service_spec.rb

@@ -737,5 +737,10 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
           .to change { merge_request.reload.force_remove_source_branch? }.from(nil).to(true)
       end
     end
+
+    it_behaves_like 'issuable record that supports quick actions' do
+      let(:existing_merge_request) { create(:merge_request, source_project: project) }
+      let(:issuable) { described_class.new(project, user, params).execute(existing_merge_request) }
+    end
   end
 end

spec/support/services/issuable_create_service_slash_commands_shared_examples.rb → spec/support/services/issuable_description_quick_actions_shared_examples.rb

@@ -3,20 +3,25 @@
 # Specifications for behavior common to all objects with executable attributes.
 # It can take a `default_params`.
 
-RSpec.shared_examples 'new issuable record that supports quick actions' do
-  let!(:project) { create(:project, :repository) }
-  let(:user) { create(:user).tap { |u| project.add_maintainer(u) } }
-  let(:assignee) { create(:user) }
-  let!(:milestone) { create(:milestone, project: project) }
-  let!(:labels) { create_list(:label, 3, project: project) }
+RSpec.shared_examples 'issuable record that supports quick actions' do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:assignee) { create(:user) }
+  let_it_be(:milestone) { create(:milestone, project: project) }
+  let_it_be(:labels) { create_list(:label, 3, project: project) }
+
   let(:base_params) { { title: 'My issuable title' } }
   let(:params) { base_params.merge(defined?(default_params) ? default_params : {}).merge(example_params) }
-  let(:issuable) { described_class.new(project, user, params).execute }
 
-  before do
+  before_all do
+    project.add_maintainer(user)
     project.add_maintainer(assignee)
   end
 
+  before do
+    issuable.reload
+  end
+
   context 'with labels in command only' do
     let(:example_params) do
       {
@@ -25,7 +30,6 @@ RSpec.shared_examples 'new issuable record that supports quick actions' do
     end
 
     it 'attaches labels to issuable' do
-      expect(issuable).to be_persisted
       expect(issuable.label_ids).to match_array([labels.first.id, labels.second.id])
     end
   end
@@ -39,7 +43,6 @@ RSpec.shared_examples 'new issuable record that supports quick actions' do
     end
 
     it 'attaches all labels to issuable' do
-      expect(issuable).to be_persisted
       expect(issuable.label_ids).to match_array([labels.first.id, labels.second.id])
     end
   end
@@ -52,22 +55,8 @@ RSpec.shared_examples 'new issuable record that supports quick actions' do
     end
 
     it 'assigns and sets milestone to issuable' do
-      expect(issuable).to be_persisted
       expect(issuable.assignees).to eq([assignee])
       expect(issuable.milestone).to eq(milestone)
     end
   end
-
-  describe '/close' do
-    let(:example_params) do
-      {
-        description: '/close'
-      }
-    end
-
-    it 'returns an open issue' do
-      expect(issuable).to be_persisted
-      expect(issuable).to be_open
-    end
-  end
 end

spec/support/shared_examples/features/editable_merge_request_shared_examples.rb

@@ -53,13 +53,17 @@ RSpec.shared_examples 'an editable merge request' do
     find('#merge_request_description').native.send_keys('')
     fill_in 'merge_request_description', with: user.to_reference[0..4]
 
-    wait_for_requests
-
     page.within('.atwho-view') do
       expect(page).to have_content(user2.name)
     end
   end
 
+  it 'description has quick action autocomplete', :js do
+    find('#merge_request_description').native.send_keys('/')
+
+    expect(page).to have_selector('.atwho-container')
+  end
+
   it 'has class js-quick-submit in form' do
     expect(page).to have_selector('.js-quick-submit')
   end

spec/support/shared_examples/quick_actions/merge_request/merge_quick_action_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'merge quick action' do
-  context 'when the current user can merge the MR' do
+  context 'when updating the description' do
     before do
       sign_in(user)
-      visit project_merge_request_path(project, merge_request)
+      visit edit_project_merge_request_path(project, merge_request)
     end
 
-    it 'merges the MR', :sidekiq_might_not_need_inline do
-      add_note("/merge")
-
-      expect(page).to have_content 'Merged this merge request.'
+    it 'merges the MR', :sidekiq_inline do
+      fill_in('Description', with: '/merge')
+      click_button('Save changes')
 
+      expect(page).to have_content('Merged')
       expect(merge_request.reload).to be_merged
     end
+  end
 
-    context 'when auto merge is avialable' do
+  context 'when creating a new note' do
+    context 'when the current user can merge the MR' do
       before do
-        create(:ci_pipeline, :detached_merge_request_pipeline,
-          project: project, merge_request: merge_request)
-        merge_request.update_head_pipeline
+        sign_in(user)
+        visit project_merge_request_path(project, merge_request)
       end
 
-      it 'schedules to merge the MR' do
+      it 'merges the MR', :sidekiq_inline do
         add_note("/merge")
 
-        expect(page).to have_content "Scheduled to merge this merge request (Merge when pipeline succeeds)."
+        expect(page).to have_content 'Merged this merge request.'
 
-        expect(merge_request.reload).to be_auto_merge_enabled
-        expect(merge_request.reload).not_to be_merged
+        expect(merge_request.reload).to be_merged
       end
-    end
-  end
 
-  context 'when the head diff changes in the meanwhile' do
-    before do
-      merge_request.source_branch = 'another_branch'
-      merge_request.save
-      sign_in(user)
-      visit project_merge_request_path(project, merge_request)
-    end
+      context 'when auto merge is available' do
+        before do
+          create(:ci_pipeline, :detached_merge_request_pipeline,
+            project: project, merge_request: merge_request)
+          merge_request.update_head_pipeline
+        end
 
-    it 'does not merge the MR' do
-      add_note("/merge")
+        it 'schedules to merge the MR' do
+          add_note("/merge")
 
-      expect(page).not_to have_content 'Your commands have been executed!'
+          expect(page).to have_content "Scheduled to merge this merge request (Merge when pipeline succeeds)."
 
-      expect(merge_request.reload).not_to be_merged
+          expect(merge_request.reload).to be_auto_merge_enabled
+          expect(merge_request.reload).not_to be_merged
+        end
+      end
     end
-  end
 
-  context 'when the current user cannot merge the MR' do
-    before do
-      project.add_guest(guest)
-      sign_in(guest)
-      visit project_merge_request_path(project, merge_request)
+    context 'when the head diff changes in the meanwhile' do
+      before do
+        merge_request.source_branch = 'another_branch'
+        merge_request.save
+        sign_in(user)
+        visit project_merge_request_path(project, merge_request)
+      end
+
+      it 'does not merge the MR' do
+        add_note("/merge")
+
+        expect(page).not_to have_content 'Your commands have been executed!'
+
+        expect(merge_request.reload).not_to be_merged
+      end
     end
 
-    it 'does not merge the MR' do
-      add_note("/merge")
+    context 'when the current user cannot merge the MR' do
+      before do
+        project.add_guest(guest)
+        sign_in(guest)
+        visit project_merge_request_path(project, merge_request)
+      end
+
+      it 'does not merge the MR' do
+        add_note("/merge")
 
-      expect(page).not_to have_content 'Your commands have been executed!'
+        expect(page).not_to have_content 'Your commands have been executed!'
 
-      expect(merge_request.reload).not_to be_merged
+        expect(merge_request.reload).not_to be_merged
+      end
     end
   end
 end