From 0b942541da1dc616cea266dc1f4d517fe81f6e5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 18 Mar 2016 23:27:35 +0100 Subject: [PATCH] Improve the "easy WIP & un-WIP from link" feature --- .../javascripts/issuable_form.js.coffee | 42 +++++++++---------- app/models/merge_request.rb | 2 +- app/services/merge_requests/base_service.rb | 7 +--- app/services/system_note_service.rb | 4 +- .../merge_requests/widget/open/_wip.html.haml | 16 +++---- app/views/shared/issuable/_form.html.haml | 17 +++++--- spec/models/merge_request_spec.rb | 28 +++++-------- 7 files changed, 55 insertions(+), 61 deletions(-) diff --git a/app/assets/javascripts/issuable_form.js.coffee b/app/assets/javascripts/issuable_form.js.coffee index db494347a28..6c1699c178c 100644 --- a/app/assets/javascripts/issuable_form.js.coffee +++ b/app/assets/javascripts/issuable_form.js.coffee @@ -1,5 +1,5 @@ class @IssuableForm - wipRegex: /^\[?WIP(\]|:| )\s*/i + wipRegex: /^\s*(\[WIP\]\s*|WIP:\s*|WIP\s+)+\s*/i constructor: (@form) -> GitLab.GfmAutoComplete.setup() new UsersSelect() @@ -35,39 +35,39 @@ class @IssuableForm @descriptionField.data("autosave").reset() initWip: -> - return unless @form.find(".js-wip-explanation").length - - @form.on "click", ".js-remove-wip", @removeWip + @$wipExplanation = @form.find(".js-wip-explanation") + @$noWipExplanation = @form.find(".js-no-wip-explanation") + return unless @$wipExplanation.length and @$noWipExplanation.length - @form.on "click", ".js-add-wip", @addWip + @form.on "click", ".js-toggle-wip", @toggleWip - @titleField.on "change", @renderWipExplanation + @titleField.on "keyup blur", @renderWipExplanation @renderWipExplanation() workInProgress: -> - @titleField.val().match(@wipRegex) + @wipRegex.test @titleField.val() renderWipExplanation: => if @workInProgress() - @form.find(".js-wip-explanation").show() - @form.find(".js-no-wip-explanation").hide() + @$wipExplanation.show() + @$noWipExplanation.hide() else - @form.find(".js-wip-explanation").hide() - @form.find(".js-no-wip-explanation").show() + @$wipExplanation.hide() + @$noWipExplanation.show() - removeWip: (event) => + toggleWip: (event) => event.preventDefault() - return unless @workInProgress() + if @workInProgress() + @removeWip() + else + @addWip() + + @renderWipExplanation() + + removeWip: -> @titleField.val @titleField.val().replace(@wipRegex, "") - @renderWipExplanation() - - addWip: (event) => - event.preventDefault() - - return if @workInProgress() + addWip: -> @titleField.val "WIP: #{@titleField.val()}" - - @renderWipExplanation() diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 04c378691f3..4739d700b85 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -259,7 +259,7 @@ class MergeRequest < ActiveRecord::Base self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last end - WIP_REGEX = /\A\[?WIP(\]|:| )\s*/i.freeze + WIP_REGEX = /\A\s*(\[WIP\]\s*|WIP:\s*|WIP\s+)+\s*/i.freeze def work_in_progress? title =~ WIP_REGEX diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 9370b4c01a6..ac5b58db862 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -6,11 +6,8 @@ module MergeRequests end def create_title_change_note(issuable, old_title) - wipless_old_title = old_title.sub(MergeRequest::WIP_REGEX, "") - wipless_new_title = issuable.title.sub(MergeRequest::WIP_REGEX, "") - - removed_wip = wipless_old_title == issuable.title - added_wip = wipless_new_title == old_title + removed_wip = old_title =~ MergeRequest::WIP_REGEX && !issuable.work_in_progress? + added_wip = old_title !~ MergeRequest::WIP_REGEX && issuable.work_in_progress? if removed_wip SystemNoteService.remove_merge_request_wip(issuable, issuable.project, current_user) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index e579ca76565..2404ac2ff4c 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -145,13 +145,13 @@ class SystemNoteService end def self.remove_merge_request_wip(noteable, project, author) - body = 'Unmarked this merge request as Work In Progress' + body = 'Unmarked this merge request as a Work In Progress' create_note(noteable: noteable, project: project, author: author, note: body) end def self.add_merge_request_wip(noteable, project, author) - body = 'Marked this merge request as **Work In Progress**' + body = 'Marked this merge request as a **Work In Progress**' create_note(noteable: noteable, project: project, author: author, note: body) end diff --git a/app/views/projects/merge_requests/widget/open/_wip.html.haml b/app/views/projects/merge_requests/widget/open/_wip.html.haml index 3f62818280f..c296422a9cf 100644 --- a/app/views/projects/merge_requests/widget/open/_wip.html.haml +++ b/app/views/projects/merge_requests/widget/open/_wip.html.haml @@ -1,11 +1,11 @@ %h4 This merge request is currently a Work In Progress -%p - When this merge request is ready, - - text = 'remove the "WIP" prefix from the title' - - if can?(current_user, :update_merge_request, @merge_request) - = link_to text, remove_wip_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), method: :post - - else - = text - to allow it to be merged. +- if can?(current_user, :update_merge_request, @merge_request) + %p + When this merge request is ready, + = link_to remove_wip_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), method: :post do + remove the + %code WIP: + prefix from the title + to allow it to be merged. diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index d93395a3e85..bf140210115 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -14,15 +14,20 @@ - if issuable.is_a?(MergeRequest) %p.help-block .js-wip-explanation - %a{href: "#", class: "js-remove-wip", data: { }} - Remove the WIP prefix from the title + %a.js-toggle-wip{href: ""} + Remove the + %code WIP: + prefix from the title to allow this - Work In Progress merge request to be merged when it's ready. + %strong Work In Progress + merge request to be merged when it's ready. .js-no-wip-explanation - %a{href: "#", class: "js-add-wip"} - Start the title with [WIP] or WIP: + %a.js-toggle-wip{href: ""} + Start the title with + %code WIP: to prevent a - Work In Progress merge request from being merged before it's ready. + %strong Work In Progress + merge request from being merged before it's ready. .form-group.detail-page-description = f.label :description, 'Description', class: 'control-label' .col-sm-10 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c51f34034d7..c33dda01c4f 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -174,24 +174,11 @@ describe MergeRequest, models: true do end describe "#work_in_progress?" do - it "detects the 'WIP ' prefix" do - subject.title = "WIP #{subject.title}" - expect(subject).to be_work_in_progress - end - - it "detects the 'WIP: ' prefix" do - subject.title = "WIP: #{subject.title}" - expect(subject).to be_work_in_progress - end - - it "detects the '[WIP] ' prefix" do - subject.title = "[WIP] #{subject.title}" - expect(subject).to be_work_in_progress - end - - it "detects the '[WIP]' prefix" do - subject.title = "[WIP]#{subject.title}" - expect(subject).to be_work_in_progress + ['WIP ', 'WIP:', 'WIP: ', '[WIP]', '[WIP] ', ' [WIP] WIP [WIP] WIP: WIP '].each do |wip_prefix| + it "detects the '#{wip_prefix}' prefix" do + subject.title = "#{wip_prefix}#{subject.title}" + expect(subject).to be_work_in_progress + end end it "doesn't detect WIP for words starting with WIP" do @@ -199,6 +186,11 @@ describe MergeRequest, models: true do expect(subject).not_to be_work_in_progress end + it "doesn't detect WIP for words containing with WIP" do + subject.title = "WupWipwap #{subject.title}" + expect(subject).not_to be_work_in_progress + end + it "doesn't detect WIP by default" do expect(subject).not_to be_work_in_progress end