Merge branch '216045-capture-todo-resolution' into 'master'

Add resolution mechanism for todos

Closes #216045

See merge request gitlab-org/gitlab!32753

app/controllers/dashboard/todos_controller.rb

@@ -17,7 +17,9 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def destroy
-    TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
+    todo = current_user.todos.find(params[:id])
+
+    TodoService.new.resolve_todo(todo, current_user, resolved_by_action: :mark_done)
 
     respond_to do |format|
       format.html do
@@ -31,7 +33,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def destroy_all
-    updated_ids = TodoService.new.mark_todos_as_done(@todos, current_user)
+    updated_ids = TodoService.new.resolve_todos(@todos, current_user, resolved_by_action: :mark_all_done)
 
     respond_to do |format|
       format.html { redirect_to dashboard_todos_path, status: :found, notice: _('Everything on your to-do list is marked as done.') }
@@ -41,13 +43,13 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def restore
-    TodoService.new.mark_todos_as_pending_by_ids(params[:id], current_user)
+    TodoService.new.restore_todo(current_user.todos.find(params[:id]), current_user)
 
     render json: todos_counts
   end
 
   def bulk_restore
-    TodoService.new.mark_todos_as_pending_by_ids(params[:ids], current_user)
+    TodoService.new.restore_todos(current_user.todos.for_ids(params[:ids]), current_user)
 
     render json: todos_counts
   end

app/graphql/mutations/todos/mark_all_done.rb

@@ -28,7 +28,9 @@ module Mutations
       def mark_all_todos_done
         return [] unless current_user
 
-        TodoService.new.mark_all_todos_as_done_by_user(current_user)
+        todos = TodosFinder.new(current_user).execute
+
+        TodoService.new.resolve_todos(todos, current_user, resolved_by_action: :api_all_done)
       end
     end
   end

app/graphql/mutations/todos/mark_done.rb

@@ -30,7 +30,7 @@ module Mutations
       private
 
       def mark_done(todo)
-        TodoService.new.mark_todo_as_done(todo, current_user)
+        TodoService.new.resolve_todo(todo, current_user, resolved_by_action: :api_done)
       end
     end
   end

app/graphql/mutations/todos/restore.rb

@@ -18,7 +18,7 @@ module Mutations
 
       def resolve(id:)
         todo = authorized_find!(id: id)
-        restore(todo.id) if todo.done?
+        restore(todo)
 
         {
           todo: todo.reset,
@@ -28,8 +28,8 @@ module Mutations
 
       private
 
-      def restore(id)
-        TodoService.new.mark_todos_as_pending_by_ids([id], current_user)
+      def restore(todo)
+        TodoService.new.restore_todo(todo, current_user)
       end
     end
   end

app/graphql/mutations/todos/restore_many.rb

@@ -68,7 +68,7 @@ module Mutations
       end
 
       def restore(todos)
-        TodoService.new.mark_todos_as_pending(todos, current_user)
+        TodoService.new.restore_todos(todos, current_user)
       end
     end
   end

app/models/todo.rb

@@ -66,6 +66,8 @@ class Todo < ApplicationRecord
   scope :with_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: :route }]) }
   scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) }
 
+  enum resolved_by_action: { system_done: 0, api_all_done: 1, api_done: 2, mark_all_done: 3, mark_done: 4 }, _prefix: :resolved_by
+
   state_machine :state, initial: :pending do
     event :done do
       transition [:pending] => :done
@@ -100,17 +102,17 @@ class Todo < ApplicationRecord
       state.nil? ? exists?(target: target) : exists?(target: target, state: state)
     end
 
-    # Updates the state of a relation of todos to the new state.
+    # Updates attributes of a relation of todos to the new state.
     #
-    # new_state - The new state of the todos.
+    # new_attributes - The new attributes of the todos.
     #
     # Returns an `Array` containing the IDs of the updated todos.
-    def update_state(new_state)
-      # Only update those that are not really on that state
-      base = where.not(state: new_state).except(:order)
+    def batch_update(**new_attributes)
+      # Only update those that have different state
+      base = where.not(state: new_attributes[:state]).except(:order)
       ids = base.pluck(:id)
 
-      base.update_all(state: new_state, updated_at: Time.current)
+      base.update_all(new_attributes.merge(updated_at: Time.current))
 
       ids
     end

app/models/user.rb

@@ -1636,10 +1636,6 @@ class User < ApplicationRecord
     super.presence || build_user_detail
   end
 
-  def todos_limited_to(ids)
-    todos.where(id: ids)
-  end
-
   def pending_todo_for(target)
     todos.find_by(target: target, state: :pending)
   end

app/services/issuable_base_service.rb

@@ -350,7 +350,7 @@ class IssuableBaseService < BaseService
       todo_service.mark_todo(issuable, current_user)
     when 'done'
       todo = TodosFinder.new(current_user).find_by(target: issuable)
-      todo_service.mark_todos_as_done_by_ids(todo, current_user) if todo
+      todo_service.resolve_todo(todo, current_user) if todo
     end
   end
   # rubocop: enable CodeReuse/ActiveRecord

app/services/issues/update_service.rb

@@ -32,7 +32,7 @@ module Issues
       old_assignees = old_associations.fetch(:assignees, [])
 
       if has_changes?(issue, old_labels: old_labels, old_assignees: old_assignees)
-        todo_service.mark_pending_todos_as_done(issue, current_user)
+        todo_service.resolve_todos_for_target(issue, current_user)
       end
 
       if issue.previous_changes.include?('title') ||
@@ -68,7 +68,7 @@ module Issues
     end
 
     def handle_task_changes(issuable)
-      todo_service.mark_pending_todos_as_done(issuable, current_user)
+      todo_service.resolve_todos_for_target(issuable, current_user)
       todo_service.update_issue(issuable, current_user)
     end
 

app/services/merge_requests/update_service.rb

@@ -27,7 +27,7 @@ module MergeRequests
       old_assignees = old_associations.fetch(:assignees, [])
 
       if has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees)
-        todo_service.mark_pending_todos_as_done(merge_request, current_user)
+        todo_service.resolve_todos_for_target(merge_request, current_user)
       end
 
       if merge_request.previous_changes.include?('title') ||
@@ -73,7 +73,7 @@ module MergeRequests
     end
 
     def handle_task_changes(merge_request)
-      todo_service.mark_pending_todos_as_done(merge_request, current_user)
+      todo_service.resolve_todos_for_target(merge_request, current_user)
       todo_service.update_merge_request(merge_request, current_user)
     end
 

app/services/todo_service.rb

@@ -30,7 +30,7 @@ class TodoService
   #  * mark all pending todos related to the target for the current user as done
   #
   def close_issue(issue, current_user)
-    mark_pending_todos_as_done(issue, current_user)
+    resolve_todos_for_target(issue, current_user)
   end
 
   # When we destroy a todo target we should:
@@ -79,7 +79,7 @@ class TodoService
   #  * mark all pending todos related to the target for the current user as done
   #
   def close_merge_request(merge_request, current_user)
-    mark_pending_todos_as_done(merge_request, current_user)
+    resolve_todos_for_target(merge_request, current_user)
   end
 
   # When merge a merge request we should:
@@ -87,7 +87,7 @@ class TodoService
   #  * mark all pending todos related to the target for the current user as done
   #
   def merge_merge_request(merge_request, current_user)
-    mark_pending_todos_as_done(merge_request, current_user)
+    resolve_todos_for_target(merge_request, current_user)
   end
 
   # When a build fails on the HEAD of a merge request we should:
@@ -105,7 +105,7 @@ class TodoService
   #  * mark all pending todos related to the merge request for that user as done
   #
   def merge_request_push(merge_request, current_user)
-    mark_pending_todos_as_done(merge_request, current_user)
+    resolve_todos_for_target(merge_request, current_user)
   end
 
   # When a build is retried to a merge request we should:
@@ -114,7 +114,7 @@ class TodoService
   #
   def merge_request_build_retried(merge_request)
     merge_request.merge_participants.each do |user|
-      mark_pending_todos_as_done(merge_request, user)
+      resolve_todos_for_target(merge_request, user)
     end
   end
 
