Commit aa2fe07d authored by Sean McGivern's avatar Sean McGivern

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

Enable fast task lists for merge requests

Closes #19745

See merge request gitlab-org/gitlab-ce!24779
parents 68f9453c 8bf5e479
<script> <script>
import $ from 'jquery'; import $ from 'jquery';
import { __ } from '~/locale'; import { s__, sprintf } from '~/locale';
import createFlash from '~/flash';
import animateMixin from '../mixins/animate'; import animateMixin from '../mixins/animate';
import TaskList from '../../task_list'; import TaskList from '../../task_list';
import recaptchaModalImplementor from '../../vue_shared/mixins/recaptcha_modal_implementor'; import recaptchaModalImplementor from '../../vue_shared/mixins/recaptcha_modal_implementor';
...@@ -91,9 +92,14 @@ export default { ...@@ -91,9 +92,14 @@ export default {
}, },
taskListUpdateError() { taskListUpdateError() {
window.Flash( createFlash(
__( sprintf(
'Someone edited this issue at the same time you did. The description has been updated and you will need to make your changes again.', 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,
},
), ),
); );
......
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
import $ from 'jquery'; import $ from 'jquery';
import { __ } from '~/locale'; import { __ } from '~/locale';
import createFlash from '~/flash';
import TaskList from './task_list'; import TaskList from './task_list';
import MergeRequestTabs from './merge_request_tabs'; import MergeRequestTabs from './merge_request_tabs';
import IssuablesHelper from './helpers/issuables_helper'; import IssuablesHelper from './helpers/issuables_helper';
...@@ -40,6 +41,13 @@ function MergeRequest(opts) { ...@@ -40,6 +41,13 @@ function MergeRequest(opts) {
document.querySelector('#task_status').innerText = result.task_status; document.querySelector('#task_status').innerText = result.task_status;
document.querySelector('#task_status_short').innerText = result.task_status_short; 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.',
),
);
},
}); });
} }
} }
......
...@@ -34,7 +34,8 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont ...@@ -34,7 +34,8 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
:task_num, :task_num,
:title, :title,
:discussion_locked, :discussion_locked,
label_ids: [] label_ids: [],
update_task: [:index, :checked, :line_number, :line_source]
] ]
end end
......
...@@ -21,7 +21,7 @@ module MergeRequests ...@@ -21,7 +21,7 @@ module MergeRequests
end end
handle_wip_event(merge_request) handle_wip_event(merge_request)
update(merge_request) update_task_event(merge_request) || update(merge_request)
end end
# rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/AbcSize
...@@ -83,6 +83,11 @@ module MergeRequests ...@@ -83,6 +83,11 @@ module MergeRequests
end end
# rubocop:enable Metrics/AbcSize # 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) def merge_from_quick_action(merge_request)
last_diff_sha = params.delete(:merge) last_diff_sha = params.delete(:merge)
return unless merge_request.mergeable_with_quick_action?(current_user, last_diff_sha: last_diff_sha) return unless merge_request.mergeable_with_quick_action?(current_user, last_diff_sha: last_diff_sha)
......
...@@ -32,7 +32,8 @@ class TaskListToggleService ...@@ -32,7 +32,8 @@ class TaskListToggleService
source_line_index = line_number - 1 source_line_index = line_number - 1
markdown_task = source_lines[source_line_index] 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) return unless source_checkbox = Taskable::ITEM_PATTERN.match(markdown_task)
currently_checked = TaskList::Item.new(source_checkbox[1]).complete? currently_checked = TaskList::Item.new(source_checkbox[1]).complete?
......
...@@ -6643,7 +6643,10 @@ msgstr "" ...@@ -6643,7 +6643,10 @@ msgstr ""
msgid "Snippets" msgid "Snippets"
msgstr "" 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 "" msgstr ""
msgid "Something went wrong on our end" msgid "Something went wrong on our end"
......
...@@ -21,7 +21,8 @@ describe('Description component', () => { ...@@ -21,7 +21,8 @@ describe('Description component', () => {
if (!document.querySelector('.issuable-meta')) { if (!document.querySelector('.issuable-meta')) {
const metaData = document.createElement('div'); const metaData = document.createElement('div');
metaData.classList.add('issuable-meta'); 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); document.body.appendChild(metaData);
} }
...@@ -33,6 +34,10 @@ describe('Description component', () => { ...@@ -33,6 +34,10 @@ describe('Description component', () => {
vm.$destroy(); vm.$destroy();
}); });
afterAll(() => {
$('.issuable-meta .flash-container').remove();
});
it('animates description changes', done => { it('animates description changes', done => {
vm.descriptionHtml = 'changed'; vm.descriptionHtml = 'changed';
...@@ -192,12 +197,11 @@ describe('Description component', () => { ...@@ -192,12 +197,11 @@ describe('Description component', () => {
it('should create flash notification and emit an event to parent', () => { it('should create flash notification and emit an event to parent', () => {
const msg = 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.'; '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'); spyOn(vm, '$emit');
vm.taskListUpdateError(); vm.taskListUpdateError();
expect(window.Flash).toHaveBeenCalledWith(msg); expect(document.querySelector('.flash-container .flash-text').innerText.trim()).toBe(msg);
expect(vm.$emit).toHaveBeenCalledWith('taskListUpdateFailed'); expect(vm.$emit).toHaveBeenCalledWith('taskListUpdateFailed');
}); });
}); });
......
...@@ -40,12 +40,13 @@ describe('MergeRequest', function() { ...@@ -40,12 +40,13 @@ describe('MergeRequest', function() {
expect($('.js-task-list-field').val()).toBe('- [x] Task List Item'); 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 lineNumber = 8;
const lineSource = '- [ ] item 8'; const lineSource = '- [ ] item 8';
const index = 3; const index = 3;
const checked = true; const checked = true;
it('submits an ajax request on tasklist:changed', done => {
$('.js-task-list-field').trigger({ $('.js-task-list-field').trigger({
type: 'tasklist:changed', type: 'tasklist:changed',
detail: { lineNumber, lineSource, index, checked }, detail: { lineNumber, lineSource, index, checked },
...@@ -66,6 +67,26 @@ describe('MergeRequest', function() { ...@@ -66,6 +67,26 @@ describe('MergeRequest', function() {
done(); done();
}); });
}); });
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();
});
});
});
}); });
describe('class constructor', () => { describe('class constructor', () => {
......
...@@ -471,6 +471,8 @@ describe Issues::UpdateService, :mailer do ...@@ -471,6 +471,8 @@ describe Issues::UpdateService, :mailer do
it { expect(issue.tasks?).to eq(true) } it { expect(issue.tasks?).to eq(true) }
it_behaves_like 'updating a single task'
context 'when tasks are marked as completed' do context 'when tasks are marked as completed' do
before do before do
update_issue(description: "- [x] Task 1\n- [X] Task 2") update_issue(description: "- [x] Task 1\n- [X] Task 2")
...@@ -543,76 +545,6 @@ describe Issues::UpdateService, :mailer do ...@@ -543,76 +545,6 @@ describe Issues::UpdateService, :mailer do
end end
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 context 'updating labels' do
let(:label3) { create(:label, project: project) } let(:label3) { create(:label, project: project) }
let(:result) { described_class.new(project, user, params).execute(issue).reload } let(:result) { described_class.new(project, user, params).execute(issue).reload }
......
...@@ -466,6 +466,8 @@ describe MergeRequests::UpdateService, :mailer do ...@@ -466,6 +466,8 @@ describe MergeRequests::UpdateService, :mailer do
it { expect(@merge_request.tasks?).to eq(true) } it { expect(@merge_request.tasks?).to eq(true) }
it_behaves_like 'updating a single task'
context 'when tasks are marked as completed' do context 'when tasks are marked as completed' do
before do before do
update_merge_request({ description: "- [x] Task 1\n- [X] Task 2" }) update_merge_request({ description: "- [x] Task 1\n- [X] Task 2" })
......
...@@ -67,6 +67,17 @@ describe TaskListToggleService do ...@@ -67,6 +67,17 @@ describe TaskListToggleService do
expect(toggler.execute).to be_falsey expect(toggler.execute).to be_falsey
end 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 it 'returns false if markdown is nil' do
toggler = described_class.new(nil, markdown_html, toggler = described_class.new(nil, markdown_html,
toggle_as_checked: false, toggle_as_checked: false,
......
...@@ -36,3 +36,76 @@ shared_examples 'system notes for milestones' do ...@@ -36,3 +36,76 @@ shared_examples 'system notes for milestones' do
end end
end 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment