[backend] Addressed review comments

* Group filtering now includes also issues/MRs from
subgroups/subprojects
* fixed due_date
* Also DRYed todo controller specs

app/controllers/concerns/todos_actions.rb

+module TodosActions
+  include Gitlab::Utils::StrongMemoize
+  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

 class Projects::TodosController < Projects::ApplicationController
-  before_action :authenticate_user!, only: [:create]
-
-  def create
-    todo = TodoService.new.mark_todo(issuable, current_user)
+  include TodosActions
 
-    render json: {
-      count: TodosFinder.new(current_user, state: :pending).execute.count,
-      delete_path: dashboard_todo_path(todo)
-    }
-  end
+  before_action :authenticate_user!, only: [:create]
 
   private
 
   def issuable
-    @issuable ||= begin
+    strong_memoize(:issuable) do
       case params[:issuable_type]
       when "issue"
         IssuesFinder.new(current_user, project_id: @project.id).find(params[:issuable_id])

app/finders/todos_finder.rb

@@ -113,16 +113,6 @@ class TodosFinder
     end
   end
 
-  def project_ids(items)
-    ids = items.except(:order).select(:project_id)
-    if Gitlab::Database.mysql?
-      # To make UPDATE work on MySQL, wrap it in a SELECT with an alias
-      ids = Todo.except(:order).select('*').from("(#{ids.to_sql}) AS t")
-    end
-
-    ids
-  end
-
   def type?
     type.present? && %w(Issue MergeRequest Epic).include?(type)
   end
@@ -169,7 +159,12 @@ class TodosFinder
 
   def by_group(items)
     if group?
-      items = items.where(group: group)
+      groups = group.self_and_descendants
+      items = items.where(
+        'project_id IN (?) OR group_id IN  (?)',
+        Project.where(group: groups).select(:id),
+        groups.select(:id)
+      )
     end
 
     items
@@ -184,8 +179,8 @@ class TodosFinder
       .joins('LEFT JOIN projects ON projects.id = todos.project_id')
       .where(
         'project_id IN (?) OR group_id IN  (?)',
-        projects.map(&:id),
-        groups.map(&:id)
+        projects.select(:id),
+        groups.select(:id)
       )
   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/issue.rb

@@ -275,10 +275,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

@@ -229,6 +229,10 @@ class Note < ActiveRecord::Base
     !for_personal_snippet?
   end
 
+  def for_issuable_with_ability?
+    for_issue? || for_merge_request?
+  end
+
   def skip_project_check?
     !for_project_noteable?
   end

app/models/todo.rb

@@ -32,8 +32,8 @@ class Todo < ActiveRecord::Base
   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
-  validates :group, presence: true, unless: :project
+  validates :project, presence: true, unless: :group_id
+  validates :group, presence: true, unless: :project_id
 
   scope :pending, -> { with_state(:pending) }
   scope :done, -> { with_state(:done) }

app/services/todo_service.rb

