Merge branch 'issuable-todo-improvements'

app/assets/javascripts/right_sidebar.js.coffee

@@ -51,15 +51,19 @@ class @Sidebar
     $this = $(e.currentTarget)
     $todoLoading = $('.js-issuable-todo-loading')
     $btnText = $('.js-issuable-todo-text', $this)
-    ajaxType = if $this.attr('data-id') then 'PATCH' else 'POST'
-    ajaxUrlExtra = if $this.attr('data-id') then "/#{$this.attr('data-id')}" else ''
+    ajaxType = if $this.attr('data-delete-path') then 'DELETE' else 'POST'
+
+    if $this.attr('data-delete-path')
+      url = "#{$this.attr('data-delete-path')}"
+    else
+      url = "#{$this.data('url')}"
 
     $.ajax(
-      url: "#{$this.data('url')}#{ajaxUrlExtra}"
+      url: url
       type: ajaxType
       dataType: 'json'
       data:
-        issuable_id: $this.data('issuable')
+        issuable_id: $this.data('issuable-id')
         issuable_type: $this.data('issuable-type')
       beforeSend: =>
         @beforeTodoSend($this, $todoLoading)
@@ -82,15 +86,15 @@ class @Sidebar
     else
       $todoPendingCount.removeClass 'hidden'
 
-    if data.todo?
+    if data.delete_path?
       $btn
         .attr 'aria-label', $btn.data('mark-text')
-        .attr 'data-id', data.todo.id
+        .attr 'data-delete-path', data.delete_path
       $btnText.text $btn.data('mark-text')
     else
       $btn
         .attr 'aria-label', $btn.data('todo-text')
-        .removeAttr 'data-id'
+        .removeAttr 'data-delete-path'
       $btnText.text $btn.data('todo-text')
 
   sidebarDropdownLoading: (e) ->

app/controllers/projects/todos_controller.rb

 class Projects::TodosController < Projects::ApplicationController
-  def create
-    todos = TodoService.new.mark_todo(issuable, current_user)
-
-    render json: {
-      todo: todos,
-      count: current_user.todos_pending_count,
-    }
-  end
+  before_action :authenticate_user!, only: [:create]
 
-  def update
-    current_user.todos.find_by_id(params[:id]).update(state: :done)
+  def create
+    todo = TodoService.new.mark_todo(issuable, current_user)
 
     render json: {
       count: current_user.todos_pending_count,
+      delete_path: dashboard_todo_path(todo)
     }
   end
 
@@ -22,7 +16,13 @@ class Projects::TodosController < Projects::ApplicationController
     @issuable ||= begin
       case params[:issuable_type]
       when "issue"
-        @project.issues.find(params[:issuable_id])
+        issue = @project.issues.find(params[:issuable_id])
+
+        if can?(current_user, :read_issue, issue)
+          issue
+        else
+          render_404
+        end
       when "merge_request"
         @project.merge_requests.find(params[:issuable_id])
       end

app/helpers/issuables_helper.rb

@@ -67,9 +67,9 @@ module IssuablesHelper
     end
   end
 
-  def has_todo(issuable)
-    unless current_user.nil?
-      current_user.todos.find_by(target_id: issuable.id, state: :pending)
+  def issuable_todo(issuable)
+    if current_user
+      current_user.todos.find_by(target: issuable, state: :pending)
     end
   end
 

app/helpers/todos_helper.rb

@@ -12,7 +12,7 @@ module TodosHelper
     when Todo::ASSIGNED then 'assigned you'
     when Todo::MENTIONED then 'mentioned you on'
     when Todo::BUILD_FAILED then 'The build failed for your'
-    when Todo::MARKED then 'marked this as a Todo for'
+    when Todo::MARKED then 'added a todo for'
     end
   end
 

app/views/shared/issuable/_sidebar.html.haml

-- todo = has_todo(issuable)
+- todo = issuable_todo(issuable)
 %aside.right-sidebar{ class: sidebar_gutter_collapsed_class }
   .issuable-sidebar
     - can_edit_issuable = can?(current_user, :"admin_#{issuable.to_ability_name}", @project)
@@ -9,12 +9,12 @@
       %a.gutter-toggle.pull-right.js-sidebar-toggle{ role: "button", href: "#", aria: { label: "Toggle sidebar" } }
         = sidebar_gutter_toggle_icon
       - if current_user
-        %button.btn.btn-default.issuable-header-btn.pull-right.js-issuable-todo{ type: "button", aria: { label: (todo.nil? ? "Add Todo" : "Mark Done") }, data: { todo_text: "Add Todo", mark_text: "Mark Done", id: (todo.id unless todo.nil?), issuable: issuable.id, issuable_type: issuable.class.name.underscore, url: namespace_project_todos_path(@project.namespace, @project) } }
+        %button.btn.btn-default.issuable-header-btn.pull-right.js-issuable-todo{ type: "button", aria: { label: (todo.nil? ? "Add Todo" : "Mark Done") }, data: { todo_text: "Add Todo", mark_text: "Mark Done", issuable_id: issuable.id, issuable_type: issuable.class.name.underscore, url: namespace_project_todos_path(@project.namespace, @project), delete_path: (dashboard_todo_path(todo) if todo) } }
           %span.js-issuable-todo-text
-            - if todo.nil?
-              Add Todo
-            - else
+            - if todo
               Mark Done
+            - else
+              Add Todo
           = icon('spin spinner', class: 'hidden js-issuable-todo-loading')
 
     = form_for [@project.namespace.becomes(Namespace), @project, issuable], remote: true, format: :json, html: {class: 'issuable-context-form inline-update js-issuable-update'} do |f|

config/routes.rb

@@ -812,7 +812,7 @@ Rails.application.routes.draw do
           end
         end
 
-        resources :todos, only: [:create, :update], constraints: { id: /\d+/ }
+        resources :todos, only: [:create]
 
         resources :uploads, only: [:create] do
           collection do

spec/controllers/projects/todo_controller_spec.rb

+require('spec_helper')
+
+describe Projects::TodosController do
+  let(:user)          { create(:user) }
+  let(:project)       { create(:project) }
+  let(:issue)         { create(:issue, project: project) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+
+  context 'Issues' do
+    describe 'POST create' do
+      context 'when authorized' do
+        before do
+          sign_in(user)
+          project.team << [user, :developer]
+        end
+
+        it 'should create todo for issue' do
+          expect do
+            post(:create, namespace_id: project.namespace.path,
+                          project_id: project.path,
+                          issuable_id: issue.id,
+                          issuable_type: 'issue')
+          end.to change { user.todos.count }.by(1)
+
+          expect(response.status).to eq(200)
+        end
+      end
+
+      context 'when not authorized' do
+        it 'should not create todo for issue that user has no access to' do
+          sign_in(user)
+          expect do
+            post(:create, namespace_id: project.namespace.path,
+                          project_id: project.path,
+                          issuable_id: issue.id,
+                          issuable_type: 'issue')
+          end.to change { user.todos.count }.by(0)
+
+          expect(response.status).to eq(404)
+        end
+
+        it 'should not create todo for issue when user not logged in' do
+          expect do
+            post(:create, namespace_id: project.namespace.path,
+                          project_id: project.path,
+                          issuable_id: issue.id,
+                          issuable_type: 'issue')
+          end.to change { user.todos.count }.by(0)
+
+          expect(response.status).to eq(302)
+        end
+      end
+    end
+  end
+
+  context 'Merge Requests' do
+    describe 'POST create' do
+      context 'when authorized' do
+        before do
+          sign_in(user)
+          project.team << [user, :developer]
+        end
+
+        it 'should create todo for merge request' do
+          expect do
+            post(:create, namespace_id: project.namespace.path,
+                          project_id: project.path,
+                          issuable_id: merge_request.id,
+                          issuable_type: 'merge_request')
+          end.to change { user.todos.count }.by(1)
+
+          expect(response.status).to eq(200)
+        end
+      end
+
+      context 'when not authorized' do
+        it 'should not create todo for merge request user has no access to' do
+          sign_in(user)
+          expect do
+            post(:create, namespace_id: project.namespace.path,
+                          project_id: project.path,
+                          issuable_id: merge_request.id,
+                          issuable_type: 'merge_request')
+          end.to change { user.todos.count }.by(0)
+
+          expect(response.status).to eq(404)
+        end
+
+        it 'should not create todo for merge request user has no access to' do
+          expect do
+            post(:create, namespace_id: project.namespace.path,
+                          project_id: project.path,
+                          issuable_id: merge_request.id,
+                          issuable_type: 'merge_request')
+          end.to change { user.todos.count }.by(0)
+
+          expect(response.status).to eq(302)
+        end
+      end
+    end
+  end
+end

spec/features/issues/todo_spec.rb

@@ -20,6 +20,12 @@ feature 'Manually create a todo item from issue', feature: true, js: true do
     page.within '.header-content .todos-pending-count' do
       expect(page).to have_content '1'
     end
+
+    visit namespace_project_issue_path(project.namespace, project, issue)
+
+    page.within '.header-content .todos-pending-count' do
+      expect(page).to have_content '1'
+    end
   end
 
   it 'should mark a todo as done' do
@@ -29,5 +35,9 @@ feature 'Manually create a todo item from issue', feature: true, js: true do
     end
 
     expect(page).to have_selector('.todos-pending-count', visible: false)
+
+    visit namespace_project_issue_path(project.namespace, project, issue)
+
+    expect(page).to have_selector('.todos-pending-count', visible: false)
   end
 end