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.

3 files changed, 60 insertions, 53 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.