diff options
| -rw-r--r-- | app/finders/todos_finder.rb | 10 | ||||
| -rw-r--r-- | app/models/group.rb | 6 | ||||
| -rw-r--r-- | app/services/todos/destroy/entity_leave_service.rb | 5 | ||||
| -rw-r--r-- | app/workers/all_queues.yml | 1 | ||||
| -rw-r--r-- | db/migrate/20180608091413_add_group_to_todos.rb | 18 | ||||
| -rw-r--r-- | spec/requests/api/todos_spec.rb | 14 | ||||
| -rw-r--r-- | spec/services/todos/destroy/group_private_service_spec.rb | 2 | ||||
| -rw-r--r-- | spec/services/todos/destroy/project_private_service_spec.rb | 1 |
8 files changed, 33 insertions, 24 deletions
diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index c505a5cc8d5..6e9c8ea6fde 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -155,11 +155,11 @@ class TodosFinder def by_group(items) if 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) - ) + project_todos = items.where(project_id: Project.where(group: groups).select(:id)) + group_todos = items.where(group_id: groups.select(:id)) + + union = Gitlab::SQL::Union.new([project_todos, group_todos]) + items = Todo.from("(#{union.to_sql}) #{Todo.table_name}") end items diff --git a/app/models/group.rb b/app/models/group.rb index 28677320e28..084cae3101a 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -84,12 +84,6 @@ class Group < Namespace where(id: user.authorized_groups.select(:id).reorder(nil)) end - def public_or_visible_to_user(user) - where('id IN (?) OR namespaces.visibility_level IN (?)', - user.authorized_groups.select(:id), - Gitlab::VisibilityLevel.levels_for_user(user)) - end - def select_for_project_authorization if current_scope.joins_values.include?(:shared_projects) joins('INNER JOIN namespaces project_namespace ON project_namespace.id = projects.namespace_id') diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb index ef6add352e7..045f5ecaae7 100644 --- a/app/services/todos/destroy/entity_leave_service.rb +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -2,7 +2,6 @@ module Todos module Destroy class EntityLeaveService < ::Todos::Destroy::BaseService extend ::Gitlab::Utils::Override - include Gitlab::Utils::StrongMemoize attr_reader :user, :entity @@ -19,7 +18,7 @@ module Todos return unless entity && user # if at least reporter, all entities including confidential issues can be accessed - return if main_group_reporter? + return if user_has_reporter_access? remove_confidential_issue_todos @@ -81,7 +80,7 @@ module Todos .where('id NOT IN (?)', user.membership_groups.select(:id)) end - def main_group_reporter? + def user_has_reporter_access? return unless entity.is_a?(Namespace) entity.member?(User.find(user.id), Gitlab::Access::REPORTER) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index e8b9999f83b..f95df7ecf03 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -77,6 +77,7 @@ - todos_destroyer:todos_destroyer_entity_leave - todos_destroyer:todos_destroyer_project_private - todos_destroyer:todos_destroyer_private_features +- todos_destroyer:todos_destroyer_group_private - default - mailers # ActionMailer::DeliveryJob.queue_name diff --git a/db/migrate/20180608091413_add_group_to_todos.rb b/db/migrate/20180608091413_add_group_to_todos.rb index af3ee48b29d..20ba4849057 100644 --- a/db/migrate/20180608091413_add_group_to_todos.rb +++ b/db/migrate/20180608091413_add_group_to_todos.rb @@ -5,8 +5,14 @@ class AddGroupToTodos < ActiveRecord::Migration disable_ddl_transaction! + class Todo < ActiveRecord::Base + self.table_name = 'todos' + + include ::EachBatch + end + def up - add_column :todos, :group_id, :integer + add_column(:todos, :group_id, :integer) unless group_id_exists? add_concurrent_foreign_key :todos, :namespaces, column: :group_id, on_delete: :cascade add_concurrent_index :todos, :group_id @@ -14,13 +20,11 @@ class AddGroupToTodos < ActiveRecord::Migration end def down - return unless group_id_exists? - - remove_foreign_key :todos, column: :group_id - remove_index :todos, :group_id if index_exists?(:todos, :group_id) - remove_column :todos, :group_id + remove_foreign_key_without_error(:todos, column: :group_id) + remove_concurrent_index(:todos, :group_id) + remove_column(:todos, :group_id) if group_id_exists? - execute "DELETE FROM todos WHERE project_id IS NULL" + Todo.where(project_id: nil).each_batch { |batch| batch.delete_all } change_column_null :todos, :project_id, false end diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb index 2ee8d150dc8..b5cf04e7f22 100644 --- a/spec/requests/api/todos_spec.rb +++ b/spec/requests/api/todos_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe API::Todos do - let(:project_1) { create(:project, :repository) } + let(:group) { create(:group) } + let(:project_1) { create(:project, :repository, group: group) } let(:project_2) { create(:project) } let(:author_1) { create(:user) } let(:author_2) { create(:user) } @@ -92,6 +93,17 @@ describe API::Todos do end end + context 'and using the group filter' do + it 'filters based on project_id param' do + get api('/todos', john_doe), { group_id: group.id, sort: :target_id } + + expect(response.status).to eq(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.length).to eq(2) + end + end + context 'and using the action filter' do it 'filters based on action param' do get api('/todos', john_doe), { action: 'mentioned' } diff --git a/spec/services/todos/destroy/group_private_service_spec.rb b/spec/services/todos/destroy/group_private_service_spec.rb index c4ee6ebed50..2f49b68f544 100644 --- a/spec/services/todos/destroy/group_private_service_spec.rb +++ b/spec/services/todos/destroy/group_private_service_spec.rb @@ -33,7 +33,7 @@ describe Todos::Destroy::GroupPrivateService do expect(project_member.todos).to match_array([todo_project_member]) end - context 'with nested groups' do + context 'with nested groups', :nested_groups do let(:parent_group) { create(:group) } let(:subgroup) { create(:group, :private, parent: group) } let(:subproject) { create(:project, group: subgroup) } diff --git a/spec/services/todos/destroy/project_private_service_spec.rb b/spec/services/todos/destroy/project_private_service_spec.rb index 9ebc0231795..128d3487514 100644 --- a/spec/services/todos/destroy/project_private_service_spec.rb +++ b/spec/services/todos/destroy/project_private_service_spec.rb @@ -10,7 +10,6 @@ describe Todos::Destroy::ProjectPrivateService do let!(:todo_non_member) { create(:todo, user: user, project: project) } let!(:todo2_non_member) { create(:todo, user: user, project: project) } let!(:todo_member) { create(:todo, user: project_member, project: project) } - let!(:todo_member) { create(:todo, user: project_member, project: project) } let!(:todo_group_member) { create(:todo, user: group_member, project: project) } describe '#execute' do |