Commit 296b1d42 authored by Yorick Peterse's avatar Yorick Peterse
Browse files

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.
parent 4aa098f87662
......@@ -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 @@ def author?
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 @@ def group?
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 @@ def group
end
def type?
type.present? && %w(Issue MergeRequest Epic).include?(type)
type.present? && TODO_TYPES.include?(type)
end
def type
......@@ -119,77 +110,66 @@ def type
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
......@@ -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 @@ def self.with_feature_available_for_user(feature, user)
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`.
......
......@@ -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.
......
......@@ -105,7 +105,6 @@
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
......
......@@ -4073,6 +4073,29 @@ def domain_variable
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
......
......@@ -173,4 +173,73 @@
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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment