Improve the "easy WIP & un-WIP from link" feature

7 files changed, 53 insertions, 59 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()
-    @titleField.val @titleField.val().replace(@wipRegex, "")
+    if @workInProgress()
+      @removeWip()
+    else
+      @addWip()
 
     @renderWipExplanation()
 
-  addWip: (event) =>
-    event.preventDefault()
+  removeWip: ->
+    @titleField.val @titleField.val().replace(@wipRegex, "")
 
-    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 <code>WIP</code> prefix from the title
+          %a.js-toggle-wip{href: ""}
+            Remove the
+            %code WIP:
+            prefix from the title
           to allow this
-          <strong>Work In Progress</strong> 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 <code>[WIP]</code> or <code>WIP:</code>
+          %a.js-toggle-wip{href: ""}
+            Start the title with
+            %code WIP:
           to prevent a
-          <strong>Work In Progress</strong> 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