Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ce

8 files changed, 196 insertions, 32 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 98ad5e76da7..68138688aab 100644
--- a/app/models/concerns/issuable.rb
+++ b/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
diff --git a/app/models/concerns/taskable.rb b/app/models/concerns/taskable.rb
index 660e58b876d..df2a9e3e84b 100644
--- a/app/models/concerns/taskable.rb
+++ b/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
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 11d2b08bba7..2556f06e2d3 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
@@ -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
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..7e2bc834176 100644
--- a/app/services/system_note_service.rb
+++ b/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
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index f55527ee9a3..cc3e6483261 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,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
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