diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml index 78c086d2a08f0736b2dab4589ea78da7591f466d..7fa37562b46bdc019b842e2193914ac03fce4547 100644 --- a/.rubocop_todo/layout/line_length.yml +++ b/.rubocop_todo/layout/line_length.yml @@ -3270,7 +3270,6 @@ Layout/LineLength: - 'lib/gitlab/lograge/custom_options.rb' - 'lib/gitlab/mail_room/authenticator.rb' - 'lib/gitlab/markdown_cache/active_record/extension.rb' - - 'lib/gitlab/merge_requests/commit_message_generator.rb' - 'lib/gitlab/metrics/dashboard/importer.rb' - 'lib/gitlab/metrics/dashboard/importers/prometheus_metrics.rb' - 'lib/gitlab/metrics/dashboard/stages/cluster_endpoint_inserter.rb' @@ -4606,7 +4605,6 @@ Layout/LineLength: - 'spec/lib/gitlab/legacy_github_import/pull_request_formatter_spec.rb' - 'spec/lib/gitlab/lfs/client_spec.rb' - 'spec/lib/gitlab/mail_room/authenticator_spec.rb' - - 'spec/lib/gitlab/merge_requests/commit_message_generator_spec.rb' - 'spec/lib/gitlab/metrics/background_transaction_spec.rb' - 'spec/lib/gitlab/metrics/boot_time_tracker_spec.rb' - 'spec/lib/gitlab/metrics/dashboard/finder_spec.rb' diff --git a/.rubocop_todo/layout/space_in_lambda_literal.yml b/.rubocop_todo/layout/space_in_lambda_literal.yml index 6bb31295fbd9da3a42be39ef3bfcf9be900688e9..31b73fbe8a3a2552a8489dd0d09672c7824bb634 100644 --- a/.rubocop_todo/layout/space_in_lambda_literal.yml +++ b/.rubocop_todo/layout/space_in_lambda_literal.yml @@ -396,7 +396,6 @@ Layout/SpaceInLambdaLiteral: - 'lib/gitlab/gl_repository.rb' - 'lib/gitlab/health_checks/server.rb' - 'lib/gitlab/import_export/import_failure_service.rb' - - 'lib/gitlab/merge_requests/commit_message_generator.rb' - 'lib/gitlab/metrics/dashboard/transformers/yml/v1/prometheus_metrics.rb' - 'lib/gitlab/metrics/exporter/base_exporter.rb' - 'lib/gitlab/visibility_level.rb' diff --git a/.rubocop_todo/rails/pluck.yml b/.rubocop_todo/rails/pluck.yml index 6aeca1b7ea8b35c6be52e619c2af010a544d5f41..e094a3397a6fc50eca0f5008229421f719722387 100644 --- a/.rubocop_todo/rails/pluck.yml +++ b/.rubocop_todo/rails/pluck.yml @@ -126,7 +126,7 @@ Rails/Pluck: - 'lib/gitlab/git_access.rb' - 'lib/gitlab/github_import/representation/issue.rb' - 'lib/gitlab/jira_import/metadata_collector.rb' - - 'lib/gitlab/merge_requests/commit_message_generator.rb' + - 'lib/gitlab/merge_requests/message_generator.rb' - 'lib/gitlab/metrics/dashboard/importers/prometheus_metrics.rb' - 'lib/gitlab/metrics/dashboard/stages/custom_metrics_details_inserter.rb' - 'lib/gitlab/sidekiq_config/cli_methods.rb' diff --git a/.rubocop_todo/rspec/context_wording.yml b/.rubocop_todo/rspec/context_wording.yml index 523cc1bfad9d4fbb9b93571254b6d44751445059..7131626d15b51b1425b91ca5192abeac26943e00 100644 --- a/.rubocop_todo/rspec/context_wording.yml +++ b/.rubocop_todo/rspec/context_wording.yml @@ -2023,7 +2023,7 @@ RSpec/ContextWording: - 'spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb' - 'spec/lib/gitlab/memory/reports_daemon_spec.rb' - 'spec/lib/gitlab/memory/watchdog_spec.rb' - - 'spec/lib/gitlab/merge_requests/commit_message_generator_spec.rb' + - 'spec/lib/gitlab/merge_requests/message_generator_spec.rb' - 'spec/lib/gitlab/metrics/dashboard/cache_spec.rb' - 'spec/lib/gitlab/metrics/dashboard/importer_spec.rb' - 'spec/lib/gitlab/metrics/dashboard/importers/prometheus_metrics_spec.rb' diff --git a/.rubocop_todo/rspec/verified_doubles.yml b/.rubocop_todo/rspec/verified_doubles.yml index 1d4b6d23e0d65d2ae57e0910550301d3f2385158..8fa03c775be607c6457945439aa8e88fb80061dd 100644 --- a/.rubocop_todo/rspec/verified_doubles.yml +++ b/.rubocop_todo/rspec/verified_doubles.yml @@ -633,7 +633,7 @@ RSpec/VerifiedDoubles: - 'spec/lib/gitlab/mail_room/mail_room_spec.rb' - 'spec/lib/gitlab/manifest_import/metadata_spec.rb' - 'spec/lib/gitlab/markdown_cache/field_data_spec.rb' - - 'spec/lib/gitlab/merge_requests/commit_message_generator_spec.rb' + - 'spec/lib/gitlab/merge_requests/message_generator_spec.rb' - 'spec/lib/gitlab/merge_requests/mergeability/redis_interface_spec.rb' - 'spec/lib/gitlab/metrics/boot_time_tracker_spec.rb' - 'spec/lib/gitlab/metrics/dashboard/importers/prometheus_metrics_spec.rb' diff --git a/.rubocop_todo/style/accessor_grouping.yml b/.rubocop_todo/style/accessor_grouping.yml index 27b14007afde179c4752b504265bf9004c86ac4b..a558681388544143ac1d97c67a980e20947293a8 100644 --- a/.rubocop_todo/style/accessor_grouping.yml +++ b/.rubocop_todo/style/accessor_grouping.yml @@ -57,7 +57,6 @@ Style/AccessorGrouping: - 'lib/gitlab/http_io.rb' - 'lib/gitlab/import_export/group/legacy_tree_restorer.rb' - 'lib/gitlab/import_export/project/tree_restorer.rb' - - 'lib/gitlab/merge_requests/commit_message_generator.rb' - 'lib/gitlab/sidekiq_daemon/monitor.rb' - 'lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb' - 'lib/gitlab/suggestions/file_suggestion.rb' diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9b7c14c6a0e22fd9eef3aea497174fa0e19f3b97..f9d1185498197caa59d097721442863eb0b434d8 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1383,7 +1383,7 @@ def target_branch_exists? def default_merge_commit_message(include_description: false, user: nil) if self.target_project.merge_commit_template.present? && !include_description - return ::Gitlab::MergeRequests::CommitMessageGenerator.new(merge_request: self, current_user: user).merge_message + return ::Gitlab::MergeRequests::MessageGenerator.new(merge_request: self, current_user: user).merge_commit_message end closes_issues_references = visible_closing_issues_for.map do |issue| @@ -1407,7 +1407,7 @@ def default_merge_commit_message(include_description: false, user: nil) def default_squash_commit_message(user: nil) if self.target_project.squash_commit_template.present? - return ::Gitlab::MergeRequests::CommitMessageGenerator.new(merge_request: self, current_user: user).squash_message + return ::Gitlab::MergeRequests::MessageGenerator.new(merge_request: self, current_user: user).squash_commit_message end title diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index cc786ac02bd601837f889d10f7a567341119861b..b9a681f29dbb6de17391a00eccaa1c95daba4810 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -224,6 +224,7 @@ def set_draft_title_if_needed # def assign_title_and_description assign_description_from_repository_template + replace_variables_in_description assign_title_and_description_from_commits merge_request.title ||= title_from_issue if target_project.issues_enabled? || target_project.external_issue_tracker merge_request.title ||= source_branch.titleize.humanize @@ -318,6 +319,15 @@ def assign_description_from_repository_template merge_request.description = repository_template.content end + def replace_variables_in_description + return unless merge_request.description.present? + + merge_request.description = ::Gitlab::MergeRequests::MessageGenerator.new( + merge_request: merge_request, + current_user: current_user + ).new_mr_description + end + def issue_iid strong_memoize(:issue_iid) do @params_issue_iid || begin diff --git a/doc/user/project/description_templates.md b/doc/user/project/description_templates.md index 40c36236932ff5b06c7ec846c9234ec1bc2fc0ff..7774b567e8b529eceda1a788f6fea1a14dd12625 100644 --- a/doc/user/project/description_templates.md +++ b/doc/user/project/description_templates.md @@ -83,6 +83,22 @@ NOTE: You can create shortcut links to create an issue using a designated template. For example: `https://gitlab.com/gitlab-org/gitlab/-/issues/new?issuable_template=Feature%20proposal`. Read more about [creating issues using a URL with prefilled values](issues/managing_issues.md#using-a-url-with-prefilled-values). +### Supported variables in merge request templates + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/89810) in GitLab 15.7. + +When you save a merge request for the first time, GitLab replaces these variables in +your merge request template with their values: + +| Variable | Description | Output example | +|----------|-------------|----------------| +| `%{all_commits}` | Messages from all commits in the merge request. Limited to 100 most recent commits. Skips commit bodies exceeding 100 KiB and merge commit messages. | `* Feature introduced`

