Refactor toggling of task list item

app/models/concerns/taskable.rb

@@ -11,6 +11,8 @@ require 'task_list/filter'
 module Taskable
   COMPLETED          = 'completed'.freeze
   INCOMPLETE         = 'incomplete'.freeze
+  COMPLETE_PATTERN   = /(\[[xX]\])/.freeze
+  INCOMPLETE_PATTERN = /(\[[\s]\])/.freeze
   ITEM_PATTERN       = %r{
     ^
     \s*(?:[-+*]|(?:\d+\.)) # list prefix required - task item has to be always in a list
@@ -37,6 +39,44 @@ module Taskable
     end
   end
 
+  def self.toggle_task(content, content_html, index:, currently_checked:, line_source:, line_number:)
+    source_lines  = content.split("\n")
+    markdown_task = source_lines[line_number - 1]
+    output        = {}
+
+    if markdown_task == line_source
+      html          = Nokogiri::HTML.fragment(content_html)
+      html_checkbox = html.css('.task-list-item-checkbox')[index - 1]
+      # html_checkbox = html.css(".task-list-item[data-sourcepos^='#{changed_line_number}:'] > input.task-list-item-checkbox").first
+      updated_task  = toggle_task_source(line_source, currently_checked: currently_checked)
+
+      if html_checkbox && updated_task
+        source_lines[line_number - 1] = updated_task
+
+        if currently_checked
+          html_checkbox.remove_attribute('checked')
+        else
+          html_checkbox[:checked] = 'checked'
+        end
+
+        output[:content]      = source_lines.join("\n")
+        output[:content_html] = html.to_html
+      end
+    end
+
+    output
+  end
+
+  def self.toggle_task_source(markdown_line, currently_checked:)
+    if source_checkbox = ITEM_PATTERN.match(markdown_line)
+      if TaskList::Item.new(source_checkbox[1]).complete?
+        markdown_line.sub(COMPLETE_PATTERN, '[ ]') if currently_checked
+      else
+        markdown_line.sub(INCOMPLETE_PATTERN, '[x]') unless currently_checked
+      end
+    end
+  end
+
   # Called by `TaskList::Summary`
   def task_list_items
     return [] if description.blank?

app/services/issuable/common_system_notes_service.rb

@@ -20,7 +20,7 @@ module Issuable
 
       create_due_date_note if issuable.previous_changes.include?('due_date')
       create_milestone_note if issuable.previous_changes.include?('milestone_id')
-      create_labels_note(old_labels) if issuable.labels != old_labels
+      create_labels_note(old_labels) if old_labels && issuable.labels != old_labels
     end
 
     private

app/services/issuable_base_service.rb

@@ -235,6 +235,34 @@ class IssuableBaseService < BaseService
     issuable
   end
 
+  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)
+
+      before_update(issuable)
+
+      if issuable.with_transaction_returning_status { issuable.save }
+        # We do not touch as it will affect a update on updated_at field
+        ActiveRecord::Base.no_touching do
+          Issuable::CommonSystemNotesService.new(project, current_user).execute(issuable, old_labels: nil)
+        end
+
+        handle_task_changes(issuable)
+
+        invalidate_cache_counts(issuable, users: issuable.assignees.to_a)
+
+        after_update(issuable)
+
+        # execute_hooks(issuable, 'update', old_associations: old_associations)
+      end
+    end
+
+    issuable
+  end
+
   def labels_changing?(old_label_ids, new_label_ids)
     old_label_ids.sort != new_label_ids.sort
   end
@@ -317,6 +345,10 @@ class IssuableBaseService < BaseService
   def handle_changes(issuable, options)
   end
 
+  # override if needed
+  def handle_task_changes(issuable)
+  end
+
   # override if needed
   def execute_hooks(issuable, action = 'open', params = {})
   end

app/services/issues/update_service.rb

@@ -8,8 +8,7 @@ module Issues
       handle_move_between_ids(issue)
       filter_spam_check_params
       change_issue_duplicate(issue)
-      handle_update_task(issue)
-      move_issue_to_new_project(issue) || update(issue)
+      move_issue_to_new_project(issue) || task_change_event(issue) || update(issue)
     end
 
     def update(issue)
@@ -64,6 +63,11 @@ module Issues
       end
     end
 
+    def handle_task_changes(issuable)
+      todo_service.mark_pending_todos_as_done(issuable, current_user)
+      todo_service.update_issue(issuable, current_user)
+    end
+
     def handle_move_between_ids(issue)
       return unless params[:move_between_ids]
 
@@ -79,6 +83,8 @@ module Issues
     # rubocop: disable CodeReuse/ActiveRecord
     def change_issue_duplicate(issue)
       canonical_issue_id = params.delete(:canonical_issue_id)
+      return unless canonical_issue_id
+
       canonical_issue = IssuesFinder.new(current_user).find_by(id: canonical_issue_id)
 
       if canonical_issue
@@ -119,64 +125,23 @@ module Issues
       end
     end
 
-    def handle_update_task(issue)
+    def task_change_event(issue)
       update_task_params = params.delete(:update_task)
       return unless update_task_params
 
-      checkbox_index      = update_task_params[:index]
-      currently_checked   = !update_task_params[:checked]
-      changed_line_number = update_task_params[:line_number]
-      changed_line_source = update_task_params[:line_source] + "\n"
-
-      source_lines  = issue.description.lines
-      markdown_task = source_lines[changed_line_number - 1]
-
-      # binding.pry
-
-      if markdown_task == changed_line_source
-        source_checkbox = /(\[[\sxX]\])/.match(markdown_task)
-
-        if source_checkbox
-          if currently_checked
-            if source_checkbox[1] == '[x]'
-              changed_line_source.sub!(/(\[[xX]\])/, '[ ]')
-            else
-              # it's already checked by someone else, nothing to do
-              ignore = true
-            end
-          else
-            if source_checkbox[1] == '[ ]'
-              changed_line_source.sub!(/(\[[\s]\])/, '[x]')
-            else
-              # it's already unchecked by someone else, nothing to do
-              ignore = true
-            end
-          end
-        end
-
-        unless ignore
-          # replace line with proper checkbox
-          source_lines[changed_line_number - 1] = changed_line_source
+      updated_content = Taskable.toggle_task(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])
 
-          params[:description] = source_lines.join
+      unless updated_content.empty?
+        # by updating the description_html field at the same time,
+        # the markdown cache won't be considered invalid
+        params[:description]      = updated_content[:content]
+        params[:description_html] = updated_content[:content_html]
 
-          # if we update the description_html field at the same time,
-          # the cache won't be considered invalid (unless something else
-          # changed)
-          html = Nokogiri::HTML.fragment(issue.description_html)
-          html_checkbox = html.css('.task-list-item-checkbox')[checkbox_index - 1]
-          # html_checkbox = html.css(".task-list-item[data-sourcepos^='#{changed_line_number}:'] > input.task-list-item-checkbox").first
-
-          if currently_checked
-            # checkboxes[checkbox_index - 1].remove_attribute('checked')
-            html_checkbox.remove_attribute('checked')
-          else
-            # checkboxes[checkbox_index - 1][:checked] = 'checked'
-            html_checkbox[:checked] = 'checked'
-          end
-
-          params[:description_html] = html.to_html
-        end
+        update_task(issue)
       end
     end