Merge branch 'wip-mr-from-commits' into 'master'

Mark MR as WIP when pushing WIP commits

Closes #25036

See merge request !8124

app/models/commit.rb

@@ -326,6 +326,12 @@ class Commit
     # no-op but needs to be defined since #persisted? is defined
   end
 
+  WIP_REGEX = /\A\s*(((?i)(\[WIP\]|WIP:|WIP)\s|WIP$))|(fixup!|squash!)\s/.freeze
+
+  def work_in_progress?
+    !!(title =~ WIP_REGEX)
+  end
+
   private
 
   def commit_reference(from_project, referable_commit_id, full: false)

app/services/merge_requests/refresh_service.rb

@@ -21,6 +21,7 @@ module MergeRequests
       end
 
       comment_mr_with_commits
+      mark_mr_as_wip_from_commits
       execute_mr_web_hooks
 
       true
@@ -136,6 +137,24 @@ module MergeRequests
       end
     end
 
+    def mark_mr_as_wip_from_commits
+      return unless @commits.present?
+
+      merge_requests_for_source_branch.each do |merge_request|
+        wip_commit = @commits.detect(&:work_in_progress?)
+
+        if wip_commit && !merge_request.work_in_progress?
+          merge_request.update(title: merge_request.wip_title)
+          SystemNoteService.add_merge_request_wip_from_commit(
+            merge_request,
+            merge_request.project,
+            @current_user,
+            wip_commit
+          )
+        end
+      end
+    end
+
     # Call merge request webhook with update branches
     def execute_mr_web_hooks
       merge_requests_for_source_branch.each do |merge_request|

app/services/system_note_service.rb

@@ -208,6 +208,12 @@ module SystemNoteService
     create_note(noteable: noteable, project: project, author: author, note: body)
   end
 
+  def add_merge_request_wip_from_commit(noteable, project, author, commit)
+    body = "marked as a **Work In Progress** from #{commit.to_reference(project)}"
+
+    create_note(noteable: noteable, project: project, author: author, note: body)
+  end
+
   def self.resolve_all_discussions(merge_request, project, author)
     body = "resolved all discussions"
 

app/views/projects/merge_requests/_new_submit.html.haml

@@ -11,7 +11,7 @@
     = link_to 'Change branches', mr_change_branches_path(@merge_request)
 %hr
 = form_for [@project.namespace.becomes(Namespace), @project, @merge_request], html: { class: 'merge-request-form form-horizontal common-note-form js-requires-input js-quick-submit' } do |f|
-  = render 'shared/issuable/form', f: f, issuable: @merge_request
+  = render 'shared/issuable/form', f: f, issuable: @merge_request, commits: @commits
   = f.hidden_field :source_project_id
   = f.hidden_field :source_branch
   = f.hidden_field :target_project_id

app/views/shared/issuable/_form.html.haml

@@ -1,4 +1,5 @@
 - form = local_assigns.fetch(:f)
+- commits = local_assigns[:commits]
 - project = @target_project || @project
 
 = form_errors(issuable)
@@ -14,7 +15,7 @@
   = form.label :title, class: 'control-label'
 
   = render 'shared/issuable/form/template_selector', issuable: issuable
-  = render 'shared/issuable/form/title', issuable: issuable, form: form
+  = render 'shared/issuable/form/title', issuable: issuable, form: form, has_wip_commits: commits && commits.detect(&:work_in_progress?)
 
 = render 'shared/issuable/form/description', issuable: issuable, form: form
 

app/views/shared/issuable/form/_title.html.haml

@@ -1,4 +1,5 @@
 - issuable = local_assigns.fetch(:issuable)
+- has_wip_commits = local_assigns.fetch(:has_wip_commits)
 - form = local_assigns.fetch(:form)
 - no_issuable_templates = issuable_templates(issuable).empty?
 - div_class = no_issuable_templates ? 'col-sm-10' : 'col-sm-7 col-lg-8'
@@ -18,6 +19,9 @@
         %strong Work In Progress
         merge request to be merged when it's ready.
       .js-no-wip-explanation
+        - if has_wip_commits
+          It looks like you have some WIP commits in this branch.
+          %br
         %a.js-toggle-wip{ href: '', tabindex: -1 }
           Start the title with
           %code WIP:

changelogs/unreleased/wip-mr-from-commits.yml

@@ -0,0 +1,4 @@
+---
+title: Mark MR as WIP when pushing WIP commits
+merge_request: 8124
+author: Jurre Stender @jurre

spec/features/merge_requests/wip_message_spec.rb

