From 57a44f2da3d2a0b59209b6c2d653d04efd0d3d41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Kadlecov=C3=A1?= Date: Wed, 13 Jun 2018 18:11:10 +0200 Subject: Support todos for epics backport --- app/finders/todos_finder.rb | 43 +++++++++++++++++--- app/helpers/todos_helper.rb | 2 +- app/models/group.rb | 9 +++++ app/models/todo.rb | 11 ++++- app/services/todo_service.rb | 31 ++++++++------- db/migrate/20180608091413_add_group_to_todos.rb | 32 +++++++++++++++ db/schema.rb | 5 ++- lib/api/entities.rb | 7 ++-- spec/factories/todos.rb | 5 ++- spec/finders/todos_finder_spec.rb | 53 +++++++++++++++++++++++++ spec/models/todo_spec.rb | 1 + 11 files changed, 170 insertions(+), 29 deletions(-) create mode 100644 db/migrate/20180608091413_add_group_to_todos.rb diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb index 09e2c586f2a..1dfcf19b78d 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -15,6 +15,7 @@ class TodosFinder prepend FinderWithCrossProjectAccess include FinderMethods + include Gitlab::Utils::StrongMemoize requires_cross_project_access unless: -> { project? } @@ -34,9 +35,11 @@ class TodosFinder items = by_author(items) items = by_state(items) items = by_type(items) + items = by_group(items) # Filtering by project HAS TO be the last because we use # the project IDs yielded by the todos query thus far items = by_project(items) + items = visible_to_user(items) sort(items) end @@ -82,6 +85,10 @@ class TodosFinder params[:project_id].present? end + def group? + params[:group_id].present? + end + def project return @project if defined?(@project) @@ -100,6 +107,12 @@ class TodosFinder @project end + def group + strong_memoize(:group) do + Group.find(params[:group_id]) + end + end + def project_ids(items) ids = items.except(:order).select(:project_id) if Gitlab::Database.mysql? @@ -111,7 +124,7 @@ class TodosFinder end def type? - type.present? && %w(Issue MergeRequest).include?(type) + type.present? && %w(Issue MergeRequest Epic).include?(type) end def type @@ -148,12 +161,32 @@ class TodosFinder def by_project(items) if project? - items.where(project: project) - else - projects = Project.public_or_visible_to_user(current_user) + items = items.where(project: project) + end + + items + end - items.joins(:project).merge(projects) + def by_group(items) + if group? + items = items.where(group: group) end + + items + end + + def visible_to_user(items) + projects = Project.public_or_visible_to_user(current_user) + groups = Group.public_or_visible_to_user(current_user) + + items + .joins('LEFT JOIN namespaces ON namespaces.id = todos.group_id') + .joins('LEFT JOIN projects ON projects.id = todos.project_id') + .where( + 'project_id IN (?) OR group_id IN (?)', + projects.map(&:id), + groups.map(&:id) + ) end def by_state(items) diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index f7620e0b6b8..7d78ceb1f9a 100644 --- a/app/helpers/todos_helper.rb +++ b/app/helpers/todos_helper.rb @@ -43,7 +43,7 @@ module TodosHelper project_commit_path(todo.project, todo.target, anchor: anchor) else - path = [todo.project.namespace.becomes(Namespace), todo.project, todo.target] + path = [todo.parent, todo.target] path.unshift(:pipelines) if todo.build_failed? diff --git a/app/models/group.rb b/app/models/group.rb index 9c171de7fc3..9baf2cfd810 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -39,6 +39,8 @@ class Group < Namespace has_many :boards has_many :badges, class_name: 'GroupBadge' + has_many :todos + accepts_nested_attributes_for :variables, allow_destroy: true validate :visibility_level_allowed_by_projects @@ -82,6 +84,13 @@ 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/models/todo.rb b/app/models/todo.rb index a2ab405fdbe..5ce77d5ddc2 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -22,15 +22,18 @@ class Todo < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :note belongs_to :project + belongs_to :group belongs_to :target, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations belongs_to :user delegate :name, :email, to: :author, prefix: true, allow_nil: true - validates :action, :project, :target_type, :user, presence: true + validates :action, :target_type, :user, presence: true 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 scope :pending, -> { with_state(:pending) } scope :done, -> { with_state(:done) } @@ -44,7 +47,7 @@ class Todo < ActiveRecord::Base state :done end - after_save :keep_around_commit + after_save :keep_around_commit, if: :commit_id class << self # Priority sorting isn't displayed in the dropdown, because we don't show @@ -79,6 +82,10 @@ class Todo < ActiveRecord::Base end end + def parent + project || group + end + def unmergeable? action == UNMERGEABLE end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index f91cd03bf5c..f355d6b8ea1 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -260,15 +260,15 @@ class TodoService end end - def create_mention_todos(project, target, author, note = nil, skip_users = []) + def create_mention_todos(parent, target, author, note = nil, skip_users = []) # Create Todos for directly addressed users - directly_addressed_users = filter_directly_addressed_users(project, note || target, author, skip_users) - attributes = attributes_for_todo(project, target, author, Todo::DIRECTLY_ADDRESSED, note) + directly_addressed_users = filter_directly_addressed_users(parent, note || target, author, skip_users) + attributes = attributes_for_todo(parent, target, author, Todo::DIRECTLY_ADDRESSED, note) create_todos(directly_addressed_users, attributes) # Create Todos for mentioned users - mentioned_users = filter_mentioned_users(project, note || target, author, skip_users) - attributes = attributes_for_todo(project, target, author, Todo::MENTIONED, note) + mentioned_users = filter_mentioned_users(parent, note || target, author, skip_users) + attributes = attributes_for_todo(parent, target, author, Todo::MENTIONED, note) create_todos(mentioned_users, attributes) end @@ -299,36 +299,37 @@ class TodoService def attributes_for_todo(project, target, author, action, note = nil) attributes_for_target(target).merge!( - project_id: project.id, + project_id: project&.id, + group_id: target.respond_to?(:group) ? target.group.id : nil, author_id: author.id, action: action, note: note ) end - def filter_todo_users(users, project, target) - reject_users_without_access(users, project, target).uniq + def filter_todo_users(users, parent, target) + reject_users_without_access(users, parent, target).uniq end - def filter_mentioned_users(project, target, author, skip_users = []) + def filter_mentioned_users(parent, target, author, skip_users = []) mentioned_users = target.mentioned_users(author) - skip_users - filter_todo_users(mentioned_users, project, target) + filter_todo_users(mentioned_users, parent, target) end - def filter_directly_addressed_users(project, target, author, skip_users = []) + def filter_directly_addressed_users(parent, target, author, skip_users = []) directly_addressed_users = target.directly_addressed_users(author) - skip_users - filter_todo_users(directly_addressed_users, project, target) + filter_todo_users(directly_addressed_users, parent, target) end - def reject_users_without_access(users, project, target) - if target.is_a?(Note) && (target.for_issue? || target.for_merge_request?) + def reject_users_without_access(users, parent, target) + if target.is_a?(Note) && (target.for_issue? || target.for_merge_request? || target.for_epic?) target = target.noteable end if target.is_a?(Issuable) select_users(users, :"read_#{target.to_ability_name}", target) else - select_users(users, :read_project, project) + select_users(users, :read_project, parent) end end diff --git a/db/migrate/20180608091413_add_group_to_todos.rb b/db/migrate/20180608091413_add_group_to_todos.rb new file mode 100644 index 00000000000..ca08de835a1 --- /dev/null +++ b/db/migrate/20180608091413_add_group_to_todos.rb @@ -0,0 +1,32 @@ +class AddGroupToTodos < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column :todos, :group_id, :integer + add_foreign_key :todos, :namespaces, column: :group_id, on_delete: :cascade + add_concurrent_index :todos, :group_id + + change_column_null :todos, :project_id, true + 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 + + execute "DELETE FROM todos WHERE project_id IS NULL" + change_column_null :todos, :project_id, false + end + + private + + def group_id_exists? + column_exists?(:todos, :group_id) + end +end diff --git a/db/schema.rb b/db/schema.rb index 0112fc726d4..504d57b8aa2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1929,7 +1929,7 @@ ActiveRecord::Schema.define(version: 20180626125654) do create_table "todos", force: :cascade do |t| t.integer "user_id", null: false - t.integer "project_id", null: false + t.integer "project_id" t.integer "target_id" t.string "target_type", null: false t.integer "author_id", null: false @@ -1939,10 +1939,12 @@ ActiveRecord::Schema.define(version: 20180626125654) do t.datetime "updated_at" t.integer "note_id" t.string "commit_id" + t.integer "group_id" end add_index "todos", ["author_id"], name: "index_todos_on_author_id", using: :btree add_index "todos", ["commit_id"], name: "index_todos_on_commit_id", using: :btree + add_index "todos", ["group_id"], name: "index_todos_on_group_id", using: :btree add_index "todos", ["note_id"], name: "index_todos_on_note_id", using: :btree add_index "todos", ["project_id"], name: "index_todos_on_project_id", using: :btree add_index "todos", ["target_type", "target_id"], name: "index_todos_on_target_type_and_target_id", using: :btree @@ -2313,6 +2315,7 @@ ActiveRecord::Schema.define(version: 20180626125654) do add_foreign_key "term_agreements", "users", on_delete: :cascade add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade + add_foreign_key "todos", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "todos", "notes", name: "fk_91d1f47b13", on_delete: :cascade add_foreign_key "todos", "projects", name: "fk_45054f9c45", on_delete: :cascade add_foreign_key "todos", "users", column: "author_id", name: "fk_ccf0373936", on_delete: :cascade diff --git a/lib/api/entities.rb b/lib/api/entities.rb index bb48a86fe9e..375114f524b 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -769,7 +769,8 @@ module API class Todo < Grape::Entity expose :id - expose :project, using: Entities::BasicProjectDetails + expose :project, using: Entities::ProjectIdentity, if: -> (todo, _) { todo.project } + expose :group, using: 'API::Entities::NamespaceBasic', if: -> (todo, _) { todo.group } expose :author, using: Entities::UserBasic expose :action_name expose :target_type @@ -780,12 +781,12 @@ module API expose :target_url do |todo, options| target_type = todo.target_type.underscore - target_url = "namespace_project_#{target_type}_url" + target_url = "#{todo.parent.class.to_s.underscore}_#{target_type}_url" target_anchor = "note_#{todo.note_id}" if todo.note_id? Gitlab::Routing .url_helpers - .public_send(target_url, todo.project.namespace, todo.project, todo.target, anchor: target_anchor) # rubocop:disable GitlabSecurity/PublicSend + .public_send(target_url, todo.parent, todo.target, anchor: target_anchor) # rubocop:disable GitlabSecurity/PublicSend end expose :body diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb index 94f8caedfa6..484aabea4d0 100644 --- a/spec/factories/todos.rb +++ b/spec/factories/todos.rb @@ -1,8 +1,9 @@ FactoryBot.define do factory :todo do project - author { project.creator } - user { project.creator } + group + author { project&.creator || user } + user { project&.creator || user } target factory: :issue action { Todo::ASSIGNED } diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb index 9747b9402a7..50046db5497 100644 --- a/spec/finders/todos_finder_spec.rb +++ b/spec/finders/todos_finder_spec.rb @@ -5,12 +5,65 @@ describe TodosFinder do let(:user) { create(:user) } let(:group) { create(:group) } let(:project) { create(:project, namespace: group) } + let(:issue) { create(:issue, project: project) } + let(:merge_request) { create(:merge_request, source_project: project) } let(:finder) { described_class } before do group.add_developer(user) end + describe '#execute' do + context 'visibility' do + let(:private_group_access) { create(:group, :private) } + let(:private_group_hidden) { create(:group, :private) } + let(:public_project) { create(:project, :public) } + let(:private_project_hidden) { create(:project) } + let(:public_group) { create(:group) } + + let!(:todo1) { create(:todo, user: user, project: project, group: nil) } + let!(:todo2) { create(:todo, user: user, project: public_project, group: nil) } + let!(:todo3) { create(:todo, user: user, project: private_project_hidden, group: nil) } + let!(:todo4) { create(:todo, user: user, project: nil, group: group) } + let!(:todo5) { create(:todo, user: user, project: nil, group: private_group_access) } + let!(:todo6) { create(:todo, user: user, project: nil, group: private_group_hidden) } + let!(:todo7) { create(:todo, user: user, project: nil, group: public_group) } + + before do + private_group_access.add_developer(user) + end + + it 'returns only todos with a target a user has access to' do + todos = finder.new(user).execute + + expect(todos).to match_array([todo1, todo2, todo4, todo5, todo7]) + end + end + + context 'filtering' do + let!(:todo1) { create(:todo, user: user, project: project, target: issue) } + let!(:todo2) { create(:todo, user: user, group: group, target: merge_request) } + + it 'returns correct todos when filtered by a project' do + todos = finder.new(user, { project_id: project.id }).execute + + expect(todos).to match_array([todo1]) + end + + 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]) + end + + it 'returns correct todos when filtered by a type' do + todos = finder.new(user, { type: 'Issue' }).execute + + expect(todos).to match_array([todo1]) + end + end + end + describe '#sort' do context 'by date' do let!(:todo1) { create(:todo, user: user, project: project) } diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb index bd498269798..f29abcf536e 100644 --- a/spec/models/todo_spec.rb +++ b/spec/models/todo_spec.rb @@ -7,6 +7,7 @@ describe Todo do it { is_expected.to belong_to(:author).class_name("User") } it { is_expected.to belong_to(:note) } it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:group) } it { is_expected.to belong_to(:target).touch(true) } it { is_expected.to belong_to(:user) } end -- cgit v1.2.3