Merge branch '21225-wip-slash-command-for-mrs' into 'master'

Add a /wip slash command to toggle the 'WIP' prefix in the MR title

## Why was this MR needed?

As explained in #21225, it prevents you from having to click through the entire edit form just to mark an MR as WIP.

Closes #21225

See merge request !6259

CHANGELOG

@@ -6,6 +6,7 @@ v 8.13.0 (unreleased)
   - Fix centering of custom header logos (Ashley Dumaine)
   - AbstractReferenceFilter caches project_refs on RequestStore when active
   - Replaced the check sign to arrow in the show build view. !6501
+  - Add a /wip slash command to toggle the Work In Progress status of a merge request. !6259 (tbalthazar)
   - Speed-up group milestones show page
   - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller)
   - Add more tests for calendar contribution (ClemMakesApps)

app/controllers/projects/merge_requests_controller.rb

@@ -276,7 +276,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def remove_wip
-    MergeRequests::UpdateService.new(project, current_user, title: @merge_request.wipless_title).execute(@merge_request)
+    MergeRequests::UpdateService.new(project, current_user, wip_event: 'unwip').execute(@merge_request)
 
     redirect_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request),
       notice: "The merge request can now be merged."

app/models/merge_request.rb

@@ -155,6 +155,20 @@ class MergeRequest < ActiveRecord::Base
     where("merge_requests.id IN (#{union.to_sql})")
   end
 
+  WIP_REGEX = /\A\s*(\[WIP\]\s*|WIP:\s*|WIP\s+)+\s*/i.freeze
+
+  def self.work_in_progress?(title)
+    !!(title =~ WIP_REGEX)
+  end
+
+  def self.wipless_title(title)
+    title.sub(WIP_REGEX, "")
+  end
+
+  def self.wip_title(title)
+    work_in_progress?(title) ? title : "WIP: #{title}"
+  end
+
   def to_reference(from_project = nil)
     reference = "#{self.class.reference_prefix}#{iid}"
 
@@ -389,14 +403,16 @@ class MergeRequest < ActiveRecord::Base
     @closed_event ||= target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last
   end
 
-  WIP_REGEX = /\A\s*(\[WIP\]\s*|WIP:\s*|WIP\s+)+\s*/i.freeze
-
   def work_in_progress?
-    !!(title =~ WIP_REGEX)
+    self.class.work_in_progress?(title)
   end
 
   def wipless_title
-    self.title.sub(WIP_REGEX, "")
+    self.class.wipless_title(self.title)
+  end
+
+  def wip_title
+    self.class.wip_title(self.title)
   end
 
   def mergeable?(skip_ci_check: false)

app/services/merge_requests/base_service.rb

@@ -5,16 +5,17 @@ module MergeRequests
     end
 
     def create_title_change_note(issuable, old_title)
-      removed_wip = old_title =~ MergeRequest::WIP_REGEX && !issuable.work_in_progress?
-      added_wip = old_title !~ MergeRequest::WIP_REGEX && issuable.work_in_progress?
+      removed_wip = MergeRequest.work_in_progress?(old_title) && !issuable.work_in_progress?
+      added_wip = !MergeRequest.work_in_progress?(old_title) && issuable.work_in_progress?
+      changed_title = MergeRequest.wipless_title(old_title) != issuable.wipless_title
 
       if removed_wip
         SystemNoteService.remove_merge_request_wip(issuable, issuable.project, current_user)
       elsif added_wip
         SystemNoteService.add_merge_request_wip(issuable, issuable.project, current_user)
-      else
-        super
       end
+
+      super if changed_title
     end
 
     def hook_data(merge_request, action, oldrev = nil)

app/services/merge_requests/update_service.rb

@@ -16,7 +16,7 @@ module MergeRequests
       end
 
       merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
-
+      handle_wip_event(merge_request)
       update(merge_request)
     end
 
@@ -81,5 +81,18 @@ module MergeRequests
     def after_update(issuable)
       issuable.cache_merge_request_closes_issues!(current_user)
     end
