diff --git a/app/assets/javascripts/todos.js.es6 b/app/assets/javascripts/todos.js.es6
index ded683f2ca1d278dd7553c3782e1f365cbc804ad..e9513725d9d882b0270ccb613b00eea0aac639ca 100644
--- a/app/assets/javascripts/todos.js.es6
+++ b/app/assets/javascripts/todos.js.es6
@@ -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);
}
}
}
diff --git a/app/assets/stylesheets/pages/todos.scss b/app/assets/stylesheets/pages/todos.scss
index 0d5604aae6939706ef3f947497464746d8b0d132..551a66fbf3aba2b1f2d3a3135f884320ae9512be 100644
--- a/app/assets/stylesheets/pages/todos.scss
+++ b/app/assets/stylesheets/pages/todos.scss
@@ -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));
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index e3933e3d7b1bc49f8d6597e3d5de10c2ab19fc5e..4e61b0886d8746d3f20012126d819eac1f23f6df 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -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
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 8ab943f4639cac3f691082654ccb83d4073c6264..ad86b4f9f4250967ebb7ad8b24a1e3e15001bd41 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -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?
diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml
index 605bfd0cf8d02055e8e6ccac67476573f18fbf54..dc2d924f212a69d4e64049657a7c9dc8406249fe 100644
--- a/app/views/dashboard/todos/_todo.html.haml
+++ b/app/views/dashboard/todos/_todo.html.haml
@@ -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')
diff --git a/changelogs/unreleased/25465-todo-done-clicking-is-kind-of-unsafe.yml b/changelogs/unreleased/25465-todo-done-clicking-is-kind-of-unsafe.yml
new file mode 100644
index 0000000000000000000000000000000000000000..e9d46f6b12269308356ea81ca83302e38eb2e447
--- /dev/null
+++ b/changelogs/unreleased/25465-todo-done-clicking-is-kind-of-unsafe.yml
@@ -0,0 +1,4 @@
+---
+title: Todo done clicking is kind of unusable
+merge_request: 8691
+author: Jacopo Beschi @jacopo-beschi
diff --git a/config/routes/dashboard.rb b/config/routes/dashboard.rb
index fb20c63bc631ea7b7ac899a385573a1e063247fd..adc3ad207cc79ef92a0c6d76421dba73d6086b9e 100644
--- a/config/routes/dashboard.rb
+++ b/config/routes/dashboard.rb
@@ -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
diff --git a/features/steps/dashboard/todos.rb b/features/steps/dashboard/todos.rb
index 2bbc43b491f9fa784ed425509424ef9a7160acfe..eb906a55a831a20125797e06fa5ec0f89c4aedd6 100644
--- a/features/steps/dashboard/todos.rb
+++ b/features/steps/dashboard/todos.rb
@@ -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
diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb
index 79ef3a1adadab1c60ed30520d93af697e1a1c6b3..0a3ac9f95126ebe3e1a4bc3bb6c6f22c814659b1 100644
--- a/spec/controllers/dashboard/todos_controller_spec.rb
+++ b/spec/controllers/dashboard/todos_controller_spec.rb
@@ -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
diff --git a/spec/factories/todos.rb b/spec/factories/todos.rb
index b4e4cd97780d55d6017def67444434bc4f4a2999..a5265f1b1897787ef2c2efa0d12d000dfe489cab 100644
--- a/spec/factories/todos.rb
+++ b/spec/factories/todos.rb
@@ -40,6 +40,10 @@ FactoryGirl.define do
action { Todo::UNMERGEABLE }
end
+ trait :pending do
+ state :pending
+ end
+
trait :done do
state :done
end
diff --git a/spec/features/todos/todos_spec.rb b/spec/features/todos/todos_spec.rb
index 1b352be9331536e7c51d882f8c0b349852eb1ee1..fb19dac1d6af395518bbf3e524b6cdaf9887919e 100644
--- a/spec/features/todos/todos_spec.rb
+++ b/spec/features/todos/todos_spec.rb
@@ -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,40 +36,65 @@ 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 marked as done-reversible in the list' do
+ expect(page).to have_selector('.todos-list .todo.todo-pending.done-reversible')
end
- it 'is removed from the list' do
- expect(page).not_to have_selector('.todos-list .todo')
+ 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 'shows "All done" message' do
- expect(page).to have_selector('.todos-all-done', count: 1)
+ 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
- context 'todo is stale on the page' do
+ shared_examples 'deleting and restoring the todo' do
before do
- todos = TodosFinder.new(user, state: :pending).execute
- TodoService.new.mark_todos_as_done(todos, user)
+ first('.js-done-todo').click
+ wait_for_ajax
+ first('.js-undo-todo').click
end
- describe 'deleting the todo' do
- before do
- first('.done-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 'is removed from the list' do
- expect(page).not_to have_selector('.todos-list .todo')
- 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 'shows "All done" message' do
- expect(page).to have_selector('.todos-all-done', count: 1)
- 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
+
+ it_behaves_like 'deleting the todo'
+ it_behaves_like 'deleting and restoring the todo'
+ end
end
context 'User has Todos with labels spanning multiple projects' do
@@ -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
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 4320365ab57b94c2266519956dbe0818ea324a5f..9f24cc0f3f230fde10873c1d41cd8108b536bcde 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -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