Merge branch 'ee-19745-new-tasklists-for-merge-requests' into 'master'

Enable fast task lists for merge requests and epics

See merge request gitlab-org/gitlab-ee!9431

app/assets/javascripts/issue_show/components/description.vue

 <script>
 import $ from 'jquery';
-import { __ } from '~/locale';
+import { s__, sprintf } from '~/locale';
+import createFlash from '~/flash';
 import animateMixin from '../mixins/animate';
 import TaskList from '../../task_list';
 import recaptchaModalImplementor from '../../vue_shared/mixins/recaptcha_modal_implementor';
@@ -91,9 +92,14 @@ export default {
     },
 
     taskListUpdateError() {
-      window.Flash(
-        __(
-          'Someone edited this issue at the same time you did. The description has been updated and you will need to make your changes again.',
+      createFlash(
+        sprintf(
+          s__(
+            'Someone edited this %{issueType} at the same time you did. The description has been updated and you will need to make your changes again.',
+          ),
+          {
+            issueType: this.issuableType,
+          },
         ),
       );
 

app/assets/javascripts/merge_request.js

@@ -2,6 +2,7 @@
 
 import $ from 'jquery';
 import { __ } from '~/locale';
+import createFlash from '~/flash';
 import TaskList from './task_list';
 import MergeRequestTabs from './merge_request_tabs';
 import IssuablesHelper from './helpers/issuables_helper';
@@ -40,6 +41,13 @@ function MergeRequest(opts) {
         document.querySelector('#task_status').innerText = result.task_status;
         document.querySelector('#task_status_short').innerText = result.task_status_short;
       },
+      onError: () => {
+        createFlash(
+          __(
+            'Someone edited this merge request at the same time you did. Please refresh the page to see changes.',
+          ),
+        );
+      },
     });
   }
 }

app/controllers/projects/merge_requests/application_controller.rb

@@ -34,7 +34,8 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
       :task_num,
       :title,
       :discussion_locked,
-      label_ids: []
+      label_ids: [],
+      update_task: [:index, :checked, :line_number, :line_source]
     ]
   end
 

app/services/merge_requests/update_service.rb

@@ -23,7 +23,7 @@ module MergeRequests
       end
 
       handle_wip_event(merge_request)
-      update(merge_request)
+      update_task_event(merge_request) || update(merge_request)
     end
 
     # rubocop:disable Metrics/AbcSize
@@ -85,6 +85,11 @@ module MergeRequests
     end
     # rubocop:enable Metrics/AbcSize
 
+    def handle_task_changes(merge_request)
+      todo_service.mark_pending_todos_as_done(merge_request, current_user)
+      todo_service.update_merge_request(merge_request, current_user)
+    end
+
     def merge_from_quick_action(merge_request)
       last_diff_sha = params.delete(:merge)
       return unless merge_request.mergeable_with_quick_action?(current_user, last_diff_sha: last_diff_sha)

app/services/task_list_toggle_service.rb

@@ -32,7 +32,8 @@ class TaskListToggleService
     source_line_index = line_number - 1
     markdown_task     = source_lines[source_line_index]
 
-    return unless markdown_task == line_source
+    # The source in the DB could be using either \n or \r\n line endings
+    return unless markdown_task == line_source || markdown_task == line_source + "\r"
     return unless source_checkbox = Taskable::ITEM_PATTERN.match(markdown_task)
 
     currently_checked = TaskList::Item.new(source_checkbox[1]).complete?

ee/app/controllers/groups/epics_controller.rb

@@ -73,7 +73,8 @@ class Groups::EpicsController < Groups::ApplicationController
       :due_date_fixed,
       :due_date_is_fixed,
       :state_event,
-      label_ids: []
+      label_ids: [],
+      update_task: [:index, :checked, :line_number, :line_source]
     ]
   end
 

ee/app/services/ee/issuable_base_service.rb

@@ -2,6 +2,8 @@
 
 module EE
   module IssuableBaseService
+    include ::Gitlab::Utils::StrongMemoize
+
     private
 
     def filter_params(issuable)
@@ -15,5 +17,11 @@ module EE
 
       super
     end
+
+    def update_task_event?
+      strong_memoize(:update_task_event) do
+        params.key?(:update_task)
+      end
+    end
   end
 end

ee/app/services/ee/merge_requests/update_service.rb

@@ -9,8 +9,10 @@ module EE
 
       override :execute
       def execute(merge_request)
-        should_remove_old_approvers = params.delete(:remove_old_approvers)
-        old_approvers = merge_request.overall_approvers(exclude_code_owners: true)
+        unless update_task_event?
+          should_remove_old_approvers = params.delete(:remove_old_approvers)
+          old_approvers = merge_request.overall_approvers(exclude_code_owners: true)
+        end
 
         merge_request = super(merge_request)
         sync_approval_rules(merge_request)
