diff options
author | Phil Hughes <me@iamphill.com> | 2016-06-17 09:01:03 +0100 |
---|---|---|
committer | Phil Hughes <me@iamphill.com> | 2016-06-17 09:01:03 +0100 |
commit | 85fab13ebaf10982c0957daca0afd1ea145e64df (patch) | |
tree | 0515480dd7d509dca5e0a7e2b714b6e1bdf29573 | |
parent | f011b86beb89557afdaf2b0ec5ae904d0be237d8 (diff) | |
download | gitlab-ce-85fab13ebaf10982c0957daca0afd1ea145e64df.tar.gz |
Improved manual todos
Based on feedback from !4502
-rw-r--r-- | app/assets/javascripts/right_sidebar.js.coffee | 16 | ||||
-rw-r--r-- | app/controllers/projects/todos_controller.rb | 25 | ||||
-rw-r--r-- | app/helpers/issuables_helper.rb | 6 | ||||
-rw-r--r-- | app/helpers/todos_helper.rb | 2 | ||||
-rw-r--r-- | app/views/shared/issuable/_sidebar.html.haml | 10 | ||||
-rw-r--r-- | config/routes.rb | 2 | ||||
-rw-r--r-- | spec/controllers/projects/todo_controller_spec.rb | 42 | ||||
-rw-r--r-- | spec/features/issues/todo_spec.rb | 10 |
8 files changed, 85 insertions, 28 deletions
diff --git a/app/assets/javascripts/right_sidebar.js.coffee b/app/assets/javascripts/right_sidebar.js.coffee index 8eb005b0a22..be4e0dcaaf7 100644 --- a/app/assets/javascripts/right_sidebar.js.coffee +++ b/app/assets/javascripts/right_sidebar.js.coffee @@ -51,11 +51,15 @@ 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: @@ -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) -> diff --git a/app/controllers/projects/todos_controller.rb b/app/controllers/projects/todos_controller.rb index a51bd5e2b49..f58f7f34574 100644 --- a/app/controllers/projects/todos_controller.rb +++ b/app/controllers/projects/todos_controller.rb @@ -1,18 +1,13 @@ class Projects::TodosController < Projects::ApplicationController - def create - todos = TodoService.new.mark_todo(issuable, current_user) + before_action :authorize_read_issue!, only: [:create] - render json: { - todo: todos, - count: current_user.todos.pending.count, - } - end - - 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, + todo: todo, + count: TodosFinder.new(current_user, state: :pending).execute.count, + delete_path: dashboard_todo_path(todo) } end @@ -22,7 +17,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 diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 8dbc51a689f..8ad45e24f35 100644 --- a/app/helpers/issuables_helper.rb +++ b/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 issuable + current_user.todos.find_by(target: issuable, state: :pending) end end diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb index 9adf5ef29f7..badfd5ee355 100644 --- a/app/helpers/todos_helper.rb +++ b/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 diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 210c9b9aab5..d707b2cff95 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -1,4 +1,4 @@ -- 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", id: (todo.id unless todo.nil?), 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) } } %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| diff --git a/config/routes.rb b/config/routes.rb index d52cbb22428..a908513cc7a 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], constraints: { id: /\d+/ } + resources :todos, only: [:create, :update] 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 new file mode 100644 index 00000000000..ce9c57873fc --- /dev/null +++ b/spec/controllers/projects/todo_controller_spec.rb @@ -0,0 +1,42 @@ +require('spec_helper') + +describe Projects::TodosController do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:issue) { create(:issue, project: project) } + + describe 'POST #create' 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 + + 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 +end diff --git a/spec/features/issues/todo_spec.rb b/spec/features/issues/todo_spec.rb index b69cce3e7d7..bc0f437a8ce 100644 --- a/spec/features/issues/todo_spec.rb +++ b/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 |