Repository: obsidian-tasks-group/obsidian-tasks
Pull request #2508 (2023-12-15 16:33:13)

fix: Bug-fixes and tests for the new Statuses context menu
<!--- Provide a general summary of your changes in the Title above -->

# Description

The context menu added in #2476 enables a few status transitions that were not previously available to users.

I found in exploratory testing that changing from one DONE status to a different DONE status did unexpected things:

- it created a new recurrence, if the task was recurring
- it updated the Done Date to the current date.

Also, I found that:

- Selecting the status that the task already had caused a new, identical task to be created unnecessarily.

This PR:

- Adds tests for the code used in the new statuses menu.
    - These were initially-failing tests for the issues I found in exploratory testing.
- Then fixes the issues
- Adds test of the 'Set Done Date' setting (having spotted a gap in coverage before refactoring)

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

Enable me to release the feature added in #2476.

## How has this been tested?

- By adding new tests
- By exploratory testing, to confirm that the behaviour really has been fixed.

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, tests ran to see how -->
<!--- your change affects other areas of the code, etc. -->

## 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/Status.ts b/src/Status.ts
index 86291beb..c161f69a 100644
--- a/src/Status.ts
+++ b/src/Status.ts
@@ -192,6 +192,13 @@ export class Status {
         return new Status(new StatusConfiguration('/', 'In Progress', 'x', true, StatusType.IN_PROGRESS));
     }
 
+    /**
+     * A sample Non-Task status. Goes to NON_TASK when toggled.
+     */
+    static makeNonTask(): Status {
+        return new Status(new StatusConfiguration('Q', 'Non-Task', 'A', true, StatusType.NON_TASK));
+    }
+
     /**
      * Return the StatusType to use for a symbol, if it is not in the StatusRegistry.
      * The core symbols are recognised.
diff --git a/src/Task.ts b/src/Task.ts
index 8a13c387..7f3b64ed 100644
--- a/src/Task.ts
+++ b/src/Task.ts
@@ -383,24 +383,33 @@ export class Task {
         return newTasks;
     }
 
-    private handleNewStatus(newStatus: Status): Task[] {
+    public handleNewStatus(newStatus: Status): Task[] {
+        if (newStatus.identicalTo(this.status)) {
+            // There is no need to create a new Task object if the new status behaviour is identical to the current one.
+            return [this];
+        }
+
         let newDoneDate = null;
+        if (newStatus.isCompleted()) {
+            if (!this.status.isCompleted()) {
+                // Set done date only if setting value is true
+                const { setDoneDate } = getSettings();
+                if (setDoneDate) {
+                    newDoneDate = window.moment();
+                }
+            } else {
+                // This task was already completed, so preserve its done date.
+                newDoneDate = this.doneDate;
+            }
+        }
 
         let nextOccurrence: {
             startDate: Moment | null;
             scheduledDate: Moment | null;
             dueDate: Moment | null;
         } | null = null;
-
         if (newStatus.isCompleted()) {
-            // Set done date only if setting value is true
-            const { setDoneDate } = getSettings();
-            if (setDoneDate) {
-                newDoneDate = window.moment();
-            }
-
-            // If this task is no longer todo, we need to check if it is recurring:
-            if (this.recurrence !== null) {
+            if (!this.status.isCompleted() && this.recurrence !== null) {
                 nextOccurrence = this.recurrence.next();
             }
         }
@@ -463,9 +472,9 @@ export class Task {
         return recurrenceOnNextLine ? newTasks.reverse() : newTasks;
     }
 
-    public handleStatusChangeFromContextMenuWithRecurrenceInUsersOrder(newStatus: Status): Task[] {
+    public handleNewStatusWithRecurrenceInUsersOrder(newStatus: Status): Task[] {
         const logger = logging.getLogger('tasks.Task');
-        logger.trace(
+        logger.debug(
             `changed task ${this.taskLocation.path} ${this.taskLocation.lineNumber} ${this.originalMarkdown} status to ${newStatus}`,
         );
 
diff --git a/src/ui/Menus/StatusMenu.ts b/src/ui/Menus/StatusMenu.ts
index f192168e..71c9d14b 100644
--- a/src/ui/Menus/StatusMenu.ts
+++ b/src/ui/Menus/StatusMenu.ts
@@ -37,7 +37,7 @@ export class StatusMenu extends TaskEditingMenu {
                 .onClick(async () => {
                     if (newStatusSymbol !== task.status.symbol) {
                         const status = this.statusRegistry.bySymbol(newStatusSymbol);
-                        const newTask = task.handleStatusChangeFromContextMenuWithRecurrenceInUsersOrder(status);
+                        const newTask = task.handleNewStatusWithRecurrenceInUsersOrder(status);
                         await this.taskSaver(task, newTask);
                     }
                 });


Test patch:
diff --git a/tests/Status.test.ts b/tests/Status.test.ts
index 08699fde..e5110e8f 100644
--- a/tests/Status.test.ts
+++ b/tests/Status.test.ts
@@ -29,6 +29,7 @@ describe('Status', () => {
         expect(Status.makeInProgress().previewText()).toEqual(
             "- [/] => [x], name: 'In Progress', type: 'IN_PROGRESS'.",
         );
+        expect(Status.makeNonTask().previewText()).toEqual("- [Q] => [A], name: 'Non-Task', type: 'NON_TASK'.");
     });
 
     it('should initialize with valid properties', () => {
diff --git a/tests/Task.test.ts b/tests/Task.test.ts
index 860fdb68..4ec6f124 100644
--- a/tests/Task.test.ts
+++ b/tests/Task.test.ts
@@ -688,23 +688,24 @@ describe('to string', () => {
     });
 });
 
+const sampleStatusesForToggling: StatusCollection = [
+    // A custom set of 3 statuses that form a cycle.
+    // The last one has a conventional symbol, 'X' that is recognised as DONE.fix
+    ['!', 'Important', 'D', 'TODO'],
+    ['D', 'Doing - Important', 'X', 'IN_PROGRESS'],
+    ['X', 'Done - Important', '!', 'DONE'],
+    // A set that uses an unconventional symbol for DONE
+    ['1', 'Status 1', '2', 'TODO'],
+    ['2', 'Status 2', '3', 'IN_PROGRESS'],
+    ['3', 'Status 3', '1', 'DONE'],
+    // 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
+];
+
 describe('toggle done', () => {
     beforeAll(() => {
-        const statuses: StatusCollection = [
-            // A custom set of 3 statuses that form a cycle.
-            // The last one has a conventional symbol, 'X' that is recognised as DONE.fix
-            ['!', 'Important', 'D', 'TODO'],
-            ['D', 'Doing - Important', 'X', 'IN_PROGRESS'],
-            ['X', 'Done - Important', '!', 'DONE'],
-            // A set that uses an unconventional symbol for DONE
-            ['1', 'Status 1', '2', 'TODO'],
-            ['2', 'Status 2', '3', 'IN_PROGRESS'],
-            ['3', 'Status 3', '1', 'DONE'],
-            // 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
-        ];
-        statuses.forEach((s) => {
+        sampleStatusesForToggling.forEach((s) => {
             StatusRegistry.getInstance().add(Status.createFromImportedValue(s));
         });
     });
@@ -713,6 +714,15 @@ describe('toggle done', () => {
         StatusRegistry.getInstance().resetToDefaultStatuses();
     });
 
+    beforeEach(() => {
+        jest.useFakeTimers();
+    });
+
+    afterEach(() => {
+        jest.useRealTimers();
+        resetSettings();
+    });
+
     it('retains the block link', () => {
         // Arrange
         const line = '- [ ] this is a task 📅 2021-09-12 ^my-precious';
@@ -751,6 +761,20 @@ describe('toggle done', () => {
         expect(toggled!.doneDate).toBeNull();
     });
 
+    it('should not add done date to completed task, if disabled in settings', () => {
+        // Arrange
+        const task = new TaskBuilder().build();
+        updateSettings({ setDoneDate: false });
+
+        // Act
+        const tasks = task.toggle();
+
+        // Assert
+        expect(tasks.length).toEqual(1);
+        const toggled: Task = tasks[0];
+        expect(toggled.doneDate).toBeNull();
+    });
+
     type RecurrenceCase = {
         // inputs:
         interval: string;
@@ -1070,7 +1094,9 @@ describe('toggle done', () => {
             nextStart,
             nextInterval,
         }) => {
-            const todaySpy = jest.spyOn(Date, 'now').mockReturnValue(moment(today).valueOf());
+            if (today) {
+                jest.setSystemTime(new Date(today));
+            }
 
             // If this test fails, the RecurrenceCase had no expected new dates set, and so
             // is accidentally not doing any testing.
@@ -1118,7 +1144,6 @@ describe('toggle done', () => {
             } else {
                 expect(nextTask.recurrence?.toText()).toBe(interval);
             }
-            todaySpy.mockClear();
         },
     );
 
@@ -1150,6 +1175,83 @@ describe('toggle done', () => {
     });
 });
 
+describe('handle new status', () => {
+    beforeAll(() => {
+        sampleStatusesForToggling.forEach((s) => {
+            StatusRegistry.getInstance().add(Status.createFromImportedValue(s));
+        });
+    });
+
+    afterAll(() => {
+        StatusRegistry.getInstance().resetToDefaultStatuses();
+    });
+
+    beforeEach(() => {
+        jest.useFakeTimers();
+        jest.setSystemTime(new Date('2023-06-26'));
+    });
+
+    // Note: We only need to test transitions which are not covered by the standard 'toggle done' tests above.
+
+    it('should not create a new task, if the new status is the same object', () => {
+        const task = fromLine({ line: '- [!] An important task' });
+        const newTasks = task.handleNewStatus(StatusRegistry.getInstance().bySymbol('!'));
+
+        expect(newTasks.length).toEqual(1);
+        expect(Object.is(task, newTasks[0])).toEqual(true);
+    });
+
+    it('should not create a new task, if the new status is different object but same behaviour', () => {
+        const task = fromLine({ line: '- [!] An important task' });
+        const newStatus = Status.createFromImportedValue(['!', 'Important', 'D', 'TODO']);
+        const newTasks = task.handleNewStatus(newStatus);
+
+        expect(newTasks.length).toEqual(1);
+        expect(Object.is(task, newTasks[0])).toEqual(true);
+    });
+
+    it('should create a new task, if the status symbol is unchanged but represents a different behaviour', () => {
+        const task = fromLine({ line: '- [!] An important task' });
+        const newStatus = Status.createFromImportedValue(['!', 'a different status', 'D', 'TODO']);
+        const newTasks = task.handleNewStatus(newStatus);
+
+        expect(newTasks.length).toEqual(1);
+        expect(Object.is(task, newTasks[0])).toEqual(false);
+    });
+
+    it('should not change the done date, if changing from one DONE status to another', () => {
+        // Arrange
+        const doneTask = fromLine({
+            line: '- [X] Stuff 📅 2023-12-15 ✅ 2019-01-17',
+        });
+
+        // Act
+        const newTasks = doneTask.handleNewStatus(Status.makeDone());
+
+        // Assert
+        expect(newTasks.length).toEqual(1);
+        // Check that the done date was not modified:
+        expect(newTasks[0].doneDate).toEqualMoment(moment('2019-01-17'));
+    });
+
+    it('should not create new recurrence if converting from one DONE status to another', () => {
+        // Arrange
+        const originalTask = fromLine({
+            line: '- [x] A recurring, done task 🔁 every day 📅 2023-12-15 ✅ 2023-12-15',
+        });
+        const newStatus = StatusRegistry.getInstance().bySymbol('X');
+
+        // Act
+        const newTasks = originalTask.handleNewStatus(newStatus);
+
+        // Assert
+        // Because the old task was already DONE, we should not have created a new recurrence:
+        expect(newTasks.length).toEqual(1);
+        // But check that the new symbol has been applied:
+        expect(newTasks[0].status.symbol).toEqual('X');
+    });
+});
+
 describe('created dates on recurring task', () => {
     beforeEach(() => {
         jest.useFakeTimers();