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.
This commit is contained in:
Yorick Peterse 2018-09-20 17:05:26 +02:00
parent 4c1dc31051
commit 38b8ae641f
No known key found for this signature in database
GPG key ID: EDD30D2BEB691AC9
10 changed files with 279 additions and 21 deletions

View file

@ -0,0 +1,64 @@
# 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

View file

@ -47,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?

View file

@ -0,0 +1,16 @@
# 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

View file

@ -45,6 +45,8 @@ class Todo < ActiveRecord::Base
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
@ -72,6 +74,28 @@ class Todo < ActiveRecord::Base
])
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.

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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

View file

@ -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(described_class.for_group_and_descendants(parent_group))
.to include(todo1, todo2)
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