`This commit implements feature`
`Changelog:added`

`* Bug fixed`

`* Documentation improved`

`This commit introduced better docs.` | +| `%{co_authored_by}` | Names and emails of commit authors in a `Co-authored-by` Git commit trailer format. Limited to authors of 100 most recent commits in merge request. | `Co-authored-by: Zane Doe `
`Co-authored-by: Blake Smith ` | +| `%{first_commit}` | Full message of the first commit in merge request diff. | `Update README.md` | +| `%{first_multiline_commit}` | Full message of the first commit that's not a merge commit and has more than one line in message body. Merge request title if all commits aren't multiline. | `Update README.md`

`Improved project description in readme file.` | +| `%{source_branch}` | The name of the branch being merged. | `my-feature-branch` | +| `%{target_branch}` | The name of the branch that the changes are applied to. | `main` | + ### Set instance-level description templates **(PREMIUM SELF)** You can set a description template at the **instance level** for issues diff --git a/lib/gitlab/merge_requests/commit_message_generator.rb b/lib/gitlab/merge_requests/message_generator.rb similarity index 58% rename from lib/gitlab/merge_requests/commit_message_generator.rb rename to lib/gitlab/merge_requests/message_generator.rb index b09e2150012dc833e6956c285bc8481149a41026..5113fbdcd7b0229c210e26337c515efb96a68ad6 100644 --- a/lib/gitlab/merge_requests/commit_message_generator.rb +++ b/lib/gitlab/merge_requests/message_generator.rb @@ -1,28 +1,41 @@ # frozen_string_literal: true module Gitlab module MergeRequests - class CommitMessageGenerator + class MessageGenerator def initialize(merge_request:, current_user:) @merge_request = merge_request @current_user = @merge_request.metrics&.merged_by || @merge_request.merge_user || current_user end - def merge_message + def merge_commit_message return unless @merge_request.target_project.merge_commit_template.present? - replace_placeholders(@merge_request.target_project.merge_commit_template) + replace_placeholders(@merge_request.target_project.merge_commit_template, allowed_placeholders: PLACEHOLDERS) end - def squash_message + def squash_commit_message return unless @merge_request.target_project.squash_commit_template.present? - replace_placeholders(@merge_request.target_project.squash_commit_template, squash: true) + replace_placeholders( + @merge_request.target_project.squash_commit_template, + allowed_placeholders: PLACEHOLDERS, + squash: true + ) + end + + def new_mr_description + return unless @merge_request.description.present? + + replace_placeholders( + @merge_request.description, + allowed_placeholders: ALLOWED_NEW_MR_PLACEHOLDERS, + keep_carriage_return: true + ) end private - attr_reader :merge_request - attr_reader :current_user + attr_reader :merge_request, :current_user PLACEHOLDERS = { 'source_branch' => ->(merge_request, _, _) { merge_request.source_branch.to_s }, @@ -38,11 +51,25 @@ def squash_message end, 'description' => ->(merge_request, _, _) { merge_request.description }, 'reference' => ->(merge_request, _, _) { merge_request.to_reference(full: true) }, - 'first_commit' => -> (merge_request, _, _) { merge_request.first_commit&.safe_message&.strip }, - 'first_multiline_commit' => -> (merge_request, _, _) { merge_request.first_multiline_commit&.safe_message&.strip.presence || merge_request.title }, + 'first_commit' => -> (merge_request, _, _) { + return unless merge_request.persisted? || merge_request.compare_commits.present? + + merge_request.first_commit&.safe_message&.strip + }, + 'first_multiline_commit' => -> (merge_request, _, _) { + merge_request.first_multiline_commit&.safe_message&.strip.presence || merge_request.title + }, 'url' => ->(merge_request, _, _) { Gitlab::UrlBuilder.build(merge_request) }, - 'reviewed_by' => ->(merge_request, _, _) { merge_request.reviewed_by_users.map { |user| "Reviewed-by: #{user.name} <#{user.commit_email_or_default}>" }.join("\n") }, - 'approved_by' => ->(merge_request, _, _) { merge_request.approved_by_users.map { |user| "Approved-by: #{user.name} <#{user.commit_email_or_default}>" }.join("\n") }, + 'reviewed_by' => ->(merge_request, _, _) { + merge_request.reviewed_by_users + .map { |user| "Reviewed-by: #{user.name} <#{user.commit_email_or_default}>" } + .join("\n") + }, + 'approved_by' => ->(merge_request, _, _) { + merge_request.approved_by_users + .map { |user| "Approved-by: #{user.name} <#{user.commit_email_or_default}>" } + .join("\n") + }, 'merged_by' => ->(_, user, _) { "#{user&.name} <#{user&.commit_email_or_default}>" }, 'co_authored_by' => ->(merge_request, merged_by, squash) do commit_author = squash ? merge_request.author : merged_by @@ -67,15 +94,34 @@ def squash_message end }.freeze + # A new merge request that is in the process of being created and hasn't + # been persisted to the database. + # + # Limit the placeholders to a subset of the available ones where the + # placeholders wouldn't make sense in context. Disallowed placeholders + # will be replaced with an empty string. + ALLOWED_NEW_MR_PLACEHOLDERS = %w[ + source_branch + target_branch + first_commit + first_multiline_commit + co_authored_by + all_commits + ].freeze + PLACEHOLDERS_COMBINED_REGEX = /%{(#{Regexp.union(PLACEHOLDERS.keys)})}/.freeze - def replace_placeholders(message, squash: false) + def replace_placeholders(message, allowed_placeholders: [], squash: false, keep_carriage_return: false) # Convert CRLF to LF. - message = message.delete("\r") + message = message.delete("\r") unless keep_carriage_return used_variables = message.scan(PLACEHOLDERS_COMBINED_REGEX).map { |value| value[0] }.uniq values = used_variables.to_h do |variable_name| - ["%{#{variable_name}}", PLACEHOLDERS[variable_name].call(merge_request, current_user, squash)] + replacement = if allowed_placeholders.include?(variable_name) + PLACEHOLDERS[variable_name].call(merge_request, current_user, squash) + end + + ["%{#{variable_name}}", replacement] end names_of_empty_variables = values.filter_map { |name, value| name if value.blank? } diff --git a/spec/lib/gitlab/merge_requests/commit_message_generator_spec.rb b/spec/lib/gitlab/merge_requests/message_generator_spec.rb similarity index 87% rename from spec/lib/gitlab/merge_requests/commit_message_generator_spec.rb rename to spec/lib/gitlab/merge_requests/message_generator_spec.rb index bdc9879f36298cd85379635886a1db9c7dfa664e..fbdd6a1e56d58eae31151096d0f4597d44a8d8fa 100644 --- a/spec/lib/gitlab/merge_requests/commit_message_generator_spec.rb +++ b/spec/lib/gitlab/merge_requests/message_generator_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::MergeRequests::CommitMessageGenerator do +RSpec.describe Gitlab::MergeRequests::MessageGenerator do let(:merge_commit_template) { nil } let(:squash_commit_template) { nil } let(:project) do @@ -59,7 +59,14 @@ context 'when project has commit template with only the title' do let(:merge_request) do - double(:merge_request, title: 'Fixes', target_project: project, to_reference: '!123', metrics: nil, merge_user: nil) + double( + :merge_request, + title: 'Fixes', + target_project: project, + to_reference: '!123', + metrics: nil, + merge_user: nil + ) end let(message_template_name) { '%{title}' } @@ -214,7 +221,7 @@ context 'when project has template with CRLF newlines' do let(message_template_name) do - "Merge branch '%{source_branch}' into '%{target_branch}'\r\n\r\n%{title}\r\n\r\n%{description}\r\n\r\nSee merge request %{reference}" + "Merge branch '%{source_branch}' into '%{target_branch}'\r\n\r\n%{title}\r\n\r\n%{description}\r\n\r\nSee merge request %{reference}" # rubocop: disable Layout/LineLength end it 'converts it to LF newlines' do @@ -717,8 +724,8 @@ end end - describe '#merge_message' do - let(:result_message) { subject.merge_message } + describe '#merge_commit_message' do + let(:result_message) { subject.merge_commit_message } it_behaves_like 'commit message with template', :merge_commit_template @@ -749,8 +756,8 @@ end end - describe '#squash_message' do - let(:result_message) { subject.squash_message } + describe '#squash_commit_message' do + let(:result_message) { subject.squash_commit_message } it_behaves_like 'commit message with template', :squash_commit_template @@ -780,4 +787,95 @@ end end end + + describe '#new_mr_description' do + let(:merge_request) do + build( + :merge_request, + source_project: project, + target_project: project, + target_branch: 'master', + source_branch: source_branch, + author: author, + description: merge_request_description, + title: merge_request_title + ) + end + + let(:result_message) { subject.new_mr_description } + + before do + compare = CompareService.new( + project, + merge_request.source_branch + ).execute( + project, + merge_request.target_branch + ) + + merge_request.compare_commits = compare.commits + merge_request.compare = compare + end + + context 'when project has template with all variables' do + let(:merge_request_description) { <<~MSG.rstrip } + source_branch:%{source_branch} + target_branch:%{target_branch} + title:%{title} + issues:%{issues} + description:%{description} + first_commit:%{first_commit} + first_multiline_commit:%{first_multiline_commit} + url:%{url} + approved_by:%{approved_by} + merged_by:%{merged_by} + co_authored_by:%{co_authored_by} + all_commits:%{all_commits} + MSG + + it 'renders only variables specific to a new non-persisted merge request' do + expect(result_message).to eq <<~MSG.rstrip + source_branch:feature + target_branch:master + title: + issues: + description: + first_commit:Feature added + + Signed-off-by: Dmitriy Zaporozhets + first_multiline_commit:Feature added + + Signed-off-by: Dmitriy Zaporozhets + url: + approved_by: + merged_by: + co_authored_by:Co-authored-by: Dmitriy Zaporozhets + all_commits:* Feature added + + Signed-off-by: Dmitriy Zaporozhets + MSG + end + + context 'when no first commit exists' do + let(:source_branch) { 'master' } + + it 'does not populate any commit-related variables' do + expect(result_message).to eq <<~MSG.rstrip + source_branch:master + target_branch:master + title: + issues: + description: + first_commit: + first_multiline_commit:Bugfix + url: + approved_by: + merged_by: + co_authored_by: + all_commits: + MSG + end + end + end + end end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index 4f27ff30da717fc4979d9e082f22bf7673784f01..79c779678a45efc23088a6c4e678f1e16b72b8e9 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -25,7 +25,9 @@ safe_message: 'Initial commit', gitaly_commit?: false, id: 'f00ba6', - parent_ids: ['f00ba5']) + parent_ids: ['f00ba5'], + author_email: 'tom@example.com', + author_name: 'Tom Example') end let(:commit_2) do @@ -34,7 +36,9 @@ safe_message: "Closes #1234 Second commit\n\nCreate the app", gitaly_commit?: false, id: 'f00ba7', - parent_ids: ['f00ba6']) + parent_ids: ['f00ba6'], + author_email: 'alice@example.com', + author_name: 'Alice Example') end let(:commit_3) do @@ -43,7 +47,9 @@ safe_message: 'This is a bad commit message!', gitaly_commit?: false, id: 'f00ba8', - parent_ids: ['f00ba7']) + parent_ids: ['f00ba7'], + author_email: 'jo@example.com', + author_name: 'Jo Example') end let(:commits) { nil } @@ -742,4 +748,91 @@ def stub_compare end end end + + describe '#replace_variables_in_description' do + context 'when the merge request description is blank' do + let(:description) { nil } + + it 'does not update the description' do + expect(merge_request.description).to eq(nil) + end + end + + context 'when the merge request description contains template variables' do + let(:description) { <<~MSG.rstrip } + source_branch:%{source_branch} + target_branch:%{target_branch} + title:%{title} + issues:%{issues} + description:%{description} + first_commit:%{first_commit} + first_multiline_commit:%{first_multiline_commit} + url:%{url} + approved_by:%{approved_by} + merged_by:%{merged_by} + co_authored_by:%{co_authored_by} + all_commits:%{all_commits} + MSG + + context 'when there are multiple commits in the diff' do + let(:commits) { Commit.decorate([commit_1, commit_2, commit_3], project) } + + before do + stub_compare + end + + it 'replaces the variables in the description' do + expect(merge_request.description).to eq <<~MSG.rstrip + source_branch:feature-branch + target_branch:master + title: + issues: + description: + first_commit:Initial commit + first_multiline_commit:Closes #1234 Second commit + + Create the app + url: + approved_by: + merged_by: + co_authored_by:Co-authored-by: Jo Example + Co-authored-by: Alice Example + Co-authored-by: Tom Example + all_commits:* This is a bad commit message! + + * Closes #1234 Second commit + + Create the app + + * Initial commit + MSG + end + end + + context 'when there are no commits in the diff' do + let(:commits) { [] } + + before do + stub_compare + end + + it 'replaces the variables in the description' do + expect(merge_request.description).to eq <<~MSG.rstrip + source_branch:feature-branch + target_branch:master + title: + issues: + description: + first_commit: + first_multiline_commit: + url: + approved_by: + merged_by: + co_authored_by: + all_commits: + MSG + end + end + end + end end