Removed update methodissuable-todo-improvements

Re-structured controller spec
Renamed issuable param to issuable_id

4 files changed, 83 insertions, 89 deletions

diff --git a/app/assets/javascripts/right_sidebar.js.coffee b/app/assets/javascripts/right_sidebar.js.coffee
index be4e0dcaaf7..12340bbce54 100644
--- a/app/assets/javascripts/right_sidebar.js.coffee
+++ b/app/assets/javascripts/right_sidebar.js.coffee
@@ -63,7 +63,7 @@ class @Sidebar
       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)
diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml
index cd29fff581e..adfab1af53e 100644
--- a/app/views/shared/issuable/_sidebar.html.haml
+++ b/app/views/shared/issuable/_sidebar.html.haml
@@ -9,7 +9,7 @@
       %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", issuable: issuable.id, issuable_type: issuable.class.name.underscore, url: namespace_project_todos_path(@project.namespace, @project), delete_path: (dashboard_todo_path(todo) if todo) } }
+        %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
               Mark Done
diff --git a/config/routes.rb b/config/routes.rb
index a908513cc7a..026b2691323 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -797,7 +797,7 @@ Rails.application.routes.draw do
           end
         end
 
-        resources :todos, only: [:create, :update]
+        resources :todos, only: [:create]
 
         resources :uploads, only: [:create] do
           collection do
diff --git a/spec/controllers/projects/todo_controller_spec.rb b/spec/controllers/projects/todo_controller_spec.rb
index 2160f0b802c..40a3403b660 100644
--- a/spec/controllers/projects/todo_controller_spec.rb
+++ b/spec/controllers/projects/todo_controller_spec.rb
@@ -7,101 +7,95 @@ describe Projects::TodosController do
   let(:merge_request) { create(:merge_request, source_project: project) }
 
   context 'Issues' do
-    describe 'POST #create' do
-      before do
-        sign_in(user)
-        project.team << [user, :developer]
+    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
 
-      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
-
-    describe 'POST #create when not authorized' do
-      before do
-        sign_in(user)
-      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(0)
-
-        expect(response.status).to eq(404)
-      end
-    end
-
-    describe 'POST #create when not logged in' do
-      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(0)
-
-        expect(response.status).to eq(302)
+      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
-      before do
-        sign_in(user)
-        project.team << [user, :developer]
+    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
 
-      it 'should create todo for issue' 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
-
-    describe 'POST #create when not authorized' do
-      before do
-        sign_in(user)
-      end
-
-      it 'should create todo for issue' 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(404)
-      end
-    end
-
-    describe 'POST #create when not logged in' do
-      it 'should create todo for issue' 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)
+      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