[backend] Addressed review comments

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

9 files changed, 39 insertions, 32 deletions

diff --git a/app/controllers/concerns/todos_actions.rb b/app/controllers/concerns/todos_actions.rb
new file mode 100644
index 00000000000..7e5a12a11f4
--- /dev/null
+++ b/app/controllers/concerns/todos_actions.rb
@@ -0,0 +1,13 @@
+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
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index f9e8fe624e8..bd7111e28bc 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -70,7 +70,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def todo_params
-    params.permit(:action_id, :author_id, :project_id, :type, :sort, :state)
+    params.permit(:action_id, :author_id, :project_id, :type, :sort, :state, :group_id)
   end
 
   def redirect_out_of_range(todos)
diff --git a/app/controllers/projects/todos_controller.rb b/app/controllers/projects/todos_controller.rb
index a41fcb85c40..248fb8a4381 100644
--- a/app/controllers/projects/todos_controller.rb
+++ b/app/controllers/projects/todos_controller.rb
@@ -1,19 +1,12 @@
 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])
diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index 1dfcf19b78d..ea1420712a7 100644
--- a/app/finders/todos_finder.rb
+++ b/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
 
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index b93c1145f82..7a459078151 100644
--- a/app/models/concerns/issuable.rb
+++ b/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
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 4715d942c8d..983684a5e05 100644
--- a/app/models/issue.rb
+++ b/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
diff --git a/app/models/note.rb b/app/models/note.rb
index abc40d9016e..9191cae6391 100644
--- a/app/models/note.rb
+++ b/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
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 5ce77d5ddc2..61158285ea9 100644
--- a/app/models/todo.rb
+++ b/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) }
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index f355d6b8ea1..5a2460a0cf5 100644
--- a/app/services/todo_service.rb
+++ b/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