+
+    private
+
+    def handle_wip_event(merge_request)
+      if wip_event = params.delete(:wip_event)
+        # We update the title that is provided in the params or we use the mr title
+        title = params[:title] || merge_request.title
+        params[:title] = case wip_event
+                         when 'wip' then MergeRequest.wip_title(title)
+                         when 'unwip' then MergeRequest.wipless_title(title)
+                         end
+      end
+    end
   end
 end

app/services/slash_commands/interpret_service.rb

@@ -214,6 +214,18 @@ module SlashCommands
       @updates[:due_date] = nil
     end
 
+    desc do
+      "Toggle the Work In Progress status"
+    end
+    condition do
+      issuable.persisted? &&
+        issuable.respond_to?(:work_in_progress?) &&
+        current_user.can?(:"update_#{issuable.to_ability_name}", issuable)
+    end
+    command :wip do
+      @updates[:wip_event] = issuable.work_in_progress? ? 'unwip' : 'wip'
+    end
+
     # This is a dummy command, so that it appears in the autocomplete commands
     desc 'CC'
     params '@user'

doc/user/project/slash_commands.md

@@ -27,4 +27,5 @@ do.
 | `/subscribe`               | Subscribe |
 | `/unsubscribe`             | Unsubscribe |
 | <code>/due &lt;in 2 days &#124; this Friday &#124; December 31st&gt;</code> | Set due date |
-| `/remove_due_date`          | Remove due date |
+| `/remove_due_date`         | Remove due date |
+| `/wip`                     | Toggle the Work In Progress status |

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -644,6 +644,20 @@ describe Projects::MergeRequestsController do
     end
   end
 
+  context 'POST remove_wip' do
+    it 'removes the wip status' do
+      merge_request.title = merge_request.wip_title
+      merge_request.save
+
+      post :remove_wip,
+           namespace_id: merge_request.project.namespace.to_param,
+           project_id: merge_request.project.to_param,
+           id: merge_request.iid
+
+      expect(merge_request.reload.title).to eq(merge_request.wipless_title)
+    end
+  end
+
   context 'POST resolve_conflicts' do
     let(:json_response) { JSON.parse(response.body) }
     let!(:original_head_sha) { merge_request_with_conflicts.diff_head_sha }

spec/features/issues/user_uses_slash_commands_spec.rb

@@ -99,5 +99,15 @@ feature 'Issues > User uses slash commands', feature: true, js: true do
         end
       end
     end
+
+    describe 'toggling the WIP prefix from the title from note' do
+      let(:issue) { create(:issue, project: project) }
+
+      it 'does not recognize the command nor create a note' do
+        write_note("/wip")
+
+        expect(page).not_to have_content '/wip'
+      end
+    end
   end
 end

spec/features/merge_requests/user_uses_slash_commands_spec.rb

@@ -14,21 +14,66 @@ feature 'Merge Requests > User uses slash commands', feature: true, js: true do
     let(:new_url_opts) { { merge_request: { source_branch: 'feature' } } }
   end
 
-  describe 'adding a due date from note' do
+  describe 'merge-request-only commands' do
     before do
       project.team << [user, :master]
       login_with(user)
       visit namespace_project_merge_request_path(project.namespace, project, merge_request)
     end
-
+    
     after do
       wait_for_ajax
     end
 
-    it 'does not recognize the command nor create a note' do
-      write_note("/due 2016-08-28")
+    describe 'toggling the WIP prefix in the title from note' do
+      context 'when the current user can toggle the WIP prefix' do
+        it 'adds the WIP: prefix to the title' do
+          write_note("/wip")
+
+          expect(page).not_to have_content '/wip'
+          expect(page).to have_content 'Your commands have been executed!'
+
+          expect(merge_request.reload.work_in_progress?).to eq true
+        end 
+
+        it 'removes the WIP: prefix from the title' do
+          merge_request.title = merge_request.wip_title
+          merge_request.save
+          write_note("/wip")
+
+          expect(page).not_to have_content '/wip'
+          expect(page).to have_content 'Your commands have been executed!'
+
+          expect(merge_request.reload.work_in_progress?).to eq false
+        end 
+      end
+
+      context 'when the current user cannot toggle the WIP prefix' do
+        let(:guest) { create(:user) }
+        before do
+          project.team << [guest, :guest]
+          logout
+          login_with(guest)
+          visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+        end
+
+        it 'does not change the WIP prefix' do
+          write_note("/wip")
+
+          expect(page).not_to have_content '/wip'
+          expect(page).not_to have_content 'Your commands have been executed!'
+
+          expect(merge_request.reload.work_in_progress?).to eq false
+        end
+      end
+    end
+
+    describe 'adding a due date from note' do
+      it 'does not recognize the command nor create a note' do
+        write_note('/due 2016-08-28')
 
-      expect(page).not_to have_content '/due 2016-08-28'
+        expect(page).not_to have_content '/due 2016-08-28'
+      end
     end
   end
 end

spec/models/merge_request_spec.rb

@@ -287,6 +287,46 @@ describe MergeRequest, models: true do
     end
   end
 
+  describe "#wipless_title" do
+    ['WIP ', 'WIP:', 'WIP: ', '[WIP]', '[WIP] ', ' [WIP] WIP [WIP] WIP: WIP '].each do |wip_prefix|
+      it "removes the '#{wip_prefix}' prefix" do
+        wipless_title = subject.title
+        subject.title = "#{wip_prefix}#{subject.title}"
+
+        expect(subject.wipless_title).to eq wipless_title
+      end
+
+      it "is satisfies the #work_in_progress? method" do
+        subject.title = "#{wip_prefix}#{subject.title}"
+        subject.title = subject.wipless_title
+
+        expect(subject.work_in_progress?).to eq false
+      end
+    end
+  end
+
+  describe "#wip_title" do
+    it "adds the WIP: prefix to the title" do
+      wip_title = "WIP: #{subject.title}"
+
+      expect(subject.wip_title).to eq wip_title
+    end 
+
+    it "does not add the WIP: prefix multiple times" do
+      wip_title = "WIP: #{subject.title}"
+      subject.title = subject.wip_title
+      subject.title = subject.wip_title
+
+      expect(subject.wip_title).to eq wip_title
+    end
+
+    it "is satisfies the #work_in_progress? method" do
+      subject.title = subject.wip_title
+
+      expect(subject.work_in_progress?).to eq true
+    end
+  end
+
   describe '#can_remove_source_branch?' do
     let(:user) { create(:user) }
     let(:user2) { create(:user) }

spec/services/slash_commands/interpret_service_spec.rb

@@ -165,6 +165,23 @@ describe SlashCommands::InterpretService, services: true do
       end
     end
 
+    shared_examples 'wip command' do
+      it 'returns wip_event: "wip" if content contains /wip' do
+        _, updates = service.execute(content, issuable)
+
+        expect(updates).to eq(wip_event: 'wip')
+      end
+    end
+
+    shared_examples 'unwip command' do
+      it 'returns wip_event: "unwip" if content contains /wip' do
+        issuable.update(title: issuable.wip_title)
+        _, updates = service.execute(content, issuable)
+
+        expect(updates).to eq(wip_event: 'unwip')
+      end
+    end
+
     shared_examples 'empty command' do
       it 'populates {} if content contains an unsupported command' do
         _, updates = service.execute(content, issuable)
@@ -376,6 +393,16 @@ describe SlashCommands::InterpretService, services: true do
       let(:issuable) { issue }
     end
 
+    it_behaves_like 'wip command' do
+      let(:content) { '/wip' }
+      let(:issuable) { merge_request }
+    end
+
+    it_behaves_like 'unwip command' do
+      let(:content) { '/wip' }
+      let(:issuable) { merge_request }
+    end
+
     it_behaves_like 'empty command' do
       let(:content) { '/remove_due_date' }
       let(:issuable) { merge_request }