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.

12 files changed, 214 insertions, 149 deletions

diff --git a/app/assets/javascripts/todos.js.es6 b/app/assets/javascripts/todos.js.es6
index ded683f2ca1..e9513725d9d 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 0d5604aae69..551a66fbf3a 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 e3933e3d7b1..4e61b0886d8 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 8ab943f4639..ad86b4f9f42 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 605bfd0cf8d..dc2d924f212 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 00000000000..e9d46f6b122
--- /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 fb20c63bc63..adc3ad207cc 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 2bbc43b491f..eb906a55a83 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 79ef3a1adad..0a3ac9f9512 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 b4e4cd97780..a5265f1b189 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 1b352be9331..fb19dac1d6a 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 4320365ab57..9f24cc0f3f2 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