Merge branch 'group-todos' into 'master'

Group todos

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

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

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>

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

app/assets/stylesheets/pages/issuable.scss

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

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

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

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)

app/controllers/projects/todos_controller.rb

@@ -1,19 +1,13 @@
 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
-    @issuable ||= begin
+    strong_memoize(:issuable) do
       case params[:issuable_type]
       when "issue"
         IssuesFinder.new(current_user, project_id: @project.id).find(params[:issuable_id])

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)
+  def group
-    ids = items.except(:order).select(:project_id)
+    strong_memoize(:group) do
-    if Gitlab::Database.mysql?
+      Group.find(params[:group_id])
-      # 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).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)
+      items = items.where(project: project)
-    else
-      projects = Project.public_or_visible_to_user(current_user)
-
-      items.joins(:project).merge(projects)
     end
+
+    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)

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

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

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

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

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

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

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

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

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)
+    directly_addressed_users = filter_directly_addressed_users(parent, note || target, author, skip_users)
-    attributes = attributes_for_todo(project, target, author, Todo::DIRECTLY_ADDRESSED, note)
+    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)
+    mentioned_users = filter_mentioned_users(parent, note || target, author, skip_users)
-    attributes = attributes_for_todo(project, target, author, Todo::MENTIONED, note)
+    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)
+  def filter_todo_users(users, parent, target)
-    reject_users_without_access(users, project, target).uniq
+    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)
+  def reject_users_without_access(users, parent, target)
-    if target.is_a?(Note) && (target.for_issue? || target.for_merge_request?)
+    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
 

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
 
+      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
+
+        if entity.private?
+          remove_project_todos
+          remove_group_todos
+        else
+          enqueue_private_features_worker
+        end
+      end
+
       private
 
-      override :todos
+      def enqueue_private_features_worker
-      def todos
-        if entity.private?
-          Todo.where(project_id: project_ids, user_id: user_id)
-        else
         project_ids.each do |project_id|
-            TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_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
+          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
+        condition = case entity
                     when Project
-          [entity.id]
+                      { id: entity.id }
                     when Namespace
-          Project.select(:id).where(namespace_id: entity.self_and_descendants.select(:id))
+                      { namespace_id: non_member_groups }
-        end
                     end
 
-      override :todos_to_remove?
+        Project.where(condition).select(:id)
-      def todos_to_remove?
+      end
-        # 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

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

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

app/views/dashboard/todos/index.html.haml

@@ -30,7 +30,13 @@
 
   .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
+      = 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])

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

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

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

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

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

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 |

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

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
+      it_behaves_like 'project todos actions'
-        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 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
+      it_behaves_like 'project todos actions'
-        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

@@ -1,8 +1,8 @@
 FactoryBot.define do
   factory :todo do
     project
-    author { project.creator }
+    author { project&.creator || user }
-    user { project.creator }
+    user { project&.creator || user }
     target factory: :issue
     action { Todo::ASSIGNED }
 

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

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

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

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

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

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

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
 

spec/services/todos/destroy/entity_leave_service_spec.rb

@@ -5,23 +5,65 @@ 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
+        it 'removes project todos for the provided user' do
-          expect { subject }.to change { Todo.count }.from(3).to(1)
+          expect { subject }.to change { Todo.count }.from(5).to(3)
 
-          expect(user.todos).to be_empty
+          expect(user.todos).to match_array([todo_group_user])
-          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 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 the 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 the user is member of a parent 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 guest of a parent group' 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
       end
 
@@ -31,37 +73,55 @@ describe Todos::Destroy::EntityLeaveService do
           project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
         end
 
+        context 'confidential issues' do
           context 'when a user is not an author of confidential issue' do
-          before do
-            issue.update!(confidential: true)
-          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
             before do
-            issue.update!(author: user, confidential: true)
+              issue.update!(author: 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
             before do
-            issue.update!(confidential: true)
               issue.assignees << 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 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
+
         context 'feature visibility check' do
           context 'when issues are visible only to project members' do
             before do
@@ -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
+        it 'removes group and subproject todos for the user' do
-          expect { subject }.to change { Todo.count }.from(3).to(1)
+          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_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(5).to(2)
+              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_subproject_user2])
+                .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 'does not remove group and subproject todos' do
+              expect { subject }.to change { Todo.count }.from(11).to(7)
+
+              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
 
+        context 'when user is not member' do
           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 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

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

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_non_member)   { create(:todo, user: user, project: project) }
-  let!(:todo_issue_member)       { create(:todo, user: project_member, project: project) }
+  let!(:todo2_non_member)  { create(:todo, user: user, project: project) }
-  let!(:todo_another_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
 

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

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