Merge branch 'revert-todos-epic' into 'master'

Revert "Merge branch 'ee-5481-epic-todos' into 'master'"

See merge request gitlab-org/gitlab-ce!20560

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(),
     });
   }
 

app/assets/javascripts/sidebar/components/todo_toggle/todo.vue

-<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>

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

app/assets/stylesheets/pages/issuable.scss

@@ -449,7 +449,6 @@
 
       .todo-undone {
         color: $gl-link-color;
-        fill: $gl-link-color;
       }
 
       .author {

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%;
     }

app/controllers/concerns/todos_actions.rb

-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

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)

app/controllers/projects/todos_controller.rb

 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])

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)

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

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

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

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')

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

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

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

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
 

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' }

db/migrate/20180608091413_add_group_to_todos.rb

-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

db/schema.rb

@@ -1949,7 +1949,7 @@ ActiveRecord::Schema.define(version: 20180702120647) 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
@@ -1959,12 +1959,10 @@ ActiveRecord::Schema.define(version: 20180702120647) 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
@@ -2338,7 +2336,6 @@ ActiveRecord::Schema.define(version: 20180702120647) 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

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` |
 

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 |

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

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

spec/factories/todos.rb

 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 }
 

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) }

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")

spec/javascripts/sidebar/todo_spec.js

-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);
-    });
-  });
-});

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

spec/support/shared_examples/controllers/todos_shared_examples.rb

-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