Addressed review comments5481-epic-todos

Also DRYed todo controller specs

12 files changed, 120 insertions, 185 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/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..c592217fda9 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -184,8 +184,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/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 8190fc90eb6..c96a8fefb4e 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -305,6 +305,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
@@ -320,7 +321,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
diff --git a/doc/api/todos.md b/doc/api/todos.md
index 71afd5e7ecc..9543ec80898 100644
--- a/doc/api/todos.md
+++ b/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` |
 
diff --git a/ee/app/controllers/groups/todos_controller.rb b/ee/app/controllers/groups/todos_controller.rb
index adcbcba6e37..98abfc73e11 100644
--- a/ee/app/controllers/groups/todos_controller.rb
+++ b/ee/app/controllers/groups/todos_controller.rb
@@ -1,20 +1,11 @@
 class Groups::TodosController < Groups::ApplicationController
-  include Gitlab::Utils::StrongMemoize
+  include TodosActions
 
   before_action :authenticate_user!, only: [:create]
 
-  def create
-    todo = TodoService.new.mark_todo(epic, current_user)
-
-    render json: {
-      count: TodosFinder.new(current_user, state: :pending).execute.count,
-      delete_path: dashboard_todo_path(todo)
-    }
-  end
-
   private
 
-  def epic
+  def issuable
     strong_memoize(:epic) do
       case params[:issuable_type]
       when "epic"
diff --git a/ee/lib/ee/api/entities.rb b/ee/lib/ee/api/entities.rb
index 55ffeea2ad5..c1776993d14 100644
--- a/ee/lib/ee/api/entities.rb
+++ b/ee/lib/ee/api/entities.rb
@@ -120,6 +120,16 @@ module EE
         end
       end
 
+      module Todo
+        extend ActiveSupport::Concern
+
+        def todo_target_class(target_type)
+          ::EE::API::Entities.const_get(target_type, false)
+        rescue NameError
+          super
+        end
+      end
+
       ########################
       # EE-specific entities #
       ########################
diff --git a/ee/spec/controllers/groups/todos_controller_spec.rb b/ee/spec/controllers/groups/todos_controller_spec.rb
index aa2765065ae..5e6a34fc747 100644
--- a/ee/spec/controllers/groups/todos_controller_spec.rb
+++ b/ee/spec/controllers/groups/todos_controller_spec.rb
@@ -1,48 +1,20 @@
-require('spec_helper')
+require 'spec_helper'
 
 describe Groups::TodosController do
-  let(:user)  { create(:user) }
-  let(:group) { create(:group) }
-  let(:epic)  { create(:epic, group: group) }
+  let(:user)   { create(:user) }
+  let(:group)  { create(:group, :private) }
+  let(:epic)   { create(:epic, group: group) }
+  let(:parent) { group }
 
   describe 'POST create' do
-    subject { post :create, group_id: group, issuable_id: epic.id, issuable_type: 'epic', format: :json }
-
-    context 'when authorized' do
-      before do
-        sign_in(user)
-      end
-
-      it 'creates todo for epic' do
-        expect { subject }.to change { user.todos.count }.by(1)
-
-        expect(response).to have_gitlab_http_status(200)
-      end
-
-      it 'returns todo path and pending count' do
-        subject
-
-        expect(json_response['count']).to eq 1
-        expect(json_response['delete_path']).to match(%r{/dashboard/todos/\d{1}})
-      end
+    def post_create
+      post :create,
+        group_id: group,
+        issuable_id: epic.id,
+        issuable_type: 'epic',
+        format: :json
     end
 
-    context 'when not authorized for project' do
-      it 'does not create todo for epic that user has no access to' do
-        group.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
-
-        sign_in(user)
-
-        expect { subject }.not_to change { user.todos.count }
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-
-      it 'does not create todo for epic when user not logged in' do
-        expect { subject }.not_to change { user.todos.count }
-
-        expect(response).to have_gitlab_http_status(401)
-      end
-    end
+    it_behaves_like 'todos actions'
   end
 end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 02134e7a829..e0bd7860084 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -793,20 +793,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|
-        begin
-          klass = "EE::API::Entities::#{todo.target_type}".constantize
-        rescue
-          klass = "API::Entities::#{todo.target_type}".constantize
-        end
-
-        klass.represent(todo.target, options)
+        todo_target_class(todo.target_type).represent(todo.target, options)
       end
 
       expose :target_url do |todo, options|
@@ -822,6 +816,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
@@ -1416,3 +1414,4 @@ API::Entities.prepend_entity(::API::Entities::Project, with: EE::API::Entities::
 API::Entities.prepend_entity(::API::Entities::ProtectedRefAccess, with: EE::API::Entities::ProtectedRefAccess)
 API::Entities.prepend_entity(::API::Entities::UserPublic, with: EE::API::Entities::UserPublic)
 API::Entities.prepend_entity(::API::Entities::Variable, with: EE::API::Entities::Variable)
+API::Entities.prepend_entity(::API::Entities::Todo, with: EE::API::Entities::Todo)
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/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