Todo done clicking is kind of unusable.

The Done button will change to an Undo button and the line item will be greyed out.
Bold links will be unbolded.
The user can undo the task by clicking the Undo button.

app/assets/javascripts/todos.js.es6

-/* 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);
       }
     }
   }

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));

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

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?

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')

changelogs/unreleased/25465-todo-done-clicking-is-kind-of-unsafe.yml

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

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

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

spec/controllers/dashboard/todos_controller_spec.rb

 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
 
+  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

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

spec/features/todos/todos_spec.rb

 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
 
-      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)
+        it 'has not "All done" message' do
+          expect(page).not_to have_selector('.todos-all-done')
+        end
       end
 
-        describe 'deleting the todo' do
+      shared_examples 'deleting and restoring the todo' do
         before do
-            first('.done-todo').click
+          first('.js-done-todo').click
+          wait_for_ajax
+          first('.js-undo-todo').click
         end
 
-          it 'is removed from the list' do
-            expect(page).not_to have_selector('.todos-list .todo')
+        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 "All done" message' do
-            expect(page).to have_selector('.todos-all-done', count: 1)
+        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
+
+        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

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