Merge branch 'group-todos' into 'master'

Group todos

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

14 files changed, 607 insertions, 160 deletions

diff --git a/spec/controllers/projects/todos_controller_spec.rb b/spec/controllers/projects/todos_controller_spec.rb
index 1ce7e84bef9..58f2817c7cc 100644
--- a/spec/controllers/projects/todos_controller_spec.rb
+++ b/spec/controllers/projects/todos_controller_spec.rb
@@ -5,10 +5,29 @@ describe Projects::TodosController do
   let(:project)       { create(:project) }
   let(:issue)         { create(:issue, project: project) }
   let(:merge_request) { create(:merge_request, source_project: project) }
+  let(:parent)        { project }
+
+  shared_examples 'project todos actions' do
+    it_behaves_like 'todos actions'
+
+    context 'when not authorized for resource' do
+      before do
+        project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+        project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
+        project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+        sign_in(user)
+      end
+
+      it "doesn't create todo" do
+        expect { post_create }.not_to change { user.todos.count }
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
 
   context 'Issues' do
     describe 'POST create' do
-      def go
+      def post_create
         post :create,
           namespace_id: project.namespace,
           project_id: project,
@@ -17,66 +36,13 @@ describe Projects::TodosController do
           format: 'html'
       end
 
-      context 'when authorized' do
-        before do
-          sign_in(user)
-          project.add_developer(user)
-        end
-
-        it 'creates todo for issue' do
-          expect do
-            go
-          end.to change { user.todos.count }.by(1)
-
-          expect(response).to have_gitlab_http_status(200)
-        end
-
-        it 'returns todo path and pending count' do
-          go
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response['count']).to eq 1
-          expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}})
-        end
-      end
-
-      context 'when not authorized for project' do
-        it 'does not create todo for issue that user has no access to' do
-          sign_in(user)
-          expect do
-            go
-          end.to change { user.todos.count }.by(0)
-
-          expect(response).to have_gitlab_http_status(404)
-        end
-
-        it 'does not create todo for issue when user not logged in' do
-          expect do
-            go
-          end.to change { user.todos.count }.by(0)
-
-          expect(response).to have_gitlab_http_status(302)
-        end
-      end
-
-      context 'when not authorized for issue' do
-        before do
-          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
-          project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
-          sign_in(user)
-        end
-
-        it "doesn't create todo" do
-          expect { go }.not_to change { user.todos.count }
-          expect(response).to have_gitlab_http_status(404)
-        end
-      end
+      it_behaves_like 'project todos actions'
     end
   end
 
   context 'Merge Requests' do
     describe 'POST create' do
-      def go
+      def post_create
         post :create,
           namespace_id: project.namespace,
           project_id: project,
@@ -85,60 +51,7 @@ describe Projects::TodosController do
           format: 'html'
       end
 
-      context 'when authorized' do
-        before do
-          sign_in(user)
-          project.add_developer(user)
-        end
-
-        it 'creates todo for merge request' do
-          expect do
-            go
-          end.to change { user.todos.count }.by(1)
-
-          expect(response).to have_gitlab_http_status(200)
-        end
-
-        it 'returns todo path and pending count' do
-          go
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response['count']).to eq 1
-          expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}})
-        end
-      end
-
-      context 'when not authorized for project' do
-        it 'does not create todo for merge request user has no access to' do
-          sign_in(user)
-          expect do
-            go
-          end.to change { user.todos.count }.by(0)
-
-          expect(response).to have_gitlab_http_status(404)
-        end
-
-        it 'does not create todo for merge request user has no access to' do
-          expect do
-            go
-          end.to change { user.todos.count }.by(0)
-
-          expect(response).to have_gitlab_http_status(302)
-        end
-      end
-
-      context 'when not authorized for merge_request' do
-        before do
-          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
-          project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
-          sign_in(user)
-        end
-
-        it "doesn't create todo" do
-          expect { go }.not_to change { user.todos.count }
-          expect(response).to have_gitlab_http_status(404)
-        end
-      end
+      it_behaves_like 'project todos actions'
     end
   end
 end
diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb
index 94f8caedfa6..14486c80341 100644
--- a/spec/factories/todos.rb
+++ b/spec/factories/todos.rb
@@ -1,8 +1,8 @@
 FactoryBot.define do
   factory :todo do
     project
-    author { project.creator }
-    user { project.creator }
+    author { project&.creator || user }
+    user { project&.creator || user }
     target factory: :issue
     action { Todo::ASSIGNED }
 
diff --git a/spec/finders/todos_finder_spec.rb b/spec/finders/todos_finder_spec.rb
index 9747b9402a7..7f7cfb2cb98 100644
--- a/spec/finders/todos_finder_spec.rb
+++ b/spec/finders/todos_finder_spec.rb
@@ -5,12 +5,50 @@ describe TodosFinder do
     let(:user) { create(:user) }
     let(:group) { create(:group) }
     let(:project) { create(:project, namespace: group) }
+    let(:issue) { create(:issue, project: project) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
     let(:finder) { described_class }
 
     before do
       group.add_developer(user)
     end
 
+    describe '#execute' do
+      context 'filtering' do
+        let!(:todo1) { create(:todo, user: user, project: project, target: issue) }
+        let!(:todo2) { create(:todo, user: user, group: group, target: merge_request) }
+
+        it 'returns correct todos when filtered by a project' do
+          todos = finder.new(user, { project_id: project.id }).execute
+
+          expect(todos).to match_array([todo1])
+        end
+
+        it 'returns correct todos when filtered by a group' do
+          todos = finder.new(user, { group_id: group.id }).execute
+
+          expect(todos).to match_array([todo1, todo2])
+        end
+
+        it 'returns correct todos when filtered by a type' do
+          todos = finder.new(user, { type: 'Issue' }).execute
+
+          expect(todos).to match_array([todo1])
+        end
+
+        context 'with subgroups', :nested_groups do
+          let(:subgroup) { create(:group, parent: group) }
+          let!(:todo3) { create(:todo, user: user, group: subgroup, target: issue) }
+
+          it 'returns todos from subgroups when filtered by a group' do
+            todos = finder.new(user, { group_id: group.id }).execute
+
+            expect(todos).to match_array([todo1, todo2, todo3])
+          end
+        end
+      end
+    end
+
     describe '#sort' do
       context 'by date' do
         let!(:todo1) { create(:todo, user: user, project: project) }
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 77410e0070c..f76ed4bfda4 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -21,6 +21,27 @@ describe IssuablesHelper do
     end
   end
 
+  describe '#group_dropdown_label' do
+    let(:group)  { create(:group) }
+    let(:default) { 'default label' }
+
+    it 'returns default group label when group_id is nil' do
+      expect(group_dropdown_label(nil, default)).to eq('default label')
+    end
+
+    it 'returns "any group" when group_id is 0' do
+      expect(group_dropdown_label('0', default)).to eq('Any group')
+    end
+
+    it 'returns group full path when a group was found for the provided id' do
+      expect(group_dropdown_label(group.id, default)).to eq(group.full_name)
+    end
+
+    it 'returns default label when a group was not found for the provided id' do
+      expect(group_dropdown_label(9999, default)).to eq('default label')
+    end
+  end
+
   describe '#issuable_labels_tooltip' do
     it 'returns label text with no labels' do
       expect(issuable_labels_tooltip([])).to eq("Labels")
diff --git a/spec/javascripts/sidebar/todo_spec.js b/spec/javascripts/sidebar/todo_spec.js
new file mode 100644
index 00000000000..a929b804a29
--- /dev/null
+++ b/spec/javascripts/sidebar/todo_spec.js
@@ -0,0 +1,158 @@
+import Vue from 'vue';
+
+import SidebarTodos from '~/sidebar/components/todo_toggle/todo.vue';
+import mountComponent from 'spec/helpers/vue_mount_component_helper';
+
+const createComponent = ({
+  issuableId = 1,
+  issuableType = 'epic',
+  isTodo,
+  isActionActive,
+  collapsed,
+}) => {
+  const Component = Vue.extend(SidebarTodos);
+
+  return mountComponent(Component, {
+    issuableId,
+    issuableType,
+    isTodo,
+    isActionActive,
+    collapsed,
+  });
+};
+
+describe('SidebarTodo', () => {
+  let vm;
+
+  beforeEach(() => {
+    vm = createComponent({});
+  });
+
+  afterEach(() => {
+    vm.$destroy();
+  });
+
+  describe('computed', () => {
+    describe('buttonClasses', () => {
+      it('returns todo button classes for when `collapsed` prop is `false`', () => {
+        expect(vm.buttonClasses).toBe('btn btn-default btn-todo issuable-header-btn float-right');
+      });
+
+      it('returns todo button classes for when `collapsed` prop is `true`', done => {
+        vm.collapsed = true;
+        Vue.nextTick()
+          .then(() => {
+            expect(vm.buttonClasses).toBe('btn-blank btn-todo sidebar-collapsed-icon dont-change-state');
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+
+    describe('buttonLabel', () => {
+      it('returns todo button text for marking todo as done when `isTodo` prop is `true`', () => {
+        expect(vm.buttonLabel).toBe('Mark todo as done');
+      });
+
+      it('returns todo button text for add todo when `isTodo` prop is `false`', done => {
+        vm.isTodo = false;
+        Vue.nextTick()
+          .then(() => {
+            expect(vm.buttonLabel).toBe('Add todo');
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+
+    describe('collapsedButtonIconClasses', () => {
+      it('returns collapsed button icon class when `isTodo` prop is `true`', () => {
+        expect(vm.collapsedButtonIconClasses).toBe('todo-undone');
+      });
+
+      it('returns empty string when `isTodo` prop is `false`', done => {
+        vm.isTodo = false;
+        Vue.nextTick()
+          .then(() => {
+            expect(vm.collapsedButtonIconClasses).toBe('');
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+
+    describe('collapsedButtonIcon', () => {
+      it('returns button icon name when `isTodo` prop is `true`', () => {
+        expect(vm.collapsedButtonIcon).toBe('todo-done');
+      });
+
+      it('returns button icon name when `isTodo` prop is `false`', done => {
+        vm.isTodo = false;
+        Vue.nextTick()
+          .then(() => {
+            expect(vm.collapsedButtonIcon).toBe('todo-add');
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+  });
+
+  describe('methods', () => {
+    describe('handleButtonClick', () => {
+      it('emits `toggleTodo` event on component', () => {
+        spyOn(vm, '$emit');
+        vm.handleButtonClick();
+        expect(vm.$emit).toHaveBeenCalledWith('toggleTodo');
+      });
+    });
+  });
+
+  describe('template', () => {
+    it('renders component container element', () => {
+      const dataAttributes = {
+        issuableId: '1',
+        issuableType: 'epic',
+        originalTitle: 'Mark todo as done',
+        placement: 'left',
+        container: 'body',
+        boundary: 'viewport',
+      };
+      expect(vm.$el.nodeName).toBe('BUTTON');
+
+      const elDataAttrs = vm.$el.dataset;
+      Object.keys(elDataAttrs).forEach((attr) => {
+        expect(elDataAttrs[attr]).toBe(dataAttributes[attr]);
+      });
+    });
+
+    it('renders button label element when `collapsed` prop is `false`', () => {
+      const buttonLabelEl = vm.$el.querySelector('span.issuable-todo-inner');
+      expect(buttonLabelEl).not.toBeNull();
+      expect(buttonLabelEl.innerText.trim()).toBe('Mark todo as done');
+    });
+
+    it('renders button icon when `collapsed` prop is `true`', done => {
+      vm.collapsed = true;
+      Vue.nextTick()
+        .then(() => {
+          const buttonIconEl = vm.$el.querySelector('svg');
+          expect(buttonIconEl).not.toBeNull();
+          expect(buttonIconEl.querySelector('use').getAttribute('xlink:href')).toContain('todo-done');
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('renders loading icon when `isActionActive` prop is true', done => {
+      vm.isActionActive = true;
+      Vue.nextTick()
+        .then(() => {
+          const loadingEl = vm.$el.querySelector('span.loading-container');
+          expect(loadingEl).not.toBeNull();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+});
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index bd498269798..f29abcf536e 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -7,6 +7,7 @@ describe Todo do
     it { is_expected.to belong_to(:author).class_name("User") }
     it { is_expected.to belong_to(:note) }
     it { is_expected.to belong_to(:project) }
+    it { is_expected.to belong_to(:group) }
     it { is_expected.to belong_to(:target).touch(true) }
     it { is_expected.to belong_to(:user) }
   end
diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb
index 2ee8d150dc8..b5cf04e7f22 100644
--- a/spec/requests/api/todos_spec.rb
+++ b/spec/requests/api/todos_spec.rb
@@ -1,7 +1,8 @@
 require 'spec_helper'
 
 describe API::Todos do
-  let(:project_1) { create(:project, :repository) }
+  let(:group)     { create(:group) }
+  let(:project_1) { create(:project, :repository, group: group) }
   let(:project_2) { create(:project) }
   let(:author_1) { create(:user) }
   let(:author_2) { create(:user) }
@@ -92,6 +93,17 @@ describe API::Todos do
         end
       end
 
+      context 'and using the group filter' do
+        it 'filters based on project_id param' do
+          get api('/todos', john_doe), { group_id: group.id, sort: :target_id }
+
+          expect(response.status).to eq(200)
+          expect(response).to include_pagination_headers
+          expect(json_response).to be_an Array
+          expect(json_response.length).to eq(2)
+        end
+      end
+
       context 'and using the action filter' do
         it 'filters based on action param' do
           get api('/todos', john_doe), { action: 'mentioned' }
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index 48d689e11d4..7c5c7409cc1 100644
--- a/spec/services/groups/update_service_spec.rb
+++ b/spec/services/groups/update_service_spec.rb
@@ -12,13 +12,17 @@ describe Groups::UpdateService do
         let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
 
         before do
-          public_group.add_user(user, Gitlab::Access::MAINTAINER)
+          public_group.add_user(user, Gitlab::Access::OWNER)
           create(:project, :public, group: public_group)
+
+          expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in)
         end
 
         it "does not change permission level" do
           service.execute
           expect(public_group.errors.count).to eq(1)
+
+          expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in)
         end
       end
 
@@ -26,8 +30,10 @@ describe Groups::UpdateService do
         let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
 
         before do
-          internal_group.add_user(user, Gitlab::Access::MAINTAINER)
+          internal_group.add_user(user, Gitlab::Access::OWNER)
           create(:project, :internal, group: internal_group)
+
+          expect(TodosDestroyer::GroupPrivateWorker).not_to receive(:perform_in)
         end
 
         it "does not change permission level" do
@@ -35,6 +41,24 @@ describe Groups::UpdateService do
           expect(internal_group.errors.count).to eq(1)
         end
       end
+
+      context "internal group with private project" do
+        let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
+
+        before do
+          internal_group.add_user(user, Gitlab::Access::OWNER)
+          create(:project, :private, group: internal_group)
+
+          expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in)
+            .with(1.hour, internal_group.id)
+        end
+
+        it "changes permission level to private" do
+          service.execute
+          expect(internal_group.visibility_level)
+            .to eq(Gitlab::VisibilityLevel::PRIVATE)
+        end
+      end
     end
 
     context "with parent_id user doesn't have permissions for" do
diff --git a/spec/services/todos/destroy/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb
index 54d1d7e83f1..3294f7509aa 100644
--- a/spec/services/todos/destroy/confidential_issue_service_spec.rb
+++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb
@@ -29,12 +29,8 @@ describe Todos::Destroy::ConfidentialIssueService do
         issue.update!(confidential: true)
       end
 
-      it 'removes issue todos for a user who is not a project member' do
+      it 'removes issue todos for users who can not access the confidential issue' do
         expect { subject }.to change { Todo.count }.from(6).to(4)
-
-        expect(user.todos).to match_array([todo_another_non_member])
-        expect(author.todos).to match_array([todo_issue_author])
-        expect(project_member.todos).to match_array([todo_issue_member])
       end
     end
 
diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb
index bad408a314e..8cb91e7c1b9 100644
--- a/spec/services/todos/destroy/entity_leave_service_spec.rb
+++ b/spec/services/todos/destroy/entity_leave_service_spec.rb
@@ -5,60 +5,120 @@ describe Todos::Destroy::EntityLeaveService do
   let(:project) { create(:project, group: group) }
   let(:user)    { create(:user) }
   let(:user2)   { create(:user) }
-  let(:issue)   { create(:issue, project: project) }
+  let(:issue)   { create(:issue, project: project, confidential: true) }
   let(:mr)      { create(:merge_request, source_project: project) }
 
   let!(:todo_mr_user)     { create(:todo, user: user, target: mr, project: project) }
   let!(:todo_issue_user)  { create(:todo, user: user, target: issue, project: project) }
+  let!(:todo_group_user)  { create(:todo, user: user, group: group) }
   let!(:todo_issue_user2) { create(:todo, user: user2, target: issue, project: project) }
+  let!(:todo_group_user2) { create(:todo, user: user2, group: group) }
 
   describe '#execute' do
     context 'when a user leaves a project' do
       subject { described_class.new(user.id, project.id, 'Project').execute }
 
       context 'when project is private' do
-        it 'removes todos for the provided user' do
-          expect { subject }.to change { Todo.count }.from(3).to(1)
+        it 'removes project todos for the provided user' do
+          expect { subject }.to change { Todo.count }.from(5).to(3)
 
-          expect(user.todos).to be_empty
-          expect(user2.todos).to match_array([todo_issue_user2])
+          expect(user.todos).to match_array([todo_group_user])
+          expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2])
         end
-      end
 
-      context 'when project is not private' do
-        before do
-          group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
-          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        context 'when the user is member of the project' do
+          before do
+            project.add_developer(user)
+          end
+
+          it 'does not remove any todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
         end
 
-        context 'when a user is not an author of confidential issue' do
+        context 'when the user is a project guest' do
           before do
-            issue.update!(confidential: true)
+            project.add_guest(user)
           end
 
           it 'removes only confidential issues todos' do
-            expect { subject }.to change { Todo.count }.from(3).to(2)
+            expect { subject }.to change { Todo.count }.from(5).to(4)
           end
         end
 
-        context 'when a user is an author of confidential issue' do
+        context 'when the user is member of a parent group' do
           before do
-            issue.update!(author: user, confidential: true)
+            group.add_developer(user)
           end
 
-          it 'removes only confidential issues todos' do
+          it 'does not remove any todos' do
             expect { subject }.not_to change { Todo.count }
           end
         end
 
-        context 'when a user is an assignee of confidential issue' do
+        context 'when the user is guest of a parent group' do
           before do
-            issue.update!(confidential: true)
-            issue.assignees << user
+            project.add_guest(user)
           end
 
           it 'removes only confidential issues todos' do
-            expect { subject }.not_to change { Todo.count }
+            expect { subject }.to change { Todo.count }.from(5).to(4)
+          end
+        end
+      end
+
+      context 'when project is not private' do
+        before do
+          group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        end
+
+        context 'confidential issues' do
+          context 'when a user is not an author of confidential issue' do
+            it 'removes only confidential issues todos' do
+              expect { subject }.to change { Todo.count }.from(5).to(4)
+            end
+          end
+
+          context 'when a user is an author of confidential issue' do
+            before do
+              issue.update!(author: user)
+            end
+
+            it 'does not remove any todos' do
+              expect { subject }.not_to change { Todo.count }
+            end
+          end
+
+          context 'when a user is an assignee of confidential issue' do
+            before do
+              issue.assignees << user
+            end
+
+            it 'does not remove any todos' do
+              expect { subject }.not_to change { Todo.count }
+            end
+          end
+
+          context 'when a user is a project guest' do
+            before do
+              project.add_guest(user)
+            end
+
+            it 'removes only confidential issues todos' do
+              expect { subject }.to change { Todo.count }.from(5).to(4)
+            end
+          end
+
+          context 'when a user is a project guest but group developer' do
+            before do
+              project.add_guest(user)
+              group.add_developer(user)
+            end
+
+            it 'does not remove any todos' do
+              expect { subject }.not_to change { Todo.count }
+            end
           end
         end
 
@@ -69,7 +129,7 @@ describe Todos::Destroy::EntityLeaveService do
             end
 
             it 'removes only users issue todos' do
-              expect { subject }.to change { Todo.count }.from(3).to(2)
+              expect { subject }.to change { Todo.count }.from(5).to(4)
             end
           end
         end
@@ -80,40 +140,135 @@ describe Todos::Destroy::EntityLeaveService do
       subject { described_class.new(user.id, group.id, 'Group').execute }
 
       context 'when group is private' do
-        it 'removes todos for the user' do
-          expect { subject }.to change { Todo.count }.from(3).to(1)
+        it 'removes group and subproject todos for the user' do
+          expect { subject }.to change { Todo.count }.from(5).to(2)
 
           expect(user.todos).to be_empty
-          expect(user2.todos).to match_array([todo_issue_user2])
+          expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2])
+        end
+
+        context 'when the user is member of the group' do
+          before do
+            group.add_developer(user)
+          end
+
+          it 'does not remove any todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
+        end
+
+        context 'when the user is member of the group project but not the group' do
+          before do
+            project.add_developer(user)
+          end
+
+          it 'does not remove any todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
         end
 
         context 'with nested groups', :nested_groups do
           let(:subgroup) { create(:group, :private, parent: group) }
+          let(:subgroup2) { create(:group, :private, parent: group) }
           let(:subproject) { create(:project, group: subgroup) }
+          let(:subproject2) { create(:project, group: subgroup2) }
 
-          let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) }
+          let!(:todo_subproject_user)  { create(:todo, user: user, project: subproject) }
+          let!(:todo_subproject2_user)  { create(:todo, user: user, project: subproject2) }
+          let!(:todo_subgroup_user)    { create(:todo, user: user, group: subgroup) }
+          let!(:todo_subgroup2_user)    { create(:todo, user: user, group: subgroup2) }
           let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
+          let!(:todo_subpgroup_user2)  { create(:todo, user: user2, group: subgroup) }
+
+          context 'when the user is not a member of any groups/projects' do
+            it 'removes todos for the user including subprojects todos' do
+              expect { subject }.to change { Todo.count }.from(11).to(4)
+
+              expect(user.todos).to be_empty
+              expect(user2.todos)
+                .to match_array(
+                  [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
+                )
+            end
+          end
+
+          context 'when the user is member of a parent group' do
+            before do
+              parent_group = create(:group)
+              group.update!(parent: parent_group)
+              parent_group.add_developer(user)
+            end
+
+            it 'does not remove any todos' do
+              expect { subject }.not_to change { Todo.count }
+            end
+          end
+
+          context 'when the user is member of a subgroup' do
+            before do
+              subgroup.add_developer(user)
+            end
 
-          it 'removes todos for the user including subprojects todos' do
-            expect { subject }.to change { Todo.count }.from(5).to(2)
+            it 'does not remove group and subproject todos' do
+              expect { subject }.to change { Todo.count }.from(11).to(7)
 
-            expect(user.todos).to be_empty
-            expect(user2.todos)
-              .to match_array([todo_issue_user2, todo_subproject_user2])
+              expect(user.todos).to match_array([todo_group_user, todo_subgroup_user, todo_subproject_user])
+              expect(user2.todos)
+                .to match_array(
+                  [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
+                )
+            end
+          end
+
+          context 'when the user is member of a child project' do
+            before do
+              subproject.add_developer(user)
+            end
+
+            it 'does not remove subproject and group todos' do
+              expect { subject }.to change { Todo.count }.from(11).to(7)
+
+              expect(user.todos).to match_array([todo_subgroup_user, todo_group_user, todo_subproject_user])
+              expect(user2.todos)
+                .to match_array(
+                  [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
+                )
+            end
           end
         end
       end
 
       context 'when group is not private' do
         before do
-          issue.update!(confidential: true)
-
           group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
           project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
         end
 
-        it 'removes only confidential issues todos' do
-          expect { subject }.to change { Todo.count }.from(3).to(2)
+        context 'when user is not member' do
+          it 'removes only confidential issues todos' do
+            expect { subject }.to change { Todo.count }.from(5).to(4)
+          end
+        end
+
+        context 'when user is a project guest' do
+          before do
+            project.add_guest(user)
+          end
+
+          it 'removes only confidential issues todos' do
+            expect { subject }.to change { Todo.count }.from(5).to(4)
+          end
+        end
+
+        context 'when user is a project guest & group developer' do
+          before do
+            project.add_guest(user)
+            group.add_developer(user)
+          end
+
+          it 'does not remove any todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
         end
       end
     end
diff --git a/spec/services/todos/destroy/group_private_service_spec.rb b/spec/services/todos/destroy/group_private_service_spec.rb
new file mode 100644
index 00000000000..2f49b68f544
--- /dev/null
+++ b/spec/services/todos/destroy/group_private_service_spec.rb
@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe Todos::Destroy::GroupPrivateService do
+  let(:group)         { create(:group, :public) }
+  let(:project)       { create(:project, group: group) }
+  let(:user)          { create(:user) }
+  let(:group_member)  { create(:user) }
+  let(:project_member)  { create(:user) }
+
+  let!(:todo_non_member)         { create(:todo, user: user, group: group) }
+  let!(:todo_another_non_member) { create(:todo, user: user, group: group) }
+  let!(:todo_group_member)       { create(:todo, user: group_member, group: group) }
+  let!(:todo_project_member)     { create(:todo, user: project_member, group: group) }
+
+  describe '#execute' do
+    before do
+      group.add_developer(group_member)
+      project.add_developer(project_member)
+    end
+
+    subject { described_class.new(group.id).execute }
+
+    context 'when a group set to private' do
+      before do
+        group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'removes todos only for users who are not group users' do
+        expect { subject }.to change { Todo.count }.from(4).to(2)
+
+        expect(user.todos).to be_empty
+        expect(group_member.todos).to match_array([todo_group_member])
+        expect(project_member.todos).to match_array([todo_project_member])
+      end
+
+      context 'with nested groups', :nested_groups do
+        let(:parent_group) { create(:group) }
+        let(:subgroup)     { create(:group, :private, parent: group) }
+        let(:subproject)   { create(:project, group: subgroup) }
+
+        let(:parent_member)  { create(:user) }
+        let(:subgroup_member)  { create(:user) }
+        let(:subgproject_member)  { create(:user) }
+
+        let!(:todo_parent_member)     { create(:todo, user: parent_member, group: group) }
+        let!(:todo_subgroup_member)   { create(:todo, user: subgroup_member, group: group) }
+        let!(:todo_subproject_member) { create(:todo, user: subgproject_member, group: group) }
+
+        before do
+          group.update!(parent: parent_group)
+
+          parent_group.add_developer(parent_member)
+          subgroup.add_developer(subgroup_member)
+          subproject.add_developer(subgproject_member)
+        end
+
+        it 'removes todos only for users who are not group users' do
+          expect { subject }.to change { Todo.count }.from(7).to(5)
+        end
+      end
+    end
+
+    context 'when group is not private' do
+      it 'does not remove any todos' do
+        expect { subject }.not_to change { Todo.count }
+      end
+    end
+  end
+end
diff --git a/spec/services/todos/destroy/project_private_service_spec.rb b/spec/services/todos/destroy/project_private_service_spec.rb
index badf3f913a5..128d3487514 100644
--- a/spec/services/todos/destroy/project_private_service_spec.rb
+++ b/spec/services/todos/destroy/project_private_service_spec.rb
@@ -1,17 +1,21 @@
 require 'spec_helper'
 
 describe Todos::Destroy::ProjectPrivateService do
-  let(:project)        { create(:project, :public) }
+  let(:group)          { create(:group, :public) }
+  let(:project)        { create(:project, :public, group: group) }
   let(:user)           { create(:user) }
   let(:project_member) { create(:user) }
+  let(:group_member)   { create(:user) }
 
-  let!(:todo_issue_non_member)   { create(:todo, user: user, project: project) }
-  let!(:todo_issue_member)       { create(:todo, user: project_member, project: project) }
-  let!(:todo_another_non_member) { create(:todo, user: user, project: project) }
+  let!(:todo_non_member)   { create(:todo, user: user, project: project) }
+  let!(:todo2_non_member)  { create(:todo, user: user, project: project) }
+  let!(:todo_member)       { create(:todo, user: project_member, project: project) }
+  let!(:todo_group_member) { create(:todo, user: group_member, project: project) }
 
   describe '#execute' do
     before do
       project.add_developer(project_member)
+      group.add_developer(group_member)
     end
 
     subject { described_class.new(project.id).execute }
@@ -22,10 +26,11 @@ describe Todos::Destroy::ProjectPrivateService do
       end
 
       it 'removes issue todos for a user who is not a member' do
-        expect { subject }.to change { Todo.count }.from(3).to(1)
+        expect { subject }.to change { Todo.count }.from(4).to(2)
 
         expect(user.todos).to be_empty
-        expect(project_member.todos).to match_array([todo_issue_member])
+        expect(project_member.todos).to match_array([todo_member])
+        expect(group_member.todos).to match_array([todo_group_member])
       end
     end
 
diff --git a/spec/support/shared_examples/controllers/todos_shared_examples.rb b/spec/support/shared_examples/controllers/todos_shared_examples.rb
new file mode 100644
index 00000000000..bafd9bac8d0
--- /dev/null
+++ b/spec/support/shared_examples/controllers/todos_shared_examples.rb
@@ -0,0 +1,43 @@
+shared_examples 'todos actions' do
+  context 'when authorized' do
+    before do
+      sign_in(user)
+      parent.add_developer(user)
+    end
+
+    it 'creates todo' do
+      expect do
+        post_create
+      end.to change { user.todos.count }.by(1)
+
+      expect(response).to have_gitlab_http_status(200)
+    end
+
+    it 'returns todo path and pending count' do
+      post_create
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['count']).to eq 1
+      expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}})
+    end
+  end
+
+  context 'when not authorized for project/group' do
+    it 'does not create todo for resource that user has no access to' do
+      sign_in(user)
+      expect do
+        post_create
+      end.to change { user.todos.count }.by(0)
+
+      expect(response).to have_gitlab_http_status(404)
+    end
+
+    it 'does not create todo when user is not logged in' do
+      expect do
+        post_create
+      end.to change { user.todos.count }.by(0)
+
+      expect(response).to have_gitlab_http_status(parent.is_a?(Group) ? 401 : 302)
+    end
+  end
+end
diff --git a/spec/workers/todos_destroyer/group_private_worker_spec.rb b/spec/workers/todos_destroyer/group_private_worker_spec.rb
new file mode 100644
index 00000000000..fcc38989ced
--- /dev/null
+++ b/spec/workers/todos_destroyer/group_private_worker_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe TodosDestroyer::GroupPrivateWorker do
+  it "calls the Todos::Destroy::GroupPrivateService with the params it was given" do
+    service = double
+
+    expect(::Todos::Destroy::GroupPrivateService).to receive(:new).with(100).and_return(service)
+    expect(service).to receive(:execute)
+
+    described_class.new.perform(100)
+  end
+end