@@ -285,6 +285,7 @@ class TodoService
   def attributes_for_target(target)
     attributes = {
       project_id: target&.project&.id,
+      group_id: target.respond_to?(:group) ? target.group_id : nil,
       target_id: target.id,
       target_type: target.class.name,
       commit_id: nil
@@ -300,7 +301,6 @@ class TodoService
   def attributes_for_todo(project, target, author, action, note = nil)
     attributes_for_target(target).merge!(
       project_id: project&.id,
-      group_id: target.respond_to?(:group) ? target.group.id : nil,
       author_id: author.id,
       action: action,
       note: note
@@ -322,7 +322,7 @@ class TodoService
   end
 
   def reject_users_without_access(users, parent, target)
-    if target.is_a?(Note) && (target.for_issue? || target.for_merge_request? || target.for_epic?)
+    if target.is_a?(Note) && target.for_issuable_with_ability?
       target = target.noteable
     end
 

db/migrate/20180608091413_add_group_to_todos.rb

@@ -7,7 +7,7 @@ class AddGroupToTodos < ActiveRecord::Migration
 
   def up
     add_column :todos, :group_id, :integer
-    add_foreign_key :todos, :namespaces, column: :group_id, on_delete: :cascade
+    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

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

lib/api/entities.rb

@@ -769,14 +769,14 @@ module API
 
     class Todo < Grape::Entity
       expose :id
-      expose :project, using: Entities::ProjectIdentity, if: -> (todo, _) { todo.project }
-      expose :group, using: 'API::Entities::NamespaceBasic', if: -> (todo, _) { todo.group }
+      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|
@@ -792,6 +792,10 @@ module API
       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,140 +5,53 @@ 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 }
 
-  context 'Issues' do
-    describe 'POST create' do
-      def go
-        post :create,
-          namespace_id: project.namespace,
-          project_id: project,
-          issuable_id: issue.id,
-          issuable_type: 'issue',
-          format: 'html'
-      end
+  shared_examples 'project todos actions' do
+    it_behaves_like 'todos actions'
 
-      context 'when authorized' do
-        before do
-          sign_in(user)
-          project.add_developer(user)
-        end
-
-        it 'creates todo for issue' do
-          expect do
-            go
-          end.to change { user.todos.count }.by(1)
-
-          expect(response).to have_gitlab_http_status(200)
-        end
-
-        it 'returns todo path and pending count' do
-          go
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response['count']).to eq 1
-          expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}})
-        end
-      end
-
-      context 'when not authorized for project' do
-        it 'does not create todo for issue that user has no access to' do
-          sign_in(user)
-          expect do
-            go
-          end.to change { user.todos.count }.by(0)
-
-          expect(response).to have_gitlab_http_status(404)
-        end
-
-        it 'does not create todo for issue when user not logged in' do
-          expect do
-            go
-          end.to change { user.todos.count }.by(0)
-
-          expect(response).to have_gitlab_http_status(302)
-        end
-      end
-
-      context 'when not authorized for issue' do
+    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 { go }.not_to change { user.todos.count }
+        expect { post_create }.not_to change { user.todos.count }
         expect(response).to have_gitlab_http_status(404)
       end
     end
   end
-  end
 
-  context 'Merge Requests' do
+  context 'Issues' do
     describe 'POST create' do
-      def go
+      def post_create
         post :create,
           namespace_id: project.namespace,
           project_id: project,
-          issuable_id: merge_request.id,
-          issuable_type: 'merge_request',
+          issuable_id: issue.id,
+          issuable_type: 'issue',
           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}})
+      it_behaves_like 'project todos actions'
     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)
+  context 'Merge Requests' do
+    describe 'POST create' do
+      def post_create
+        post :create,
+          namespace_id: project.namespace,
+          project_id: project,
+          issuable_id: merge_request.id,
+          issuable_type: 'merge_request',
+          format: 'html'
       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

spec/factories/todos.rb

 FactoryBot.define do
   factory :todo do
     project
-    group
     author { project&.creator || user }
     user { project&.creator || user }
     target factory: :issue

spec/finders/todos_finder_spec.rb

@@ -53,7 +53,7 @@ describe TodosFinder do
         it 'returns correct todos when filtered by a group' do
           todos = finder.new(user, { group_id: group.id }).execute
 
-          expect(todos).to match_array([todo2])
+          expect(todos).to match_array([todo1, todo2])
         end
 
         it 'returns correct todos when filtered by a type' do
@@ -61,6 +61,17 @@ describe TodosFinder do
 
           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
 

spec/support/shared_examples/controllers/todos_shared_examples.rb

+shared_examples 'todos actions' do
+  context 'when authorized' do
+    before do
+      sign_in(user)
+      parent.add_developer(user)
+    end
+
+    it 'creates todo' do
+      expect do
+        post_create
+      end.to change { user.todos.count }.by(1)
+
+      expect(response).to have_gitlab_http_status(200)
+    end
+
+    it 'returns todo path and pending count' do
+      post_create
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['count']).to eq 1
+      expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}})
+    end
+  end
+
+  context 'when not authorized for project/group' do
+    it 'does not create todo for resource that user has no access to' do
+      sign_in(user)
+      expect do
+        post_create
+      end.to change { user.todos.count }.by(0)
+
+      expect(response).to have_gitlab_http_status(404)
+    end
+
+    it 'does not create todo when user is not logged in' do
+      expect do
+        post_create
+      end.to change { user.todos.count }.by(0)
+
+      expect(response).to have_gitlab_http_status(parent.is_a?(Group) ? 401 : 302)
+    end
+  end
+end