diff options
43 files changed, 1036 insertions, 262 deletions
diff --git a/app/assets/javascripts/pages/dashboard/todos/index/todos.js b/app/assets/javascripts/pages/dashboard/todos/index/todos.js index ff19b9a9c30..9aa83ce6269 100644 --- a/app/assets/javascripts/pages/dashboard/todos/index/todos.js +++ b/app/assets/javascripts/pages/dashboard/todos/index/todos.js @@ -39,6 +39,7 @@ export default class Todos { } initFilters() { + this.initFilterDropdown($('.js-group-search'), 'group_id', ['text']); this.initFilterDropdown($('.js-project-search'), 'project_id', ['text']); this.initFilterDropdown($('.js-type-search'), 'type'); this.initFilterDropdown($('.js-action-search'), 'action_id'); @@ -53,7 +54,16 @@ export default class Todos { filterable: searchFields ? true : false, search: { fields: searchFields }, data: $dropdown.data('data'), - clicked: () => $dropdown.closest('form.filter-form').submit(), + clicked: () => { + const $formEl = $dropdown.closest('form.filter-form'); + const mutexDropdowns = { + group_id: 'project_id', + project_id: 'group_id', + }; + + $formEl.find(`input[name="${mutexDropdowns[fieldName]}"]`).remove(); + $formEl.submit(); + }, }); } diff --git a/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue b/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue new file mode 100644 index 00000000000..ffaed9c7193 --- /dev/null +++ b/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue @@ -0,0 +1,98 @@ +<script> +import { __ } from '~/locale'; +import tooltip from '~/vue_shared/directives/tooltip'; + +import Icon from '~/vue_shared/components/icon.vue'; +import LoadingIcon from '~/vue_shared/components/loading_icon.vue'; + +const MARK_TEXT = __('Mark todo as done'); +const TODO_TEXT = __('Add todo'); + +export default { + directives: { + tooltip, + }, + components: { + Icon, + LoadingIcon, + }, + props: { + issuableId: { + type: Number, + required: true, + }, + issuableType: { + type: String, + required: true, + }, + isTodo: { + type: Boolean, + required: false, + default: true, + }, + isActionActive: { + type: Boolean, + required: false, + default: false, + }, + collapsed: { + type: Boolean, + required: false, + default: false, + }, + }, + computed: { + buttonClasses() { + return this.collapsed ? + 'btn-blank btn-todo sidebar-collapsed-icon dont-change-state' : + 'btn btn-default btn-todo issuable-header-btn float-right'; + }, + buttonLabel() { + return this.isTodo ? MARK_TEXT : TODO_TEXT; + }, + collapsedButtonIconClasses() { + return this.isTodo ? 'todo-undone' : ''; + }, + collapsedButtonIcon() { + return this.isTodo ? 'todo-done' : 'todo-add'; + }, + }, + methods: { + handleButtonClick() { + this.$emit('toggleTodo'); + }, + }, +}; +</script> + +<template> + <button + v-tooltip + :class="buttonClasses" + :title="buttonLabel" + :aria-label="buttonLabel" + :data-issuable-id="issuableId" + :data-issuable-type="issuableType" + type="button" + data-container="body" + data-placement="left" + data-boundary="viewport" + @click="handleButtonClick" + > + <icon + v-show="collapsed" + :css-classes="collapsedButtonIconClasses" + :name="collapsedButtonIcon" + /> + <span + v-show="!collapsed" + class="issuable-todo-inner" + > + {{ buttonLabel }} + </span> + <loading-icon + v-show="isActionActive" + :inline="true" + /> + </button> +</template> diff --git a/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue b/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue index ac2e99abe77..80dc7d3557c 100644 --- a/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue +++ b/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue @@ -12,6 +12,11 @@ export default { type: Boolean, required: true, }, + cssClasses: { + type: String, + required: false, + default: '', + }, }, computed: { tooltipLabel() { @@ -30,10 +35,12 @@ export default { <button v-tooltip :title="tooltipLabel" + :class="cssClasses" type="button" class="btn btn-blank gutter-toggle btn-sidebar-action" data-container="body" data-placement="left" + data-boundary="viewport" @click="toggle" > <i diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index d5ae2b673d9..8e78d9f65eb 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -449,6 +449,7 @@ .todo-undone { color: $gl-link-color; + fill: $gl-link-color; } .author { diff --git a/app/assets/stylesheets/pages/todos.scss b/app/assets/stylesheets/pages/todos.scss index e5d7dd13915..010a2c05a1c 100644 --- a/app/assets/stylesheets/pages/todos.scss +++ b/app/assets/stylesheets/pages/todos.scss @@ -174,6 +174,18 @@ } } +@include media-breakpoint-down(lg) { + .todos-filters { + .filter-categories { + width: 75%; + + .filter-item { + margin-bottom: 10px; + } + } + } +} + @include media-breakpoint-down(xs) { .todo { .avatar { @@ -199,6 +211,10 @@ } .todos-filters { + .filter-categories { + width: auto; + } + .dropdown-menu-toggle { width: 100%; } diff --git a/app/controllers/concerns/todos_actions.rb b/app/controllers/concerns/todos_actions.rb new file mode 100644 index 00000000000..c0acdb3498d --- /dev/null +++ b/app/controllers/concerns/todos_actions.rb @@ -0,0 +1,12 @@ +module TodosActions + 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..93fb9da6510 100644 --- a/app/controllers/projects/todos_controller.rb +++ b/app/controllers/projects/todos_controller.rb @@ -1,19 +1,13 @@ class Projects::TodosController < Projects::ApplicationController - before_action :authenticate_user!, only: [:create] - - def create - todo = TodoService.new.mark_todo(issuable, current_user) + include Gitlab::Utils::StrongMemoize + 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 09e2c586f2a..6e9c8ea6fde 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,6 +35,7 @@ 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) @@ -82,6 +84,10 @@ class TodosFinder params[:project_id].present? end + def group? + params[:group_id].present? + end + def project return @project if defined?(@project) @@ -89,10 +95,6 @@ class TodosFinder @project = Project.find(params[:project_id]) @project = nil if @project.pending_delete? - - unless Ability.allowed?(current_user, :read_project, @project) - @project = nil - end else @project = nil end @@ -100,18 +102,14 @@ class TodosFinder @project 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") + def group + strong_memoize(:group) do + Group.find(params[:group_id]) end - - ids end def type? - type.present? && %w(Issue MergeRequest).include?(type) + type.present? && %w(Issue MergeRequest Epic).include?(type) end def type @@ -148,12 +146,23 @@ 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.joins(:project).merge(projects) + items + end + + def by_group(items) + if group? + groups = group.self_and_descendants + 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 end def by_state(items) diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 678fed9c414..c84ed8091c3 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -131,6 +131,19 @@ module IssuablesHelper end end + def group_dropdown_label(group_id, default_label) + return default_label if group_id.nil? + return "Any group" if group_id == "0" + + group = ::Group.find_by(id: group_id) + + if group + group.full_name + else + default_label + end + end + def milestone_dropdown_label(milestone_title, default_label = "Milestone") title = case milestone_title diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index f7620e0b6b8..7cd74358168 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? @@ -167,4 +167,12 @@ module TodosHelper def show_todo_state?(todo) (todo.target.is_a?(MergeRequest) || todo.target.is_a?(Issue)) && %w(closed merged).include?(todo.target.state) end + + def todo_group_options + groups = current_user.authorized_groups.map do |group| + { id: group.id, text: group.full_name } + end + + groups.unshift({ id: '', text: 'Any Group' }).to_json + end 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/group.rb b/app/models/group.rb index cd548fc0061..106a1f4a94c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -41,6 +41,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 diff --git a/app/models/issue.rb b/app/models/issue.rb index 0d135f54038..94cf12f3c2b 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -278,10 +278,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 969d34ae09a..2e343b8f9f8 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -231,6 +231,10 @@ class Note < ActiveRecord::Base !for_personal_snippet? end + def for_issuable? + 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 5f5c2f9073d..48d92ad04b3 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -24,15 +24,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_id + validates :group, presence: true, unless: :project_id scope :pending, -> { with_state(:pending) } scope :done, -> { with_state(:done) } @@ -46,7 +49,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 @@ -81,6 +84,10 @@ class Todo < ActiveRecord::Base end end + def parent + project + end + def unmergeable? action == UNMERGEABLE end diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 436a6b18cb1..fe47aa2f140 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -14,7 +14,9 @@ module Groups group.assign_attributes(params) begin - group.save + after_update if group.save + + true rescue Gitlab::UpdatePathError => e group.errors.add(:base, e.message) @@ -24,6 +26,13 @@ module Groups private + def after_update + if group.previous_changes.include?(:visibility_level) && group.private? + # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::GroupPrivateWorker.perform_in(1.hour, group.id) + end + end + def reject_parent_id! params.except!(:parent_id) end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 0bcd53c76a9..0df61ad3bce 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -262,15 +262,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 @@ -301,36 +301,36 @@ 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, 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_issuable? 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/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb index 2ff9f94b718..045f5ecaae7 100644 --- a/app/services/todos/destroy/entity_leave_service.rb +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -3,55 +3,97 @@ module Todos class EntityLeaveService < ::Todos::Destroy::BaseService extend ::Gitlab::Utils::Override - attr_reader :user_id, :entity + attr_reader :user, :entity def initialize(user_id, entity_id, entity_type) unless %w(Group Project).include?(entity_type) raise ArgumentError.new("#{entity_type} is not an entity user can leave") end - @user_id = user_id + @user = User.find_by(id: user_id) @entity = entity_type.constantize.find_by(id: entity_id) end - private + def execute + return unless entity && user + + # if at least reporter, all entities including confidential issues can be accessed + return if user_has_reporter_access? + + remove_confidential_issue_todos - override :todos - def todos if entity.private? - Todo.where(project_id: project_ids, user_id: user_id) + remove_project_todos + remove_group_todos else - project_ids.each do |project_id| - TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_id) - end + enqueue_private_features_worker + end + end + + private - Todo.where( - target_id: confidential_issues.select(:id), target_type: Issue, user_id: user_id - ) + def enqueue_private_features_worker + project_ids.each do |project_id| + TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user.id) end end + def remove_confidential_issue_todos + Todo.where( + target_id: confidential_issues.select(:id), target_type: Issue, user_id: user.id + ).delete_all + end + + def remove_project_todos + Todo.where(project_id: non_authorized_projects, user_id: user.id).delete_all + end + + def remove_group_todos + Todo.where(group_id: non_authorized_groups, user_id: user.id).delete_all + end + override :project_ids def project_ids - case entity - when Project - [entity.id] - when Namespace - Project.select(:id).where(namespace_id: entity.self_and_descendants.select(:id)) - end + condition = case entity + when Project + { id: entity.id } + when Namespace + { namespace_id: non_member_groups } + end + + Project.where(condition).select(:id) end - override :todos_to_remove? - def todos_to_remove? - # if an entity is provided we want to check always at least private features - !!entity + def non_authorized_projects + project_ids.where('id NOT IN (?)', user.authorized_projects.select(:id)) + end + + def non_authorized_groups + return [] unless entity.is_a?(Namespace) + + entity.self_and_descendants.select(:id) + .where('id NOT IN (?)', GroupsFinder.new(user).execute.select(:id)) + end + + def non_member_groups + entity.self_and_descendants.select(:id) + .where('id NOT IN (?)', user.membership_groups.select(:id)) + end + + def user_has_reporter_access? + return unless entity.is_a?(Namespace) + + entity.member?(User.find(user.id), Gitlab::Access::REPORTER) end def confidential_issues - assigned_ids = IssueAssignee.select(:issue_id).where(user_id: user_id) + assigned_ids = IssueAssignee.select(:issue_id).where(user_id: user.id) + authorized_reporter_projects = user + .authorized_projects(Gitlab::Access::REPORTER).select(:id) Issue.where(project_id: project_ids, confidential: true) - .where('author_id != ?', user_id) + .where('project_id NOT IN(?)', authorized_reporter_projects) + .where('author_id != ?', user.id) .where('id NOT IN (?)', assigned_ids) end end diff --git a/app/services/todos/destroy/group_private_service.rb b/app/services/todos/destroy/group_private_service.rb new file mode 100644 index 00000000000..d13fa7a6516 --- /dev/null +++ b/app/services/todos/destroy/group_private_service.rb @@ -0,0 +1,30 @@ +module Todos + module Destroy + class GroupPrivateService < ::Todos::Destroy::BaseService + extend ::Gitlab::Utils::Override + + attr_reader :group + + def initialize(group_id) + @group = Group.find_by(id: group_id) + end + + private + + override :todos + def todos + Todo.where(group_id: group.id) + end + + override :authorized_users + def authorized_users + group.direct_and_indirect_users.select(:id) + end + + override :todos_to_remove? + def todos_to_remove? + group&.private? + end + end + end +end diff --git a/app/services/todos/destroy/project_private_service.rb b/app/services/todos/destroy/project_private_service.rb index 171933e7cbc..315a0c33398 100644 --- a/app/services/todos/destroy/project_private_service.rb +++ b/app/services/todos/destroy/project_private_service.rb @@ -13,7 +13,7 @@ module Todos override :todos def todos - Todo.where(project_id: project_ids) + Todo.where(project_id: project.id) end override :project_ids diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml index d5a9cc646a6..8b3974d97f8 100644 --- a/app/views/dashboard/todos/index.html.haml +++ b/app/views/dashboard/todos/index.html.haml @@ -30,27 +30,33 @@ .todos-filters .row-content-block.second-block - = form_tag todos_filter_path(without: [:project_id, :author_id, :type, :action_id]), method: :get, class: 'filter-form' do - .filter-item.inline - - if params[:project_id].present? - = hidden_field_tag(:project_id, params[:project_id]) - = dropdown_tag(project_dropdown_label(params[:project_id], 'Project'), options: { toggle_class: 'js-project-search js-filter-submit', title: 'Filter by project', filter: true, filterInput: 'input#project-search', dropdown_class: 'dropdown-menu-selectable dropdown-menu-project js-filter-submit', - placeholder: 'Search projects', data: { data: todo_projects_options, default_label: 'Project', display: 'static' } }) - .filter-item.inline - - if params[:author_id].present? - = hidden_field_tag(:author_id, params[:author_id]) - = dropdown_tag(user_dropdown_label(params[:author_id], 'Author'), options: { toggle_class: 'js-user-search js-filter-submit js-author-search', title: 'Filter by author', filter: true, filterInput: 'input#author-search', dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-author js-filter-submit', - placeholder: 'Search authors', data: { any_user: 'Any Author', first_user: (current_user.username if current_user), project_id: (@project.id if @project), selected: params[:author_id], field_name: 'author_id', default_label: 'Author', todo_filter: true, todo_state_filter: params[:state] || 'pending' } }) - .filter-item.inline - - if params[:type].present? - = hidden_field_tag(:type, params[:type]) - = dropdown_tag(todo_types_dropdown_label(params[:type], 'Type'), options: { toggle_class: 'js-type-search js-filter-submit', dropdown_class: 'dropdown-menu-selectable dropdown-menu-type js-filter-submit', - data: { data: todo_types_options, default_label: 'Type' } }) - .filter-item.inline.actions-filter - - if params[:action_id].present? - = hidden_field_tag(:action_id, params[:action_id]) - = dropdown_tag(todo_actions_dropdown_label(params[:action_id], 'Action'), options: { toggle_class: 'js-action-search js-filter-submit', dropdown_class: 'dropdown-menu-selectable dropdown-menu-action js-filter-submit', - data: { data: todo_actions_options, default_label: 'Action' } }) + = form_tag todos_filter_path(without: [:project_id, :author_id, :type, :action_id]), method: :get, class: 'filter-form d-sm-flex' do + .filter-categories.flex-fill + .filter-item.inline + - if params[:group_id].present? + = hidden_field_tag(:group_id, params[:group_id]) + = dropdown_tag(group_dropdown_label(params[:group_id], 'Group'), options: { toggle_class: 'js-group-search js-filter-submit', title: 'Filter by group', filter: true, filterInput: 'input#group-search', dropdown_class: 'dropdown-menu-selectable dropdown-menu-group js-filter-submit', + placeholder: 'Search groups', data: { data: todo_group_options, default_label: 'Group', display: 'static' } }) + .filter-item.inline + - if params[:project_id].present? + = hidden_field_tag(:project_id, params[:project_id]) + = dropdown_tag(project_dropdown_label(params[:project_id], 'Project'), options: { toggle_class: 'js-project-search js-filter-submit', title: 'Filter by project', filter: true, filterInput: 'input#project-search', dropdown_class: 'dropdown-menu-selectable dropdown-menu-project js-filter-submit', + placeholder: 'Search projects', data: { data: todo_projects_options, default_label: 'Project', display: 'static' } }) + .filter-item.inline + - if params[:author_id].present? + = hidden_field_tag(:author_id, params[:author_id]) + = dropdown_tag(user_dropdown_label(params[:author_id], 'Author'), options: { toggle_class: 'js-user-search js-filter-submit js-author-search', title: 'Filter by author', filter: true, filterInput: 'input#author-search', dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-author js-filter-submit', + placeholder: 'Search authors', data: { any_user: 'Any Author', first_user: (current_user.username if current_user), project_id: (@project.id if @project), selected: params[:author_id], field_name: 'author_id', default_label: 'Author', todo_filter: true, todo_state_filter: params[:state] || 'pending' } }) + .filter-item.inline + - if params[:type].present? + = hidden_field_tag(:type, params[:type]) + = dropdown_tag(todo_types_dropdown_label(params[:type], 'Type'), options: { toggle_class: 'js-type-search js-filter-submit', dropdown_class: 'dropdown-menu-selectable dropdown-menu-type js-filter-submit', + data: { data: todo_types_options, default_label: 'Type' } }) + .filter-item.inline.actions-filter + - if params[:action_id].present? + = hidden_field_tag(:action_id, params[:action_id]) + = dropdown_tag(todo_actions_dropdown_label(params[:action_id], 'Action'), options: { toggle_class: 'js-action-search js-filter-submit', dropdown_class: 'dropdown-menu-selectable dropdown-menu-action js-filter-submit', + data: { data: todo_actions_options, default_label: 'Action' } }) .filter-item.sort-filter .dropdown %button.dropdown-menu-toggle.dropdown-menu-toggle-sort{ type: 'button', 'data-toggle' => 'dropdown' } 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/app/workers/todos_destroyer/group_private_worker.rb b/app/workers/todos_destroyer/group_private_worker.rb new file mode 100644 index 00000000000..3e47eec7461 --- /dev/null +++ b/app/workers/todos_destroyer/group_private_worker.rb @@ -0,0 +1,10 @@ +module TodosDestroyer + class GroupPrivateWorker + include ApplicationWorker + include TodosDestroyerQueue + + def perform(group_id) + ::Todos::Destroy::GroupPrivateService.new(group_id).execute + end + 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..20ba4849057 --- /dev/null +++ b/db/migrate/20180608091413_add_group_to_todos.rb @@ -0,0 +1,36 @@ +class AddGroupToTodos < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class Todo < ActiveRecord::Base + self.table_name = 'todos' + + include ::EachBatch + end + + def up + 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 + + change_column_null :todos, :project_id, true + end + + def down + remove_foreign_key_without_error(:todos, column: :group_id) + remove_concurrent_index(:todos, :group_id) + remove_column(:todos, :group_id) if group_id_exists? + + Todo.where(project_id: nil).each_batch { |batch| batch.delete_all } + 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 2bef2971f29..709c7a9c121 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1988,7 +1988,7 @@ ActiveRecord::Schema.define(version: 20180726172057) 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 @@ -1998,10 +1998,12 @@ ActiveRecord::Schema.define(version: 20180726172057) 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 @@ -2389,6 +2391,7 @@ ActiveRecord::Schema.define(version: 20180726172057) 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/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/doc/workflow/todos.md b/doc/workflow/todos.md index 760cd87d4cc..dda82352c67 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -109,6 +109,7 @@ There are four kinds of filters you can use on your Todos dashboard. | Filter | Description | | ------- | ----------- | | Project | Filter by project | +| Group | Filter by group | | Author | Filter by the author that triggered the Todo | | Type | Filter by issue or merge request | | Action | Filter by the action that triggered the Todo | diff --git a/lib/api/entities.rb b/lib/api/entities.rb index f858d9fa23d..27f28e1df93 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -795,28 +795,33 @@ module API class Todo < Grape::Entity expose :id - expose :project, using: Entities::BasicProjectDetails + 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| 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 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 94f8caedfa6..14486c80341 100644 --- a/spec/factories/todos.rb +++ b/spec/factories/todos.rb @@ -1,8 +1,8 @@ FactoryBot.define do factory :todo do project - author { project.creator } - user { project.creator } + 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..7f7cfb2cb98 100644 --- a/spec/finders/todos_finder_spec.rb +++ b/spec/finders/todos_finder_spec.rb @@ -5,12 +5,50 @@ 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 '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([todo1, 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 + + 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 + describe '#sort' do context 'by date' do let!(:todo1) { create(:todo, user: user, project: project) } diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb index 77410e0070c..f76ed4bfda4 100644 --- a/spec/helpers/issuables_helper_spec.rb +++ b/spec/helpers/issuables_helper_spec.rb @@ -21,6 +21,27 @@ describe IssuablesHelper do end end + describe '#group_dropdown_label' do + let(:group) { create(:group) } + let(:default) { 'default label' } + + it 'returns default group label when group_id is nil' do + expect(group_dropdown_label(nil, default)).to eq('default label') + end + + it 'returns "any group" when group_id is 0' do + expect(group_dropdown_label('0', default)).to eq('Any group') + end + + it 'returns group full path when a group was found for the provided id' do + expect(group_dropdown_label(group.id, default)).to eq(group.full_name) + end + + it 'returns default label when a group was not found for the provided id' do + expect(group_dropdown_label(9999, default)).to eq('default label') + end + end + describe '#issuable_labels_tooltip' do it 'returns label text with no labels' do expect(issuable_labels_tooltip([])).to eq("Labels") diff --git a/spec/javascripts/sidebar/todo_spec.js b/spec/javascripts/sidebar/todo_spec.js new file mode 100644 index 00000000000..a929b804a29 --- /dev/null +++ b/spec/javascripts/sidebar/todo_spec.js @@ -0,0 +1,158 @@ +import Vue from 'vue'; + +import SidebarTodos from '~/sidebar/components/todo_toggle/todo.vue'; +import mountComponent from 'spec/helpers/vue_mount_component_helper'; + +const createComponent = ({ + issuableId = 1, + issuableType = 'epic', + isTodo, + isActionActive, + collapsed, +}) => { + const Component = Vue.extend(SidebarTodos); + + return mountComponent(Component, { + issuableId, + issuableType, + isTodo, + isActionActive, + collapsed, + }); +}; + +describe('SidebarTodo', () => { + let vm; + + beforeEach(() => { + vm = createComponent({}); + }); + + afterEach(() => { + vm.$destroy(); + }); + + describe('computed', () => { + describe('buttonClasses', () => { + it('returns todo button classes for when `collapsed` prop is `false`', () => { + expect(vm.buttonClasses).toBe('btn btn-default btn-todo issuable-header-btn float-right'); + }); + + it('returns todo button classes for when `collapsed` prop is `true`', done => { + vm.collapsed = true; + Vue.nextTick() + .then(() => { + expect(vm.buttonClasses).toBe('btn-blank btn-todo sidebar-collapsed-icon dont-change-state'); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('buttonLabel', () => { + it('returns todo button text for marking todo as done when `isTodo` prop is `true`', () => { + expect(vm.buttonLabel).toBe('Mark todo as done'); + }); + + it('returns todo button text for add todo when `isTodo` prop is `false`', done => { + vm.isTodo = false; + Vue.nextTick() + .then(() => { + expect(vm.buttonLabel).toBe('Add todo'); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('collapsedButtonIconClasses', () => { + it('returns collapsed button icon class when `isTodo` prop is `true`', () => { + expect(vm.collapsedButtonIconClasses).toBe('todo-undone'); + }); + + it('returns empty string when `isTodo` prop is `false`', done => { + vm.isTodo = false; + Vue.nextTick() + .then(() => { + expect(vm.collapsedButtonIconClasses).toBe(''); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('collapsedButtonIcon', () => { + it('returns button icon name when `isTodo` prop is `true`', () => { + expect(vm.collapsedButtonIcon).toBe('todo-done'); + }); + + it('returns button icon name when `isTodo` prop is `false`', done => { + vm.isTodo = false; + Vue.nextTick() + .then(() => { + expect(vm.collapsedButtonIcon).toBe('todo-add'); + }) + .then(done) + .catch(done.fail); + }); + }); + }); + + describe('methods', () => { + describe('handleButtonClick', () => { + it('emits `toggleTodo` event on component', () => { + spyOn(vm, '$emit'); + vm.handleButtonClick(); + expect(vm.$emit).toHaveBeenCalledWith('toggleTodo'); + }); + }); + }); + + describe('template', () => { + it('renders component container element', () => { + const dataAttributes = { + issuableId: '1', + issuableType: 'epic', + originalTitle: 'Mark todo as done', + placement: 'left', + container: 'body', + boundary: 'viewport', + }; + expect(vm.$el.nodeName).toBe('BUTTON'); + + const elDataAttrs = vm.$el.dataset; + Object.keys(elDataAttrs).forEach((attr) => { + expect(elDataAttrs[attr]).toBe(dataAttributes[attr]); + }); + }); + + it('renders button label element when `collapsed` prop is `false`', () => { + const buttonLabelEl = vm.$el.querySelector('span.issuable-todo-inner'); + expect(buttonLabelEl).not.toBeNull(); + expect(buttonLabelEl.innerText.trim()).toBe('Mark todo as done'); + }); + + it('renders button icon when `collapsed` prop is `true`', done => { + vm.collapsed = true; + Vue.nextTick() + .then(() => { + const buttonIconEl = vm.$el.querySelector('svg'); + expect(buttonIconEl).not.toBeNull(); + expect(buttonIconEl.querySelector('use').getAttribute('xlink:href')).toContain('todo-done'); + }) + .then(done) + .catch(done.fail); + }); + + it('renders loading icon when `isActionActive` prop is true', done => { + vm.isActionActive = true; + Vue.nextTick() + .then(() => { + const loadingEl = vm.$el.querySelector('span.loading-container'); + expect(loadingEl).not.toBeNull(); + }) + .then(done) + .catch(done.fail); + }); + }); +}); 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 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/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 48d689e11d4..7c5c7409cc1 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -12,13 +12,17 @@ describe Groups::UpdateService do let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) } before do - public_group.add_user(user, Gitlab::Access::MAINTAINER) + public_group.add_user(user, Gitlab::Access::OWNER) create(:project, :public, group: public_group) + + expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in) end it "does not change permission level" do service.execute expect(public_group.errors.count).to eq(1) + + expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in) end end @@ -26,8 +30,10 @@ describe Groups::UpdateService do let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } before do - internal_group.add_user(user, Gitlab::Access::MAINTAINER) + internal_group.add_user(user, Gitlab::Access::OWNER) create(:project, :internal, group: internal_group) + + expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in) end it "does not change permission level" do @@ -35,6 +41,24 @@ describe Groups::UpdateService do expect(internal_group.errors.count).to eq(1) end end + + context "internal group with private project" do + let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) } + + before do + internal_group.add_user(user, Gitlab::Access::OWNER) + create(:project, :private, group: internal_group) + + expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in) + .with(1.hour, internal_group.id) + end + + it "changes permission level to private" do + service.execute + expect(internal_group.visibility_level) + .to eq(Gitlab::VisibilityLevel::PRIVATE) + end + end end context "with parent_id user doesn't have permissions for" do diff --git a/spec/services/todos/destroy/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb index 54d1d7e83f1..3294f7509aa 100644 --- a/spec/services/todos/destroy/confidential_issue_service_spec.rb +++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb @@ -29,12 +29,8 @@ describe Todos::Destroy::ConfidentialIssueService do issue.update!(confidential: true) end - it 'removes issue todos for a user who is not a project member' do + it 'removes issue todos for users who can not access the confidential issue' do expect { subject }.to change { Todo.count }.from(6).to(4) - - expect(user.todos).to match_array([todo_another_non_member]) - expect(author.todos).to match_array([todo_issue_author]) - expect(project_member.todos).to match_array([todo_issue_member]) end end diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index bad408a314e..8cb91e7c1b9 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -5,60 +5,120 @@ describe Todos::Destroy::EntityLeaveService do let(:project) { create(:project, group: group) } let(:user) { create(:user) } let(:user2) { create(:user) } - let(:issue) { create(:issue, project: project) } + let(:issue) { create(:issue, project: project, confidential: true) } let(:mr) { create(:merge_request, source_project: project) } let!(:todo_mr_user) { create(:todo, user: user, target: mr, project: project) } let!(:todo_issue_user) { create(:todo, user: user, target: issue, project: project) } + let!(:todo_group_user) { create(:todo, user: user, group: group) } let!(:todo_issue_user2) { create(:todo, user: user2, target: issue, project: project) } + let!(:todo_group_user2) { create(:todo, user: user2, group: group) } describe '#execute' do context 'when a user leaves a project' do subject { described_class.new(user.id, project.id, 'Project').execute } context 'when project is private' do - it 'removes todos for the provided user' do - expect { subject }.to change { Todo.count }.from(3).to(1) + it 'removes project todos for the provided user' do + expect { subject }.to change { Todo.count }.from(5).to(3) - expect(user.todos).to be_empty - expect(user2.todos).to match_array([todo_issue_user2]) + expect(user.todos).to match_array([todo_group_user]) + expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2]) end - end - context 'when project is not private' do - before do - group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + context 'when the user is member of the project' do + before do + project.add_developer(user) + end + + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end end - context 'when a user is not an author of confidential issue' do + context 'when the user is a project guest' do before do - issue.update!(confidential: true) + project.add_guest(user) end it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(3).to(2) + expect { subject }.to change { Todo.count }.from(5).to(4) end end - context 'when a user is an author of confidential issue' do + context 'when the user is member of a parent group' do before do - issue.update!(author: user, confidential: true) + group.add_developer(user) end - it 'removes only confidential issues todos' do + it 'does not remove any todos' do expect { subject }.not_to change { Todo.count } end end - context 'when a user is an assignee of confidential issue' do + context 'when the user is guest of a parent group' do before do - issue.update!(confidential: true) - issue.assignees << user + project.add_guest(user) end it 'removes only confidential issues todos' do - expect { subject }.not_to change { Todo.count } + expect { subject }.to change { Todo.count }.from(5).to(4) + end + end + end + + context 'when project is not private' do + before do + group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + end + + context 'confidential issues' do + context 'when a user is not an author of confidential issue' do + it 'removes only confidential issues todos' do + expect { subject }.to change { Todo.count }.from(5).to(4) + end + end + + context 'when a user is an author of confidential issue' do + before do + issue.update!(author: user) + end + + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end + end + + context 'when a user is an assignee of confidential issue' do + before do + issue.assignees << user + end + + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end + end + + context 'when a user is a project guest' do + before do + project.add_guest(user) + end + + it 'removes only confidential issues todos' do + expect { subject }.to change { Todo.count }.from(5).to(4) + end + end + + context 'when a user is a project guest but group developer' do + before do + project.add_guest(user) + group.add_developer(user) + end + + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end end end @@ -69,7 +129,7 @@ describe Todos::Destroy::EntityLeaveService do end it 'removes only users issue todos' do - expect { subject }.to change { Todo.count }.from(3).to(2) + expect { subject }.to change { Todo.count }.from(5).to(4) end end end @@ -80,40 +140,135 @@ describe Todos::Destroy::EntityLeaveService do subject { described_class.new(user.id, group.id, 'Group').execute } context 'when group is private' do - it 'removes todos for the user' do - expect { subject }.to change { Todo.count }.from(3).to(1) + it 'removes group and subproject todos for the user' do + expect { subject }.to change { Todo.count }.from(5).to(2) expect(user.todos).to be_empty - expect(user2.todos).to match_array([todo_issue_user2]) + expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2]) + end + + context 'when the user is member of the group' do + before do + group.add_developer(user) + end + + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end + end + + context 'when the user is member of the group project but not the group' do + before do + project.add_developer(user) + end + + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end end context 'with nested groups', :nested_groups do let(:subgroup) { create(:group, :private, parent: group) } + let(:subgroup2) { create(:group, :private, parent: group) } let(:subproject) { create(:project, group: subgroup) } + let(:subproject2) { create(:project, group: subgroup2) } - let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) } + let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) } + let!(:todo_subproject2_user) { create(:todo, user: user, project: subproject2) } + let!(:todo_subgroup_user) { create(:todo, user: user, group: subgroup) } + let!(:todo_subgroup2_user) { create(:todo, user: user, group: subgroup2) } let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) } + let!(:todo_subpgroup_user2) { create(:todo, user: user2, group: subgroup) } + + context 'when the user is not a member of any groups/projects' do + it 'removes todos for the user including subprojects todos' do + expect { subject }.to change { Todo.count }.from(11).to(4) + + expect(user.todos).to be_empty + expect(user2.todos) + .to match_array( + [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + ) + end + end + + context 'when the user is member of a parent group' do + before do + parent_group = create(:group) + group.update!(parent: parent_group) + parent_group.add_developer(user) + end + + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end + end + + context 'when the user is member of a subgroup' do + before do + subgroup.add_developer(user) + end - it 'removes todos for the user including subprojects todos' do - expect { subject }.to change { Todo.count }.from(5).to(2) + it 'does not remove group and subproject todos' do + expect { subject }.to change { Todo.count }.from(11).to(7) - expect(user.todos).to be_empty - expect(user2.todos) - .to match_array([todo_issue_user2, todo_subproject_user2]) + expect(user.todos).to match_array([todo_group_user, todo_subgroup_user, todo_subproject_user]) + expect(user2.todos) + .to match_array( + [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + ) + end + end + + context 'when the user is member of a child project' do + before do + subproject.add_developer(user) + end + + it 'does not remove subproject and group todos' do + expect { subject }.to change { Todo.count }.from(11).to(7) + + expect(user.todos).to match_array([todo_subgroup_user, todo_group_user, todo_subproject_user]) + expect(user2.todos) + .to match_array( + [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + ) + end end end end context 'when group is not private' do before do - issue.update!(confidential: true) - group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) end - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(3).to(2) + context 'when user is not member' do + it 'removes only confidential issues todos' do + expect { subject }.to change { Todo.count }.from(5).to(4) + end + end + + context 'when user is a project guest' do + before do + project.add_guest(user) + end + + it 'removes only confidential issues todos' do + expect { subject }.to change { Todo.count }.from(5).to(4) + end + end + + context 'when user is a project guest & group developer' do + before do + project.add_guest(user) + group.add_developer(user) + end + + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end end end end diff --git a/spec/services/todos/destroy/group_private_service_spec.rb b/spec/services/todos/destroy/group_private_service_spec.rb new file mode 100644 index 00000000000..2f49b68f544 --- /dev/null +++ b/spec/services/todos/destroy/group_private_service_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' + +describe Todos::Destroy::GroupPrivateService do + let(:group) { create(:group, :public) } + let(:project) { create(:project, group: group) } + let(:user) { create(:user) } + let(:group_member) { create(:user) } + let(:project_member) { create(:user) } + + let!(:todo_non_member) { create(:todo, user: user, group: group) } + let!(:todo_another_non_member) { create(:todo, user: user, group: group) } + let!(:todo_group_member) { create(:todo, user: group_member, group: group) } + let!(:todo_project_member) { create(:todo, user: project_member, group: group) } + + describe '#execute' do + before do + group.add_developer(group_member) + project.add_developer(project_member) + end + + subject { described_class.new(group.id).execute } + + context 'when a group set to private' do + before do + group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + it 'removes todos only for users who are not group users' do + expect { subject }.to change { Todo.count }.from(4).to(2) + + expect(user.todos).to be_empty + expect(group_member.todos).to match_array([todo_group_member]) + expect(project_member.todos).to match_array([todo_project_member]) + end + + 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) } + + let(:parent_member) { create(:user) } + let(:subgroup_member) { create(:user) } + let(:subgproject_member) { create(:user) } + + let!(:todo_parent_member) { create(:todo, user: parent_member, group: group) } + let!(:todo_subgroup_member) { create(:todo, user: subgroup_member, group: group) } + let!(:todo_subproject_member) { create(:todo, user: subgproject_member, group: group) } + + before do + group.update!(parent: parent_group) + + parent_group.add_developer(parent_member) + subgroup.add_developer(subgroup_member) + subproject.add_developer(subgproject_member) + end + + it 'removes todos only for users who are not group users' do + expect { subject }.to change { Todo.count }.from(7).to(5) + end + end + end + + context 'when group is not private' do + it 'does not remove any todos' do + expect { subject }.not_to change { Todo.count } + end + end + end +end diff --git a/spec/services/todos/destroy/project_private_service_spec.rb b/spec/services/todos/destroy/project_private_service_spec.rb index badf3f913a5..128d3487514 100644 --- a/spec/services/todos/destroy/project_private_service_spec.rb +++ b/spec/services/todos/destroy/project_private_service_spec.rb @@ -1,17 +1,21 @@ require 'spec_helper' describe Todos::Destroy::ProjectPrivateService do - let(:project) { create(:project, :public) } + let(:group) { create(:group, :public) } + let(:project) { create(:project, :public, group: group) } let(:user) { create(:user) } let(:project_member) { create(:user) } + let(:group_member) { create(:user) } - let!(:todo_issue_non_member) { create(:todo, user: user, project: project) } - let!(:todo_issue_member) { create(:todo, user: project_member, project: project) } - let!(:todo_another_non_member) { create(:todo, user: user, project: project) } + 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_group_member) { create(:todo, user: group_member, project: project) } describe '#execute' do before do project.add_developer(project_member) + group.add_developer(group_member) end subject { described_class.new(project.id).execute } @@ -22,10 +26,11 @@ describe Todos::Destroy::ProjectPrivateService do end it 'removes issue todos for a user who is not a member' do - expect { subject }.to change { Todo.count }.from(3).to(1) + expect { subject }.to change { Todo.count }.from(4).to(2) expect(user.todos).to be_empty - expect(project_member.todos).to match_array([todo_issue_member]) + expect(project_member.todos).to match_array([todo_member]) + expect(group_member.todos).to match_array([todo_group_member]) 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 diff --git a/spec/workers/todos_destroyer/group_private_worker_spec.rb b/spec/workers/todos_destroyer/group_private_worker_spec.rb new file mode 100644 index 00000000000..fcc38989ced --- /dev/null +++ b/spec/workers/todos_destroyer/group_private_worker_spec.rb @@ -0,0 +1,12 @@ +require 'spec_helper' + +describe TodosDestroyer::GroupPrivateWorker do + it "calls the Todos::Destroy::GroupPrivateService with the params it was given" do + service = double + + expect(::Todos::Destroy::GroupPrivateService).to receive(:new).with(100).and_return(service) + expect(service).to receive(:execute) + + described_class.new.perform(100) + end +end |