Commit fcb44608 authored by Jan Provaznik's avatar Jan Provaznik Committed by Jarka Kadlecová
Browse files

[backend] Addressed review comments

* Group filtering now includes also issues/MRs from
subgroups/subprojects
* fixed due_date
* Also DRYed todo controller specs
parent 1e98b8cb1afb
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
......@@ -70,7 +70,7 @@ def todos_counts
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)
......
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])
......
......@@ -113,16 +113,6 @@ def group
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 @@ def by_project(items)
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 @@ def visible_to_user(items)
.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
......
......@@ -243,6 +243,12 @@ def open?
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
......
......@@ -275,10 +275,6 @@ def visible_to_user?(user = nil)
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
......
......@@ -229,6 +229,10 @@ def for_project_noteable?
!for_personal_snippet?
end
def for_issuable_with_ability?
for_issue? || for_merge_request?
end
def skip_project_check?
!for_project_noteable?
end
......
......@@ -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) }
......
......@@ -285,6 +285,7 @@ def create_unmergeable_todo(merge_request, todo_author)
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 @@ def attributes_for_target(target)
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 @@ def filter_directly_addressed_users(parent, target, author, skip_users = [])
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
......
......@@ -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
......
......@@ -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` |
......
......@@ -769,14 +769,14 @@ class ProjectGroupLink < Grape::Entity
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 @@ class Todo < Grape::Entity
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
......
......@@ -5,10 +5,29 @@
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 @@ def go
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 @@ def go
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
FactoryBot.define do
factory :todo do
project
group
author { project&.creator || user }
user { project&.creator || user }
target factory: :issue
......
......@@ -53,7 +53,7 @@
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 @@
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
......
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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment