From 97afb84b310cfdaa9305e8b79fc00bdbb866bbca Mon Sep 17 00:00:00 2001
From: Ruben Davila <rdavila84@gmail.com>
Date: Thu, 22 Oct 2015 10:18:59 -0500
Subject: Generate system note after Task item has been updated on Issue or
 Merge Request. #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.
---
 app/models/concerns/issuable.rb                    |  5 ++
 app/models/concerns/taskable.rb                    | 31 +++++++++--
 app/services/issuable_base_service.rb              | 17 ++++++
 app/services/issues/update_service.rb              |  4 --
 app/services/merge_requests/update_service.rb      |  4 --
 app/services/system_note_service.rb                | 17 ++++++
 config/initializers/task_list_ext.rb               | 12 ++++
 spec/services/issues/update_service_spec.rb        | 64 ++++++++++++++++++++--
 .../services/merge_requests/update_service_spec.rb | 51 +++++++++++++++--
 9 files changed, 181 insertions(+), 24 deletions(-)
 create mode 100644 config/initializers/task_list_ext.rb

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 2dafb5e752f..62e8e66b1e0 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -151,4 +151,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
diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb
index 660e58b876d..3daa4dbe24e 100644
--- a/app/models/concerns/taskable.rb
+++ b/app/models/concerns/taskable.rb
@@ -7,14 +7,37 @@ require 'task_list/filter'
 #
 # Used by MergeRequest and Issue
 module Taskable
+  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 == new_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
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 11d2b08bba7..19055fb67ff 100644
--- a/app/services/issuable_base_service.rb
+++ b/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
@@ -55,6 +61,7 @@ class IssuableBaseService < BaseService
           old_labels - issuable.labels)
       end
 
+      handle_common_system_notes(issuable)
       handle_changes(issuable)
       issuable.create_new_cross_references!(current_user)
       execute_hooks(issuable, 'update')
@@ -71,4 +78,14 @@ class IssuableBaseService < BaseService
       close_service.new(project, current_user, {}).execute(issuable)
     end
   end
+
+  def handle_common_system_notes(issuable)
+    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
+  end
 end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 7c112f731a7..a55a04dd5e0 100644
--- a/app/services/issues/update_service.rb
+++ b/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
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index a5db3776eb6..5ff2cc03dda 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/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
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 708c2f00486..7c5d523ef39 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -341,4 +341,21 @@ 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)
+    body = "Marked the task **#{new_task.source}** as #{new_task.status_label}"
+    create_note(noteable: noteable, project: project, author: author, note: body)
+  end
 end
diff --git a/config/initializers/task_list_ext.rb b/config/initializers/task_list_ext.rb
new file mode 100644
index 00000000000..c05b683b5be
--- /dev/null
+++ b/config/initializers/task_list_ext.rb
@@ -0,0 +1,12 @@
+require 'task_list'
+
+class TaskList
+  class Item
+    COMPLETED = 'completed'.freeze
+    INCOMPLETE = 'incomplete'.freeze
+
+    def status_label
+      complete? ? COMPLETED : INCOMPLETE
+    end
+  end
+end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index f55527ee9a3..adb3aa143ae 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/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,52 @@ 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
+    end
+
   end
 end
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index 2ed51d223b7..97f5c009aec 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/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
-- 
cgit v1.2.1


From fc18e96db38f5d5ab5e102fe630682f6779203ba Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= <ruben@gitlab.com>
Date: Fri, 20 Nov 2015 10:36:12 -0500
Subject: Refactor creation of system notes for Issue/MR labels. #2296

---
 app/services/issuable_base_service.rb | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 19055fb67ff..2556f06e2d3 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -53,15 +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)
+      handle_common_system_notes(issuable, old_labels: old_labels)
       handle_changes(issuable)
       issuable.create_new_cross_references!(current_user)
       execute_hooks(issuable, 'update')
@@ -79,7 +71,7 @@ class IssuableBaseService < BaseService
     end
   end
 
-  def handle_common_system_notes(issuable)
+  def handle_common_system_notes(issuable, options = {})
     if issuable.previous_changes.include?('title')
       create_title_change_note(issuable, issuable.previous_changes['title'].first)
     end
@@ -87,5 +79,10 @@ class IssuableBaseService < BaseService
     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
-- 
cgit v1.2.1


From fa9f2dec0e07ff3ae3a2acd6ee0586e317bdb7b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= <ruben@gitlab.com>
Date: Fri, 20 Nov 2015 10:49:12 -0500
Subject: Monkey patching TaskList::Item is no longer required. #2296

---
 app/models/concerns/taskable.rb      |  2 ++
 app/services/system_note_service.rb  |  3 ++-
 config/initializers/task_list_ext.rb | 12 ------------
 3 files changed, 4 insertions(+), 13 deletions(-)
 delete mode 100644 config/initializers/task_list_ext.rb

diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb
index 3daa4dbe24e..de7588fea86 100644
--- a/app/models/concerns/taskable.rb
+++ b/app/models/concerns/taskable.rb
@@ -7,6 +7,8 @@ require 'task_list/filter'
 #
 # Used by MergeRequest and Issue
 module Taskable
+  COMPLETED    = 'completed'.freeze
+  INCOMPLETE   = 'incomplete'.freeze
   ITEM_PATTERN = /
     ^
     (?:\s*[-+*]|(?:\d+\.))? # optional list prefix
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 7c5d523ef39..7e2bc834176 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -355,7 +355,8 @@ class SystemNoteService
   #
   # Returns the created Note object
   def self.change_task_status(noteable, project, author, new_task)
-    body = "Marked the task **#{new_task.source}** as #{new_task.status_label}"
+    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
diff --git a/config/initializers/task_list_ext.rb b/config/initializers/task_list_ext.rb
deleted file mode 100644
index c05b683b5be..00000000000
--- a/config/initializers/task_list_ext.rb
+++ /dev/null
@@ -1,12 +0,0 @@
-require 'task_list'
-
-class TaskList
-  class Item
-    COMPLETED = 'completed'.freeze
-    INCOMPLETE = 'incomplete'.freeze
-
-    def status_label
-      complete? ? COMPLETED : INCOMPLETE
-    end
-  end
-end
-- 
cgit v1.2.1


From 3aabed3456506d1a917e6daba29cd46ce6a25dab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= <ruben@gitlab.com>
Date: Fri, 20 Nov 2015 13:58:45 -0500
Subject: Fix bug that happened when replacing the Task list. #2296

REF: https://gitlab.com/gitlab-org/gitlab-ce/issues/2296#note_2724697
---
 app/models/concerns/taskable.rb             |  2 +-
 spec/services/issues/update_service_spec.rb | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb
index de7588fea86..df2a9e3e84b 100644
--- a/app/models/concerns/taskable.rb
+++ b/app/models/concerns/taskable.rb
@@ -31,7 +31,7 @@ module Taskable
       old_task = old_tasks[i]
       next unless old_task
 
-      new_task.source == new_task.source && new_task.complete? != old_task.complete?
+      new_task.source == old_task.source && new_task.complete? != old_task.complete?
     end
   end
 
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index adb3aa143ae..bc6a26416a2 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -121,6 +121,25 @@ describe Issues::UpdateService do
           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 {
+            update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" })
+          }.not_to change { Note.count }
+        end
+      end
     end
 
   end
-- 
cgit v1.2.1


From 976cebe45681766764509c3b4df630ced6cc6e03 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= <ruben@gitlab.com>
Date: Fri, 20 Nov 2015 14:27:31 -0500
Subject: Little fix for Rubocop's complaints. #2296

---
 spec/services/issues/update_service_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index bc6a26416a2..cc3e6483261 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -135,9 +135,9 @@ describe Issues::UpdateService do
         end
 
         it 'should not generate a new note at all' do
-          expect {
+          expect do
             update_issue({ description: "- [ ] One\n- [ ] Two\n- [ ] Three" })
-          }.not_to change { Note.count }
+          end.not_to change { Note.count }
         end
       end
     end
-- 
cgit v1.2.1