diff options
| -rw-r--r-- | app/finders/pending_todos_finder.rb | 64 | ||||
| -rw-r--r-- | app/finders/todos_finder.rb | 93 | ||||
| -rw-r--r-- | app/finders/users_with_pending_todos_finder.rb | 16 | ||||
| -rw-r--r-- | app/models/project.rb | 8 | ||||
| -rw-r--r-- | app/models/todo.rb | 43 | ||||
| -rw-r--r-- | app/models/user.rb | 5 | ||||
| -rw-r--r-- | app/services/todo_service.rb | 26 | ||||
| -rw-r--r-- | spec/finders/pending_todos_finder_spec.rb | 63 | ||||
| -rw-r--r-- | spec/finders/todos_finder_spec.rb | 17 | ||||
| -rw-r--r-- | spec/finders/users_with_pending_todos_finder_spec.rb | 19 | ||||
| -rw-r--r-- | spec/models/project_spec.rb | 23 | ||||
| -rw-r--r-- | spec/models/todo_spec.rb | 125 |
12 files changed, 429 insertions, 73 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 74baf79e4f2..d001e18fea9 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 = {}) @@ -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,14 +81,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 +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) + 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/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/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..7b64615f699 100644 --- a/app/models/todo.rb +++ b/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. diff --git a/app/models/user.rb b/app/models/user.rb index cd3b1c95b7e..8a7acfb73b1 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. # @@ -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 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 diff --git a/spec/finders/pending_todos_finder_spec.rb b/spec/finders/pending_todos_finder_spec.rb new file mode 100644 index 00000000000..32fad5e225f --- /dev/null +++ b/spec/finders/pending_todos_finder_spec.rb @@ -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 diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb index 7f7cfb2cb98..d4ed41d54f0 100644 --- a/spec/finders/todos_finder_spec.rb +++ b/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 diff --git a/spec/finders/users_with_pending_todos_finder_spec.rb b/spec/finders/users_with_pending_todos_finder_spec.rb new file mode 100644 index 00000000000..fa15355531c --- /dev/null +++ b/spec/finders/users_with_pending_todos_finder_spec.rb @@ -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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 31c69e5bd2c..3fecddefff2 100644 --- a/spec/models/project_spec.rb +++ b/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 diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb index f29abcf536e..2c01578aaca 100644 --- a/spec/models/todo_spec.rb +++ b/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 |