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.

4 files changed, 156 insertions, 2 deletions

diff --git a/spec/finders/pending_todos_finder_spec.rb b/spec/finders/pending_todos_finder_spec.rb
new file mode 100644
index 00000000000..32fad5e225f
--- /dev/null
+++ b/spec/finders/pending_todos_finder_spec.rb
@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe PendingTodosFinder do
+  let(:user) { create(:user) }
+
+  describe '#execute' do
+    it 'returns only pending todos' do
+      create(:todo, :done, user: user)
+
+      todo = create(:todo, :pending, user: user)
+      todos = described_class.new(user).execute
+
+      expect(todos).to eq([todo])
+    end
+
+    it 'supports retrieving of todos for a specific project' do
+      project1 = create(:project)
+      project2 = create(:project)
+
+      create(:todo, :pending, user: user, project: project2)
+
+      todo = create(:todo, :pending, user: user, project: project1)
+      todos = described_class.new(user, project_id: project1.id).execute
+
+      expect(todos).to eq([todo])
+    end
+
+    it 'supports retrieving of todos for a specific todo target' do
+      issue = create(:issue)
+      note = create(:note)
+      todo = create(:todo, :pending, user: user, target: issue)
+
+      create(:todo, :pending, user: user, target: note)
+
+      todos = described_class.new(user, target_id: issue.id).execute
+
+      expect(todos).to eq([todo])
+    end
+
+    it 'supports retrieving of todos for a specific target type' do
+      issue = create(:issue)
+      note = create(:note)
+      todo = create(:todo, :pending, user: user, target: issue)
+
+      create(:todo, :pending, user: user, target: note)
+
+      todos = described_class.new(user, target_type: issue.class).execute
+
+      expect(todos).to eq([todo])
+    end
+
+    it 'supports retrieving of todos for a specific commit ID' do
+      create(:todo, :pending, user: user, commit_id: '456')
+
+      todo = create(:todo, :pending, user: user, commit_id: '123')
+      todos = described_class.new(user, commit_id: '123').execute
+
+      expect(todos).to eq([todo])
+    end
+  end
+end
diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb
index 64289224933..d4ed41d54f0 100644
--- a/spec/finders/todos_finder_spec.rb
+++ b/spec/finders/todos_finder_spec.rb
@@ -109,4 +109,20 @@ describe TodosFinder do
       end
     end
   end
+
+  describe '#any_for_target?' do
+    it 'returns true if there are any todos for the given target' do
+      todo = create(:todo, :pending)
+      finder = described_class.new(todo.user)
+
+      expect(finder.any_for_target?(todo.target)).to eq(true)
+    end
+
+    it 'returns false if there are no todos for the given target' do
+      issue = create(:issue)
+      finder = described_class.new(issue.author)
+
+      expect(finder.any_for_target?(issue)).to eq(false)
+    end
+  end
 end
diff --git a/spec/finders/users_with_pending_todos_finder_spec.rb b/spec/finders/users_with_pending_todos_finder_spec.rb
new file mode 100644
index 00000000000..fa15355531c
--- /dev/null
+++ b/spec/finders/users_with_pending_todos_finder_spec.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe UsersWithPendingTodosFinder do
+  describe '#execute' do
+    it 'returns the users for all pending todos of a target' do
+      issue = create(:issue)
+      note = create(:note)
+      todo = create(:todo, :pending, target: issue)
+
+      create(:todo, :pending, target: note)
+
+      users = described_class.new(issue).execute
+
+      expect(users).to eq([todo.user])
+    end
+  end
+end
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index f1a9d81da08..2c01578aaca 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -230,6 +230,26 @@ describe Todo do
     end
   end
 
+  describe '.for_target' do
+    it 'returns the todos for a given target' do
+      todo = create(:todo, target: create(:issue))
+
+      create(:todo, target: create(:merge_request))
+
+      expect(described_class.for_target(todo.target)).to eq([todo])
+    end
+  end
+
+  describe '.for_commit' do
+    it 'returns the todos for a commit ID' do
+      todo = create(:todo, commit_id: '123')
+
+      create(:todo, commit_id: '456')
+
+      expect(described_class.for_commit('123')).to eq([todo])
+    end
+  end
+
   describe '.for_group_and_descendants' do
     it 'returns the todos for a group and its descendants' do
       parent_group = create(:group)
@@ -237,9 +257,45 @@ describe Todo do
 
       todo1 = create(:todo, group: parent_group)
       todo2 = create(:todo, group: child_group)
+      todos = described_class.for_group_and_descendants(parent_group)
+
+      expect(todos).to include(todo1)
+
+      # Nested groups only work on PostgreSQL, so on MySQL todo2 won't be
+      # present.
+      expect(todos).to include(todo2) if Gitlab::Database.postgresql?
+    end
+  end
+
+  describe '.any_for_target?' do
+    it 'returns true if there are todos for a given target' do
+      todo = create(:todo)
+
+      expect(described_class.any_for_target?(todo.target)).to eq(true)
+    end
+
+    it 'returns false if there are no todos for a given target' do
+      issue = create(:issue)
+
+      expect(described_class.any_for_target?(issue)).to eq(false)
+    end
+  end
+
+  describe '.update_state' do
+    it 'updates the state of todos' do
+      todo = create(:todo, :pending)
+      ids = described_class.update_state(:done)
+
+      todo.reload
+
+      expect(ids).to eq([todo.id])
+      expect(todo.state).to eq('done')
+    end
+
+    it 'does not update todos that already have the given state' do
+      create(:todo, :pending)
 
-      expect(described_class.for_group_and_descendants(parent_group))
-        .to include(todo1, todo2)
+      expect(described_class.update_state(:pending)).to be_empty
     end
   end
 end