Clean up ActiveRecord code in TodoService

This refactors the TodoService class according to our code reuse
guidelines. The resulting code is a wee bit more verbose, but it allows
us to decouple the column names from the input, resulting in fewer
changes being necessary when we change the schema.

One particular noteworthy line in TodoService is the following:

    todos_ids = todos.update_state(state)

Technically this is a violation of the guidelines, because
`update_state` is a class method, which services are not supposed to use
(safe for a few allowed ones). I decided to keep this, since there is no
alternative. `update_state` doesn't produce a relation so it doesn't
belong in a Finder, and we can't move it to another Service either. As
such I opted to just use the method directly.

Cases like this may happen more frequently, at which point we should
update our documentation with some sort of recommendation. For now, I
want to refrain from doing so until we have a few more examples.

1 files changed, 7 insertions, 19 deletions

diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 4fe6c1ec986..f357dc37fe7 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -41,15 +41,13 @@ class TodoService
   # collects the todo users before the todos themselves are deleted, then
   # updates the todo counts for those users.
   #
-  # rubocop: disable CodeReuse/ActiveRecord
   def destroy_target(target)
-    todo_users = User.where(id: target.todos.pending.select(:user_id)).to_a
+    todo_users = UsersWithPendingTodosFinder.new(target).execute.to_a
 
     yield target
 
     todo_users.each(&:update_todos_count_cache)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   # When we reassign an issue we should:
   #
@@ -200,30 +198,23 @@ class TodoService
     create_todos(current_user, attributes)
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def todo_exist?(issuable, current_user)
-    TodosFinder.new(current_user).execute.exists?(target: issuable)
+    TodosFinder.new(current_user).any_for_target?(issuable)
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def todos_by_ids(ids, current_user)
-    current_user.todos.where(id: Array(ids))
+    current_user.todos_limited_to(Array(ids))
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def update_todos_state(todos, current_user, state)
-    # Only update those that are not really on that state
-    todos = todos.where.not(state: state)
-    todos_ids = todos.pluck(:id)
-    todos.unscope(:order).update_all(state: state)
+    todos_ids = todos.update_state(state)
+
     current_user.update_todos_count_cache
+
     todos_ids
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def create_todos(users, attributes)
     Array(users).map do |user|
@@ -348,10 +339,7 @@ class TodoService
     end
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def pending_todos(user, criteria = {})
-    valid_keys = [:project_id, :target_id, :target_type, :commit_id]
-    user.todos.pending.where(criteria.slice(*valid_keys))
+    PendingTodosFinder.new(user, criteria).execute
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 end