Commit 1e5c50de authored by Brett Walker's avatar Brett Walker

Enable fast tasklists for merge requests and epics

Allow single tasks to be updated quickly
parent 78e60903
...@@ -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
......
...@@ -23,7 +23,7 @@ module MergeRequests ...@@ -23,7 +23,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
...@@ -85,6 +85,11 @@ module MergeRequests ...@@ -85,6 +85,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?
......
...@@ -73,7 +73,8 @@ class Groups::EpicsController < Groups::ApplicationController ...@@ -73,7 +73,8 @@ class Groups::EpicsController < Groups::ApplicationController
:due_date_fixed, :due_date_fixed,
:due_date_is_fixed, :due_date_is_fixed,
:state_event, :state_event,
label_ids: [] label_ids: [],
update_task: [:index, :checked, :line_number, :line_source]
] ]
end end
......
...@@ -2,6 +2,8 @@ ...@@ -2,6 +2,8 @@
module EE module EE
module IssuableBaseService module IssuableBaseService
include ::Gitlab::Utils::StrongMemoize
private private
def filter_params(issuable) def filter_params(issuable)
...@@ -15,5 +17,11 @@ module EE ...@@ -15,5 +17,11 @@ module EE
super super
end end
def update_task_event?
strong_memoize(:update_task_event) do
params.key?(:update_task)
end
end
end end
end end
...@@ -9,8 +9,10 @@ module EE ...@@ -9,8 +9,10 @@ module EE
override :execute override :execute
def execute(merge_request) def execute(merge_request)
should_remove_old_approvers = params.delete(:remove_old_approvers) unless update_task_event?
old_approvers = merge_request.overall_approvers(exclude_code_owners: true) 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) merge_request = super(merge_request)
sync_approval_rules(merge_request) sync_approval_rules(merge_request)
...@@ -21,6 +23,8 @@ module EE ...@@ -21,6 +23,8 @@ module EE
merge_request.reset_approval_cache! merge_request.reset_approval_cache!
return merge_request if update_task_event?
new_approvers = merge_request.overall_approvers(exclude_code_owners: true) - old_approvers new_approvers = merge_request.overall_approvers(exclude_code_owners: true) - old_approvers
if new_approvers.any? if new_approvers.any?
......
...@@ -14,7 +14,7 @@ module Epics ...@@ -14,7 +14,7 @@ module Epics
# are composite fields managed by the system. # are composite fields managed by the system.
params.except!(:start_date, :end_date) params.except!(:start_date, :end_date)
update(epic) update_task_event(epic) || update(epic)
if have_epic_dates_changed?(epic) if have_epic_dates_changed?(epic)
epic.update_start_and_due_dates epic.update_start_and_due_dates
...@@ -36,6 +36,11 @@ module Epics ...@@ -36,6 +36,11 @@ module Epics
todo_service.update_epic(epic, current_user, old_mentioned_users) todo_service.update_epic(epic, current_user, old_mentioned_users)
end 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 private
def have_epic_dates_changed?(epic) def have_epic_dates_changed?(epic)
......
...@@ -11,6 +11,19 @@ describe Epics::UpdateService do ...@@ -11,6 +11,19 @@ describe Epics::UpdateService do
group.add_master(user) group.add_master(user)
end 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) def update_epic(opts)
described_class.new(group, user, opts).execute(epic) described_class.new(group, user, opts).execute(epic)
end end
...@@ -123,6 +136,55 @@ describe Epics::UpdateService do ...@@ -123,6 +136,55 @@ describe Epics::UpdateService do
end end
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 context 'filter out start_date and end_date' do
it 'ignores 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 } expect { update_epic(start_date: Date.today, end_date: Date.today) }.not_to change { Note.count }
......
...@@ -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