@@ -21,6 +23,8 @@ module EE
 
         merge_request.reset_approval_cache!
 
+        return merge_request if update_task_event?
+
         new_approvers = merge_request.overall_approvers(exclude_code_owners: true) - old_approvers
 
         if new_approvers.any?

ee/app/services/epics/update_service.rb

@@ -14,7 +14,7 @@ module Epics
       # are composite fields managed by the system.
       params.except!(:start_date, :end_date)
 
-      update(epic)
+      update_task_event(epic) || update(epic)
 
       if have_epic_dates_changed?(epic)
         epic.update_start_and_due_dates
@@ -36,6 +36,11 @@ module Epics
       todo_service.update_epic(epic, current_user, old_mentioned_users)
     end
 
+    def handle_task_changes(epic)
+      todo_service.mark_pending_todos_as_done(epic, current_user)
+      todo_service.update_epic(epic, current_user)
+    end
+
     private
 
     def have_epic_dates_changed?(epic)

ee/spec/services/epics/update_service_spec.rb

@@ -11,6 +11,19 @@ describe Epics::UpdateService do
       group.add_master(user)
     end
 
+    def find_note(starting_with)
+      epic.notes.find do |note|
+        note && note.note.start_with?(starting_with)
+      end
+    end
+
+    def find_notes(action)
+      epic
+        .notes
+        .joins(:system_note_metadata)
+        .where(system_note_metadata: { action: action })
+    end
+
     def update_epic(opts)
       described_class.new(group, user, opts).execute(epic)
     end
@@ -123,6 +136,55 @@ describe Epics::UpdateService do
       end
     end
 
+    context 'when Epic has tasks' do
+      before do
+        update_epic({ description: "- [ ] Task 1\n- [ ] Task 2" })
+      end
+
+      it { expect(epic.tasks?).to eq(true) }
+
+      it_behaves_like 'updating a single task' do
+        def update_issuable(opts)
+          described_class.new(group, user, opts).execute(epic)
+        end
+      end
+
+      context 'when tasks are marked as completed' do
+        before do
+          update_epic({ description: "- [x] Task 1\n- [X] Task 2" })
+        end
+
+        it 'creates system note about task status change' do
+          note1 = find_note('marked the task **Task 1** as completed')
+          note2 = find_note('marked the task **Task 2** as completed')
+
+          expect(note1).not_to be_nil
+          expect(note2).not_to be_nil
+
+          description_notes = find_notes('description')
+          expect(description_notes.length).to eq(1)
+        end
+      end
+
+      context 'when tasks are marked as incomplete' do
+        before do
+          update_epic({ description: "- [x] Task 1\n- [X] Task 2" })
+          update_epic({ description: "- [ ] Task 1\n- [ ] Task 2" })
+        end
+
+        it 'creates system note about task status change' do
+          note1 = find_note('marked the task **Task 1** as incomplete')
+          note2 = find_note('marked the task **Task 2** as incomplete')
+
+          expect(note1).not_to be_nil
+          expect(note2).not_to be_nil
+
+          description_notes = find_notes('description')
+          expect(description_notes.length).to eq(1)
+        end
+      end
+    end
+
     context 'filter out start_date and end_date' do
       it 'ignores start_date and end_date' do
         expect { update_epic(start_date: Date.today, end_date: Date.today) }.not_to change { Note.count }

locale/gitlab.pot

@@ -8671,7 +8671,10 @@ msgstr ""
 msgid "Snippets"
 msgstr ""
 
-msgid "Someone edited this issue at the same time you did. The description has been updated and you will need to make your changes again."
+msgid "Someone edited this %{issueType} at the same time you did. The description has been updated and you will need to make your changes again."
+msgstr ""
+
+msgid "Someone edited this merge request at the same time you did. Please refresh the page to see changes."
 msgstr ""
 
 msgid "Something went wrong on our end"

spec/javascripts/issue_show/components/description_spec.js

