Commit 78328eee authored by Douwe Maan's avatar Douwe Maan Committed by Robert Speicher

Merge branch 'issuable-todo-improvements'

# Conflicts:
#	app/controllers/projects/todos_controller.rb
parent 5c8a1b34
...@@ -51,15 +51,19 @@ class @Sidebar ...@@ -51,15 +51,19 @@ class @Sidebar
$this = $(e.currentTarget) $this = $(e.currentTarget)
$todoLoading = $('.js-issuable-todo-loading') $todoLoading = $('.js-issuable-todo-loading')
$btnText = $('.js-issuable-todo-text', $this) $btnText = $('.js-issuable-todo-text', $this)
ajaxType = if $this.attr('data-id') then 'PATCH' else 'POST' ajaxType = if $this.attr('data-delete-path') then 'DELETE' else 'POST'
ajaxUrlExtra = if $this.attr('data-id') then "/#{$this.attr('data-id')}" else ''
if $this.attr('data-delete-path')
url = "#{$this.attr('data-delete-path')}"
else
url = "#{$this.data('url')}"
$.ajax( $.ajax(
url: "#{$this.data('url')}#{ajaxUrlExtra}" url: url
type: ajaxType type: ajaxType
dataType: 'json' dataType: 'json'
data: data:
issuable_id: $this.data('issuable') issuable_id: $this.data('issuable-id')
issuable_type: $this.data('issuable-type') issuable_type: $this.data('issuable-type')
beforeSend: => beforeSend: =>
@beforeTodoSend($this, $todoLoading) @beforeTodoSend($this, $todoLoading)
...@@ -82,15 +86,15 @@ class @Sidebar ...@@ -82,15 +86,15 @@ class @Sidebar
else else
$todoPendingCount.removeClass 'hidden' $todoPendingCount.removeClass 'hidden'
if data.todo? if data.delete_path?
$btn $btn
.attr 'aria-label', $btn.data('mark-text') .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') $btnText.text $btn.data('mark-text')
else else
$btn $btn
.attr 'aria-label', $btn.data('todo-text') .attr 'aria-label', $btn.data('todo-text')
.removeAttr 'data-id' .removeAttr 'data-delete-path'
$btnText.text $btn.data('todo-text') $btnText.text $btn.data('todo-text')
sidebarDropdownLoading: (e) -> sidebarDropdownLoading: (e) ->
......
class Projects::TodosController < Projects::ApplicationController class Projects::TodosController < Projects::ApplicationController
def create before_action :authenticate_user!, only: [:create]
todos = TodoService.new.mark_todo(issuable, current_user)
render json: {
todo: todos,
count: current_user.todos_pending_count,
}
end
def update def create
current_user.todos.find_by_id(params[:id]).update(state: :done) todo = TodoService.new.mark_todo(issuable, current_user)
render json: { render json: {
count: current_user.todos_pending_count, count: current_user.todos_pending_count,
delete_path: dashboard_todo_path(todo)
} }
end end
...@@ -22,7 +16,13 @@ class Projects::TodosController < Projects::ApplicationController ...@@ -22,7 +16,13 @@ class Projects::TodosController < Projects::ApplicationController
@issuable ||= begin @issuable ||= begin
case params[:issuable_type] case params[:issuable_type]
when "issue" 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" when "merge_request"
@project.merge_requests.find(params[:issuable_id]) @project.merge_requests.find(params[:issuable_id])
end end
......
...@@ -67,9 +67,9 @@ module IssuablesHelper ...@@ -67,9 +67,9 @@ module IssuablesHelper
end end
end end
def has_todo(issuable) def issuable_todo(issuable)
unless current_user.nil? if current_user
current_user.todos.find_by(target_id: issuable.id, state: :pending) current_user.todos.find_by(target: issuable, state: :pending)
end end
end end
......
...@@ -12,7 +12,7 @@ module TodosHelper ...@@ -12,7 +12,7 @@ module TodosHelper
when Todo::ASSIGNED then 'assigned you' when Todo::ASSIGNED then 'assigned you'
when Todo::MENTIONED then 'mentioned you on' when Todo::MENTIONED then 'mentioned you on'
when Todo::BUILD_FAILED then 'The build failed for your' 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
end end
......
- todo = has_todo(issuable) - todo = issuable_todo(issuable)
%aside.right-sidebar{ class: sidebar_gutter_collapsed_class } %aside.right-sidebar{ class: sidebar_gutter_collapsed_class }
.issuable-sidebar .issuable-sidebar
- can_edit_issuable = can?(current_user, :"admin_#{issuable.to_ability_name}", @project) - can_edit_issuable = can?(current_user, :"admin_#{issuable.to_ability_name}", @project)
...@@ -9,12 +9,12 @@ ...@@ -9,12 +9,12 @@
%a.gutter-toggle.pull-right.js-sidebar-toggle{ role: "button", href: "#", aria: { label: "Toggle sidebar" } } %a.gutter-toggle.pull-right.js-sidebar-toggle{ role: "button", href: "#", aria: { label: "Toggle sidebar" } }
= sidebar_gutter_toggle_icon = sidebar_gutter_toggle_icon
- if current_user - 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 %span.js-issuable-todo-text
- if todo.nil? - if todo
Add Todo
- else
Mark Done Mark Done
- else
Add Todo
= icon('spin spinner', class: 'hidden js-issuable-todo-loading') = 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| = form_for [@project.namespace.becomes(Namespace), @project, issuable], remote: true, format: :json, html: {class: 'issuable-context-form inline-update js-issuable-update'} do |f|
......
...@@ -818,7 +818,7 @@ Rails.application.routes.draw do ...@@ -818,7 +818,7 @@ Rails.application.routes.draw do
end end
end end
resources :todos, only: [:create, :update], constraints: { id: /\d+/ } resources :todos, only: [:create]
resources :uploads, only: [:create] do resources :uploads, only: [:create] do
collection do collection do
......
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
...@@ -20,6 +20,12 @@ feature 'Manually create a todo item from issue', feature: true, js: true do ...@@ -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 page.within '.header-content .todos-pending-count' do
expect(page).to have_content '1' expect(page).to have_content '1'
end 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 end
it 'should mark a todo as done' do 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 ...@@ -29,5 +35,9 @@ feature 'Manually create a todo item from issue', feature: true, js: true do
end end
expect(page).to have_selector('.todos-pending-count', visible: false) 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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment