Merge branch 'ph/227421/changeMROptionsDropdownToIncludeDraft' into 'master'

Allow users to toggle draft status on merge request

Closes #227421

See merge request gitlab-org/gitlab!43098

app/assets/javascripts/merge_request.js

@@ -3,11 +3,12 @@
 import $ from 'jquery';
 import axios from './lib/utils/axios_utils';
 import { __ } from '~/locale';
+import eventHub from '~/vue_merge_request_widget/event_hub';
 import { deprecatedCreateFlash as createFlash } from '~/flash';
 import TaskList from './task_list';
 import MergeRequestTabs from './merge_request_tabs';
-import IssuablesHelper from './helpers/issuables_helper';
 import { addDelimiter } from './lib/utils/text_utility';
+import { getParameterValues, setUrlParams } from './lib/utils/url_utility';
 
 function MergeRequest(opts) {
   // Initialize MergeRequest behavior
@@ -23,7 +24,6 @@ function MergeRequest(opts) {
   this.initTabs();
   this.initMRBtnListeners();
   this.initCommitMessageListeners();
-  this.closeReopenReportToggle = IssuablesHelper.initCloseReopenReport();
 
   if ($('.description.js-task-list-container').length) {
     this.taskList = new TaskList({
@@ -66,13 +66,38 @@ MergeRequest.prototype.showAllCommits = function() {
 
 MergeRequest.prototype.initMRBtnListeners = function() {
   const _this = this;
+  const draftToggles = document.querySelectorAll('.js-draft-toggle-button');
 
-  $('.report-abuse-link').on('click', e => {
-    // this is needed because of the implementation of
-    // the dropdown toggle and Report Abuse needing to be
-    // linked to another page.
-    e.stopPropagation();
-  });
+  if (draftToggles.length) {
+    draftToggles.forEach(draftToggle => {
+      draftToggle.addEventListener('click', e => {
+        e.preventDefault();
+        e.stopImmediatePropagation();
+
+        const url = draftToggle.href;
+        const wipEvent = getParameterValues('merge_request[wip_event]', url)[0];
+        const mobileDropdown = draftToggle.closest('.dropdown.show');
+
+        if (mobileDropdown) {
+          $(mobileDropdown.firstElementChild).dropdown('toggle');
+        }
+
+        draftToggle.setAttribute('disabled', 'disabled');
+
+        axios
+          .put(draftToggle.href, null, { params: { format: 'json' } })
+          .then(({ data }) => {
+            draftToggle.removeAttribute('disabled');
+            eventHub.$emit('MRWidgetUpdateRequested');
+            MergeRequest.toggleDraftStatus(data.title, wipEvent === 'unwip');
+          })
+          .catch(() => {
+            draftToggle.removeAttribute('disabled');
+            createFlash(__('Something went wrong. Please try again.'));
+          });
+      });
+    });
+  }
 
   return $('.btn-close, .btn-reopen').on('click', function(e) {
     const $this = $(this);
@@ -89,8 +114,6 @@ MergeRequest.prototype.initMRBtnListeners = function() {
       return;
     }
 
-    if (this.closeReopenReportToggle) this.closeReopenReportToggle.setDisable();
-
     if (shouldSubmit) {
       if ($this.hasClass('btn-comment-and-close') || $this.hasClass('btn-comment-and-reopen')) {
         e.preventDefault();
@@ -151,14 +174,35 @@ MergeRequest.hideCloseButton = function() {
   const closeDropdownItem = el.querySelector('li.close-item');
   if (closeDropdownItem) {
     closeDropdownItem.classList.add('hidden');
-    // Selects the next dropdown item
-    el.querySelector('li.report-item').click();
-  } else {
-    // No dropdown just hide the Close button
-    el.querySelector('.btn-close').classList.add('hidden');
   }
   // Dropdown for mobile screen
   el.querySelector('li.js-close-item').classList.add('hidden');
 };
 
+MergeRequest.toggleDraftStatus = function(title, isReady) {
+  if (isReady) {
+    createFlash(__('The merge request can now be merged.'), 'notice');
+  }
+  const titleEl = document.querySelector('.merge-request .detail-page-description .title');
+
+  if (titleEl) {
+    titleEl.textContent = title;
+  }
+
+  const draftToggles = document.querySelectorAll('.js-draft-toggle-button');
+
+  if (draftToggles.length) {
+    draftToggles.forEach(el => {
+      const draftToggle = el;
+      const url = setUrlParams(
+        { 'merge_request[wip_event]': isReady ? 'wip' : 'unwip' },
+        draftToggle.href,
+      );
+
+      draftToggle.setAttribute('href', url);
+      draftToggle.textContent = isReady ? __('Mark as draft') : __('Mark as ready');
+    });
+  }
+};
+
 export default MergeRequest;

app/assets/javascripts/vue_merge_request_widget/components/states/work_in_progress.vue

@@ -3,6 +3,7 @@ import $ from 'jquery';
 import { GlButton } from '@gitlab/ui';
 import { __ } from '~/locale';
 import { deprecatedCreateFlash as createFlash } from '~/flash';
+import MergeRequest from '~/merge_request';
 import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import mergeRequestQueryVariablesMixin from '../../mixins/merge_request_query_variables';
 import getStateQuery from '../../queries/get_state.query.graphql';
@@ -128,8 +129,7 @@ export default {
           .then(res => res.data)
           .then(data => {
             eventHub.$emit('UpdateWidgetData', data);
-            createFlash(__('The merge request can now be merged.'), 'notice');
-            $('.merge-request .detail-page-description .title').text(this.mr.title);
+            MergeRequest.toggleDraftStatus(this.mr.title, true);
           })
           .catch(() => {
             this.isMakingRequest = false;

app/controllers/projects/merge_requests/application_controller.rb

@@ -35,6 +35,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
       :source_branch,
       :source_project_id,
       :state_event,
+      :wip_event,
       :squash,
       :target_branch,
       :target_project_id,

app/helpers/issuables_helper.rb

@@ -342,6 +342,12 @@ module IssuablesHelper
     issuable.closed? ^ should_inverse ? reopen_issuable_path(issuable) : close_issuable_path(issuable)
   end
 
+  def toggle_draft_issuable_path(issuable)
+    wip_event = issuable.work_in_progress? ? 'unwip' : 'wip'
+
+    issuable_path(issuable, { merge_request: { wip_event: wip_event } })
+  end
+
   def issuable_path(issuable, *options)
     polymorphic_path(issuable, *options)
   end

app/presenters/merge_request_presenter.rb

@@ -37,7 +37,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def remove_wip_path
-    if work_in_progress? && can?(current_user, :update_merge_request, merge_request.project)
+    if can?(current_user, :update_merge_request, merge_request.project)
       remove_wip_project_merge_request_path(project, merge_request)
     end
   end

app/serializers/merge_request_basic_entity.rb

 # frozen_string_literal: true
 
 class MergeRequestBasicEntity < Grape::Entity
+  expose :title
   expose :public_merge_status, as: :merge_status
   expose :merge_error
   expose :state

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

@@ -32,16 +32,19 @@
         %ul
           - if can_update_merge_request
             %li= link_to 'Edit', edit_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)
-          - unless current_user == @merge_request.author
-            %li= link_to 'Report abuse', new_abuse_report_path(user_id: @merge_request.author.id, ref_url: merge_request_url(@merge_request))
           - if can_update_merge_request
+            - unless @merge_request.closed?
+              %li
+                = link_to @merge_request.work_in_progress? ? _('Mark as ready') : _('Mark as draft'), toggle_draft_issuable_path(@merge_request), method: :put, class: "js-draft-toggle-button"
             %li{ class: [merge_request_button_visibility(@merge_request, true), 'js-close-item'] }
               = link_to 'Close', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, title: 'Close merge request'
           - if can_reopen_merge_request
             %li{ class: merge_request_button_visibility(@merge_request, false) }
               = link_to 'Reopen', merge_request_path(@merge_request, merge_request: { state_event: :reopen }), method: :put, class: 'reopen-mr-link', title: 'Reopen merge request'
+          - unless current_user == @merge_request.author
+            %li= link_to 'Report abuse', new_abuse_report_path(user_id: @merge_request.author.id, ref_url: merge_request_url(@merge_request))
 
       - if can_update_merge_request
-        = link_to 'Edit', edit_project_merge_request_path(@project, @merge_request), class: "d-none d-sm-none d-md-block btn btn-grouped js-issuable-edit qa-edit-button"
+        = link_to 'Edit', edit_project_merge_request_path(@project, @merge_request), class: "d-none d-sm-none d-md-block btn gl-button btn-grouped js-issuable-edit qa-edit-button"
 
       = render 'shared/issuable/close_reopen_button', issuable: @merge_request, can_update: can_update_merge_request, can_reopen: can_reopen_merge_request

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

@@ -5,7 +5,7 @@
 - if defined? warn_before_close
   - add_blocked_class = warn_before_close
 
-- if is_current_user
+- if is_current_user && !issuable.is_a?(MergeRequest)
   - if can_update
     %button{ class: "d-none d-sm-none d-md-block btn btn-grouped btn-close js-btn-issue-action #{issuable_button_visibility(issuable, true)} #{(add_blocked_class ? 'btn-issue-blocked' : '')}",
              data: { remote: 'true', endpoint: close_issuable_path(issuable), qa_selector: 'close_issue_button' } }
@@ -16,7 +16,10 @@
       = _("Reopen %{display_issuable_type}") % { display_issuable_type: display_issuable_type }
 - else
   - if can_update && !are_close_and_open_buttons_hidden
-    = render 'shared/issuable/close_reopen_report_toggle', issuable: issuable, warn_before_close: add_blocked_class
+    - if issuable.is_a?(MergeRequest)
+      = render 'shared/issuable/close_reopen_draft_report_toggle', issuable: issuable
+    - else
+      = render 'shared/issuable/close_reopen_report_toggle', issuable: issuable, warn_before_close: add_blocked_class
   - else
     = link_to _('Report abuse'), new_abuse_report_path(user_id: issuable.author.id, ref_url: issuable_url(issuable)),
               class: 'd-none d-sm-none d-md-block btn btn-grouped btn-close-color', title: _('Report abuse')

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

+- display_issuable_type = issuable_display_type(issuable)
+- button_action_class = issuable.closed? ? 'btn-default' : 'btn-warning btn-warning-secondary'
+- button_class = "btn gl-button #{!issuable.closed? && 'js-draft-toggle-button'}"
+- toggle_class = "btn gl-button dropdown-toggle"
+
+.float-left.btn-group.gl-ml-3.issuable-close-dropdown.d-none.d-md-inline-flex.js-issuable-close-dropdown
+  = link_to issuable.closed? ? reopen_issuable_path(issuable) : toggle_draft_issuable_path(issuable), method: :put, class: "#{button_class} #{button_action_class}" do
+    - if issuable.closed?
+      = _('Reopen')
+      = display_issuable_type
+    - else
+      = issuable.work_in_progress? ? _('Mark as ready') : _('Mark as draft')
+
+  - if !issuable.closed? || !issuable_author_is_current_user(issuable)
+    = button_tag type: 'button', class: "#{toggle_class} #{button_action_class}", data: { 'toggle' => 'dropdown' } do
+      %span.sr-only= _('Toggle dropdown')
+      = sprite_icon "angle-down", size: 12
+
+    %ul.js-issuable-close-menu.dropdown-menu.dropdown-menu-right
+      - if issuable.open?
+        %li
+          = link_to close_issuable_path(issuable), method: :put do
+            .description
+              %strong.title
+                = _('Close')
+                = display_issuable_type
+
+      - unless issuable_author_is_current_user(issuable)
+        - unless issuable.closed?
+          %li.divider.droplab-item-ignore
+
+        %li.report-item
+          %a.report-abuse-link{ href: new_abuse_report_path(user_id: issuable.author.id, ref_url: issuable_url(issuable)) }
+            .description
+              %strong.title= _('Report abuse')
+              %p.text
+                = _('Report %{display_issuable_type} that are abusive, inappropriate or spam.') % { display_issuable_type: display_issuable_type.pluralize }

changelogs/unreleased/ph-227421-changeMROptionsDropdownToIncludeDraft.yml

+---
+title: Adds button to update merge request draft status on merge request show page
+merge_request: 43098
+author:
+type: added

locale/gitlab.pot

@@ -15450,6 +15450,12 @@ msgstr ""
 msgid "Mark as done"
 msgstr ""
 
+msgid "Mark as draft"
+msgstr ""
+
+msgid "Mark as ready"
+msgstr ""
+
 msgid "Mark as resolved"
 msgstr ""
 

spec/features/issuables/close_reopen_report_toggle_spec.rb

@@ -23,7 +23,15 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
       expect(container).to have_content("Close #{human_model_name}")
       expect(container).to have_content('Report abuse')
       expect(container).to have_content("Report #{human_model_name.pluralize} that are abusive, inappropriate or spam.")
-      expect(container).to have_selector('.close-item.droplab-item-selected')
+
+      if issuable.is_a?(MergeRequest)
+        page.within('.js-issuable-close-dropdown') do
+          expect(page).to have_link('Close merge request')
+        end
+      else
+        expect(container).to have_selector('.close-item.droplab-item-selected')
+      end
+
       expect(container).to have_selector('.report-item')
       expect(container).not_to have_selector('.report-item.droplab-item-selected')
       expect(container).not_to have_selector('.reopen-item')
@@ -123,7 +131,7 @@ RSpec.describe 'Issuables Close/Reopen/Report toggle' do
 
           it 'shows only the `Edit` button' do
             expect(page).to have_link('Edit')
-            expect(page).not_to have_link('Report abuse')
+            expect(page).to have_link('Report abuse')
             expect(page).not_to have_button('Close merge request')
             expect(page).not_to have_button('Reopen merge request')
           end

spec/features/merge_request/user_marks_merge_request_as_draft_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Merge request > User marks merge request as draft', :js do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public, :repository) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+
+  before do
+    project.add_maintainer(user)
+
+    sign_in(user)
+
+    visit project_merge_request_path(project, merge_request)
+  end
+
+  it 'toggles draft status' do
+    click_link 'Mark as draft'
+
+    expect(page).to have_content("Draft: #{merge_request.title}")
+
+    page.within('.detail-page-header-actions') do
+      click_link 'Mark as ready'
+    end
+
+    expect(page).to have_content(merge_request.title)
+  end
+end

spec/features/merge_request/user_reopens_merge_request_spec.rb

@@ -15,7 +15,11 @@ RSpec.describe 'User reopens a merge requests', :js do
   end
 
   it 'reopens a merge request' do
-    click_button('Reopen merge request', match: :first)
+    find('.js-issuable-close-dropdown .dropdown-toggle').click
+
+    click_link('Reopen merge request', match: :first)
+
+    wait_for_requests
 
     page.within('.status-box') do
       expect(page).to have_content('Open')

spec/features/merge_request/user_resolves_wip_mr_spec.rb

@@ -35,7 +35,9 @@ RSpec.describe 'Merge request > User resolves Work in Progress', :js do
       expect(page.find('.ci-widget-content')).to have_content("Pipeline ##{pipeline.id}")
       expect(page).to have_content "This merge request is still a work in progress."
 
-      click_button('Mark as ready')
+      page.within('.mr-state-widget') do
+        click_button('Mark as ready')
+      end
 
       wait_for_requests
 

spec/fixtures/api/schemas/entities/merge_request_basic.json

 {
   "type": "object",
   "properties" : {
+    "title": { "type": "string" },
     "state": { "type": "string" },
     "merge_status": { "type": "string" },
     "source_branch_exists": { "type": "boolean" },

spec/frontend/merge_request_spec.js

@@ -3,8 +3,6 @@ import MockAdapter from 'axios-mock-adapter';
 import { TEST_HOST } from 'spec/test_constants';
 import axios from '~/lib/utils/axios_utils';
 import MergeRequest from '~/merge_request';
-import CloseReopenReportToggle from '~/close_reopen_report_toggle';
-import IssuablesHelper from '~/helpers/issuables_helper';
 
 describe('MergeRequest', () => {
   const test = {};
@@ -112,66 +110,7 @@ describe('MergeRequest', () => {
     });
   });
 
-  describe('class constructor', () => {
-    beforeEach(() => {
-      jest.spyOn($, 'ajax').mockImplementation();
-    });
-
-    it('calls .initCloseReopenReport', () => {
-      jest.spyOn(IssuablesHelper, 'initCloseReopenReport').mockImplementation(() => {});
-
-      new MergeRequest(); // eslint-disable-line no-new
-
-      expect(IssuablesHelper.initCloseReopenReport).toHaveBeenCalled();
-    });
-
-    it('calls .initDroplab', () => {
-      const container = {
-        querySelector: jest.fn().mockName('container.querySelector'),
-      };
-      const dropdownTrigger = {};
-      const dropdownList = {};
-      const button = {};
-
-      jest.spyOn(CloseReopenReportToggle.prototype, 'initDroplab').mockImplementation(() => {});
-      jest.spyOn(document, 'querySelector').mockReturnValue(container);
-
-      container.querySelector
-        .mockReturnValueOnce(dropdownTrigger)
-        .mockReturnValueOnce(dropdownList)
-        .mockReturnValueOnce(button);
-
-      new MergeRequest(); // eslint-disable-line no-new
-
-      expect(document.querySelector).toHaveBeenCalledWith('.js-issuable-close-dropdown');
-      expect(container.querySelector).toHaveBeenCalledWith('.js-issuable-close-toggle');
-      expect(container.querySelector).toHaveBeenCalledWith('.js-issuable-close-menu');
-      expect(container.querySelector).toHaveBeenCalledWith('.js-issuable-close-button');
-      expect(CloseReopenReportToggle.prototype.initDroplab).toHaveBeenCalled();
-    });
-  });
-
   describe('hideCloseButton', () => {
-    describe('merge request of another user', () => {
-      beforeEach(() => {
-        loadFixtures('merge_requests/merge_request_with_task_list.html');
-        test.el = document.querySelector('.js-issuable-actions');
-        new MergeRequest(); // eslint-disable-line no-new
-        MergeRequest.hideCloseButton();
-      });
-
-      it('hides the dropdown close item and selects the next item', () => {
-        const closeItem = test.el.querySelector('li.close-item');
-        const smallCloseItem = test.el.querySelector('.js-close-item');
-        const reportItem = test.el.querySelector('li.report-item');
-
-        expect(closeItem).toHaveClass('hidden');
-        expect(smallCloseItem).toHaveClass('hidden');
-        expect(reportItem).toHaveClass('droplab-item-selected');
-        expect(reportItem).not.toHaveClass('hidden');
-      });
-    });
-
     describe('merge request of current_user', () => {
       beforeEach(() => {
         loadFixtures('merge_requests/merge_request_of_current_user.html');
@@ -180,10 +119,8 @@ describe('MergeRequest', () => {
       });
 
       it('hides the close button', () => {
-        const closeButton = test.el.querySelector('.btn-close');
         const smallCloseItem = test.el.querySelector('.js-close-item');
 
-        expect(closeButton).toHaveClass('hidden');
         expect(smallCloseItem).toHaveClass('hidden');
       });
     });

spec/frontend/vue_mr_widget/components/states/mr_widget_ready_to_merge_spec.js

@@ -492,19 +492,6 @@ describe('ReadyToMerge', () => {
         });
       });
 
-      it('hides close button', done => {
-        jest.spyOn(vm.service, 'poll').mockReturnValue(returnPromise('merged'));
-        jest.spyOn(vm, 'initiateRemoveSourceBranchPolling').mockImplementation(() => {});
-
-        vm.handleMergePolling(() => {}, () => {});
-
-        setImmediate(() => {
-          expect(document.querySelector('.btn-close').classList.contains('hidden')).toBeTruthy();
-
-          done();
-        });
-      });
-
       it('updates merge request count badge', done => {
         jest.spyOn(vm.service, 'poll').mockReturnValue(returnPromise('merged'));
         jest.spyOn(vm, 'initiateRemoveSourceBranchPolling').mockImplementation(() => {});