diff options
30 files changed, 184 insertions, 640 deletions
diff --git a/app/assets/javascripts/pages/dashboard/todos/index/todos.js b/app/assets/javascripts/pages/dashboard/todos/index/todos.js index 9aa83ce6269..ff19b9a9c30 100644 --- a/app/assets/javascripts/pages/dashboard/todos/index/todos.js +++ b/app/assets/javascripts/pages/dashboard/todos/index/todos.js @@ -39,7 +39,6 @@ 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'); @@ -54,16 +53,7 @@ export default class Todos { filterable: searchFields ? true : false, search: { fields: searchFields }, data: $dropdown.data('data'), - 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(); - }, + clicked: () => $dropdown.closest('form.filter-form').submit(), }); } diff --git a/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue b/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue deleted file mode 100644 index ffaed9c7193..00000000000 --- a/app/assets/javascripts/sidebar/components/todo_toggle/todo.vue +++ /dev/null @@ -1,98 +0,0 @@ -<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 80dc7d3557c..ac2e99abe77 100644 --- a/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue +++ b/app/assets/javascripts/vue_shared/components/sidebar/toggle_sidebar.vue @@ -12,11 +12,6 @@ export default { type: Boolean, required: true, }, - cssClasses: { - type: String, - required: false, - default: '', - }, }, computed: { tooltipLabel() { @@ -35,12 +30,10 @@ 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 f6617380cc0..f9fd9f1ab8b 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -449,7 +449,6 @@ .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 010a2c05a1c..e5d7dd13915 100644 --- a/app/assets/stylesheets/pages/todos.scss +++ b/app/assets/stylesheets/pages/todos.scss @@ -174,18 +174,6 @@ } } -@include media-breakpoint-down(lg) { - .todos-filters { - .filter-categories { - width: 75%; - - .filter-item { - margin-bottom: 10px; - } - } - } -} - @include media-breakpoint-down(xs) { .todo { .avatar { @@ -211,10 +199,6 @@ } .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 deleted file mode 100644 index c0acdb3498d..00000000000 --- a/app/controllers/concerns/todos_actions.rb +++ /dev/null @@ -1,12 +0,0 @@ -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 bd7111e28bc..f9e8fe624e8 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, :group_id) + params.permit(:action_id, :author_id, :project_id, :type, :sort, :state) end def redirect_out_of_range(todos) diff --git a/app/controllers/projects/todos_controller.rb b/app/controllers/projects/todos_controller.rb index 93fb9da6510..a41fcb85c40 100644 --- a/app/controllers/projects/todos_controller.rb +++ b/app/controllers/projects/todos_controller.rb @@ -1,13 +1,19 @@ class Projects::TodosController < Projects::ApplicationController - include Gitlab::Utils::StrongMemoize - include TodosActions - before_action :authenticate_user!, only: [:create] + 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 + private def issuable - strong_memoize(:issuable) do + @issuable ||= begin 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 2156413fb26..09e2c586f2a 100644 --- a/app/finders/todos_finder.rb +++ b/app/finders/todos_finder.rb @@ -15,7 +15,6 @@ class TodosFinder prepend FinderWithCrossProjectAccess include FinderMethods - include Gitlab::Utils::StrongMemoize requires_cross_project_access unless: -> { project? } @@ -35,11 +34,9 @@ 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 @@ -85,10 +82,6 @@ class TodosFinder params[:project_id].present? end - def group? - params[:group_id].present? - end - def project return @project if defined?(@project) @@ -107,14 +100,18 @@ class TodosFinder @project end - def group - strong_memoize(:group) do - Group.find(params[:group_id]) + 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) + type.present? && %w(Issue MergeRequest).include?(type) end def type @@ -151,37 +148,12 @@ class TodosFinder def by_project(items) if project? - items = items.where(project: project) - end - - items - end + items.where(project: project) + else + projects = Project.public_or_visible_to_user(current_user) - 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) - ) + items.joins(:project).merge(projects) 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.select(:id), - groups.select(:id) - ) end def by_state(items) diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 7bbdc798ddd..8766bb43cac 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -131,19 +131,6 @@ 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 7cd74358168..f7620e0b6b8 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.parent, todo.target] + path = [todo.project.namespace.becomes(Namespace), todo.project, todo.target] path.unshift(:pipelines) if todo.build_failed? @@ -167,12 +167,4 @@ 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 7a459078151..b93c1145f82 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -243,12 +243,6 @@ 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 28677320e28..ddebaff50b0 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -39,8 +39,6 @@ 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 @@ -84,12 +82,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/models/issue.rb b/app/models/issue.rb index 983684a5e05..4715d942c8d 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -275,6 +275,10 @@ 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 3918bbee194..abc40d9016e 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -229,10 +229,6 @@ 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 942cbb754e3..a2ab405fdbe 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -22,18 +22,15 @@ 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, :target_type, :user, presence: true + validates :action, :project, :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) } @@ -47,7 +44,7 @@ class Todo < ActiveRecord::Base state :done end - after_save :keep_around_commit, if: :commit_id + after_save :keep_around_commit class << self # Priority sorting isn't displayed in the dropdown, because we don't show @@ -82,10 +79,6 @@ class Todo < ActiveRecord::Base end end - def parent - project - end - def unmergeable? action == UNMERGEABLE end diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 46f12086555..f91cd03bf5c 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(parent, target, author, note = nil, skip_users = []) + def create_mention_todos(project, target, author, note = nil, skip_users = []) # Create Todos for directly addressed users - directly_addressed_users = filter_directly_addressed_users(parent, note || target, author, skip_users) - attributes = attributes_for_todo(parent, target, author, Todo::DIRECTLY_ADDRESSED, note) + directly_addressed_users = filter_directly_addressed_users(project, note || target, author, skip_users) + attributes = attributes_for_todo(project, target, author, Todo::DIRECTLY_ADDRESSED, note) create_todos(directly_addressed_users, attributes) # Create Todos for mentioned users - mentioned_users = filter_mentioned_users(parent, note || target, author, skip_users) - attributes = attributes_for_todo(parent, target, author, Todo::MENTIONED, note) + mentioned_users = filter_mentioned_users(project, note || target, author, skip_users) + attributes = attributes_for_todo(project, target, author, Todo::MENTIONED, note) create_todos(mentioned_users, attributes) end @@ -299,36 +299,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, parent, target) - reject_users_without_access(users, parent, target).uniq + def filter_todo_users(users, project, target) + reject_users_without_access(users, project, target).uniq end - def filter_mentioned_users(parent, target, author, skip_users = []) + def filter_mentioned_users(project, target, author, skip_users = []) mentioned_users = target.mentioned_users(author) - skip_users - filter_todo_users(mentioned_users, parent, target) + filter_todo_users(mentioned_users, project, target) end - def filter_directly_addressed_users(parent, target, author, skip_users = []) + def filter_directly_addressed_users(project, target, author, skip_users = []) directly_addressed_users = target.directly_addressed_users(author) - skip_users - filter_todo_users(directly_addressed_users, parent, target) + filter_todo_users(directly_addressed_users, project, target) end - def reject_users_without_access(users, parent, target) - if target.is_a?(Note) && target.for_issuable? + def reject_users_without_access(users, project, target) + if target.is_a?(Note) && (target.for_issue? || target.for_merge_request?) target = target.noteable end if target.is_a?(Issuable) select_users(users, :"read_#{target.to_ability_name}", target) else - select_users(users, :read_project, parent) + select_users(users, :read_project, project) end end diff --git a/app/views/dashboard/todos/index.html.haml b/app/views/dashboard/todos/index.html.haml index 8b3974d97f8..d5a9cc646a6 100644 --- a/app/views/dashboard/todos/index.html.haml +++ b/app/views/dashboard/todos/index.html.haml @@ -30,33 +30,27 @@ .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 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' } }) + = 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' } }) .filter-item.sort-filter .dropdown %button.dropdown-menu-toggle.dropdown-menu-toggle-sort{ type: 'button', 'data-toggle' => 'dropdown' } diff --git a/db/migrate/20180608091413_add_group_to_todos.rb b/db/migrate/20180608091413_add_group_to_todos.rb deleted file mode 100644 index af3ee48b29d..00000000000 --- a/db/migrate/20180608091413_add_group_to_todos.rb +++ /dev/null @@ -1,32 +0,0 @@ -class AddGroupToTodos < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_column :todos, :group_id, :integer - 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 - 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 75a1960e2f8..d2aa31fae30 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1950,7 +1950,7 @@ ActiveRecord::Schema.define(version: 20180704204006) do create_table "todos", force: :cascade do |t| t.integer "user_id", null: false - t.integer "project_id" + t.integer "project_id", null: false t.integer "target_id" t.string "target_type", null: false t.integer "author_id", null: false @@ -1960,12 +1960,10 @@ ActiveRecord::Schema.define(version: 20180704204006) 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 @@ -2339,7 +2337,6 @@ ActiveRecord::Schema.define(version: 20180704204006) 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 0843e4eedc6..27e623007cc 100644 --- a/doc/api/todos.md +++ b/doc/api/todos.md @@ -18,7 +18,6 @@ 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 dda82352c67..760cd87d4cc 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -109,7 +109,6 @@ 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 40df1e79bc7..66b62d9ee2e 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -775,33 +775,28 @@ module API class Todo < Grape::Entity expose :id - expose :project, using: Entities::ProjectIdentity, if: -> (todo, _) { todo.project_id } - expose :group, using: 'API::Entities::NamespaceBasic', if: -> (todo, _) { todo.group_id } + expose :project, using: Entities::BasicProjectDetails expose :author, using: Entities::UserBasic expose :action_name expose :target_type expose :target do |todo, options| - todo_target_class(todo.target_type).represent(todo.target, options) + Entities.const_get(todo.target_type).represent(todo.target, options) end expose :target_url do |todo, options| target_type = todo.target_type.underscore - target_url = "#{todo.parent.class.to_s.underscore}_#{target_type}_url" + target_url = "namespace_project_#{target_type}_url" target_anchor = "note_#{todo.note_id}" if todo.note_id? Gitlab::Routing .url_helpers - .public_send(target_url, todo.parent, todo.target, anchor: target_anchor) # rubocop:disable GitlabSecurity/PublicSend + .public_send(target_url, todo.project.namespace, todo.project, 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 58f2817c7cc..1ce7e84bef9 100644 --- a/spec/controllers/projects/todos_controller_spec.rb +++ b/spec/controllers/projects/todos_controller_spec.rb @@ -5,29 +5,10 @@ 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 post_create + def go post :create, namespace_id: project.namespace, project_id: project, @@ -36,13 +17,66 @@ describe Projects::TodosController do format: 'html' end - it_behaves_like 'project todos actions' + 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 end end context 'Merge Requests' do describe 'POST create' do - def post_create + def go post :create, namespace_id: project.namespace, project_id: project, @@ -51,7 +85,60 @@ describe Projects::TodosController do format: 'html' end - it_behaves_like 'project todos actions' + 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 end end end diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb index 14486c80341..94f8caedfa6 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 } - user { project&.creator || user } + author { project.creator } + user { project.creator } target factory: :issue action { Todo::ASSIGNED } diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb index 6061021d3b0..9747b9402a7 100644 --- a/spec/finders/todos_finder_spec.rb +++ b/spec/finders/todos_finder_spec.rb @@ -5,76 +5,12 @@ 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([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 f76ed4bfda4..77410e0070c 100644 --- a/spec/helpers/issuables_helper_spec.rb +++ b/spec/helpers/issuables_helper_spec.rb @@ -21,27 +21,6 @@ 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 deleted file mode 100644 index a929b804a29..00000000000 --- a/spec/javascripts/sidebar/todo_spec.js +++ /dev/null @@ -1,158 +0,0 @@ -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 f29abcf536e..bd498269798 100644 --- a/spec/models/todo_spec.rb +++ b/spec/models/todo_spec.rb @@ -7,7 +7,6 @@ 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/support/shared_examples/controllers/todos_shared_examples.rb b/spec/support/shared_examples/controllers/todos_shared_examples.rb deleted file mode 100644 index bafd9bac8d0..00000000000 --- a/spec/support/shared_examples/controllers/todos_shared_examples.rb +++ /dev/null @@ -1,43 +0,0 @@ -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 |