Raise exception if we can't match the update_task

and some additional refactoring

3 files changed, 67 insertions, 54 deletions

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 90f971faedb..dc929aa3b28 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -238,6 +238,7 @@ class IssuableBaseService < BaseService
   def update_task(issuable)
     filter_params(issuable)
     # old_associations = associations_before_update(issuable)
+
     if issuable.changed? || params.present?
       issuable.assign_attributes(params.merge(updated_by: current_user))
       issuable.assign_attributes(last_edited_at: Time.now, last_edited_by: current_user)
@@ -263,6 +264,36 @@ class IssuableBaseService < BaseService
     issuable
   end
 
+  # Handle the `update_task` event sent from UI.  Attempts to update a specific
+  # line in the markdown and cached html, bypassing any unnecessary updates or checks.
+  def update_task_event(issue)
+    update_task_params = params.delete(:update_task)
+    return unless update_task_params
+
+    toggler = TaskListToggleService.new(issue.description, issue.description_html,
+                                        index: update_task_params[:index],
+                                        currently_checked: !update_task_params[:checked],
+                                        line_source: update_task_params[:line_source],
+                                        line_number: update_task_params[:line_number])
+
+    if toggler.execute
+      # by updating the description_html field at the same time,
+      # the markdown cache won't be considered invalid
+      params[:description]      = toggler.updated_markdown
+      params[:description_html] = toggler.updated_markdown_html
+
+      # since we're updating a very specific line, we don't care whether
+      # the `lock_version` sent from the FE is the same or not.  Just
+      # make sure the data hasn't changed since we queried it
+      params[:lock_version]     = issue.lock_version
+
+      update_task(issue)
+    else
+      # if we make it here, the data is much newer than we thought it was - fail fast
+      raise ActiveRecord::StaleObjectError
+    end
+  end
+
   def labels_changing?(old_label_ids, new_label_ids)
     old_label_ids.sort != new_label_ids.sort
   end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 9c7b8fcc6ab..cec5b5734c0 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -8,7 +8,7 @@ module Issues
       handle_move_between_ids(issue)
       filter_spam_check_params
       change_issue_duplicate(issue)
-      move_issue_to_new_project(issue) || task_change_event(issue) || update(issue)
+      move_issue_to_new_project(issue) || update_task_event(issue) || update(issue)
     end
 
     def update(issue)
@@ -125,31 +125,6 @@ module Issues
       end
     end
 
-    def task_change_event(issue)
-      update_task_params = params.delete(:update_task)
-      return unless update_task_params
-
-      toggler = TaskListToggleService.new(issue.description, issue.description_html,
-                                          index: update_task_params[:index],
-                                          currently_checked: !update_task_params[:checked],
-                                          line_source: update_task_params[:line_source],
-                                          line_number: update_task_params[:line_number])
-
-      if toggler.execute
-        # by updating the description_html field at the same time,
-        # the markdown cache won't be considered invalid
-        params[:description]      = toggler.updated_markdown
-        params[:description_html] = toggler.updated_markdown_html
-
-        # since we're updating a very specific line, we don't care whether
-        # the `lock_version` sent from the FE is the same or not.  Just
-        # make sure the data hasn't changed since we queried it
-        params[:lock_version]     = issue.lock_version
-
-        update_task(issue)
-      end
-    end
-
     # rubocop: disable CodeReuse/ActiveRecord
     def get_issue_if_allowed(id, board_group_id = nil)
       return unless id
diff --git a/spec/services/task_list_toggle_service_spec.rb b/spec/services/task_list_toggle_service_spec.rb
index abc27012452..facd8c193aa 100644
--- a/spec/services/task_list_toggle_service_spec.rb
+++ b/spec/services/task_list_toggle_service_spec.rb
@@ -2,7 +2,8 @@ require 'spec_helper'
 
 describe TaskListToggleService do
   context 'when ' do
-    let(:markdown) { <<-EOT.strip_heredoc
+    let(:markdown) do
+      <<-EOT.strip_heredoc
         * [ ] Task 1
         * [x] Task 2
 
@@ -11,33 +12,35 @@ describe TaskListToggleService do
         1. [X] Item 1
            - [ ] Sub-item 1
       EOT
-    }
+    end
 
-    let(:markdown_html) { <<-EOT.strip_heredoc
-      <ul class="task-list" dir="auto">
-        <li class="task-list-item">
-          <input type="checkbox" class="task-list-item-checkbox" disabled> Task 1
-        </li>
-        <li class="task-list-item">
-          <input type="checkbox" class="task-list-item-checkbox" disabled checked> Task 2
-        </li>
-      </ul>
-      <p dir="auto">A paragraph</p>
-      <ol class="task-list" dir="auto">
-        <li class="task-list-item">
-          <input type="checkbox" class="task-list-item-checkbox" disabled checked> Item 1
-          <ul class="task-list">
-            <li class="task-list-item">
-              <input type="checkbox" class="task-list-item-checkbox" disabled> Sub-item 1
-            </li>
-          </ul>
-        </li>
-      </ol>
+    let(:markdown_html) do
+      <<-EOT.strip_heredoc
+        <ul class="task-list" dir="auto">
+          <li class="task-list-item">
+            <input type="checkbox" class="task-list-item-checkbox" disabled> Task 1
+          </li>
+          <li class="task-list-item">
+            <input type="checkbox" class="task-list-item-checkbox" disabled checked> Task 2
+          </li>
+        </ul>
+        <p dir="auto">A paragraph</p>
+        <ol class="task-list" dir="auto">
+          <li class="task-list-item">
+            <input type="checkbox" class="task-list-item-checkbox" disabled checked> Item 1
+            <ul class="task-list">
+              <li class="task-list-item">
+                <input type="checkbox" class="task-list-item-checkbox" disabled> Sub-item 1
+              </li>
+            </ul>
+          </li>
+        </ol>
       EOT
-    }
+    end
 
     it 'checks Task 1' do
-      toggler = described_class.new(markdown, markdown_html, index: 1, currently_checked: false,
+      toggler = described_class.new(markdown, markdown_html,
+                                    index: 1, currently_checked: false,
                                     line_source: '* [ ] Task 1', line_number: 1)
 
       expect(toggler.execute).to be_truthy
@@ -46,7 +49,8 @@ describe TaskListToggleService do
     end
 
     it 'unchecks Item 1' do
-      toggler = described_class.new(markdown, markdown_html, index: 3, currently_checked: true,
+      toggler = described_class.new(markdown, markdown_html,
+                                    index: 3, currently_checked: true,
                                     line_source: '1. [X] Item 1', line_number: 6)
 
       expect(toggler.execute).to be_truthy
@@ -55,21 +59,24 @@ describe TaskListToggleService do
     end
 
     it 'returns false if line_source does not match the text' do
-      toggler = described_class.new(markdown, markdown_html, index: 2, currently_checked: true,
+      toggler = described_class.new(markdown, markdown_html,
+                                    index: 2, currently_checked: true,
                                     line_source: '* [x] Task Added', line_number: 2)
 
       expect(toggler.execute).to be_falsey
     end
 
     it 'returns false if markdown is nil' do
-      toggler = described_class.new(nil, markdown_html, index: 2, currently_checked: true,
+      toggler = described_class.new(nil, markdown_html,
+                                    index: 2, currently_checked: true,
                                     line_source: '* [x] Task Added', line_number: 2)
 
       expect(toggler.execute).to be_falsey
     end
 
     it 'returns false if markdown_html is nil' do
-      toggler = described_class.new(markdown, nil, index: 2, currently_checked: true,
+      toggler = described_class.new(markdown, nil,
+                                    index: 2, currently_checked: true,
                                     line_source: '* [x] Task Added', line_number: 2)
 
       expect(toggler.execute).to be_falsey