Repository: obsidian-tasks-group/obsidian-tasks
Pull request #2527 (2023-12-23 10:03:04)

fix: Next recurrent of recurring tasks is now always TODO, IN_PROGRESS or space
<!--- Provide a general summary of your changes in the Title above -->

# Description

<!--- Describe your changes in detail -->

More improvements to the selection of status of next task, when a recurring task reaches DONE status:

- fix: If can't find a TODO status for new recurrence, look for IN_PROGRESS
- fix: if no TODO or IN_PROGRESS in chain, return space symbol: '[ ]'

## Motivation and Context

Remove more pitfalls from recurring tasks.

## How has this been tested?

- Existing tests
- New tests
- Smoke tests
- Exploratory testing

## Screenshots (if appropriate)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

Changes visible to users:

- [x] **Bug fix** (prefix: `fix` - non-breaking change which fixes an issue)

Internal changes:

- [x] **Refactor** (prefix: `refactor` - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
- [x] **Tests** (prefix: `test` - additions and improvements to unit tests and the smoke tests)

## Checklist

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] My code follows the code style of this project and passes `yarn run lint`.
- [x] My change requires a change to the documentation.
- [ ] I have [updated the documentation](https://publish.obsidian.md/tasks-contributing/Documentation/About+Documentation) accordingly.
- [x] My change has adequate [Unit Test coverage](https://publish.obsidian.md/tasks-contributing/Testing/About+Testing).

## Terms

<!--
By submitting this pull request, you must agree to follow our
[contributing guide](https://publish.obsidian.md/tasks-contributing) and
[Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md).
Put an x in the boxes to confirm you agree.
-->

- [x] My contribution follow this project's [contributing guide](https://publish.obsidian.md/tasks-contributing)
- [x] I agree to follow this project's [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md)



Patch:
diff --git a/src/StatusRegistry.ts b/src/StatusRegistry.ts
index a7232b19..340fcee6 100644
--- a/src/StatusRegistry.ts
+++ b/src/StatusRegistry.ts
@@ -201,28 +201,51 @@ export class StatusRegistry {
 
     /**
      * Return the status to use for a recurring task that has just been completed.
-     * If the next status is not TODO, it advances through the next statuses until it either
-     * finds a {@link StatusType.TODO} status, or goes ahead and returns the next status after {@link newStatus}.
+     *
+     * If the next status is not TODO:
+     *   - it first advances through the next statuses until it finds a {@link StatusType.TODO} status - which it returns
+     *   - or it then advances through the next statuses until it finds an {@link StatusType.IN_PROGRESS} status - which it returns
+     *   - otherwise, it uses the status with symbol 'space', which is the default 'TODO' status.
      *
      * @param newStatus - the new status of the task that has just been toggled, which
      *                    is expected to be of type {@link StatusType.DONE}, but this is not checked.
      */
     public getNextRecurrenceStatusOrCreate(newStatus: Status) {
-        let nextStatus = this.getNextStatusOrCreate(newStatus);
-        if (nextStatus.type !== StatusType.TODO) {
-            let searchStatus = nextStatus;
-            // The goal here is to avoid an infinite loop. By limiting the search to the number of
-            // configured statuses, we ensure it doesn't continue indefinitely.
-            for (let i = 0; i < this.registeredStatuses.length - 1; i++) {
-                searchStatus = this.getNextStatusOrCreate(searchStatus);
-                if (searchStatus.type === StatusType.TODO) {
-                    nextStatus = searchStatus;
-                    break;
-                }
+        const nextStatus = this.getNextStatusOrCreate(newStatus);
+
+        const result1 = this.getNextRecurrenceStatusOfType(nextStatus, StatusType.TODO);
+        if (result1) {
+            return result1;
+        }
+
+        const result2 = this.getNextRecurrenceStatusOfType(nextStatus, StatusType.IN_PROGRESS);
+        if (result2) {
+            return result2;
+        }
+
+        // There is no TODO or IN_PROGRESS status in the chain, so we will use a space.
+        // This ensures that the new recurrence is always found by 'not done' searches,
+        // for safety in vaults where users did not fully set up all statuses.
+        // This makes the simplifying assumption that no user is likely to change the
+        // status type of space to anything other than TODO - but if they do, it's not
+        // our problem.
+        return this.bySymbolOrCreate(' ');
+    }
+
+    private getNextRecurrenceStatusOfType(nextStatus: Status, wanted: StatusType) {
+        if (nextStatus.type === wanted) {
+            return nextStatus;
+        }
+        let searchStatus = nextStatus;
+        // The goal here is to avoid an infinite loop. By limiting the search to the number of
+        // configured statuses, we ensure it doesn't continue indefinitely.
+        for (let i = 0; i < this.registeredStatuses.length - 1; i++) {
+            searchStatus = this.getNextStatusOrCreate(searchStatus);
+            if (searchStatus.type === wanted) {
+                return searchStatus;
             }
-            // If it fails to find any TODO status, it will use the next symbol after DONE.
         }
-        return nextStatus;
+        return undefined;
     }
 
     /**


Test patch:
diff --git a/tests/StatusRegistry.test.ts b/tests/StatusRegistry.test.ts
index 5a12029a..fb9c28ea 100644
--- a/tests/StatusRegistry.test.ts
+++ b/tests/StatusRegistry.test.ts
@@ -7,6 +7,7 @@ import { Status } from '../src/Status';
 import { StatusConfiguration, StatusType } from '../src/StatusConfiguration';
 import { Task } from '../src/Task';
 import { TaskLocation } from '../src/TaskLocation';
+import type { StatusCollection, StatusCollectionEntry } from '../src/StatusCollection';
 import * as TestHelpers from './TestHelpers';
 import * as StatusExamples from './TestingTools/StatusExamples';
 import { constructStatuses } from './TestingTools/StatusesTestHelpers';
@@ -478,24 +479,126 @@ describe('StatusRegistry', () => {
     });
 
     describe('toggling recurring', () => {
-        it('should make CANCELLED next task TODO', () => {
-            // See #2304:
-            // Completing a recurring task setting wrong status for new task [if the next custom status is not TODO]
-
+        /**
+         * Test one scenario of toggling a recurring task: confirm the status of the done task, and the next recurrence.
+         * @param statuses
+         * @param initialStatusSymbol - typically an incomplete status
+         * @param expectedToggledStatus - the next status - is expected to be DONE, so that the next recurrence is created.
+         * @param expectedNextTaskStatus - the expected status of the new recurrence.
+         */
+        function checkToggleAndRecurrenceStatuses(
+            statuses: Array<StatusCollectionEntry>,
+            initialStatusSymbol: string,
+            expectedToggledStatus: string,
+            expectedNextTaskStatus: string,
+        ) {
             // Arrange
             const statusRegistry = new StatusRegistry();
-            const statuses = StatusExamples.doneTogglesToCancelled();
             statusRegistry.set(constructStatuses(statuses));
 
-            const initialStatusForRecurringTask = statusRegistry.bySymbol('/');
+            const initialStatusForRecurringTask = statusRegistry.bySymbol(initialStatusSymbol);
 
             // Act, Assert
             const toggledStatus = statusRegistry.getNextStatusOrCreate(initialStatusForRecurringTask);
-            expect(toggledStatus).toEqual(statusRegistry.bySymbol('x'));
+            expect(toggledStatus).toEqual(statusRegistry.bySymbol(expectedToggledStatus));
 
-            // Ensure that the next status skips through to TODO for a recurring task
             const nextStatus = statusRegistry.getNextRecurrenceStatusOrCreate(toggledStatus);
-            expect(nextStatus).toEqual(statusRegistry.bySymbol(' '));
+            expect(nextStatus).toEqual(statusRegistry.bySymbolOrCreate(expectedNextTaskStatus));
+        }
+
+        it('should make CANCELLED next task TODO', () => {
+            // See #2304:
+            // Completing a recurring task setting wrong status for new task [if the next custom status is not TODO]
+            // Ensure that the next status skips through to TODO for a recurring task:
+            const statuses = StatusExamples.doneTogglesToCancelled();
+            checkToggleAndRecurrenceStatuses(statuses, '/', 'x', ' ');
+        });
+
+        it('should find IN_PROGRESS for next recurrence, when statuses are IN_PROGRESS then DONE', () => {
+            const statuses: StatusCollection = [
+                ['1', '1 to 2', '2', 'IN_PROGRESS'],
+                ['2', '2 to 1', '1', 'DONE'],
+            ];
+            checkToggleAndRecurrenceStatuses(statuses, '1', '2', '1');
+        });
+
+        it('should find IN_PROGRESS for next recurrence, when statuses are DONE then IN_PROGRESS', () => {
+            const statuses: StatusCollection = [
+                ['1', '1 to 2', '2', 'DONE'],
+                ['2', '2 to 1', '1', 'IN_PROGRESS'],
+            ];
+            checkToggleAndRecurrenceStatuses(statuses, '2', '1', '2');
+        });
+
+        it('should make CANCELLED next task IN_PROGRESS, if TODO not found', () => {
+            const statuses: StatusCollection = [
+                ['/', '/ to x', 'x', 'IN_PROGRESS'],
+                ['x', 'x to -', '-', 'DONE'],
+                ['-', '- to /', '/', 'CANCELLED'],
+            ];
+            // Ensure that the next status skips through to IN_PROGRESS for a recurring task, if TODO not found
+            checkToggleAndRecurrenceStatuses(statuses, '/', 'x', '/');
+        });
+
+        it('should select TODO even if DONE is followed by IN_PROGRESS', () => {
+            // This is not intended to be realistic: its sole purpose is to have DONE followed by IN_PROGRESS,
+            // and ensure that this is chosen in preference to the TODO that is further ahead in the sequences
+            // of statuses.
+            const statuses: StatusCollection = [
+                ['T', 'T to D', 'D', 'TODO'],
+                ['D', 'D to I', 'I', 'DONE'],
+                ['I', 'I to T', 'T', 'IN_PROGRESS'],
+            ];
+            // Ensure that TODO is chosen, ignoring the IN_PROGRESS task immediately after DONE:
+            checkToggleAndRecurrenceStatuses(statuses, 'T', 'D', 'T');
+        });
+
+        it('should select the correct next status, when there is ambiguity', () => {
+            const statuses: StatusCollection = [
+                // A set of 4 statuses, chosen to see whether the loop size in getNextRecurrenceStatusOrCreate()
+                // affects the calculation in any way.
+                ['A', 'A to B', 'B', 'NON_TASK'],
+                ['B', 'B to C', 'C', 'NON_TASK'],
+                ['C', 'C to D', 'D', 'NON_TASK'],
+                ['D', 'D to E', 'E', 'NON_TASK'],
+                // A cycle of 6 statuses, not including any TODOs,
+                // to ensure that the correct IN_PROGRESS is selected.
+                ['1', '1 to 2', '2', 'IN_PROGRESS'],
+                ['2', '2 to 3', '3', 'DONE'],
+                ['3', '3 to 4', '4', 'CANCELLED'],
+                ['4', '4 to 5', '5', 'IN_PROGRESS'],
+                ['5', '5 to 6', '6', 'DONE'],
+                ['6', '6 to 1', '1', 'CANCELLED'],
+            ];
+            // Ensure that the IN_PROGRESS soonest after 2 is chosen:
+            checkToggleAndRecurrenceStatuses(statuses, '1', '2', '4');
+        });
+
+        it('should select the correct next status, even when it is unknown', () => {
+            const statuses: StatusCollection = [
+                // A set where the DONE task goes to an unknown symbol
+                ['a', 'Status a', 'b', 'TODO'],
+                ['b', 'Status b', 'c', 'DONE'], // c is not known
+            ];
+            checkToggleAndRecurrenceStatuses(statuses, 'a', 'b', 'c');
+        });
+
+        it('should return space for symbol if there are no TODO or IN_PROGRESS in sequence', () => {
+            const statuses: StatusCollection = [
+                [' ', 'Todo', 'x', 'TODO'],
+                ['x', 'Done', ' ', 'DONE'],
+                ['C', 'C to D', 'D', 'CANCELLED'],
+                ['D', 'D to C', 'C', 'DONE'],
+            ];
+            checkToggleAndRecurrenceStatuses(statuses, 'C', 'D', ' ');
+        });
+
+        it('should return space for symbol if it is not a known status', () => {
+            const statuses: StatusCollection = [
+                ['C', 'C to D', 'D', 'CANCELLED'],
+                ['D', 'D to C', 'C', 'DONE'],
+            ];
+            checkToggleAndRecurrenceStatuses(statuses, 'C', 'D', ' ');
         });
     });
 });