Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/concerns/todos_actions.rb13
-rw-r--r--app/controllers/dashboard/todos_controller.rb2
-rw-r--r--app/controllers/projects/todos_controller.rb13
-rw-r--r--app/finders/todos_finder.rb21
-rw-r--r--app/models/concerns/issuable.rb6
-rw-r--r--app/models/issue.rb4
-rw-r--r--app/models/note.rb4
-rw-r--r--app/models/todo.rb4
-rw-r--r--app/services/todo_service.rb4
-rw-r--r--db/migrate/20180608091413_add_group_to_todos.rb2
-rw-r--r--doc/api/todos.md1
-rw-r--r--lib/api/entities.rb10
-rw-r--r--spec/controllers/projects/todos_controller_spec.rb133
-rw-r--r--spec/factories/todos.rb1
-rw-r--r--spec/finders/todos_finder_spec.rb13
-rw-r--r--spec/support/shared_examples/controllers/todos_shared_examples.rb43
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
generated by cgit v1.2.3 (git 2.25.1) at 2024-08-23 20:17:09 +0300