@@ -21,7 +21,8 @@ describe('Description component', () => {
     if (!document.querySelector('.issuable-meta')) {
       const metaData = document.createElement('div');
       metaData.classList.add('issuable-meta');
-      metaData.innerHTML = '<span id="task_status"></span><span id="task_status_short"></span>';
+      metaData.innerHTML =
+        '<div class="flash-container"></div><span id="task_status"></span><span id="task_status_short"></span>';
 
       document.body.appendChild(metaData);
     }
@@ -33,6 +34,10 @@ describe('Description component', () => {
     vm.$destroy();
   });
 
+  afterAll(() => {
+    $('.issuable-meta .flash-container').remove();
+  });
+
   it('animates description changes', done => {
     vm.descriptionHtml = 'changed';
 
@@ -192,12 +197,11 @@ describe('Description component', () => {
     it('should create flash notification and emit an event to parent', () => {
       const msg =
         'Someone edited this issue at the same time you did. The description has been updated and you will need to make your changes again.';
-      spyOn(window, 'Flash');
       spyOn(vm, '$emit');
 
       vm.taskListUpdateError();
 
-      expect(window.Flash).toHaveBeenCalledWith(msg);
+      expect(document.querySelector('.flash-container .flash-text').innerText.trim()).toBe(msg);
       expect(vm.$emit).toHaveBeenCalledWith('taskListUpdateFailed');
     });
   });

spec/javascripts/merge_request_spec.js

@@ -40,30 +40,51 @@ describe('MergeRequest', function() {
       expect($('.js-task-list-field').val()).toBe('- [x] Task List Item');
     });
 
-    it('submits an ajax request on tasklist:changed', done => {
+    describe('tasklist', () => {
       const lineNumber = 8;
       const lineSource = '- [ ] item 8';
       const index = 3;
       const checked = true;
 
-      $('.js-task-list-field').trigger({
-        type: 'tasklist:changed',
-        detail: { lineNumber, lineSource, index, checked },
+      it('submits an ajax request on tasklist:changed', done => {
+        $('.js-task-list-field').trigger({
+          type: 'tasklist:changed',
+          detail: { lineNumber, lineSource, index, checked },
+        });
+
+        setTimeout(() => {
+          expect(axios.patch).toHaveBeenCalledWith(
+            `${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`,
+            {
+              merge_request: {
+                description: '- [ ] Task List Item',
+                lock_version: undefined,
+                update_task: { line_number: lineNumber, line_source: lineSource, index, checked },
+              },
+            },
+          );
+
+          done();
+        });
       });
 
-      setTimeout(() => {
-        expect(axios.patch).toHaveBeenCalledWith(
-          `${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`,
-          {
-            merge_request: {
-              description: '- [ ] Task List Item',
-              lock_version: undefined,
-              update_task: { line_number: lineNumber, line_source: lineSource, index, checked },
-            },
-          },
-        );
+      it('shows an error notification when tasklist update failed', done => {
+        mock
+          .onPatch(`${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`)
+          .reply(409, {});
+
+        $('.js-task-list-field').trigger({
+          type: 'tasklist:changed',
+          detail: { lineNumber, lineSource, index, checked },
+        });
+
+        setTimeout(() => {
+          expect(document.querySelector('.flash-container .flash-text').innerText.trim()).toBe(
+            'Someone edited this merge request at the same time you did. Please refresh the page to see changes.',
+          );
 
-        done();
+          done();
+        });
       });
     });
   });

spec/services/issues/update_service_spec.rb

@@ -471,6 +471,8 @@ describe Issues::UpdateService, :mailer do
 
       it { expect(issue.tasks?).to eq(true) }
 
+      it_behaves_like 'updating a single task'
+
       context 'when tasks are marked as completed' do
         before do
           update_issue(description: "- [x] Task 1\n- [X] Task 2")
@@ -543,76 +545,6 @@ describe Issues::UpdateService, :mailer do
       end
     end
 
-    context 'when updating a single task' do
-      before do
-        update_issue(description: "- [ ] Task 1\n- [ ] Task 2")
-      end
-
-      it { expect(issue.tasks?).to eq(true) }
-
-      context 'when a task is marked as completed' do
-        before do
-          update_issue(update_task: { index: 1, checked: true, line_source: '- [ ] Task 1', line_number: 1 })
-        end
-
-        it 'creates system note about task status change' do
-          note1 = find_note('marked the task **Task 1** as completed')
-
-          expect(note1).not_to be_nil
-
-          description_notes = find_notes('description')
-          expect(description_notes.length).to eq(1)
-        end
-      end
-
-      context 'when a task is marked as incomplete' do
-        before do
-          update_issue(description: "- [x] Task 1\n- [X] Task 2")
-          update_issue(update_task: { index: 2, checked: false, line_source: '- [X] Task 2', line_number: 2 })
-        end
-
-        it 'creates system note about task status change' do
-          note1 = find_note('marked the task **Task 2** as incomplete')
-
-          expect(note1).not_to be_nil
-
-          description_notes = find_notes('description')
-          expect(description_notes.length).to eq(1)
-        end
-      end
-
-      context 'when the task position has been modified' do
-        before do
-          update_issue(description: "- [ ] Task 1\n- [ ] Task 3\n- [ ] Task 2")
-        end
-
-        it 'raises an exception' do
-          expect(Note.count).to eq(2)
-          expect do
-            update_issue(update_task: { index: 2, checked: true, line_source: '- [ ] Task 2', line_number: 2 })
-          end.to raise_error(ActiveRecord::StaleObjectError)
-          expect(Note.count).to eq(2)
-        end
-      end
-
-      context 'when the content changes but not task line number' do
-        before do
-          update_issue(description: "Paragraph\n\n- [ ] Task 1\n- [x] Task 2")
-          update_issue(description: "Paragraph with more words\n\n- [ ] Task 1\n- [x] Task 2")
-          update_issue(update_task: { index: 2, checked: false, line_source: '- [x] Task 2', line_number: 4 })
-        end
-
-        it 'creates system note about task status change' do
-          note1 = find_note('marked the task **Task 2** as incomplete')
-
-          expect(note1).not_to be_nil
-
-          description_notes = find_notes('description')
-          expect(description_notes.length).to eq(2)
-        end
-      end
-    end
-
     context 'updating labels' do
       let(:label3) { create(:label, project: project) }
       let(:result) { described_class.new(project, user, params).execute(issue).reload }

spec/services/merge_requests/update_service_spec.rb

@@ -466,6 +466,8 @@ describe MergeRequests::UpdateService, :mailer do
 
       it { expect(@merge_request.tasks?).to eq(true) }
 
+      it_behaves_like 'updating a single task'
+
       context 'when tasks are marked as completed' do
         before do
           update_merge_request({ description: "- [x] Task 1\n- [X] Task 2" })

spec/services/task_list_toggle_service_spec.rb

@@ -67,6 +67,17 @@ describe TaskListToggleService do
     expect(toggler.execute).to be_falsey
   end
 
+  it 'tolerates \r\n line endings' do
+    rn_markdown = markdown.gsub("\n", "\r\n")
+    toggler = described_class.new(rn_markdown, markdown_html,
+                                  toggle_as_checked: true,
+                                  line_source: '* [ ] Task 1', line_number: 1)
+
+    expect(toggler.execute).to be_truthy
+    expect(toggler.updated_markdown.lines[0]).to eq "* [x] Task 1\r\n"
+    expect(toggler.updated_markdown_html).to include('disabled checked> Task 1')
+  end
+
   it 'returns false if markdown is nil' do
     toggler = described_class.new(nil, markdown_html,
                                   toggle_as_checked: false,

spec/support/shared_examples/issuable_shared_examples.rb

@@ -36,3 +36,76 @@ shared_examples 'system notes for milestones' do
     end
   end
 end
+
+shared_examples 'updating a single task' do
+  def update_issuable(opts)
+    issuable = try(:issue) || try(:merge_request)
+    described_class.new(project, user, opts).execute(issuable)
+  end
+
+  before do
+    update_issuable(description: "- [ ] Task 1\n- [ ] Task 2")
+  end
+
+  context 'when a task is marked as completed' do
+    before do
+      update_issuable(update_task: { index: 1, checked: true, line_source: '- [ ] Task 1', line_number: 1 })
+    end
+
+    it 'creates system note about task status change' do
+      note1 = find_note('marked the task **Task 1** as completed')
+
+      expect(note1).not_to be_nil
+
+      description_notes = find_notes('description')
+      expect(description_notes.length).to eq(1)
+    end
+  end
+
+  context 'when a task is marked as incomplete' do
+    before do
+      update_issuable(description: "- [x] Task 1\n- [X] Task 2")
+      update_issuable(update_task: { index: 2, checked: false, line_source: '- [X] Task 2', line_number: 2 })
+    end
+
+    it 'creates system note about task status change' do
+      note1 = find_note('marked the task **Task 2** as incomplete')
+
+      expect(note1).not_to be_nil
+
+      description_notes = find_notes('description')
+      expect(description_notes.length).to eq(1)
+    end
+  end
+
+  context 'when the task position has been modified' do
+    before do
+      update_issuable(description: "- [ ] Task 1\n- [ ] Task 3\n- [ ] Task 2")
+    end
+
+    it 'raises an exception' do
+      expect(Note.count).to eq(2)
+      expect do
+        update_issuable(update_task: { index: 2, checked: true, line_source: '- [ ] Task 2', line_number: 2 })
+      end.to raise_error(ActiveRecord::StaleObjectError)
+      expect(Note.count).to eq(2)
+    end
+  end
+
+  context 'when the content changes but not task line number' do
+    before do
+      update_issuable(description: "Paragraph\n\n- [ ] Task 1\n- [x] Task 2")
+      update_issuable(description: "Paragraph with more words\n\n- [ ] Task 1\n- [x] Task 2")
+      update_issuable(update_task: { index: 2, checked: false, line_source: '- [x] Task 2', line_number: 4 })
+    end
+
+    it 'creates system note about task status change' do
+      note1 = find_note('marked the task **Task 2** as incomplete')
+
+      expect(note1).not_to be_nil
+
+      description_notes = find_notes('description')
+      expect(description_notes.length).to eq(2)
+    end
+  end
+end