From e59623e7388d67433ede87db1dad134f6f176f98 Mon Sep 17 00:00:00 2001 From: jurre Date: Thu, 15 Dec 2016 21:48:26 +0100 Subject: Mark MR as WIP when pushing WIP commits --- app/models/commit.rb | 6 ++ app/services/merge_requests/refresh_service.rb | 19 +++++++ app/services/system_note_service.rb | 6 ++ changelogs/unreleased/wip-mr-from-commits.yml | 4 ++ spec/models/commit_spec.rb | 28 ++++++++++ .../merge_requests/refresh_service_spec.rb | 64 ++++++++++++++++++++++ spec/services/system_note_service_spec.rb | 23 ++++++++ spec/support/test_env.rb | 3 +- 8 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/wip-mr-from-commits.yml diff --git a/app/models/commit.rb b/app/models/commit.rb index 3365f4ffdbf..5d942cb0422 100644 --- a/app/models/commit.rb +++ b/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) diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index 0a9563ed7e7..51d5d7563fc 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/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| diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 5ca2551ee61..a11bca00687 100644 --- a/app/services/system_note_service.rb +++ b/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" diff --git a/changelogs/unreleased/wip-mr-from-commits.yml b/changelogs/unreleased/wip-mr-from-commits.yml new file mode 100644 index 00000000000..0083798be08 --- /dev/null +++ b/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 diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 74b50d2908d..0d425ab7fd4 100644 --- a/spec/models/commit_spec.rb +++ b/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 diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 7e3705983fb..00d0e20f47c 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/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 diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 4042f2b0512..9f5a0ac4ec6 100644 --- a/spec/services/system_note_service_spec.rb +++ b/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 diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 4cf81be3adc..90f1a9c8798 100644 --- a/spec/support/test_env.rb +++ b/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 -- cgit v1.2.1 From ea7bf7eda44db68b6a8aadfff3afe8117a510989 Mon Sep 17 00:00:00 2001 From: jurre Date: Thu, 22 Dec 2016 19:45:36 +0100 Subject: Show a custom WIP help message for MR's with WIP commits --- .../projects/merge_requests/_new_submit.html.haml | 2 +- app/views/shared/issuable/_form.html.haml | 3 +- app/views/shared/issuable/form/_title.html.haml | 4 ++ spec/features/merge_requests/wip_message_spec.rb | 63 ++++++++++++++++++++++ 4 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 spec/features/merge_requests/wip_message_spec.rb diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index 34ead6427e0..36c6e7a8dad 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/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 diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index dcc1f3ba676..c0e8a498316 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/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 diff --git a/app/views/shared/issuable/form/_title.html.haml b/app/views/shared/issuable/form/_title.html.haml index 83efdc7c8f7..64826d41d60 100644 --- a/app/views/shared/issuable/form/_title.html.haml +++ b/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: diff --git a/spec/features/merge_requests/wip_message_spec.rb b/spec/features/merge_requests/wip_message_spec.rb new file mode 100644 index 00000000000..3311731b33b --- /dev/null +++ b/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 -- cgit v1.2.1