Clean up ActiveRecord code in TodosFinder

This refactors the TodosFinder finder according to the new code reuse
rules, as enforced by the CodeReuse cops. I also changed some of the
methods to use regular if statements, instead of assignments and/or
early returns. This results in a more natural flow when reading the
code, and it makes it harder to accidentally return the wrong result.

6 files changed, 152 insertions, 54 deletions

diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index 74baf79e4f2..0e37f946d35 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -23,6 +23,8 @@ class TodosFinder
 
   NONE = '0'.freeze
 
+  TODO_TYPES = Set.new(%w(Issue MergeRequest Epic)).freeze
+
   attr_accessor :current_user, :params
 
   def initialize(current_user, params = {})
@@ -72,14 +74,11 @@ class TodosFinder
   end
 
   def author
-    return @author if defined?(@author)
-
-    @author =
+    strong_memoize(:author) do
       if author? && params[:author_id] != NONE
         User.find(params[:author_id])
-      else
-        nil
       end
+    end
   end
 
   def project?
@@ -91,17 +90,9 @@ class TodosFinder
   end
 
   def project
-    return @project if defined?(@project)
-
-    if project?
-      @project = Project.find(params[:project_id])
-
-      @project = nil if @project.pending_delete?
-    else
-      @project = nil
+    strong_memoize(:project) do
+      Project.find_without_deleted(params[:project_id]) if project?
     end
-
-    @project
   end
 
   def group
@@ -111,7 +102,7 @@ class TodosFinder
   end
 
   def type?
-    type.present? && %w(Issue MergeRequest Epic).include?(type)
+    type.present? && TODO_TYPES.include?(type)
   end
 
   def type
@@ -119,77 +110,66 @@ class TodosFinder
   end
 
   def sort(items)
-    params[:sort] ? items.sort_by_attribute(params[:sort]) : items.order_id_desc
+    if params[:sort]
+      items.sort_by_attribute(params[:sort])
+    else
+      items.order_id_desc
+    end
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_action(items)
     if action?
-      items = items.where(action: to_action_id)
+      items.for_action(to_action_id)
+    else
+      items
     end
-
-    items
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_action_id(items)
     if action_id?
-      items = items.where(action: action_id)
+      items.for_action(action_id)
+    else
+      items
     end
-
-    items
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_author(items)
     if author?
-      items = items.where(author_id: author.try(:id))
+      items.for_author(author)
+    else
+      items
     end
-
-    items
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_project(items)
     if project?
-      items = items.where(project: project)
+      items.for_project(project)
+    else
+      items
     end
-
-    items
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_group(items)
-    return items unless group?
-
-    groups = group.self_and_descendants
-    project_todos = items.where(project_id: Project.where(group: groups).select(:id))
-    group_todos = items.where(group_id: groups.select(:id))
-
-    Todo.from_union([project_todos, group_todos])
+    if group?
+      items.for_group_and_descendants(group)
+    else
+      items
+    end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def by_state(items)
-    case params[:state].to_s
-    when 'done'
+    if params[:state].to_s == 'done'
       items.done
     else
       items.pending
     end
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_type(items)
     if type?
-      items = items.where(target_type: type)
+      items.for_type(type)
+    else
+      items
     end
-
-    items
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 end
diff --git a/app/models/project.rb b/app/models/project.rb
index 0cdd876dc20..05e14c578b5 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -386,6 +386,13 @@ class Project < ActiveRecord::Base
                                             only_integer: true,
                                             message: 'needs to be beetween 10 minutes and 1 month' }
 
+  # Returns a project, if it is not about to be removed.
+  #
+  # id - The ID of the project to retrieve.
+  def self.find_without_deleted(id)
+    without_deleted.find_by_id(id)
+  end
+
   # Paginates a collection using a `WHERE id < ?` condition.
   #
   # before - A project ID to use for filtering out projects with an equal or
@@ -450,6 +457,7 @@ class Project < ActiveRecord::Base
 
   scope :joins_import_state, -> { joins("LEFT JOIN project_mirror_data import_state ON import_state.project_id = projects.id") }
   scope :import_started, -> { joins_import_state.where("import_state.status = 'started' OR projects.import_status = 'started'") }
+  scope :for_group, -> (group) { where(group: group) }
 
   class << self
     # Searches for a list of projects based on the query given in `query`.
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 265fb932f7c..d2890f9ca8e 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -40,6 +40,11 @@ class Todo < ActiveRecord::Base
 
   scope :pending, -> { with_state(:pending) }
   scope :done, -> { with_state(:done) }
+  scope :for_action, -> (action) { where(action: action) }
+  scope :for_author, -> (author) { where(author: author) }
+  scope :for_project, -> (project) { where(project: project) }
+  scope :for_group, -> (group) { where(group: group) }
+  scope :for_type, -> (type) { where(target_type: type) }
 
   state_machine :state, initial: :pending do
     event :done do
@@ -53,6 +58,20 @@ class Todo < ActiveRecord::Base
   after_save :keep_around_commit, if: :commit_id
 
   class << self
+    # Returns all todos for the given group and its descendants.
+    #
+    # group - A `Group` to retrieve todos for.
+    #
+    # Returns an `ActiveRecord::Relation`.
+    def for_group_and_descendants(group)
+      groups = group.self_and_descendants
+
+      from_union([
+        for_project(Project.for_group(groups)),
+        for_group(groups)
+      ])
+    end
+
     # Priority sorting isn't displayed in the dropdown, because we don't show
     # milestones, but still show something if the user has a URL with that
     # selected.
diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb
index 7f7cfb2cb98..64289224933 100644
--- a/spec/finders/todos_finder_spec.rb
+++ b/spec/finders/todos_finder_spec.rb
@@ -105,7 +105,6 @@ describe TodosFinder do
 
         todos = finder.new(user, { sort: 'priority' }).execute
 
-        puts todos.to_sql
         expect(todos).to eq([todo_3, todo_5, todo_4, todo_2, todo_1])
       end
     end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 31c69e5bd2c..3fecddefff2 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -4073,6 +4073,29 @@ describe Project do
     end
   end
 
+  describe '.find_without_deleted' do
+    it 'returns nil if the project is about to be removed' do
+      project = create(:project, pending_delete: true)
+
+      expect(described_class.find_without_deleted(project.id)).to be_nil
+    end
+
+    it 'returns a project when it is not about to be removed' do
+      project = create(:project)
+
+      expect(described_class.find_without_deleted(project.id)).to eq(project)
+    end
+  end
+
+  describe '.for_group' do
+    it 'returns the projects for a given group' do
+      group = create(:group)
+      project = create(:project, namespace: group)
+
+      expect(described_class.for_group(group)).to eq([project])
+    end
+  end
+
   def rugged_config
     rugged_repo(project.repository).config
   end
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index f29abcf536e..f1a9d81da08 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -173,4 +173,73 @@ describe Todo do
       expect(subject).not_to be_self_assigned
     end
   end
+
+  describe '.for_action' do
+    it 'returns the todos for a given action' do
+      create(:todo, action: Todo::MENTIONED)
+
+      todo = create(:todo, action: Todo::ASSIGNED)
+
+      expect(described_class.for_action(Todo::ASSIGNED)).to eq([todo])
+    end
+  end
+
+  describe '.for_author' do
+    it 'returns the todos for a given author' do
+      user1 = create(:user)
+      user2 = create(:user)
+      todo = create(:todo, author: user1)
+
+      create(:todo, author: user2)
+
+      expect(described_class.for_author(user1)).to eq([todo])
+    end
+  end
+
+  describe '.for_project' do
+    it 'returns the todos for a given project' do
+      project1 = create(:project)
+      project2 = create(:project)
+      todo = create(:todo, project: project1)
+
+      create(:todo, project: project2)
+
+      expect(described_class.for_project(project1)).to eq([todo])
+    end
+  end
+
+  describe '.for_group' do
+    it 'returns the todos for a given group' do
+      group1 = create(:group)
+      group2 = create(:group)
+      todo = create(:todo, group: group1)
+
+      create(:todo, group: group2)
+
+      expect(described_class.for_group(group1)).to eq([todo])
+    end
+  end
+
+  describe '.for_type' do
+    it 'returns the todos for a given target type' do
+      todo = create(:todo, target: create(:issue))
+
+      create(:todo, target: create(:merge_request))
+
+      expect(described_class.for_type(Issue)).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)
+      child_group = create(:group, parent: parent_group)
+
+      todo1 = create(:todo, group: parent_group)
+      todo2 = create(:todo, group: child_group)
+
+      expect(described_class.for_group_and_descendants(parent_group))
+        .to include(todo1, todo2)
+    end
+  end
 end