Merge pull request #3101 from obsidian-tasks-group/fix-3030-group-and…

…-sort-by-status

fix: Improve usefulness of 'group by status' and 'sort by status'

docs/Getting Started/Statuses/Status Types.md

@@ -107,7 +107,7 @@ The tasks shown are purely examples for context. The `~` column is just an arbit
 | Matches `status.name includes in progress` | no | YES | no | no | no |
 | Matches `status.name includes done` | no | no | YES | no | no |
 | Matches `status.name includes cancelled` | no | no | no | YES | no |
-| Name for `group by status` | Todo | Done | Done | Done | Done |
+| Name for `group by status` | Todo | Todo | Done | Done | Done |
 | Name for `group by status.type` | %%2%%TODO | %%1%%IN_PROGRESS | %%3%%DONE | %%4%%CANCELLED | %%5%%NON_TASK |
 | Name for `group by status.name` | Todo | In Progress | Done | Cancelled | My custom status |
 

src/Query/Filter/StatusField.ts

@@ -30,11 +30,9 @@ export class StatusField extends FilterInstructionsBasedField {
 
     /**
      * Return a function to compare two Task objects, for use in sorting by status.
+     * TODO and IN_PROGRESS types are sorted before the other types.
      */
     public comparator(): Comparator {
-        // Backwards-compatibility note: In Tasks 1.22.0 and earlier, the
-        // only available status names were 'Todo' and 'Done'.
-        // And 'Todo' sorted before 'Done'.
         return (a: Task, b: Task) => {
             const oldStatusNameA = StatusField.oldStatusName(a);
             const oldStatusNameB = StatusField.oldStatusName(b);
@@ -49,7 +47,7 @@ export class StatusField extends FilterInstructionsBasedField {
     }
 
     private static oldStatusName(a: Task): string {
-        if (a.status.symbol === ' ') {
+        if (!a.isDone) {
             return 'Todo';
         } else {
             return 'Done';
@@ -60,11 +58,14 @@ export class StatusField extends FilterInstructionsBasedField {
         return true;
     }
 
+    /**
+     * Return a function to name tasks, for use in grouping by status.
+     * TODO and IN_PROGRESS types are grouped in 'Todo'.
+     * Other status types are grouped in 'Done'.
+     */
+
     public grouper(): GrouperFunction {
         return (task: Task) => {
-            // Backwards-compatibility note: In Tasks 1.22.0 and earlier, the only
-            // names used by 'group by status' were 'Todo' and 'Done' - and
-            // any character other than a space was considered to be 'Done'.
             return [StatusField.oldStatusName(task)];
         };
     }

tests/DocumentationSamples/DocsSamplesForStatuses.test.Status_Transitions_status-types.approved.md

@@ -14,7 +14,7 @@
 | Matches `status.name includes in progress` | no | YES | no | no | no |
 | Matches `status.name includes done` | no | no | YES | no | no |
 | Matches `status.name includes cancelled` | no | no | no | YES | no |
-| Name for `group by status` | Todo | Done | Done | Done | Done |
+| Name for `group by status` | Todo | Todo | Done | Done | Done |
 | Name for `group by status.type` | %%2%%TODO | %%1%%IN_PROGRESS | %%3%%DONE | %%4%%CANCELLED | %%5%%NON_TASK |
 | Name for `group by status.name` | Todo | In Progress | Done | Cancelled | My custom status |
 

tests/Query/Filter/StatusField.test.ts

@@ -9,6 +9,24 @@ import {
 } from '../../CustomMatchers/CustomMatchersForSorting';
 import { StatusConfiguration, StatusType } from '../../../src/Statuses/StatusConfiguration';
 import { fromLine } from '../../TestingTools/TestHelpers';
+import { StatusRegistry } from '../../../src/Statuses/StatusRegistry';
+import type { StatusCollection } from '../../../src/Statuses/StatusCollection';
+
+beforeAll(() => {
+    StatusRegistry.getInstance().resetToDefaultStatuses();
+    const importantCycle: StatusCollection = [
+        ['!', 'todo', 'X', 'TODO'],
+        ['X', 'done', '!', 'DONE'],
+    ];
+    importantCycle.forEach((entry) => {
+        const status = Status.createFromImportedValue(entry);
+        StatusRegistry.getInstance().add(status);
+    });
+});
+
+afterAll(() => {
+    StatusRegistry.getInstance().resetToDefaultStatuses();
+});
 
 describe('status', () => {
     it('done', () => {
@@ -67,13 +85,13 @@ describe('sorting by status', () => {
         expectTaskComparesBefore(sorter, todoTask, TestHelpers.fromLine({ line: '- [-] Z' }));
         expectTaskComparesBefore(sorter, todoTask, TestHelpers.fromLine({ line: '- [x] Z' }));
         expectTaskComparesBefore(sorter, todoTask, TestHelpers.fromLine({ line: '- [X] Z' }));
-        expectTaskComparesBefore(sorter, todoTask, TestHelpers.fromLine({ line: '- [!] Z' }));
+        expectTaskComparesEqual(sorter, todoTask, TestHelpers.fromLine({ line: '- [!] Z' }));
 
         expectTaskComparesEqual(sorter, doneTask, doneTask);
         expectTaskComparesEqual(sorter, doneTask, TestHelpers.fromLine({ line: '- [-] Z' }));
         expectTaskComparesEqual(sorter, doneTask, TestHelpers.fromLine({ line: '- [x] Z' }));
         expectTaskComparesEqual(sorter, doneTask, TestHelpers.fromLine({ line: '- [X] Z' }));
-        expectTaskComparesEqual(sorter, doneTask, TestHelpers.fromLine({ line: '- [!] Z' }));
+        expectTaskComparesAfter(sorter, doneTask, TestHelpers.fromLine({ line: '- [!] Z' }));
     });
 
     it('sort by status reverse', () => {
@@ -96,15 +114,20 @@ describe('grouping by status', () => {
         ['- [ ] a', ['Todo']],
         ['- [x] a', ['Done']],
         ['- [X] a', ['Done']],
-        ['- [/] a', ['Done']],
+        ['- [/] a', ['Todo']],
         ['- [-] a', ['Done']],
-        ['- [!] a', ['Done']],
+        ['- [!] a', ['Todo']],
     ])('task "%s" should have groups: %s', (taskLine: string, groups: string[]) => {
         // Arrange
         const grouper = new StatusField().createNormalGrouper();
 
         // Assert
         const tasks = [fromLine({ line: taskLine })];
+
+        // Check this symbol has been registered, so we are not passing by luck:
+        const symbol = tasks[0].status.symbol;
+        expect(StatusRegistry.getInstance().bySymbol(symbol).type).not.toEqual(StatusType.EMPTY);
+
         expect({ grouper, tasks }).groupHeadingsToBe(groups);
     });