Merge branch '293846-handle-wip-draft-in-commit-mr-linting' into 'master'

Handle WIP/DRAFT in commit/mr linting

See merge request gitlab-org/gitlab!50718

lib/gitlab/danger/base_linter.rb

 # frozen_string_literal: true
 
+require_relative 'title_linting'
+
 module Gitlab
   module Danger
     class BaseLinter
       MIN_SUBJECT_WORDS_COUNT = 3
       MAX_LINE_LENGTH = 72
-      WIP_PREFIX = 'WIP: '
 
       attr_reader :commit, :problems
 
@@ -58,7 +59,7 @@ module Gitlab
       private
 
       def subject
-        message_parts[0].delete_prefix(WIP_PREFIX)
+        TitleLinting.remove_draft_flag(message_parts[0])
       end
 
       def subject_too_short?

lib/gitlab/danger/changelog.rb

 # frozen_string_literal: true
 
+require_relative 'title_linting'
+
 module Gitlab
   module Danger
     module Changelog
@@ -75,7 +77,7 @@ module Gitlab
       end
 
       def sanitized_mr_title
-        helper.sanitize_mr_title(gitlab.mr_json["title"])
+        TitleLinting.sanitize_mr_title(gitlab.mr_json["title"])
       end
 
       def categories_need_changelog?

lib/gitlab/danger/helper.rb

 # frozen_string_literal: true
 
 require_relative 'teammate'
+require_relative 'title_linting'
 
 module Gitlab
   module Danger
     module Helper
       RELEASE_TOOLS_BOT = 'gitlab-release-tools-bot'
-      DRAFT_REGEX = /\A*#{Regexp.union(/(?i)(\[WIP\]\s*|WIP:\s*|WIP$)/, /(?i)(\[draft\]|\(draft\)|draft:|draft\s\-\s|draft$)/)}+\s*/i.freeze
 
       # Returns a list of all files that have been added, modified or renamed.
       # `git.modified_files` might contain paths that already have been renamed,
@@ -216,14 +216,10 @@ module Gitlab
         usernames.map { |u| Gitlab::Danger::Teammate.new('username' => u) }
       end
 
-      def sanitize_mr_title(title)
-        title.gsub(DRAFT_REGEX, '').gsub(/`/, '\\\`')
-      end
-
       def draft_mr?
         return false unless gitlab_helper
 
-        DRAFT_REGEX.match?(gitlab_helper.mr_json['title'])
+        TitleLinting.has_draft_flag?(gitlab_helper.mr_json['title'])
       end
 
       def security_mr?

lib/gitlab/danger/title_linting.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Danger
+    module TitleLinting
+      DRAFT_REGEX = /\A*#{Regexp.union(/(?i)(\[WIP\]\s*|WIP:\s*|WIP$)/, /(?i)(\[draft\]|\(draft\)|draft:|draft\s\-\s|draft$)/)}+\s*/i.freeze
+
+      module_function
+
+      def sanitize_mr_title(title)
+        remove_draft_flag(title).gsub(/`/, '\\\`')
+      end
+
+      def remove_draft_flag(title)
+        title.gsub(DRAFT_REGEX, '')
+      end
+
+      def has_draft_flag?(title)
+        DRAFT_REGEX.match?(title)
+      end
+    end
+  end
+end

spec/lib/gitlab/danger/base_linter_spec.rb

 # frozen_string_literal: true
 
 require 'fast_spec_helper'
+require 'rspec-parameterized'
 require_relative 'danger_spec_helper'
 
 require 'gitlab/danger/base_linter'
@@ -70,19 +71,57 @@ RSpec.describe Gitlab::Danger::BaseLinter do
       end
     end
 
-    context 'when subject is a WIP' do
+    context 'when ignoring length issues for subject having not-ready wording' do
+      using RSpec::Parameterized::TableSyntax
+
       let(:final_message) { 'A B C' }
-      # commit message with prefix will be over max length. commit message without prefix will be of maximum size
-      let(:commit_message) { described_class::WIP_PREFIX + final_message + 'D' * (described_class::MAX_LINE_LENGTH - final_message.size) }
 
+      context 'when used as prefix' do
+        where(prefix: [
+          'WIP: ',
+          'WIP:',
+          'wIp:',
+          '[WIP] ',
+          '[WIP]',
+          '[draft]',
+          '[draft] ',
+          '(draft)',
+          '(draft) ',
+          'draft - ',
+          'draft: ',
+          'draft:',
+          'DRAFT:'
+        ])
+
+        with_them do
           it 'does not have any problems' do
-        commit_linter.lint_subject
+            commit_message = prefix + final_message + 'D' * (described_class::MAX_LINE_LENGTH - final_message.size)
+            commit = commit_class.new(commit_message, anything, anything)
 
-        expect(commit_linter.problems).to be_empty
+            linter = described_class.new(commit).lint_subject
+
+            expect(linter.problems).to be_empty
+          end
+        end
+      end
+
+      context 'when used as suffix' do
+        where(suffix: %w[WIP draft])
+
+        with_them do
+          it 'does not have any problems' do
+            commit_message = final_message + 'D' * (described_class::MAX_LINE_LENGTH - final_message.size) + suffix
+            commit = commit_class.new(commit_message, anything, anything)
+
+            linter = described_class.new(commit).lint_subject
+
+            expect(linter.problems).to be_empty
+          end
+        end
       end
     end
 
-    context 'when subject is too short and too long' do
+    context 'when subject does not have enough words and is too long' do
       let(:commit_message) { 'A ' + 'B' * described_class::MAX_LINE_LENGTH }
 
       it 'adds a problem' do

spec/lib/gitlab/danger/changelog_spec.rb

@@ -150,41 +150,80 @@ RSpec.describe Gitlab::Danger::Changelog do
   end
 
   describe '#modified_text' do
-    let(:sanitize_mr_title) { 'Fake Title' }
     let(:mr_json) { { "iid" => 1234, "title" => sanitize_mr_title } }
 
     subject { changelog.modified_text }
 
-    it do
+    context "when title is not changed from sanitization", :aggregate_failures do
+      let(:sanitize_mr_title) { 'Fake Title' }
+
+      specify do
         expect(subject).to include('CHANGELOG.md was edited')
         expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
         expect(subject).to include('bin/changelog --ee -m 1234 "Fake Title"')
       end
     end
 
+    context "when title needs sanitization", :aggregate_failures do
+      let(:sanitize_mr_title) { 'DRAFT: Fake Title' }
+
+      specify do
+        expect(subject).to include('CHANGELOG.md was edited')
+        expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
+        expect(subject).to include('bin/changelog --ee -m 1234 "Fake Title"')
+      end
+    end
+  end
+
   describe '#required_text' do
-    let(:sanitize_mr_title) { 'Fake Title' }
     let(:mr_json) { { "iid" => 1234, "title" => sanitize_mr_title } }
 
     subject { changelog.required_text }
 
-    it do
+    context "when title is not changed from sanitization", :aggregate_failures do
+      let(:sanitize_mr_title) { 'Fake Title' }
+
+      specify do
         expect(subject).to include('CHANGELOG missing')
         expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
         expect(subject).not_to include('--ee')
       end
     end
 
-  describe 'optional_text' do
-    let(:sanitize_mr_title) { 'Fake Title' }
+    context "when title needs sanitization", :aggregate_failures do
+      let(:sanitize_mr_title) { 'DRAFT: Fake Title' }
+
+      specify do
+        expect(subject).to include('CHANGELOG missing')
+        expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
+        expect(subject).not_to include('--ee')
+      end
+    end
+  end
+
+  describe '#optional_text' do
     let(:mr_json) { { "iid" => 1234, "title" => sanitize_mr_title } }
 
     subject { changelog.optional_text }
 
-    it do
+    context "when title is not changed from sanitization", :aggregate_failures do
+      let(:sanitize_mr_title) { 'Fake Title' }
+
+      specify do
         expect(subject).to include('CHANGELOG missing')
         expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
         expect(subject).to include('bin/changelog --ee -m 1234 "Fake Title"')
       end
     end
+
+    context "when title needs sanitization", :aggregate_failures do
+      let(:sanitize_mr_title) { 'DRAFT: Fake Title' }
+
+      specify do
+        expect(subject).to include('CHANGELOG missing')
+        expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
+        expect(subject).to include('bin/changelog --ee -m 1234 "Fake Title"')
+      end
+    end
+  end
 end

spec/lib/gitlab/danger/helper_spec.rb

@@ -402,24 +402,6 @@ RSpec.describe Gitlab::Danger::Helper do
     end
   end
 
-  describe '#sanitize_mr_title' do
-    where(:mr_title, :expected_mr_title) do
-      'My MR title'      | 'My MR title'
-      'WIP: My MR title' | 'My MR title'
-      'Draft: My MR title' | 'My MR title'
-      '(Draft) My MR title' | 'My MR title'
-      '[Draft] My MR title' | 'My MR title'
-      '[DRAFT] My MR title' | 'My MR title'
-      'DRAFT: My MR title' | 'My MR title'
-    end
-
-    with_them do
-      subject { helper.sanitize_mr_title(mr_title) }
-
-      it { is_expected.to eq(expected_mr_title) }
-    end
-  end
-
   describe '#security_mr?' do
     it 'returns false when `gitlab_helper` is unavailable' do
       expect(helper).to receive(:gitlab_helper).and_return(nil)

spec/lib/gitlab/danger/title_linting_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rspec-parameterized'
+
+require 'gitlab/danger/title_linting'
+
+RSpec.describe Gitlab::Danger::TitleLinting do
+  using RSpec::Parameterized::TableSyntax
+
+  describe '#sanitize_mr_title' do
+    where(:mr_title, :expected_mr_title) do
+      '`My MR title`' | "\\`My MR title\\`"
+      'WIP: My MR title' | 'My MR title'
+      'Draft: My MR title' | 'My MR title'
+      '(Draft) My MR title' | 'My MR title'
+      '[Draft] My MR title' | 'My MR title'
+      '[DRAFT] My MR title' | 'My MR title'
+      'DRAFT: My MR title' | 'My MR title'
+      'DRAFT: `My MR title`' | "\\`My MR title\\`"
+    end
+
+    with_them do
+      subject { described_class.sanitize_mr_title(mr_title) }
+
+      it { is_expected.to eq(expected_mr_title) }
+    end
+  end
+
+  describe '#remove_draft_flag' do
+    where(:mr_title, :expected_mr_title) do
+      'WIP: My MR title' | 'My MR title'
+      'Draft: My MR title' | 'My MR title'
+      '(Draft) My MR title' | 'My MR title'
+      '[Draft] My MR title' | 'My MR title'
+      '[DRAFT] My MR title' | 'My MR title'
+      'DRAFT: My MR title' | 'My MR title'
+    end
+
+    with_them do
+      subject { described_class.remove_draft_flag(mr_title) }
+
+      it { is_expected.to eq(expected_mr_title) }
+    end
+  end
+
+  describe '#has_draft_flag?' do
+    it 'returns true for a draft title' do
+      expect(described_class.has_draft_flag?('Draft: My MR title')).to be true
+    end
+
+    it 'returns false for non draft title' do
+      expect(described_class.has_draft_flag?('My MR title')).to be false
+    end
+  end
+end