root
/
gitlab-ce
mirror of https://gitlab.com/free-releases/gitlab-ce.git
1
0
Fork 0
Code Issues Actions Packages Projects Releases Wiki Activity

Merge branch '25465-todo-done-clicking-is-kind-of-unsafe' into 'master' 

Todo done clicking is kind of unusable.

Closes #25465

See merge request !8691
This commit is contained in:
Clement Ho 2017-02-18 09:06:22 +00:00
parent 5f3f6ee605 26160459b5
commit 6eb45df2fd
12 changed files with 214 additions and 149 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

143
app/assets/javascripts/todos.js.es6
View File

@ -1,28 +1,34 @@
/* eslint-disable class-methods-use-this, no-new, func-names, prefer-template, no-unneeded-ternary, object-shorthand, space-before-function-paren, comma-dangle, quote-props, consistent-return, no-else-return, no-param-reassign, max-len */
/* eslint-disable class-methods-use-this, no-new, func-names, no-unneeded-ternary, object-shorthand, quote-props, no-param-reassign, max-len */
/* global UsersSelect */
((global) => {
class Todos {
constructor({ el } = {}) {
this.allDoneClicked = this.allDoneClicked.bind(this);
this.doneClicked = this.doneClicked.bind(this);
this.el = el || $('.js-todos-options');
this.perPage = this.el.data('perPage');
this.clearListeners();
this.initBtnListeners();
constructor() {
this.initFilters();
this.bindEvents();
this.cleanupWrapper = this.cleanup.bind(this);
document.addEventListener('beforeunload', this.cleanupWrapper);
}
clearListeners() {
$('.done-todo').off('click');
$('.js-todos-mark-all').off('click');
return $('.todo').off('click');
cleanup() {
this.unbindEvents();
document.removeEventListener('beforeunload', this.cleanupWrapper);
}
initBtnListeners() {
$('.done-todo').on('click', this.doneClicked);
$('.js-todos-mark-all').on('click', this.allDoneClicked);
return $('.todo').on('click', this.goToTodoUrl);
unbindEvents() {
$('.js-done-todo, .js-undo-todo').off('click', this.updateStateClickedWrapper);
$('.js-todos-mark-all').off('click', this.allDoneClickedWrapper);
$('.todo').off('click', this.goToTodoUrl);
}
bindEvents() {
this.updateStateClickedWrapper = this.updateStateClicked.bind(this);
this.allDoneClickedWrapper = this.allDoneClicked.bind(this);
$('.js-done-todo, .js-undo-todo').on('click', this.updateStateClickedWrapper);
$('.js-todos-mark-all').on('click', this.allDoneClickedWrapper);
$('.todo').on('click', this.goToTodoUrl);
}
initFilters() {
@ -33,7 +39,7 @@
$('form.filter-form').on('submit', function (event) {
event.preventDefault();
gl.utils.visitUrl(this.action + '&' + $(this).serialize());
gl.utils.visitUrl(`${this.action}&${$(this).serialize()}`);
});
}
@ -44,105 +50,72 @@
filterable: searchFields ? true : false,
search: { fields: searchFields },
data: $dropdown.data('data'),
clicked: function() {
clicked: function () {
return $dropdown.closest('form.filter-form').submit();
}
},
});
}
doneClicked(e) {
updateStateClicked(e) {
e.preventDefault();
e.stopImmediatePropagation();
const $target = $(e.currentTarget);
$target.disable();
return $.ajax({
const target = e.target;
target.setAttribute('disabled', '');
target.classList.add('disabled');
$.ajax({
type: 'POST',
url: $target.attr('href'),
url: target.getAttribute('href'),
dataType: 'json',
data: {
'_method': 'delete'
'_method': target.getAttribute('data-method'),
},
success: (data) => {
this.redirectIfNeeded(data.count);
this.clearDone($target.closest('li'));
return this.updateBadges(data);
}
this.updateState(target);
this.updateBadges(data);
},
});
}
allDoneClicked(e) {
e.preventDefault();
e.stopImmediatePropagation();
const $target = $(e.currentTarget);
$target.disable();
return $.ajax({
$.ajax({
type: 'POST',
url: $target.attr('href'),
dataType: 'json',
data: {
'_method': 'delete'
'_method': 'delete',
},
success: (data) => {
$target.remove();
$('.js-todos-all').html('<div class="nothing-here-block">You\'re all done!</div>');
return this.updateBadges(data);
}
this.updateBadges(data);
},
});
}
clearDone($row) {
const $ul = $row.closest('ul');
$row.remove();
if (!$ul.find('li').length) {
return $ul.parents('.panel').remove();
updateState(target) {
const row = target.closest('li');
const restoreBtn = row.querySelector('.js-undo-todo');
const doneBtn = row.querySelector('.js-done-todo');
target.removeAttribute('disabled');
target.classList.remove('disabled');
target.classList.add('hidden');
if (target === doneBtn) {
row.classList.add('done-reversible');
restoreBtn.classList.remove('hidden');
} else {
row.classList.remove('done-reversible');
doneBtn.classList.remove('hidden');
}
}
updateBadges(data) {
$(document).trigger('todo:toggle', data.count);
$('.todos-pending .badge').text(data.count);
return $('.todos-done .badge').text(data.done_count);
}
getTotalPages() {
return this.el.data('totalPages');
}
getCurrentPage() {
return this.el.data('currentPage');
}
getTodosPerPage() {
return this.el.data('perPage');
}
redirectIfNeeded(total) {
const currPages = this.getTotalPages();
const currPage = this.getCurrentPage();
// Refresh if no remaining Todos
if (!total) {
window.location.reload();
return;
}
// Do nothing if no pagination
if (!currPages) {
return;
}
const newPages = Math.ceil(total / this.getTodosPerPage());
let url = location.href;
if (newPages !== currPages) {
// Redirect to previous page if there's one available
if (currPages > 1 && currPage === currPages) {
const pageParams = {
page: currPages - 1
};
url = gl.utils.mergeUrlParams(pageParams, url);
}
return gl.utils.visitUrl(url);
}
$('.todos-done .badge').text(data.done_count);
}
goToTodoUrl(e) {
@ -159,12 +132,12 @@
if (selected.tagName === 'IMG') {
const avatarUrl = selected.parentElement.getAttribute('href');
return window.open(avatarUrl, windowTarget);
window.open(avatarUrl, windowTarget);
} else {
return window.open(todoLink, windowTarget);
window.open(todoLink, windowTarget);
}
} else {
return gl.utils.visitUrl(todoLink);
gl.utils.visitUrl(todoLink);
}
}
}

12
app/assets/stylesheets/pages/todos.scss
View File

@ -60,6 +60,18 @@
}
}
.todos-list > .todo.todo-pending.done-reversible {
background-color: $gray-light;
&:hover {
border-color: $border-color;
}
.title {
font-weight: normal;
}
}
.todo-item {
.todo-title {
@include str-truncated(calc(100% - 174px));

6
app/controllers/dashboard/todos_controller.rb
View File

@ -29,6 +29,12 @@ class Dashboard::TodosController < Dashboard::ApplicationController
end
end
def restore
TodoService.new.mark_todos_as_pending_by_ids([params[:id]], current_user)
render json: todos_counts
end
private
def find_todos

25
app/services/todo_service.rb
View File

@ -170,16 +170,20 @@ class TodoService
# When user marks some todos as done
def mark_todos_as_done(todos, current_user)
mark_todos_as_done_by_ids(todos.select(&:id), current_user)
update_todos_state_by_ids(todos.select(&:id), current_user, :done)
end
def mark_todos_as_done_by_ids(ids, current_user)
todos = current_user.todos.where(id: ids)
update_todos_state_by_ids(ids, current_user, :done)
end
# Only return those that are not really on that state
marked_todos = todos.where.not(state: :done).update_all(state: :done)
current_user.update_todos_count_cache
marked_todos
# When user marks some todos as pending
def mark_todos_as_pending(todos, current_user)
update_todos_state_by_ids(todos.select(&:id), current_user, :pending)
end
def mark_todos_as_pending_by_ids(ids, current_user)
update_todos_state_by_ids(ids, current_user, :pending)
end
# When user marks an issue as todo
@ -194,6 +198,15 @@ class TodoService
private
def update_todos_state_by_ids(ids, current_user, state)
todos = current_user.todos.where(id: ids)
# Only return those that are not really on that state
marked_todos = todos.where.not(state: state).update_all(state: state)
current_user.update_todos_count_cache
marked_todos
end
def create_todos(users, attributes)
Array(users).map do |user|
next if pending_todos(user, attributes).exists?

5
app/views/dashboard/todos/_todo.html.haml
View File

@ -31,6 +31,9 @@
- if todo.pending?
.todo-actions
= link_to [:dashboard, todo], method: :delete, class: 'btn btn-loading done-todo' do
= link_to [:dashboard, todo], method: :delete, class: 'btn btn-loading js-done-todo' do
Done
= icon('spinner spin')
= link_to restore_dashboard_todo_path(todo), method: :patch, class: 'btn btn-loading js-undo-todo hidden' do
Undo
= icon('spinner spin')

4
changelogs/unreleased/25465-todo-done-clicking-is-kind-of-unsafe.yml Normal file
View File

@ -0,0 +1,4 @@
---
title: Todo done clicking is kind of unusable
merge_request: 8691
author: Jacopo Beschi @jacopo-beschi

3
config/routes/dashboard.rb
View File

@ -14,6 +14,9 @@ resource :dashboard, controller: 'dashboard', only: [] do
collection do
delete :destroy_all
end
member do
patch :restore
end
end
resources :projects, only: [:index] do

23
features/steps/dashboard/todos.rb
View File

@ -47,7 +47,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
page.within('.todos-pending-count') { expect(page).to have_content '3' }
expect(page).to have_content 'To do 3'
expect(page).to have_content 'Done 1'
should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference(full: true)}"
should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference(full: true)}", merge_request.title, state: :done_reversible)
end
step 'I mark all todos as done' do
@ -71,7 +71,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
click_link 'Done 1'
expect(page).to have_link project.name_with_namespace
should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference(full: true)}", merge_request.title, false)
should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference(full: true)}", merge_request.title, state: :done_irreversible)
end
step 'I should see all todos marked as done' do
@ -81,10 +81,10 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
click_link 'Done 4'
expect(page).to have_link project.name_with_namespace
should_see_todo(1, "John Doe assigned you merge request #{merge_request_reference}", merge_request.title, false)
should_see_todo(2, "John Doe mentioned you on issue #{issue_reference}", "#{current_user.to_reference} Wdyt?", false)
should_see_todo(3, "John Doe assigned you issue #{issue_reference}", issue.title, false)
should_see_todo(4, "Mary Jane mentioned you on issue #{issue_reference}", issue.title, false)
should_see_todo(1, "John Doe assigned you merge request #{merge_request_reference}", merge_request.title, state: :done_irreversible)
should_see_todo(2, "John Doe mentioned you on issue #{issue_reference}", "#{current_user.to_reference} Wdyt?", state: :done_irreversible)
should_see_todo(3, "John Doe assigned you issue #{issue_reference}", issue.title, state: :done_irreversible)
should_see_todo(4, "Mary Jane mentioned you on issue #{issue_reference}", issue.title, state: :done_irreversible)
end
step 'I filter by "Enterprise"' do
@ -140,15 +140,20 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
page.should have_css('.identifier', text: 'Merge Request !1')
end
def should_see_todo(position, title, body, pending = true)
def should_see_todo(position, title, body, state: :pending)
page.within(".todo:nth-child(#{position})") do
expect(page).to have_content title
expect(page).to have_content body
if pending
if state == :pending
expect(page).to have_link 'Done'
else
elsif state == :done_reversible
expect(page).to have_link 'Undo'
elsif state == :done_irreversible
expect(page).not_to have_link 'Undo'
expect(page).not_to have_link 'Done'
else
raise 'Invalid state given, valid states: :pending, :done_reversible, :done_irreversible'
end
end
end

25
spec/controllers/dashboard/todos_controller_spec.rb
View File

@ -1,16 +1,19 @@
require 'spec_helper'
describe Dashboard::TodosController do
include ApiHelpers
let(:user) { create(:user) }
let(:author) { create(:user) }
let(:project) { create(:empty_project) }
let(:todo_service) { TodoService.new }
describe 'GET #index' do
before do
sign_in(user)
project.team << [user, :developer]
end
before do
sign_in(user)
project.team << [user, :developer]
end
describe 'GET #index' do
context 'when using pagination' do
let(:last_page) { user.todos.page.total_pages }
let!(:issues) { create_list(:issue, 2, project: project, assignee: user) }
@ -34,4 +37,16 @@ describe Dashboard::TodosController do
end
end
end
describe 'PATCH #restore' do
let(:todo) { create(:todo, :done, user: user, project: project, author: author) }
it 'restores the todo to pending state' do
patch :restore, id: todo.id
expect(todo.reload).to be_pending
expect(response).to have_http_status(200)
expect(json_response).to eq({ "count" => 1, "done_count" => 0 })
end
end
end

4
spec/factories/todos.rb
View File

@ -40,6 +40,10 @@ FactoryGirl.define do
action { Todo::UNMERGEABLE }
end
trait :pending do
state :pending
end
trait :done do
state :done
end

77
spec/features/todos/todos_spec.rb
View File

@ -1,6 +1,8 @@
require 'spec_helper'
describe 'Dashboard Todos', feature: true do
include WaitForAjax
let(:user) { create(:user) }
let(:author) { create(:user) }
let(:project) { create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
@ -34,39 +36,64 @@ describe 'Dashboard Todos', feature: true do
end
end
describe 'deleting the todo' do
shared_examples 'deleting the todo' do
before do
first('.done-todo').click
first('.js-done-todo').click
end
it 'is removed from the list' do
expect(page).not_to have_selector('.todos-list .todo')
it 'is marked as done-reversible in the list' do
expect(page).to have_selector('.todos-list .todo.todo-pending.done-reversible')
end
it 'shows "All done" message' do
expect(page).to have_selector('.todos-all-done', count: 1)
it 'shows Undo button' do
expect(page).to have_selector('.js-undo-todo', visible: true)
expect(page).to have_selector('.js-done-todo', visible: false)
end
it 'updates todo count' do
expect(page).to have_content 'To do 0'
expect(page).to have_content 'Done 1'
end
it 'has not "All done" message' do
expect(page).not_to have_selector('.todos-all-done')
end
end
shared_examples 'deleting and restoring the todo' do
before do
first('.js-done-todo').click
wait_for_ajax
first('.js-undo-todo').click
end
it 'is marked back as pending in the list' do
expect(page).not_to have_selector('.todos-list .todo.todo-pending.done-reversible')
expect(page).to have_selector('.todos-list .todo.todo-pending')
end
it 'shows Done button' do
expect(page).to have_selector('.js-undo-todo', visible: false)
expect(page).to have_selector('.js-done-todo', visible: true)
end
it 'updates todo count' do
expect(page).to have_content 'To do 1'
expect(page).to have_content 'Done 0'
end
end
it_behaves_like 'deleting the todo'
it_behaves_like 'deleting and restoring the todo'
context 'todo is stale on the page' do
before do
todos = TodosFinder.new(user, state: :pending).execute
TodoService.new.mark_todos_as_done(todos, user)
end
describe 'deleting the todo' do
before do
first('.done-todo').click
end
it 'is removed from the list' do
expect(page).not_to have_selector('.todos-list .todo')
end
it 'shows "All done" message' do
expect(page).to have_selector('.todos-all-done', count: 1)
end
end
it_behaves_like 'deleting the todo'
it_behaves_like 'deleting and restoring the todo'
end
end
@ -113,18 +140,6 @@ describe 'Dashboard Todos', feature: true do
expect(page).to have_selector('.gl-pagination .page', count: 2)
end
describe 'completing last todo from last page', js: true do
it 'redirects to the previous page' do
visit dashboard_todos_path(page: 2)
expect(page).to have_css("#todo_#{Todo.last.id}")
click_link('Done')
expect(current_path).to eq dashboard_todos_path
expect(page).to have_css("#todo_#{Todo.first.id}")
end
end
describe 'mark all as done', js: true do
before do
visit dashboard_todos_path

36
spec/services/todo_service_spec.rb
View File

@ -287,39 +287,51 @@ describe TodoService, services: true do
end
end
shared_examples 'marking todos as done' do |meth|
let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
shared_examples 'updating todos state' do |meth, state, new_state|
let!(:first_todo) { create(:todo, state, user: john_doe, project: project, target: issue, author: author) }
let!(:second_todo) { create(:todo, state, user: john_doe, project: project, target: issue, author: author) }
it 'marks related todos for the user as done' do
it 'updates related todos for the user with the new_state' do
service.send(meth, collection, john_doe)
expect(first_todo.reload).to be_done
expect(second_todo.reload).to be_done
expect(first_todo.reload.state?(new_state)).to be true
expect(second_todo.reload.state?(new_state)).to be true
end
describe 'cached counts' do
it 'updates when todos change' do
expect(john_doe.todos_done_count).to eq(0)
expect(john_doe.todos_pending_count).to eq(2)
expect(john_doe.todos.where(state: new_state).count).to eq(0)
expect(john_doe.todos.where(state: state).count).to eq(2)
expect(john_doe).to receive(:update_todos_count_cache).and_call_original
service.send(meth, collection, john_doe)
expect(john_doe.todos_done_count).to eq(2)
expect(john_doe.todos_pending_count).to eq(0)
expect(john_doe.todos.where(state: new_state).count).to eq(2)
expect(john_doe.todos.where(state: state).count).to eq(0)
end
end
end
describe '#mark_todos_as_done' do
it_behaves_like 'marking todos as done', :mark_todos_as_done do
it_behaves_like 'updating todos state', :mark_todos_as_done, :pending, :done do
let(:collection) { [first_todo, second_todo] }
end
end
describe '#mark_todos_as_done_by_ids' do
it_behaves_like 'marking todos as done', :mark_todos_as_done_by_ids do
it_behaves_like 'updating todos state', :mark_todos_as_done_by_ids, :pending, :done do
let(:collection) { [first_todo, second_todo].map(&:id) }
end
end
describe '#mark_todos_as_pending' do
it_behaves_like 'updating todos state', :mark_todos_as_pending, :done, :pending do
let(:collection) { [first_todo, second_todo] }
end
end
describe '#mark_todos_as_pending_by_ids' do
it_behaves_like 'updating todos state', :mark_todos_as_pending_by_ids, :done, :pending do
let(:collection) { [first_todo, second_todo].map(&:id) }
end
end