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

5 files changed, 99 insertions, 92 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')