Merge pull request #1719 from crmaris/fix/task-reorder-hidden-completed

fix: Fix task reordering when completed tasks are hidden

Conflicted with the icon button tooltips, which had added a Tooltip import to
the same file. Both imports stay except `useToggle`, which this branch makes
redundant by lifting the per-list eye toggle up to TaskLists so the position
calculation can see it.

client/src/components/cards/CardModal/TaskLists/Item.jsx

@@ -11,7 +11,6 @@ import { useSelector } from 'react-redux';
 import { Draggable } from 'react-beautiful-dnd';
 import { useTranslation } from 'react-i18next';
 import { Button, Icon } from 'semantic-ui-react';
-import { useToggle } from '../../../../lib/hooks';
 import { Tooltip } from '../../../../lib/custom-ui';
 
 import selectors from '../../../../selectors';
@@ -22,7 +21,7 @@ import TaskList from '../../../task-lists/TaskList';
 
 import styles from './Item.module.scss';
 
-const Item = React.memo(({ id, index }) => {
+const Item = React.memo(({ id, index, isCompletedVisible, onCompletedVisibleToggle }) => {
   const selectTaskListById = useMemo(() => selectors.makeSelectTaskListById(), []);
 
   const taskList = useSelector((state) => selectTaskListById(state, id));
@@ -33,11 +32,9 @@ const Item = React.memo(({ id, index }) => {
     return !!boardMembership && boardMembership.role === BoardMembershipRoles.EDITOR;
   });
 
-  const [isCompletedVisible, toggleCompletedVisible] = useToggle();
-
   const handleToggleCompletedVisibleClick = useCallback(() => {
-    toggleCompletedVisible();
-  }, [toggleCompletedVisible]);
+    onCompletedVisibleToggle(id);
+  }, [id, onCompletedVisibleToggle]);
 
   const EditPopup = usePopupInClosableContext(EditStep);
 
@@ -115,6 +112,8 @@ const Item = React.memo(({ id, index }) => {
 Item.propTypes = {
   id: PropTypes.string.isRequired,
   index: PropTypes.number.isRequired,
+  isCompletedVisible: PropTypes.bool.isRequired,
+  onCompletedVisibleToggle: PropTypes.func.isRequired,
 };
 
 export default Item;

client/src/components/cards/CardModal/TaskLists/TaskLists.jsx

@@ -3,7 +3,7 @@
  * Licensed under the Fair Use License: https://github.com/plankanban/planka/blob/master/LICENSE.md
  */
 
-import React, { useCallback } from 'react';
+import React, { useCallback, useState } from 'react';
 import { useDispatch, useSelector } from 'react-redux';
 import { DragDropContext, Droppable } from 'react-beautiful-dnd';
 import { closePopup } from '../../../../lib/popup';
@@ -20,6 +20,14 @@ const TaskLists = React.memo(() => {
   const taskListIds = useSelector(selectors.selectTaskListIdsForCurrentCard);
 
   const dispatch = useDispatch();
+  const [completedVisibilities, setCompletedVisibilities] = useState({});
+
+  const handleCompletedVisibleToggle = useCallback((taskListId) => {
+    setCompletedVisibilities((prevCompletedVisibilities) => ({
+      ...prevCompletedVisibilities,
+      [taskListId]: !prevCompletedVisibilities[taskListId],
+    }));
+  }, []);
 
   const handleDragStart = useCallback(() => {
     document.body.classList.add(globalStyles.dragging);
@@ -45,16 +53,18 @@ const TaskLists = React.memo(() => {
           dispatch(entryActions.moveTaskList(id, destination.index));
 
           break;
-        case DroppableTypes.TASK:
-          dispatch(
-            entryActions.moveTask(id, parseDndId(destination.droppableId), destination.index),
-          );
+        case DroppableTypes.TASK: {
+          const taskListId = parseDndId(destination.droppableId);
+          const isCompletedVisible = !!completedVisibilities[taskListId];
+
+          dispatch(entryActions.moveTask(id, taskListId, destination.index, isCompletedVisible));
 
           break;
+        }
         default:
       }
     },
-    [dispatch],
+    [completedVisibilities, dispatch],
   );
 
   return (
@@ -64,7 +74,13 @@ const TaskLists = React.memo(() => {
           // eslint-disable-next-line react/jsx-props-no-spreading
           <div {...droppableProps} ref={innerRef}>
             {taskListIds.map((taskListId, index) => (
-              <Item key={taskListId} id={taskListId} index={index} />
+              <Item
+                key={taskListId}
+                id={taskListId}
+                index={index}
+                isCompletedVisible={!!completedVisibilities[taskListId]}
+                onCompletedVisibleToggle={handleCompletedVisibleToggle}
+              />
             ))}
             {placeholder}
           </div>

client/src/entry-actions/tasks.js

@@ -35,12 +35,13 @@ const handleTaskUpdate = (task) => ({
   },
 });
 
-const moveTask = (id, taskListId, index) => ({
+const moveTask = (id, taskListId, index, isCompletedVisible) => ({
   type: EntryActionTypes.TASK_MOVE,
   payload: {
     id,
     taskListId,
     index,
+    isCompletedVisible,
   },
 });
 

client/src/sagas/core/services/tasks.js

@@ -68,8 +68,14 @@ export function* handleTaskUpdate(task) {
   yield put(actions.handleTaskUpdate(task));
 }
 
-export function* moveTask(id, taskListId, index) {
-  const position = yield select(selectors.selectNextTaskPosition, taskListId, index, id);
+export function* moveTask(id, taskListId, index, isCompletedVisible) {
+  const position = yield select(
+    selectors.selectNextTaskPosition,
+    taskListId,
+    index,
+    id,
+    isCompletedVisible,
+  );
 
   yield call(updateTask, id, {
     taskListId,

client/src/sagas/core/watchers/tasks.js

@@ -22,8 +22,10 @@ export default function* tasksWatchers() {
     takeEvery(EntryActionTypes.TASK_UPDATE_HANDLE, ({ payload: { task } }) =>
       services.handleTaskUpdate(task),
     ),
-    takeEvery(EntryActionTypes.TASK_MOVE, ({ payload: { id, taskListId, index } }) =>
-      services.moveTask(id, taskListId, index),
+    takeEvery(
+      EntryActionTypes.TASK_MOVE,
+      ({ payload: { id, taskListId, index, isCompletedVisible } }) =>
+        services.moveTask(id, taskListId, index, isCompletedVisible),
     ),
     takeEvery(EntryActionTypes.TASK_DELETE, ({ payload: { id } }) => services.deleteTask(id)),
     takeEvery(EntryActionTypes.TASK_DELETE_HANDLE, ({ payload: { task } }) =>

client/src/selectors/positioning.js

@@ -116,14 +116,23 @@ export const selectNextTaskPosition = createSelector(
   (_, taskListId) => taskListId,
   (_, __, index) => index,
   (_, __, ___, excludedId) => excludedId,
-  ({ TaskList }, taskListId, index, excludedId) => {
+  (_, __, ___, ____, isCompletedVisible) => isCompletedVisible,
+  ({ TaskList }, taskListId, index, excludedId, isCompletedVisible) => {
     const taskListModel = TaskList.withId(taskListId);
 
     if (!taskListModel) {
       return taskListModel;
     }
 
-    return nextPosition(taskListModel.getTasksQuerySet().toRefArray(), index, excludedId);
+    let tasks = taskListModel.getTasksQuerySet().toRefArray();
+
+    // when an index is provided it refers to the rendered list, which excludes
+    // completed tasks if they are hidden, so the same filter must be applied here
+    if (!isUndefined(index) && taskListModel.hideCompletedTasks && !isCompletedVisible) {
+      tasks = tasks.filter((task) => !task.isCompleted);
+    }
+
+    return nextPosition(tasks, index, excludedId);
   },
 );