@@ -0,0 +1,63 @@
+require 'spec_helper'
+
+feature 'Work In Progress help message', feature: true do
+  let!(:project) { create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
+  let!(:user)    { create(:user) }
+
+  before do
+    project.team << [user, :master]
+    login_as(user)
+  end
+
+  context 'with WIP commits' do
+    it 'shows a specific WIP hint' do
+      visit new_namespace_project_merge_request_path(
+        project.namespace,
+        project,
+        merge_request: {
+          source_project_id: project.id,
+          target_project_id: project.id,
+          source_branch: 'wip',
+          target_branch: 'master'
+        }
+      )
+
+      within_wip_explanation do
+        expect(page).to have_text(
+          'It looks like you have some WIP commits in this branch'
+        )
+      end
+    end
+  end
+
+  context 'without WIP commits' do
+    it 'shows the regular WIP message' do
+      visit new_namespace_project_merge_request_path(
+        project.namespace,
+        project,
+        merge_request: {
+          source_project_id: project.id,
+          target_project_id: project.id,
+          source_branch: 'fix',
+          target_branch: 'master'
+        }
+      )
+
+      within_wip_explanation do
+        expect(page).not_to have_text(
+          'It looks like you have some WIP commits in this branch'
+        )
+        expect(page).to have_text(
+          "Start the title with WIP: to prevent a Work In Progress merge \
+request from being merged before it's ready"
+        )
+      end
+    end
+  end
+
+  def within_wip_explanation(&block)
+    page.within '.js-no-wip-explanation' do
+      yield
+    end
+  end
+end

spec/models/commit_spec.rb

@@ -323,4 +323,32 @@ eos
       expect(new_commit.message).to eq(commit.message)
     end
   end
+
+  describe '#work_in_progress?' do
+    ['squash! ', 'fixup! ', 'wip: ', 'WIP: ', '[WIP] '].each do |wip_prefix|
+      it "detects the '#{wip_prefix}' prefix" do
+        commit.message = "#{wip_prefix}#{commit.message}"
+
+        expect(commit).to be_work_in_progress
+      end
+    end
+
+    it "detects WIP for a commit just saying 'wip'" do
+      commit.message = "wip"
+
+      expect(commit).to be_work_in_progress
+    end
+
+    it "doesn't detect WIP for a commit that begins with 'FIXUP! '" do
+      commit.message = "FIXUP! #{commit.message}"
+
+      expect(commit).not_to be_work_in_progress
+    end
+
+    it "doesn't detect WIP for words starting with WIP" do
+      commit.message = "Wipout #{commit.message}"
+
+      expect(commit).not_to be_work_in_progress
+    end
+  end
 end

spec/services/merge_requests/refresh_service_spec.rb

@@ -237,6 +237,70 @@ describe MergeRequests::RefreshService, services: true do
       end
     end
 
+    context 'marking the merge request as work in progress' do
+      let(:refresh_service) { service.new(@project, @user) }
+      before do
+        allow(refresh_service).to receive(:execute_hooks)
+      end
+
+      it 'marks the merge request as work in progress from fixup commits' do
+        fixup_merge_request = create(:merge_request,
+                                     source_project: @project,
+                                     source_branch: 'wip',
+                                     target_branch: 'master',
+                                     target_project: @project)
+        commits = fixup_merge_request.commits
+        oldrev = commits.last.id
+        newrev = commits.first.id
+
+        refresh_service.execute(oldrev, newrev, 'refs/heads/wip')
+        fixup_merge_request.reload
+
+        expect(fixup_merge_request.work_in_progress?).to eq(true)
+        expect(fixup_merge_request.notes.last.note).to match(
+          /marked as a \*\*Work In Progress\*\* from #{Commit.reference_pattern}/
+        )
+      end
+
+      it 'references the commit that caused the Work in Progress status' do
+        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+
+        allow(refresh_service).to receive(:find_new_commits)
+        refresh_service.instance_variable_set("@commits", [
+          instance_double(
+            Commit,
+            id: 'aaaaaaa',
+            short_id: 'aaaaaaa',
+            title: 'Fix issue',
+            work_in_progress?: false
+          ),
+          instance_double(
+            Commit,
+            id: 'bbbbbbb',
+            short_id: 'bbbbbbb',
+            title: 'fixup! Fix issue',
+            work_in_progress?: true,
+            to_reference: 'bbbbbbb'
+          ),
+          instance_double(
+            Commit,
+            id: 'ccccccc',
+            short_id: 'ccccccc',
+            title: 'fixup! Fix issue',
+            work_in_progress?: true,
+            to_reference: 'ccccccc'
+          ),
+        ])
+
+        refresh_service.execute(@oldrev, @newrev, 'refs/heads/wip')
+        reload_mrs
+
+        expect(@merge_request.notes.last.note).to eq(
+          "marked as a **Work In Progress** from bbbbbbb"
+        )
+      end
+    end
+
     def reload_mrs
       @merge_request.reload
       @fork_merge_request.reload

spec/services/system_note_service_spec.rb

@@ -805,4 +805,27 @@ describe SystemNoteService, services: true do
       noteable.save!
     end
   end
+
+  describe '.add_merge_request_wip_from_commit' do
+    let(:noteable) do
+      create(:merge_request, source_project: project, target_project: project)
+    end
+
+    subject do
+      described_class.add_merge_request_wip_from_commit(
+        noteable,
+        project,
+        author,
+        noteable.diff_head_commit
+      )
+    end
+
+    it_behaves_like 'a system note'
+
+    it "posts the 'marked as a Work In Progress from commit' system note" do
+      expect(subject.note).to match(
+        /marked as a \*\*Work In Progress\*\* from #{Commit.reference_pattern}/
+      )
+    end
+  end
 end

spec/support/test_env.rb

@@ -35,7 +35,8 @@ module TestEnv
     'conflict-missing-side'              => 'eb227b3',
     'conflict-non-utf8'                  => 'd0a293c',
     'conflict-too-large'                 => '39fa04f',
-    'deleted-image-test'                 => '6c17798'
+    'deleted-image-test'                 => '6c17798',
+    'wip'                                => 'b9238ee'
   }
 
   # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily