Merge branch 'clean-up-todos-finder' into 'master'

Clean up ActiveRecord code in TodosFinder and TodoService

See merge request gitlab-org/gitlab-ce!21838

app/finders/pending_todos_finder.rb

+# frozen_string_literal: true
+
+# Finder for retrieving the pending todos of a user, optionally filtered using
+# various fields.
+#
+# While this finder is a bit more verbose compared to use
+# `where(params.slice(...))`, it allows us to decouple the input parameters from
+# the actual column names. For example, if we ever decide to use separate
+# columns for target types (e.g. `issue_id`, `merge_request_id`, etc), we no
+# longer need to change _everything_ that uses this finder. Instead, we just
+# change the various `by_*` methods in this finder, without having to touch
+# everything that uses it.
+class PendingTodosFinder
+  attr_reader :current_user, :params
+
+  # current_user - The user to retrieve the todos for.
+  # params - A Hash containing columns and values to use for filtering todos.
+  def initialize(current_user, params = {})
+    @current_user = current_user
+    @params = params
+  end
+
+  def execute
+    todos = current_user.todos.pending
+    todos = by_project(todos)
+    todos = by_target_id(todos)
+    todos = by_target_type(todos)
+    todos = by_commit_id(todos)
+
+    todos
+  end
+
+  def by_project(todos)
+    if (id = params[:project_id])
+      todos.for_project(id)
+    else
+      todos
+    end
+  end
+
+  def by_target_id(todos)
+    if (id = params[:target_id])
+      todos.for_target(id)
+    else
+      todos
+    end
+  end
+
+  def by_target_type(todos)
+    if (type = params[:target_type])
+      todos.for_type(type)
+    else
+      todos
+    end
+  end
+
+  def by_commit_id(todos)
+    if (id = params[:commit_id])
+      todos.for_commit(id)
+    else
+      todos
+    end
+  end
+end

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 = {})
@@ -45,6 +47,13 @@ class TodosFinder
     sort(items)
   end
 
+  # Returns `true` if the current user has any todos for the given target.
+  #
+  # target - The value of the `target_type` column, such as `Issue`.
+  def any_for_target?(target)
+    current_user.todos.any_for_target?(target)
+  end
+
   private
 
   def action_id?
@@ -72,13 +81,10 @@ 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
 
@@ -91,17 +97,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 +109,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 +117,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)
-    end
-
+      items.for_action(to_action_id)
+    else
       items
     end
-  # rubocop: enable CodeReuse/ActiveRecord
+  end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_action_id(items)
     if action_id?
-      items = items.where(action: action_id)
-    end
-
+      items.for_action(action_id)
+    else
       items
     end
-  # rubocop: enable CodeReuse/ActiveRecord
+  end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_author(items)
     if author?
-      items = items.where(author_id: author.try(:id))
-    end
-
+      items.for_author(author)
+    else
       items
     end
-  # rubocop: enable CodeReuse/ActiveRecord
+  end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def by_project(items)
     if project?
-      items = items.where(project: project)
-    end
-
+      items.for_project(project)
+    else
       items
     end
-  # rubocop: enable CodeReuse/ActiveRecord
+  end
 
-  # 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)
-    end
-
+      items.for_type(type)
+    else
       items
     end
-  # rubocop: enable CodeReuse/ActiveRecord
+  end
 end

app/finders/users_with_pending_todos_finder.rb

+# frozen_string_literal: true
+
+# Finder that given a target (e.g. an issue) finds all the users that have
+# pending todos for said target.
+class UsersWithPendingTodosFinder
+  attr_reader :target
+
+  # target - The target, such as an Issue or MergeRequest.
+  def initialize(target)
+    @target = target
+  end
+
+  def execute
+    User.for_todos(target.todos.pending)
+  end
+end

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`.

app/models/todo.rb

@@ -40,6 +40,13 @@ 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) }
+  scope :for_target, -> (id) { where(target_id: id) }
+  scope :for_commit, -> (id) { where(commit_id: id) }
 
   state_machine :state, initial: :pending do
     event :done do
@@ -53,6 +60,42 @@ 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
+
+    # Returns `true` if the current user has any todos for the given target.
+    #
+    # target - The value of the `target_type` column, such as `Issue`.
+    def any_for_target?(target)
+      exists?(target: target)
+    end
+
+    # Updates the state of a relation of todos to the new state.
+    #
+    # new_state - The new state 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)
+      ids = base.pluck(:id)
+
+      base.update_all(state: new_state)
+
+      ids
+    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.

app/models/user.rb

@@ -265,6 +265,7 @@ class User < ActiveRecord::Base
   scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) }
   scope :confirmed, -> { where.not(confirmed_at: nil) }
   scope :by_username, -> (usernames) { iwhere(username: usernames) }
+  scope :for_todos, -> (todos) { where(id: todos.select(:user_id)) }
 
   # Limits the users to those that have TODOs, optionally in the given state.
   #
@@ -1366,6 +1367,10 @@ class User < ActiveRecord::Base
     !consented_usage_stats? && 7.days.ago > self.created_at && !has_current_license? && User.single_user?
   end
 
+  def todos_limited_to(ids)
+    todos.where(id: ids)
+  end
+
   # @deprecated
   alias_method :owned_or_masters_groups, :owned_or_maintainers_groups
 

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

spec/finders/pending_todos_finder_spec.rb

+# 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

spec/finders/todos_finder_spec.rb

@@ -105,9 +105,24 @@ 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
   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

spec/finders/users_with_pending_todos_finder_spec.rb

+# 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

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

spec/models/todo_spec.rb

@@ -173,4 +173,129 @@ 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_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)
+      child_group = create(:group, parent: parent_group)
+
+      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.update_state(:pending)).to be_empty
+    end
+  end
 end