diff options
| -rw-r--r-- | app/finders/pending_todos_finder.rb | 64 | ||||
| -rw-r--r-- | app/finders/todos_finder.rb | 7 | ||||
| -rw-r--r-- | app/finders/users_with_pending_todos_finder.rb | 16 | ||||
| -rw-r--r-- | app/models/todo.rb | 24 | ||||
| -rw-r--r-- | app/models/user.rb | 5 | ||||
| -rw-r--r-- | app/services/todo_service.rb | 26 |
6 files changed, 123 insertions, 19 deletions
diff --git a/app/finders/pending_todos_finder.rb b/app/finders/pending_todos_finder.rb new file mode 100644 index 00000000000..c21d90c9182 --- /dev/null +++ b/app/finders/pending_todos_finder.rb @@ -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 diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 0e37f946d35..d001e18fea9 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -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? diff --git a/app/finders/users_with_pending_todos_finder.rb b/app/finders/users_with_pending_todos_finder.rb new file mode 100644 index 00000000000..461bd92a366 --- /dev/null +++ b/app/finders/users_with_pending_todos_finder.rb @@ -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 diff --git a/app/models/todo.rb b/app/models/todo.rb index d2890f9ca8e..7b64615f699 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -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. diff --git a/app/models/user.rb b/app/models/user.rb index eeac87e2e52..37a00d27e30 100644 --- a/app/models/user.rb +++ b/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. # @@ -1364,6 +1365,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 diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 4fe6c1ec986..f357dc37fe7 100644 --- a/app/services/todo_service.rb +++ b/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 |