Merge branch 'issue_2296' into 'master'

Generate system note after Task item has been updated on Issue or Merge Request.

Reference: #2296 

Everytime the User check or uncheck a Task Item from the Issue or
Merge Request description, a new update is going to be
added to the activity logs of the Issue or Merge Request.

Note that when using the edit form, you can only update the Task item
status or add/delete/modify existing ones. Doing both actions is not
fully supported.

See merge request !1848

app/models/concerns/issuable.rb

@@ -161,4 +161,9 @@ module Issuable
   def notes_with_associations
     notes.includes(:author, :project)
   end
+
+  def updated_tasks
+    Taskable.get_updated_tasks(old_content: previous_changes['description'].first,
+                               new_content: description)
+  end
 end

app/models/concerns/taskable.rb

@@ -7,14 +7,39 @@ require 'task_list/filter'
 #
 # Used by MergeRequest and Issue
 module Taskable
+  COMPLETED    = 'completed'.freeze
+  INCOMPLETE   = 'incomplete'.freeze
+  ITEM_PATTERN = /
+    ^
+    (?:\s*[-+*]|(?:\d+\.))? # optional list prefix
+    \s*                     # optional whitespace prefix
+    (\[\s\]|\[[xX]\])       # checkbox
+    (\s.+)                  # followed by whitespace and some text.
+  /x
+
+  def self.get_tasks(content)
+    content.to_s.scan(ITEM_PATTERN).map do |checkbox, label|
+      # ITEM_PATTERN strips out the hyphen, but Item requires it. Rabble rabble.
+      TaskList::Item.new("- #{checkbox}", label.strip)
+    end
+  end
+
+  def self.get_updated_tasks(old_content:, new_content:)
+    old_tasks, new_tasks = get_tasks(old_content), get_tasks(new_content)
+
+    new_tasks.select.with_index do |new_task, i|
+      old_task = old_tasks[i]
+      next unless old_task
+
+      new_task.source == old_task.source && new_task.complete? != old_task.complete?
+    end
+  end
+
   # Called by `TaskList::Summary`
   def task_list_items
     return [] if description.blank?
 
-    @task_list_items ||= description.scan(TaskList::Filter::ItemPattern).collect do |item|
-      # ItemPattern strips out the hyphen, but Item requires it. Rabble rabble.
-      TaskList::Item.new("- #{item}")
-    end
+    @task_list_items ||= Taskable.get_tasks(description)
   end
 
   def tasks

app/services/issuable_base_service.rb

@@ -27,6 +27,12 @@ class IssuableBaseService < BaseService
       old_branch, new_branch)
   end
 
+  def create_task_status_note(issuable)
+    issuable.updated_tasks.each do |task|
+      SystemNoteService.change_task_status(issuable, issuable.project, current_user, task)
+    end
+  end
+
   def filter_params(issuable_ability_name = :issue)
     params[:assignee_id]  = "" if params[:assignee_id] == IssuableFinder::NONE
     params[:milestone_id] = "" if params[:milestone_id] == IssuableFinder::NONE
@@ -47,14 +53,7 @@ class IssuableBaseService < BaseService
 
     if params.present? && issuable.update_attributes(params.merge(updated_by: current_user))
       issuable.reset_events_cache
-
-      if issuable.labels != old_labels
-        create_labels_note(
-          issuable,
-          issuable.labels - old_labels,
-          old_labels - issuable.labels)
-      end
-
+      handle_common_system_notes(issuable, old_labels: old_labels)
       handle_changes(issuable)
       issuable.create_new_cross_references!(current_user)
       execute_hooks(issuable, 'update')
@@ -71,4 +70,19 @@ class IssuableBaseService < BaseService
       close_service.new(project, current_user, {}).execute(issuable)
     end
   end
+
+  def handle_common_system_notes(issuable, options = {})
+    if issuable.previous_changes.include?('title')
+      create_title_change_note(issuable, issuable.previous_changes['title'].first)
+    end
+
+    if issuable.previous_changes.include?('description') && issuable.tasks?
+      create_task_status_note(issuable)
+    end
+
+    old_labels = options[:old_labels]
+    if old_labels && (issuable.labels != old_labels)
+      create_labels_note(issuable, issuable.labels - old_labels, old_labels - issuable.labels)
+    end
+  end
 end

app/services/issues/update_service.rb

@@ -13,10 +13,6 @@ module Issues
         create_assignee_note(issue)
         notification_service.reassigned_issue(issue, current_user)
       end
-
-      if issue.previous_changes.include?('title')
-        create_title_change_note(issue, issue.previous_changes['title'].first)
-      end
     end
 
     def reopen_service

app/services/merge_requests/update_service.rb

@@ -30,10 +30,6 @@ module MergeRequests
         notification_service.reassigned_merge_request(merge_request, current_user)
       end
 
-      if merge_request.previous_changes.include?('title')
-        create_title_change_note(merge_request, merge_request.previous_changes['title'].first)
-      end
-
       if merge_request.previous_changes.include?('target_branch') ||
           merge_request.previous_changes.include?('source_branch')
         merge_request.mark_as_unchecked

