diff options
-rw-r--r-- | app/controllers/concerns/todos_actions.rb | 13 | ||||
-rw-r--r-- | app/controllers/dashboard/todos_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/projects/todos_controller.rb | 13 | ||||
-rw-r--r-- | app/finders/todos_finder.rb | 21 | ||||
-rw-r--r-- | app/models/concerns/issuable.rb | 6 | ||||
-rw-r--r-- | app/models/issue.rb | 4 | ||||
-rw-r--r-- | app/models/note.rb | 4 | ||||
-rw-r--r-- | app/models/todo.rb | 4 | ||||
-rw-r--r-- | app/services/todo_service.rb | 4 | ||||
-rw-r--r-- | db/migrate/20180608091413_add_group_to_todos.rb | 2 | ||||
-rw-r--r-- | doc/api/todos.md | 1 | ||||
-rw-r--r-- | lib/api/entities.rb | 10 | ||||
-rw-r--r-- | spec/controllers/projects/todos_controller_spec.rb | 133 | ||||
-rw-r--r-- | spec/factories/todos.rb | 1 | ||||
-rw-r--r-- | spec/finders/todos_finder_spec.rb | 13 | ||||
-rw-r--r-- | spec/support/shared_examples/controllers/todos_shared_examples.rb | 43 |
16 files changed, 126 insertions, 148 deletions
diff --git a/app/controllers/concerns/todos_actions.rb b/app/controllers/concerns/todos_actions.rb new file mode 100644 index 00000000000..7e5a12a11f4 --- /dev/null +++ b/app/controllers/concerns/todos_actions.rb @@ -0,0 +1,13 @@ +module TodosActions + include Gitlab::Utils::StrongMemoize + extend ActiveSupport::Concern + + def create + todo = TodoService.new.mark_todo(issuable, current_user) + + render json: { + count: TodosFinder.new(current_user, state: :pending).execute.count, + delete_path: dashboard_todo_path(todo) + } + end +end diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index f9e8fe624e8..bd7111e28bc 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -70,7 +70,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController end def todo_params - params.permit(:action_id, :author_id, :project_id, :type, :sort, :state) + params.permit(:action_id, :author_id, :project_id, :type, :sort, :state, :group_id) end def redirect_out_of_range(todos) diff --git a/app/controllers/projects/todos_controller.rb b/app/controllers/projects/todos_controller.rb index a41fcb85c40..248fb8a4381 100644 --- a/app/controllers/projects/todos_controller.rb +++ b/app/controllers/projects/todos_controller.rb @@ -1,19 +1,12 @@ class Projects::TodosController < Projects::ApplicationController - before_action :authenticate_user!, only: [:create] - - def create - todo = TodoService.new.mark_todo(issuable, current_user) + include TodosActions - render json: { - count: TodosFinder.new(current_user, state: :pending).execute.count, - delete_path: dashboard_todo_path(todo) - } - end + before_action :authenticate_user!, only: [:create] private def issuable - @issuable ||= begin + strong_memoize(:issuable) do case params[:issuable_type] when "issue" IssuesFinder.new(current_user, project_id: @project.id).find(params[:issuable_id]) diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 1dfcf19b78d..ea1420712a7 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -113,16 +113,6 @@ class TodosFinder end end - def project_ids(items) - ids = items.except(:order).select(:project_id) - if Gitlab::Database.mysql? - # To make UPDATE work on MySQL, wrap it in a SELECT with an alias - ids = Todo.except(:order).select('*').from("(#{ids.to_sql}) AS t") - end - - ids - end - def type? type.present? && %w(Issue MergeRequest Epic).include?(type) end @@ -169,7 +159,12 @@ class TodosFinder def by_group(items) if group? - items = items.where(group: group) + groups = group.self_and_descendants + items = items.where( + 'project_id IN (?) OR group_id IN (?)', + Project.where(group: groups).select(:id), + groups.select(:id) + ) end items @@ -184,8 +179,8 @@ class TodosFinder .joins('LEFT JOIN projects ON projects.id = todos.project_id') .where( 'project_id IN (?) OR group_id IN (?)', - projects.map(&:id), - groups.map(&:id) + projects.select(:id), + groups.select(:id) ) end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index b93c1145f82..7a459078151 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -243,6 +243,12 @@ module Issuable opened? end + def overdue? + return false unless respond_to?(:due_date) + + due_date.try(:past?) || false + end + def user_notes_count if notes.loaded? # Use the in-memory association to select and count to avoid hitting the db diff --git a/app/models/issue.rb b/app/models/issue.rb index 4715d942c8d..983684a5e05 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -275,10 +275,6 @@ class Issue < ActiveRecord::Base user ? readable_by?(user) : publicly_visible? end - def overdue? - due_date.try(:past?) || false - end - def check_for_spam? project.public? && (title_changed? || description_changed?) end diff --git a/app/models/note.rb b/app/models/note.rb index abc40d9016e..9191cae6391 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -229,6 +229,10 @@ class Note < ActiveRecord::Base !for_personal_snippet? end + def for_issuable_with_ability? + for_issue? || for_merge_request? + end + def skip_project_check? !for_project_noteable? end diff --git a/app/models/todo.rb b/app/models/todo.rb index 5ce77d5ddc2..61158285ea9 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -32,8 +32,8 @@ class Todo < ActiveRecord::Base validates :author, presence: true validates :target_id, presence: true, unless: :for_commit? validates :commit_id, presence: true, if: :for_commit? - validates :project, presence: true, unless: :group - validates :group, presence: true, unless: :project + validates :project, presence: true, unless: :group_id + validates :group, presence: true, unless: :project_id scope :pending, -> { with_state(:pending) } scope :done, -> { with_state(:done) } diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index f355d6b8ea1..5a2460a0cf5 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -285,6 +285,7 @@ class TodoService def attributes_for_target(target) attributes = { project_id: target&.project&.id, + group_id: target.respond_to?(:group) ? target.group_id : nil, target_id: target.id, target_type: target.class.name, commit_id: nil @@ -300,7 +301,6 @@ class TodoService def attributes_for_todo(project, target, author, action, note = nil) attributes_for_target(target).merge!( project_id: project&.id, - group_id: target.respond_to?(:group) ? target.group.id : nil, author_id: author.id, action: action, note: note @@ -322,7 +322,7 @@ class TodoService end def reject_users_without_access(users, parent, target) - if target.is_a?(Note) && (target.for_issue? || target.for_merge_request? || target.for_epic?) + if target.is_a?(Note) && target.for_issuable_with_ability? target = target.noteable end diff --git a/db/migrate/20180608091413_add_group_to_todos.rb b/db/migrate/20180608091413_add_group_to_todos.rb index ca08de835a1..af3ee48b29d 100644 --- a/db/migrate/20180608091413_add_group_to_todos.rb +++ b/db/migrate/20180608091413_add_group_to_todos.rb @@ -7,7 +7,7 @@ class AddGroupToTodos < ActiveRecord::Migration def up add_column :todos, :group_id, :integer - add_foreign_key :todos, :namespaces, column: :group_id, on_delete: :cascade + add_concurrent_foreign_key :todos, :namespaces, column: :group_id, on_delete: :cascade add_concurrent_index :todos, :group_id change_column_null :todos, :project_id, true diff --git a/doc/api/todos.md b/doc/api/todos.md index 27e623007cc..0843e4eedc6 100644 --- a/doc/api/todos.md +++ b/doc/api/todos.md @@ -18,6 +18,7 @@ Parameters: | `action` | string | no | The action to be filtered. Can be `assigned`, `mentioned`, `build_failed`, `marked`, `approval_required`, `unmergeable` or `directly_addressed`. | | `author_id` | integer | no | The ID of an author | | `project_id` | integer | no | The ID of a project | +| `group_id` | integer | no | The ID of a group | | `state` | string | no | The state of the todo. Can be either `pending` or `done` | | `type` | string | no | The type of a todo. Can be either `Issue` or `MergeRequest` | diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 375114f524b..06671c84fab 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -769,14 +769,14 @@ module API class Todo < Grape::Entity expose :id - expose :project, using: Entities::ProjectIdentity, if: -> (todo, _) { todo.project } - expose :group, using: 'API::Entities::NamespaceBasic', if: -> (todo, _) { todo.group } + expose :project, using: Entities::ProjectIdentity, if: -> (todo, _) { todo.project_id } + expose :group, using: 'API::Entities::NamespaceBasic', if: -> (todo, _) { todo.group_id } expose :author, using: Entities::UserBasic expose :action_name expose :target_type expose :target do |todo, options| - Entities.const_get(todo.target_type).represent(todo.target, options) + todo_target_class(todo.target_type).represent(todo.target, options) end expose :target_url do |todo, options| @@ -792,6 +792,10 @@ module API expose :body expose :state expose :created_at + + def todo_target_class(target_type) + ::API::Entities.const_get(target_type) + end end class NamespaceBasic < Grape::Entity diff --git a/spec/controllers/projects/todos_controller_spec.rb b/spec/controllers/projects/todos_controller_spec.rb index 1ce7e84bef9..58f2817c7cc 100644 --- a/spec/controllers/projects/todos_controller_spec.rb +++ b/spec/controllers/projects/todos_controller_spec.rb @@ -5,10 +5,29 @@ describe Projects::TodosController do let(:project) { create(:project) } let(:issue) { create(:issue, project: project) } let(:merge_request) { create(:merge_request, source_project: project) } + let(:parent) { project } + + shared_examples 'project todos actions' do + it_behaves_like 'todos actions' + + context 'when not authorized for resource' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) + project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) + sign_in(user) + end + + it "doesn't create todo" do + expect { post_create }.not_to change { user.todos.count } + expect(response).to have_gitlab_http_status(404) + end + end + end context 'Issues' do describe 'POST create' do - def go + def post_create post :create, namespace_id: project.namespace, project_id: project, @@ -17,66 +36,13 @@ describe Projects::TodosController do format: 'html' end - context 'when authorized' do - before do - sign_in(user) - project.add_developer(user) - end - - it 'creates todo for issue' do - expect do - go - end.to change { user.todos.count }.by(1) - - expect(response).to have_gitlab_http_status(200) - end - - it 'returns todo path and pending count' do - go - - expect(response).to have_gitlab_http_status(200) - expect(json_response['count']).to eq 1 - expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}}) - end - end - - context 'when not authorized for project' do - it 'does not create todo for issue that user has no access to' do - sign_in(user) - expect do - go - end.to change { user.todos.count }.by(0) - - expect(response).to have_gitlab_http_status(404) - end - - it 'does not create todo for issue when user not logged in' do - expect do - go - end.to change { user.todos.count }.by(0) - - expect(response).to have_gitlab_http_status(302) - end - end - - context 'when not authorized for issue' do - before do - project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) - project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) - sign_in(user) - end - - it "doesn't create todo" do - expect { go }.not_to change { user.todos.count } - expect(response).to have_gitlab_http_status(404) - end - end + it_behaves_like 'project todos actions' end end context 'Merge Requests' do describe 'POST create' do - def go + def post_create post :create, namespace_id: project.namespace, project_id: project, @@ -85,60 +51,7 @@ describe Projects::TodosController do format: 'html' end - context 'when authorized' do - before do - sign_in(user) - project.add_developer(user) - end - - it 'creates todo for merge request' do - expect do - go - end.to change { user.todos.count }.by(1) - - expect(response).to have_gitlab_http_status(200) - end - - it 'returns todo path and pending count' do - go - - expect(response).to have_gitlab_http_status(200) - expect(json_response['count']).to eq 1 - expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}}) - end - end - - context 'when not authorized for project' do - it 'does not create todo for merge request user has no access to' do - sign_in(user) - expect do - go - end.to change { user.todos.count }.by(0) - - expect(response).to have_gitlab_http_status(404) - end - - it 'does not create todo for merge request user has no access to' do - expect do - go - end.to change { user.todos.count }.by(0) - - expect(response).to have_gitlab_http_status(302) - end - end - - context 'when not authorized for merge_request' do - before do - project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) - project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) - sign_in(user) - end - - it "doesn't create todo" do - expect { go }.not_to change { user.todos.count } - expect(response).to have_gitlab_http_status(404) - end - end + it_behaves_like 'project todos actions' end end end diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb index 484aabea4d0..14486c80341 100644 --- a/spec/factories/todos.rb +++ b/spec/factories/todos.rb @@ -1,7 +1,6 @@ FactoryBot.define do factory :todo do project - group author { project&.creator || user } user { project&.creator || user } target factory: :issue diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb index 50046db5497..6061021d3b0 100644 --- a/spec/finders/todos_finder_spec.rb +++ b/spec/finders/todos_finder_spec.rb @@ -53,7 +53,7 @@ describe TodosFinder do it 'returns correct todos when filtered by a group' do todos = finder.new(user, { group_id: group.id }).execute - expect(todos).to match_array([todo2]) + expect(todos).to match_array([todo1, todo2]) end it 'returns correct todos when filtered by a type' do @@ -61,6 +61,17 @@ describe TodosFinder do expect(todos).to match_array([todo1]) end + + context 'with subgroups', :nested_groups do + let(:subgroup) { create(:group, parent: group) } + let!(:todo3) { create(:todo, user: user, group: subgroup, target: issue) } + + it 'returns todos from subgroups when filtered by a group' do + todos = finder.new(user, { group_id: group.id }).execute + + expect(todos).to match_array([todo1, todo2, todo3]) + end + end end end diff --git a/spec/support/shared_examples/controllers/todos_shared_examples.rb b/spec/support/shared_examples/controllers/todos_shared_examples.rb new file mode 100644 index 00000000000..bafd9bac8d0 --- /dev/null +++ b/spec/support/shared_examples/controllers/todos_shared_examples.rb @@ -0,0 +1,43 @@ +shared_examples 'todos actions' do + context 'when authorized' do + before do + sign_in(user) + parent.add_developer(user) + end + + it 'creates todo' do + expect do + post_create + end.to change { user.todos.count }.by(1) + + expect(response).to have_gitlab_http_status(200) + end + + it 'returns todo path and pending count' do + post_create + + expect(response).to have_gitlab_http_status(200) + expect(json_response['count']).to eq 1 + expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}}) + end + end + + context 'when not authorized for project/group' do + it 'does not create todo for resource that user has no access to' do + sign_in(user) + expect do + post_create + end.to change { user.todos.count }.by(0) + + expect(response).to have_gitlab_http_status(404) + end + + it 'does not create todo when user is not logged in' do + expect do + post_create + end.to change { user.todos.count }.by(0) + + expect(response).to have_gitlab_http_status(parent.is_a?(Group) ? 401 : 302) + end + end +end |