@@ -151,76 +151,60 @@ class TodoService
   #  * mark all pending todos related to the awardable for the current user as done
   #
   def new_award_emoji(awardable, current_user)
-    mark_pending_todos_as_done(awardable, current_user)
+    resolve_todos_for_target(awardable, current_user)
   end
 
-  # When marking pending todos as done we should:
-  #
-  #  * mark all pending todos related to the target for the current user as done
-  #
-  def mark_pending_todos_as_done(target, user)
-    attributes = attributes_for_target(target)
-    pending_todos(user, attributes).update_all(state: :done)
-    user.update_todos_count_cache
+  # When user marks an issue as todo
+  def mark_todo(issuable, current_user)
+    attributes = attributes_for_todo(issuable.project, issuable, current_user, Todo::MARKED)
+    create_todos(current_user, attributes)
   end
 
-  # When user marks some todos as done
-  def mark_todos_as_done(todos, current_user)
-    update_todos_state(todos, current_user, :done)
+  def todo_exist?(issuable, current_user)
+    TodosFinder.new(current_user).any_for_target?(issuable, :pending)
   end
 
-  def mark_todos_as_done_by_ids(ids, current_user)
-    todos = todos_by_ids(ids, current_user)
-    mark_todos_as_done(todos, current_user)
-  end
+  # Resolves all todos related to target
+  def resolve_todos_for_target(target, current_user)
+    attributes = attributes_for_target(target)
 
-  def mark_all_todos_as_done_by_user(current_user)
-    todos = TodosFinder.new(current_user).execute
-    mark_todos_as_done(todos, current_user)
+    resolve_todos(pending_todos(current_user, attributes), current_user)
   end
 
-  def mark_todo_as_done(todo, current_user)
-    return if todo.done?
-
-    todo.update(state: :done)
+  def resolve_todos(todos, current_user, resolution: :done, resolved_by_action: :system_done)
+    todos_ids = todos.batch_update(state: resolution, resolved_by_action: resolved_by_action)
 
     current_user.update_todos_count_cache
-  end
 
-  # When user marks some todos as pending
-  def mark_todos_as_pending(todos, current_user)
-    update_todos_state(todos, current_user, :pending)
+    todos_ids
   end
 
-  def mark_todos_as_pending_by_ids(ids, current_user)
-    todos = todos_by_ids(ids, current_user)
-    mark_todos_as_pending(todos, current_user)
-  end
+  def resolve_todo(todo, current_user, resolution: :done, resolved_by_action: :system_done)
+    return if todo.done?
 
-  # When user marks an issue as todo
-  def mark_todo(issuable, current_user)
-    attributes = attributes_for_todo(issuable.project, issuable, current_user, Todo::MARKED)
-    create_todos(current_user, attributes)
-  end
+    todo.update(state: resolution, resolved_by_action: resolved_by_action)
 
-  def todo_exist?(issuable, current_user)
-    TodosFinder.new(current_user).any_for_target?(issuable, :pending)
+    current_user.update_todos_count_cache
   end
 
-  private
+  def restore_todos(todos, current_user)
+    todos_ids = todos.batch_update(state: :pending)
 
-  def todos_by_ids(ids, current_user)
-    current_user.todos_limited_to(Array(ids))
+    current_user.update_todos_count_cache
+
+    todos_ids
   end
 
-  def update_todos_state(todos, current_user, state)
-    todos_ids = todos.update_state(state)
+  def restore_todo(todo, current_user)
+    return if todo.pending?
 
-    current_user.update_todos_count_cache
+    todo.update(state: :pending)
 
-    todos_ids
+    current_user.update_todos_count_cache
   end
 
+  private
+
   def create_todos(users, attributes)
     Array(users).map do |user|
       next if pending_todos(user, attributes).exists?
@@ -252,9 +236,9 @@ class TodoService
     return unless note.can_create_todo?
 
     project = note.project
-    target  = note.noteable
+    target = note.noteable
 
-    mark_pending_todos_as_done(target, author)
+    resolve_todos_for_target(target, author)
     create_mention_todos(project, target, author, note, skip_users)
   end
 

changelogs/unreleased/216045-capture-todo-resolution.yml

+---
+title: Store Todo resolution method
+merge_request: 32753
+author:
+type: added

db/migrate/20200520103514_add_todo_resolved_by_action.rb

+# frozen_string_literal: true
+
+class AddTodoResolvedByAction < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :todos, :resolved_by_action, :integer, limit: 2
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :todos, :resolved_by_action
+    end
+  end
+end

db/structure.sql

@@ -6520,7 +6520,8 @@ CREATE TABLE public.todos (
     updated_at timestamp without time zone,
     note_id integer,
     commit_id character varying,
-    group_id integer
+    group_id integer,
+    resolved_by_action smallint
 );
 
 CREATE SEQUENCE public.todos_id_seq
@@ -13949,6 +13950,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200519115908
 20200519171058
 20200519194042
+20200520103514
 20200525114553
 20200525121014
 20200526120714

ee/app/services/epics/update_service.rb

@@ -31,14 +31,14 @@ module Epics
       old_labels = old_associations.fetch(:labels, [])
 
       if has_changes?(epic, old_labels: old_labels)
-        todo_service.mark_pending_todos_as_done(epic, current_user)
+        todo_service.resolve_todos_for_target(epic, current_user)
       end
 
       todo_service.update_epic(epic, current_user, old_mentioned_users)
     end
 
     def handle_task_changes(epic)
-      todo_service.mark_pending_todos_as_done(epic, current_user)
+      todo_service.resolve_todos_for_target(epic, current_user)
       todo_service.update_epic(epic, current_user)
     end
 

ee/app/services/merge_requests/approval_service.rb

@@ -44,7 +44,7 @@ module MergeRequests
     end
 
     def mark_pending_todos_as_done(merge_request)
-      todo_service.mark_pending_todos_as_done(merge_request, current_user)
+      todo_service.resolve_todos_for_target(merge_request, current_user)
     end
 
     def calculate_approvals_metrics(merge_request)

lib/api/todos.rb

@@ -89,16 +89,18 @@ module API
         requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
       end
       post ':id/mark_as_done' do
-        TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
         todo = current_user.todos.find(params[:id])
 
+        TodoService.new.resolve_todo(todo, current_user, resolved_by_action: :api_done)
+
         present todo, with: Entities::Todo, current_user: current_user
       end
 
       desc 'Mark all todos as done'
       post '/mark_as_done' do
         todos = find_todos
-        TodoService.new.mark_todos_as_done(todos, current_user)
+
+        TodoService.new.resolve_todos(todos, current_user, resolved_by_action: :api_all_done)
 
         no_content!
       end

spec/features/dashboard/todos/todos_spec.rb

@@ -114,7 +114,7 @@ describe 'Dashboard Todos' do
     context 'todo is stale on the page' do
       before do
         todos = TodosFinder.new(user, state: :pending).execute
-        TodoService.new.mark_todos_as_done(todos, user)
+        TodoService.new.resolve_todos(todos, user)
       end
 
       it_behaves_like 'deleting the todo'

spec/models/todo_spec.rb

@@ -393,10 +393,10 @@ describe Todo do
     end
   end
 
-  describe '.update_state' do
+  describe '.batch_update' do
     it 'updates the state of todos' do
       todo = create(:todo, :pending)
-      ids = described_class.update_state(:done)
+      ids = described_class.batch_update(state: :done)
 
       todo.reload
 
@@ -407,7 +407,7 @@ describe Todo do
     it 'does not update todos that already have the given state' do
       create(:todo, :pending)
 
-      expect(described_class.update_state(:pending)).to be_empty
+      expect(described_class.batch_update(state: :pending)).to be_empty
     end
 
     it 'updates updated_at' do
@@ -416,7 +416,7 @@ describe Todo do
       Timecop.freeze(1.day.from_now) do
         expected_update_date = Time.current.utc
 
-        ids = described_class.update_state(:done)
+        ids = described_class.batch_update(state: :done)
 
         expect(Todo.where(id: ids).map(&:updated_at)).to all(be_like_time(expected_update_date))
       end