app/services/system_note_service.rb

@@ -341,4 +341,22 @@ class SystemNoteService
 
     "* #{commit_ids} - #{commits_text} from branch `#{branch}`\n"
   end
+
+  # Called when the status of a Task has changed
+  #
+  # noteable  - Noteable object.
+  # project   - Project owning noteable
+  # author    - User performing the change
+  # new_task  - TaskList::Item object.
+  #
+  # Example Note text:
+  #
+  #   "Soandso marked the task Whatever as completed."
+  #
+  # Returns the created Note object
+  def self.change_task_status(noteable, project, author, new_task)
+    status_label = new_task.complete? ? Taskable::COMPLETED : Taskable::INCOMPLETE
+    body = "Marked the task **#{new_task.source}** as #{status_label}"
+    create_note(noteable: noteable, project: project, author: author, note: body)
+  end
 end

spec/services/issues/update_service_spec.rb

@@ -15,6 +15,17 @@ describe Issues::UpdateService do
   end
 
   describe 'execute' do
+    def find_note(starting_with)
+      @issue.notes.find do |note|
+        note && note.note.start_with?(starting_with)
+      end
+    end
+
+    def update_issue(opts)
+      @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+      @issue.reload
+    end
+
     context "valid params" do
       before do
         opts = {
@@ -44,12 +55,6 @@ describe Issues::UpdateService do
         expect(email.subject).to include(issue.title)
       end
 
-      def find_note(starting_with)
-        @issue.notes.find do |note|
-          note && note.note.start_with?(starting_with)
-        end
-      end
-
       it 'should create system note about issue reassign' do
         note = find_note('Reassigned to')
 
@@ -71,5 +76,71 @@ describe Issues::UpdateService do
         expect(note.note).to eq 'Title changed from **Old title** to **New title**'
       end
     end
+
+    context 'when Issue has tasks' do
+      before { update_issue({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
+
+      it { expect(@issue.tasks?).to eq(true) }
+
+      context 'when tasks are marked as completed' do
+        before { update_issue({ description: "- [x] Task 1\n- [X] Task 2" }) }
+
+        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
+        end
+      end
+
+      context 'when tasks are marked as incomplete' do
+        before do
+          update_issue({ description: "- [x] Task 1\n- [X] Task 2" })
+          update_issue({ 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
+        end
+      end
+
+      context 'when tasks position has been modified' do
+        before do
+          update_issue({ description: "- [x] Task 1\n- [X] Task 2" })
+          update_issue({ description: "- [x] Task 1\n- [ ] Task 3\n- [ ] Task 2" })
+        end
+
+        it 'does not create a system note' do
+          note = find_note('Marked the task **Task 2** as incomplete')
+
+          expect(note).to be_nil
+        end
+      end
+
+      context 'when a Task list with a completed item is totally replaced' do
+        before do
+          update_issue({ description: "- [ ] Task 1\n- [X] Task 2" })
+          update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" })
+        end
+
+        it 'does not create a system note referencing the position the old item' do
+          note = find_note('Marked the task **Two** as incomplete')
+
+          expect(note).to be_nil
+        end
+
+        it 'should not generate a new note at all' do
+          expect do
+            update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" })
+          end.not_to change { Note.count }
+        end
+      end
+    end
+
   end
 end

spec/services/merge_requests/update_service_spec.rb

@@ -14,6 +14,17 @@ describe MergeRequests::UpdateService do
   end
 
   describe 'execute' do
+    def find_note(starting_with)
+      @merge_request.notes.find do |note|
+        note && note.note.start_with?(starting_with)
+      end
+    end
+
+    def update_merge_request(opts)
+      @merge_request = MergeRequests::UpdateService.new(project, user, opts).execute(merge_request)
+      @merge_request.reload
+    end
+
     context 'valid params' do
       let(:opts) do
         {
@@ -56,12 +67,6 @@ describe MergeRequests::UpdateService do
         expect(email.subject).to include(merge_request.title)
       end
 
-      def find_note(starting_with)
-        @merge_request.notes.find do |note|
-          note && note.note.start_with?(starting_with)
-        end
-      end
-
       it 'should create system note about merge_request reassign' do
         note = find_note('Reassigned to')
 
@@ -90,5 +95,39 @@ describe MergeRequests::UpdateService do
         expect(note.note).to eq 'Target branch changed from `master` to `target`'
       end
     end
+
+    context 'when MergeRequest has tasks' do
+      before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) }
+
+      it { expect(@merge_request.tasks?).to eq(true) }
+
+      context 'when tasks are marked as completed' do
+        before { update_merge_request({ description: "- [x] Task 1\n- [X] Task 2" }) }
+
+        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
+        end
+      end
+
+      context 'when tasks are marked as incomplete' do
+        before do
+          update_merge_request({ description: "- [x] Task 1\n- [X] Task 2" })
+          update_merge_request({ 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
+        end
+      end
+    end
+
   end
 end