Merge branch 'feat/customizable-suggestion-commit-messages' into 'master'

Customizable commit messages for applied suggested changes

Closes #13086

See merge request gitlab-org/gitlab!21411

app/controllers/projects_controller.rb

@@ -387,6 +387,7 @@ class ProjectsController < Projects::ApplicationController
       :merge_method,
       :initialize_with_readme,
       :autoclose_referenced_issues,
+      :suggestion_commit_message,
 
       project_feature_attributes: %i[
         builds_access_level

app/graphql/types/project_type.rb

@@ -104,6 +104,8 @@ module Types
           description: 'Indicates if `Delete source branch` option should be enabled by default for all new merge requests of the project'
     field :autoclose_referenced_issues, GraphQL::BOOLEAN_TYPE, null: true,
           description: 'Indicates if issues referenced by merge requests and commits within the default branch are closed automatically'
+    field :suggestion_commit_message, GraphQL::STRING_TYPE, null: true,
+          description: 'The commit message used to apply merge request suggestions'
 
     field :namespace, Types::NamespaceType, null: true,
           description: 'Namespace of the project'

app/services/suggestions/apply_service.rb

@@ -2,6 +2,24 @@
 
 module Suggestions
   class ApplyService < ::BaseService
+    DEFAULT_SUGGESTION_COMMIT_MESSAGE = 'Apply suggestion to %{file_path}'
+
+    PLACEHOLDERS = {
+      'project_path' => ->(suggestion, user) { suggestion.project.path },
+      'project_name' => ->(suggestion, user) { suggestion.project.name },
+      'file_path' => ->(suggestion, user) { suggestion.file_path },
+      'branch_name' => ->(suggestion, user) { suggestion.branch },
+      'username' => ->(suggestion, user) { user.username },
+      'user_full_name' => ->(suggestion, user) { user.name }
+    }.freeze
+
+    # This regex is built dynamically using the keys from the PLACEHOLDER struct.
+    # So, we can easily add new placeholder just by modifying the PLACEHOLDER hash.
+    # This regex will build the new PLACEHOLDER_REGEX with the new information
+    PLACEHOLDERS_REGEX = Regexp.union(PLACEHOLDERS.keys.map { |key| Regexp.new(Regexp.escape(key)) }).freeze
+
+    attr_reader :current_user
+
     def initialize(current_user)
       @current_user = current_user
     end
@@ -22,7 +40,7 @@ module Suggestions
       end
 
       params = file_update_params(suggestion, diff_file)
-      result = ::Files::UpdateService.new(suggestion.project, @current_user, params).execute
+      result = ::Files::UpdateService.new(suggestion.project, current_user, params).execute
 
       if result[:status] == :success
         suggestion.update(commit_id: result[:result], applied: true)
@@ -46,13 +64,14 @@ module Suggestions
 
     def file_update_params(suggestion, diff_file)
       blob = diff_file.new_blob
+      project = suggestion.project
       file_path = suggestion.file_path
       branch_name = suggestion.branch
       file_content = new_file_content(suggestion, blob)
-      commit_message = "Apply suggestion to #{file_path}"
+      commit_message = processed_suggestion_commit_message(suggestion)
 
       file_last_commit =
-        Gitlab::Git::Commit.last_for_path(suggestion.project.repository,
+        Gitlab::Git::Commit.last_for_path(project.repository,
                                           blob.commit_id,
                                           blob.path)
 
@@ -75,5 +94,17 @@ module Suggestions
 
       content.join
     end
+
+    def suggestion_commit_message(project)
+      project.suggestion_commit_message || DEFAULT_SUGGESTION_COMMIT_MESSAGE
+    end
+
+    def processed_suggestion_commit_message(suggestion)
+      message = suggestion_commit_message(suggestion.project)
+
+      Gitlab::StringPlaceholderReplacer.replace_string_placeholders(message, PLACEHOLDERS_REGEX) do |key|
+        PLACEHOLDERS[key].call(suggestion, current_user)
+      end
+    end
   end
 end

app/views/projects/_merge_request_merge_suggestions_settings.html.haml

+- form = local_assigns.fetch(:form)
+
+.form-group
+  %b= s_('ProjectSettings|Merge suggestions')
+  %p.text-secondary
+    = s_('ProjectSettings|The commit message used to apply merge request suggestions')
+    = link_to icon('question-circle'),
+      help_page_path('user/discussions/index.md',
+      anchor: 'configure-the-commit-message-for-applied-suggestions'),
+      target: '_blank'
+  .mb-2
+    = form.text_field :suggestion_commit_message, class: 'form-control mb-2', placeholder: Suggestions::ApplyService::DEFAULT_SUGGESTION_COMMIT_MESSAGE
+    %p.form-text.text-muted
+      = s_('ProjectSettings|The variables GitLab supports:')
+      - Suggestions::ApplyService::PLACEHOLDERS.keys.each do |placeholder|
+        %code
+          = "%{#{placeholder}}".html_safe

app/views/projects/_merge_request_settings.html.haml

@@ -5,3 +5,5 @@
 = render 'projects/merge_request_merge_options_settings', project: @project, form: form
 
 = render 'projects/merge_request_merge_checks_settings', project: @project, form: form
+
+= render 'projects/merge_request_merge_suggestions_settings', project: @project, form: form

app/views/projects/_merge_request_settings_description_text.html.haml

-%p= s_('ProjectSettings|Choose your merge method, merge options, and merge checks.')
+%p= s_('ProjectSettings|Choose your merge method, merge options, merge checks, and merge suggestions.')

changelogs/unreleased/feat-customizable-suggestion-commit-messages.yml

+---
+title: Implement customizable commit messages for applied suggested changes
+merge_request: 21411
+author: Fabio Huser
+type: added

db/migrate/20191208110214_add_suggestion_commit_message_to_projects.rb

+# frozen_string_literal: true
+
+class AddSuggestionCommitMessageToProjects < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :projects, :suggestion_commit_message, :string, limit: 255
+  end
+end

db/schema.rb

@@ -3348,6 +3348,7 @@ ActiveRecord::Schema.define(version: 2020_01_08_233040) do
     t.date "marked_for_deletion_at"
     t.integer "marked_for_deletion_by_user_id"
     t.boolean "autoclose_referenced_issues"
+    t.string "suggestion_commit_message", limit: 255
     t.index "lower((name)::text)", name: "index_projects_on_lower_name"
     t.index ["created_at", "id"], name: "index_projects_on_created_at_and_id"
     t.index ["creator_id"], name: "index_projects_on_creator_id"

doc/api/graphql/reference/gitlab_schema.graphql

@@ -5154,6 +5154,11 @@ type Project {
   """
   statistics: ProjectStatistics
 
+  """
+  The commit message used to apply merge request suggestions
+  """
+  suggestionCommitMessage: String
+
   """
   List of project tags
   """

doc/api/graphql/reference/gitlab_schema.json

@@ -1525,6 +1525,20 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "suggestionCommitMessage",
+              "description": "The commit message used to apply merge request suggestions",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "tagList",
               "description": "List of project tags",

doc/api/graphql/reference/index.md

@@ -769,6 +769,7 @@ Information about pagination in a connection.
 | `printingMergeRequestLinkEnabled` | Boolean | Indicates if a link to create or view a merge request should display after a push to Git repositories of the project from the command line |
 | `removeSourceBranchAfterMerge` | Boolean | Indicates if `Delete source branch` option should be enabled by default for all new merge requests of the project |
 | `autocloseReferencedIssues` | Boolean | Indicates if issues referenced by merge requests and commits within the default branch are closed automatically |
+| `suggestionCommitMessage` | String | The commit message used to apply merge request suggestions |
 | `namespace` | Namespace | Namespace of the project |
 | `group` | Group | Group of the project |
 | `statistics` | ProjectStatistics | Statistics of the project |

doc/api/projects.md

@@ -157,6 +157,7 @@ When the user is authenticated and `simple` is not set this returns something li
     "request_access_enabled": false,
     "merge_method": "merge",
     "autoclose_referenced_issues": true,
+    "suggestion_commit_message": null,
     "statistics": {
       "commit_count": 37,
       "storage_size": 1038090,
@@ -256,6 +257,7 @@ When the user is authenticated and `simple` is not set this returns something li
     "service_desk_enabled": false,
     "service_desk_address": null,
     "autoclose_referenced_issues": true,
+    "suggestion_commit_message": null,
     "statistics": {
       "commit_count": 12,
       "storage_size": 2066080,
@@ -388,6 +390,7 @@ This endpoint supports [keyset pagination](README.md#keyset-based-pagination) fo
     "request_access_enabled": false,
     "merge_method": "merge",
     "autoclose_referenced_issues": true,
+    "suggestion_commit_message": null,
     "statistics": {
       "commit_count": 37,
       "storage_size": 1038090,
@@ -487,6 +490,7 @@ This endpoint supports [keyset pagination](README.md#keyset-based-pagination) fo
     "service_desk_enabled": false,
     "service_desk_address": null,
     "autoclose_referenced_issues": true,
+    "suggestion_commit_message": null,
     "statistics": {
       "commit_count": 12,
       "storage_size": 2066080,
@@ -598,6 +602,7 @@ Example response:
     "request_access_enabled": false,
     "merge_method": "merge",
     "autoclose_referenced_issues": true,
+    "suggestion_commit_message": null,
     "statistics": {
       "commit_count": 37,
       "storage_size": 1038090,
@@ -694,6 +699,7 @@ Example response:
     "service_desk_enabled": false,
     "service_desk_address": null,
     "autoclose_referenced_issues": true,
+    "suggestion_commit_message": null,
     "statistics": {
       "commit_count": 12,
       "storage_size": 2066080,
@@ -844,6 +850,7 @@ GET /projects/:id
   "service_desk_enabled": false,
   "service_desk_address": null,
   "autoclose_referenced_issues": true,
+  "suggestion_commit_message": null,
   "statistics": {
     "commit_count": 37,
     "storage_size": 1038090,
@@ -1068,6 +1075,7 @@ POST /projects/user/:user_id
 | `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved |
 | `merge_method` | string | no | Set the [merge method](#project-merge-method) used |
 | `autoclose_referenced_issues` | boolean | no | Set whether auto-closing referenced issues on default branch |
+| `suggestion_commit_message` | string | no | The commit message used to apply merge request suggestions |
 | `remove_source_branch_after_merge` | boolean | no | Enable `Delete source branch` option by default for all new merge requests |
 | `lfs_enabled` | boolean | no | Enable LFS |
 | `request_access_enabled` | boolean | no | Allow users to request member access |
@@ -1133,6 +1141,7 @@ PUT /projects/:id
 | `only_allow_merge_if_all_discussions_are_resolved` | boolean | no | Set whether merge requests can only be merged when all the discussions are resolved |
 | `merge_method` | string | no | Set the [merge method](#project-merge-method) used |
 | `autoclose_referenced_issues` | boolean | no | Set whether auto-closing referenced issues on default branch |
+| `suggestion_commit_message` | string | no | The commit message used to apply merge request suggestions |
 | `remove_source_branch_after_merge` | boolean | no | Enable `Delete source branch` option by default for all new merge requests |
 | `lfs_enabled` | boolean | no | Enable LFS |
 | `request_access_enabled` | boolean | no | Allow users to request member access |
@@ -1265,6 +1274,7 @@ Example responses:
     "request_access_enabled": false,
     "merge_method": "merge",
     "autoclose_referenced_issues": true,
+    "suggestion_commit_message": null,
     "_links": {
       "self": "http://example.com/api/v4/projects",
       "issues": "http://example.com/api/v4/projects/1/issues",
@@ -1354,6 +1364,7 @@ Example response:
   "request_access_enabled": false,
   "merge_method": "merge",
   "autoclose_referenced_issues": true,
+  "suggestion_commit_message": null,
   "_links": {
     "self": "http://example.com/api/v4/projects",
     "issues": "http://example.com/api/v4/projects/1/issues",
@@ -1442,6 +1453,7 @@ Example response:
   "request_access_enabled": false,
   "merge_method": "merge",
   "autoclose_referenced_issues": true,
+  "suggestion_commit_message": null,
   "_links": {
     "self": "http://example.com/api/v4/projects",
     "issues": "http://example.com/api/v4/projects/1/issues",
@@ -1617,6 +1629,7 @@ Example response:
   "request_access_enabled": false,
   "merge_method": "merge",
   "autoclose_referenced_issues": true,
+  "suggestion_commit_message": null,
   "_links": {
     "self": "http://example.com/api/v4/projects",
     "issues": "http://example.com/api/v4/projects/1/issues",
@@ -1724,6 +1737,7 @@ Example response:
   "request_access_enabled": false,
   "merge_method": "merge",
   "autoclose_referenced_issues": true,
+  "suggestion_commit_message": null,
   "_links": {
     "self": "http://example.com/api/v4/projects",
     "issues": "http://example.com/api/v4/projects/1/issues",

doc/user/discussions/index.md

@@ -414,9 +414,8 @@ the Merge Request authored by the user that applied them.
 
 Once the author applies a suggestion, it will be marked with the **Applied** label,
 the thread will be automatically resolved, and GitLab will create a new commit
-with the message `Apply suggestion to <file-name>` and push the suggested change
-directly into the codebase in the merge request's branch.
-[Developer permission](../permissions.md) is required to do so.
+and push the suggested change directly into the codebase in the merge request's
+branch. [Developer permission](../permissions.md) is required to do so.
 
 > **Note:**
 Custom commit messages will be introduced by
@@ -444,6 +443,24 @@ Suggestions covering multiple lines are limited to 100 lines _above_ and 100
 lines _below_ the commented diff line, allowing up to 200 changed lines per
 suggestion.
 
+### Configure the commit message for applied suggestions
+
+GitLab will use `Apply suggestion to %{file_path}` by default as commit messages
+when applying change suggestions. This commit message can be customized to
+follow any guidelines you might have. To do so, open the **Merge requests** tab
+within your project settings and change the **Merge suggestions** text.
+
+![Suggestion Commit Message Configuration](img/suggestion-commit-message-configuration.png)
+
+You can also use following variables besides static text:
+
+- `%{project_path}`: The full URL safe project path. E.g: *my-group/my-project*
+- `%{project_name}`: The human readable name of the project. E.g: *My Project*
+- `%{file_path}`: The full path of the file the suggestion is applied to. E.g: *docs/index.md*
+- `%{branch_name}`: The name of the branch the suggestion is applied on. E.g: *my-feature-branch*
+- `%{username}`: The username of the user applying the suggestion. E.g: *user_1*
+- `%{user_full_name}`: The full name of the user applying the suggestion. E.g: *User 1*
+
 ## Start a thread by replying to a standard comment
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/issues/30299) in GitLab 11.9

doc/user/project/settings/index.md

@@ -91,6 +91,7 @@ Set up your project's merge request settings:
 - Enable [merge only if pipeline succeeds](../merge_requests/merge_when_pipeline_succeeds.md).
 - Enable [merge only when all threads are resolved](../../discussions/index.md#only-allow-merge-requests-to-be-merged-if-all-threads-are-resolved).
 - Enable [`delete source branch after merge` option by default](../merge_requests/getting_started.md#deleting-the-source-branch)
+- Configure [suggested changes commit messages](../../discussions/index.md#configure-the-commit-message-for-applied-suggestions)
 
 ![project's merge request settings](img/merge_requests_settings.png)
 

ee/app/views/projects/_merge_request_settings.html.haml

@@ -6,6 +6,8 @@
 
 = render_ce 'projects/merge_request_merge_checks_settings', project: @project, form: form
 
+= render 'projects/merge_request_merge_suggestions_settings', project: @project, form: form
+
 - if @project.feature_available?(:issuable_default_templates)
   .form-group
     = form.label :merge_requests_template, class: 'label-bold' do

ee/app/views/projects/_merge_request_settings_description_text.html.haml

 - if @project.feature_available?(:issuable_default_templates)
-  %p= s_('ProjectSettings|Choose your merge method, merge options, merge checks, and set up a default description template for merge requests.')
+  %p= s_('ProjectSettings|Choose your merge method, merge options, merge checks, merge suggestions, and set up a default description template for merge requests.')
 - else
-  %p= s_('ProjectSettings|Choose your merge method, merge options, and merge checks.')
+  %p= s_('ProjectSettings|Choose your merge method, merge options, merge checks, and merge suggestions.')

ee/spec/features/projects/settings/merge_requests_settings_spec.rb

@@ -104,7 +104,7 @@ describe 'Project settings > [EE] Merge Requests', :js do
     it "does not mention the merge request template in the section's description text" do
       visit edit_project_path(project)
 
-      expect(page).to have_content('Choose your merge method, merge options, and merge checks.')
+      expect(page).to have_content('Choose your merge method, merge options, merge checks, and merge suggestions.')
     end
   end
 
@@ -122,7 +122,7 @@ describe 'Project settings > [EE] Merge Requests', :js do
     it "mentions the merge request template in the section's description text" do
       visit edit_project_path(project)
 
-      expect(page).to have_content('Choose your merge method, merge options, merge checks, and set up a default description template for merge requests.')
+      expect(page).to have_content('Choose your merge method, merge options, merge checks, merge suggestions, and set up a default description template for merge requests.')
     end
   end
 end

lib/api/entities.rb

@@ -335,6 +335,7 @@ module API
       expose :remove_source_branch_after_merge
       expose :printing_merge_request_link_enabled
       expose :merge_method
+      expose :suggestion_commit_message
       expose :statistics, using: 'API::Entities::ProjectStatistics', if: -> (project, options) {
         options[:statistics] && Ability.allowed?(options[:current_user], :read_statistics, project)
       }

lib/api/helpers/projects_helpers.rb

@@ -46,6 +46,7 @@ module API
         optional :avatar, type: File, desc: 'Avatar image for project' # rubocop:disable Scalability/FileUploads
         optional :printing_merge_request_link_enabled, type: Boolean, desc: 'Show link to create/view merge request when pushing from the command line'
         optional :merge_method, type: String, values: %w(ff rebase_merge merge), desc: 'The merge method used when merging merge requests'
+        optional :suggestion_commit_message, type: String, desc: 'The commit message used to apply merge request suggestions'
         optional :initialize_with_readme, type: Boolean, desc: "Initialize a project with a README.md"
         optional :ci_default_git_depth, type: Integer, desc: 'Default number of revisions for shallow cloning'
         optional :auto_devops_enabled, type: Boolean, desc: 'Flag indication if Auto DevOps is enabled'
@@ -119,6 +120,7 @@ module API
           :visibility,
           :wiki_access_level,
           :avatar,
+          :suggestion_commit_message,
 
           # TODO: remove in API v5, replaced by *_access_level
           :issues_enabled,

locale/gitlab.pot

@@ -14130,10 +14130,10 @@ msgstr ""
 msgid "ProjectSettings|Build, test, and deploy your changes"
 msgstr ""
 
-msgid "ProjectSettings|Choose your merge method, merge options, and merge checks."
+msgid "ProjectSettings|Choose your merge method, merge options, merge checks, and merge suggestions."
 msgstr ""
 
-msgid "ProjectSettings|Choose your merge method, merge options, merge checks, and set up a default description template for merge requests."
+msgid "ProjectSettings|Choose your merge method, merge options, merge checks, merge suggestions, and set up a default description template for merge requests."
 msgstr ""
 
 msgid "ProjectSettings|Contact an admin to change this setting."
@@ -14217,6 +14217,9 @@ msgstr ""
 msgid "ProjectSettings|Merge requests"
 msgstr ""
 
+msgid "ProjectSettings|Merge suggestions"
+msgstr ""
+
 msgid "ProjectSettings|No merge commits are created"
 msgstr ""
 
@@ -14268,6 +14271,12 @@ msgstr ""
 msgid "ProjectSettings|Submit changes to be merged upstream"
 msgstr ""
 
+msgid "ProjectSettings|The commit message used to apply merge request suggestions"
+msgstr ""
+
+msgid "ProjectSettings|The variables GitLab supports:"
+msgstr ""
+
 msgid "ProjectSettings|These checks must pass before merge requests can be merged"
 msgstr ""
 

spec/graphql/types/project_type_spec.rb

@@ -23,7 +23,7 @@ describe GitlabSchema.types['Project'] do
       only_allow_merge_if_all_discussions_are_resolved printing_merge_request_link_enabled
       namespace group statistics repository merge_requests merge_request issues
       issue pipelines removeSourceBranchAfterMerge sentryDetailedError snippets
-      grafanaIntegration autocloseReferencedIssues
+      grafanaIntegration autocloseReferencedIssues suggestion_commit_message
     ]
 
     is_expected.to include_graphql_fields(*expected_fields)

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -535,6 +535,7 @@ Project:
 - merge_requests_disable_committers_approval
 - require_password_to_approve
 - autoclose_referenced_issues
+- suggestion_commit_message
 ProjectTracingSetting:
 - external_url
 Author:

spec/services/suggestions/apply_service_spec.rb

@@ -48,10 +48,34 @@ describe Suggestions::ApplyService do
       expect(commit.committer_email).to eq(user.commit_email)
       expect(commit.author_name).to eq(user.name)
     end
+
+    context 'when a custom suggestion commit message' do
+      before do
+        project.update!(suggestion_commit_message: message)
+
+        apply(suggestion)
+      end
+
+      context 'is not specified' do
+        let(:message) { nil }
+
+        it 'sets default commit message' do
+          expect(project.repository.commit.message).to eq("Apply suggestion to files/ruby/popen.rb")
+        end
+      end
+
+      context 'is specified' do
+        let(:message) { 'refactor: %{project_path} %{project_name} %{file_path} %{branch_name} %{username} %{user_full_name}' }
+
+        it 'sets custom commit message' do
+          expect(project.repository.commit.message).to eq("refactor: project-1 Project_1 files/ruby/popen.rb master test.user Test User")
+        end
+      end
+    end
   end
 
-  let(:project) { create(:project, :repository) }
-  let(:user) { create(:user, :commit_email) }
+  let(:project) { create(:project, :repository, path: 'project-1', name: 'Project_1') }
+  let(:user) { create(:user, :commit_email, name: 'Test User', username: 'test.user') }
 
   let(:position) { build_position }
 
@@ -113,7 +137,8 @@ describe Suggestions::ApplyService do
     context 'non-fork project' do
       let(:merge_request) do
         create(:merge_request, source_project: project,
-                               target_project: project)
+                               target_project: project,
+                               source_branch: 'master')
       end
 
       before do

spec/views/projects/edit.html.haml_spec.rb

@@ -28,6 +28,33 @@ describe 'projects/edit' do
     end
   end
 
+  context 'merge suggestions settings' do
+    it 'displays all possible variables' do
+      render
+
+      expect(rendered).to have_content('%{project_path}')
+      expect(rendered).to have_content('%{project_name}')
+      expect(rendered).to have_content('%{file_path}')
+      expect(rendered).to have_content('%{branch_name}')
+      expect(rendered).to have_content('%{username}')
+      expect(rendered).to have_content('%{user_full_name}')
+    end
+
+    it 'displays a placeholder if none is set' do
+      render
+
+      expect(rendered).to have_field('project[suggestion_commit_message]', placeholder: 'Apply suggestion to %{file_path}')
+    end
+
+    it 'displays the user entered value' do
+      project.update!(suggestion_commit_message: 'refactor: changed %{file_path}')
+
+      render
+
+      expect(rendered).to have_field('project[suggestion_commit_message]', with: 'refactor: changed %{file_path}')
+    end
+  end
+
   context 'forking' do
     before do
       assign(:project, project)