Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1459 (2023-01-02 15:32:43)

fix: sort group & filter by status now match 1.22.0
<!--- Provide a general summary of your changes in the Title above -->

# Description

I found in manual testing that there were some accidental changes in the behaviour since the 1.22.0 release for the following instructions

- `done`
- `not done`
- `sort by status`
- `group by status`

All those instructions now behave identically to 1.22.0 for tasks with standard and custom status characters .

It turned out that there was no previous test coverage of these instructions for anything other than the standard `- [ ]` and `- [x]` lines.

To fix this, in a development build I added tests for all those instructions with custom status characters, and when they were passing, I copied the new tests to this latest code, and then fixed and documented the changes in behaviour between 'Tasks SQL Powered' and this plugin.

## Motivation and Context

Ensuring backwards-compatibility with 1.22.0 and earlier.

## How has this been tested?

By writing tests of the behaviour against 1.22.0 code, and then copying those tests in to the latest code and fixing the changes found.

## 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)
    - Note: this was fixing issues in the development code, not in anything released.
- [x] **Documentation** (prefix: `docs` - improvements to any documentation content)
- [x] **Sample vault** (prefix: `vault` - improvements to the [Tasks-Demo sample vault](https://github.com/obsidian-tasks-group/obsidian-tasks/tree/main/resources/sample_vaults/Tasks-Demo))

Internal changes:

- [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.
- [x] I have [updated the documentation](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#updating-documentation) accordingly.
- [x] My change has adequate [Unit Test coverage](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md#maintaining-the-tests).

## Terms

<!--
By submitting this pull request, you must agree to follow our
[contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md) 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://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md)
- [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/docs/getting-started/statuses.md b/docs/getting-started/statuses.md
index ea6b4b19..9a29f41c 100644
--- a/docs/getting-started/statuses.md
+++ b/docs/getting-started/statuses.md
@@ -53,8 +53,11 @@ The notes below may help anyone who is switching their vaults between the 'Tasks
 
 They describe the changes made when back-porting the 'Tasks SQL Powered' custom status code, to retain backwards compatibility.
 
-| Topic                                                 | 'Tasks SQL Powered' [^task-x-version]                                                                                                                                                                                                                | 'Tasks'                                                                         |
-| ----------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------- |
-| Handling unknown status characters, for example `[%]` | Stores and displays the task with no status character:<br>- this affects any custom CSS styling,<br>- toggling changes the line to `[]` - with no status character.<br>(The fix is to ensure all status characters are added in the plugin's settings) | Retains any unknown status characters:<br> - styling and toggling are unchanged |
+| Topic                                                 | 'Tasks SQL Powered' [^task-x-version]                                                                                                                                                                                                                  | 'Tasks'                                                                                                        |
+| ----------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------- |
+| `done`, `not done`                                    | Only tasks with status characters `x` and `X` are treated as `done`.                                                                                                                                                                                   | All tasks with status characters other than `space` are treated as `done`  - for backwards compatibility.      |
+| `sort by status`                                      | Case-sensitive sort of the status character, in reverse alphabetical order. Unrecognised status characters are treated as empty, and appear before all other statuses.                                                                                 | All tasks with `space` status character sort first, then all other tasks  - for backwards compatibility.       |
+| `group by status`                                     | Groups by the name of the status, for example `Cancelled`, `Done`, `In Progress`, `EMPTY`.                                                                                                                                                             | Groups by either `Todo` if the status character is a space, or otherwise `Done` - for backwards compatibility. |
+| Handling unknown status characters, for example `[%]` | Stores and displays the task with no status character:<br>- this affects any custom CSS styling,<br>- toggling changes the line to `[]` - with no status character.<br>(The fix is to ensure all status characters are added in the plugin's settings) | Retains any unknown status characters:<br> - styling and toggling are unchanged                                |
 
 [^task-x-version]: 'Tasks SQL Powered' as of [revision 2c0b659](https://github.com/sytone/obsidian-tasks-x/tree/2c0b659457cc80806ff18585c955496c76861b87) on 2 August 2022
diff --git a/src/Query/Filter/StatusField.ts b/src/Query/Filter/StatusField.ts
index 894e1200..3ebc84a0 100644
--- a/src/Query/Filter/StatusField.ts
+++ b/src/Query/Filter/StatusField.ts
@@ -1,4 +1,3 @@
-import { Status } from '../../Status';
 import type { Task } from '../../Task';
 import type { Comparator } from '../Sorter';
 import { FilterInstructionsBasedField } from './FilterInstructionsBasedField';
@@ -7,8 +6,11 @@ export class StatusField extends FilterInstructionsBasedField {
     constructor() {
         super();
 
-        this._filters.add('done', (task: Task) => task.status === Status.DONE);
-        this._filters.add('not done', (task: Task) => task.status !== Status.DONE);
+        // Backwards-compatibility note: In Tasks 1.22.0 and earlier, all tasks
+        // with any status character except space were considered by the status filter
+        // instructions to be done.
+        this._filters.add('done', (task: Task) => task.status.indicator !== ' ');
+        this._filters.add('not done', (task: Task) => task.status.indicator === ' ');
     }
 
     public fieldName(): string {
@@ -23,14 +25,27 @@ export class StatusField extends FilterInstructionsBasedField {
      * Return a function to compare two Task objects, for use in sorting by status.
      */
     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) => {
-            if (a.status.name < b.status.name) {
+            const oldStatusNameA = StatusField.oldStatusName(a);
+            const oldStatusNameB = StatusField.oldStatusName(b);
+            if (oldStatusNameA < oldStatusNameB) {
                 return 1;
-            } else if (a.status.name > b.status.name) {
+            } else if (oldStatusNameA > oldStatusNameB) {
                 return -1;
             } else {
                 return 0;
             }
         };
     }
+
+    private static oldStatusName(a: Task): string {
+        if (a.status.indicator === ' ') {
+            return 'Todo';
+        } else {
+            return 'Done';
+        }
+    }
 }
diff --git a/src/Query/Group.ts b/src/Query/Group.ts
index 46ce4f16..6b0eb856 100644
--- a/src/Query/Group.ts
+++ b/src/Query/Group.ts
@@ -188,7 +188,14 @@ export class Group {
     }
 
     private static groupByStatus(task: Task): string[] {
-        return [task.status.name];
+        // 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'.
+        if (task.status.indicator === ' ') {
+            return ['Todo'];
+        } else {
+            return ['Done'];
+        }
     }
 
     private static groupByHeading(task: Task): string[] {


Test patch:
diff --git a/resources/sample_vaults/Tasks-Demo/Manual Testing/Custom Task Statuses.md b/resources/sample_vaults/Tasks-Demo/Manual Testing/Custom Task Statuses.md
index 3fdb56bc..43d4e434 100644
--- a/resources/sample_vaults/Tasks-Demo/Manual Testing/Custom Task Statuses.md
+++ b/resources/sample_vaults/Tasks-Demo/Manual Testing/Custom Task Statuses.md
@@ -1,12 +1,48 @@
 # Custom Task Statuses
 
-- [ ] #task Task 1
-- [-] #task Task 2
-- [x] #task Task 3
+- [ ] #task Unchecked `space`
+- [-] #task Dropped `-`
+- [x] #task Regular `x`
+- [X] #task Checked `X`
+- [/] #task Half Done `/`
+- [!] #task Important `!`
+
+---
 
 ## group by status
 
 ```tasks
-filename includes Custom Task Statuses
+path includes Custom Task Statuses
 group by status
+sort by description
+```
+
+---
+
+## sort by status
+
+```tasks
+path includes Custom Task Statuses
+sort by status
+sort by description
+```
+
+---
+
+## Done
+
+```tasks
+path includes Custom Task Statuses
+done
+sort by description
+```
+
+---
+
+## Not Done
+
+```tasks
+path includes Custom Task Statuses
+not done
+sort by description
 ```
diff --git a/tests/Group.test.ts b/tests/Group.test.ts
index 88cbde50..8bf534a7 100644
--- a/tests/Group.test.ts
+++ b/tests/Group.test.ts
@@ -546,6 +546,26 @@ describe('Group names', () => {
             taskLine: '- [x] a',
             expectedGroupNames: ['Done'],
         },
+        {
+            groupBy: 'status',
+            taskLine: '- [X] a',
+            expectedGroupNames: ['Done'],
+        },
+        {
+            groupBy: 'status',
+            taskLine: '- [/] a',
+            expectedGroupNames: ['Done'],
+        },
+        {
+            groupBy: 'status',
+            taskLine: '- [-] a',
+            expectedGroupNames: ['Done'],
+        },
+        {
+            groupBy: 'status',
+            taskLine: '- [!] a',
+            expectedGroupNames: ['Done'],
+        },
 
         // -----------------------------------------------------------
         // group by tags
diff --git a/tests/Query/Filter/StatusField.test.ts b/tests/Query/Filter/StatusField.test.ts
index a9139a39..64add231 100644
--- a/tests/Query/Filter/StatusField.test.ts
+++ b/tests/Query/Filter/StatusField.test.ts
@@ -1,18 +1,17 @@
 import { StatusField } from '../../../src/Query/Filter/StatusField';
-import type { FilterOrErrorMessage } from '../../../src/Query/Filter/Filter';
 import { TaskBuilder } from '../../TestingTools/TaskBuilder';
-import { testFilter } from '../../TestingTools/FilterTestHelpers';
+import { toMatchTaskFromLine } from '../../CustomMatchers/CustomMatchersForFilters';
 import { Status } from '../../../src/Status';
+import * as TestHelpers from '../../TestHelpers';
 import {
     expectTaskComparesAfter,
     expectTaskComparesBefore,
     expectTaskComparesEqual,
 } from '../../CustomMatchers/CustomMatchersForSorting';
 
-function testStatusFilter(filter: FilterOrErrorMessage, status: Status, expected: boolean) {
-    const builder = new TaskBuilder();
-    testFilter(filter, builder.status(status), expected);
-}
+expect.extend({
+    toMatchTaskFromLine,
+});
 
 describe('status', () => {
     it('done', () => {
@@ -20,8 +19,12 @@ describe('status', () => {
         const filter = new StatusField().createFilterOrErrorMessage('done');
 
         // Assert
-        testStatusFilter(filter, Status.TODO, false);
-        testStatusFilter(filter, Status.DONE, true);
+        expect(filter).not.toMatchTaskFromLine('- [ ] X');
+        expect(filter).toMatchTaskFromLine('- [x] X');
+        expect(filter).toMatchTaskFromLine('- [X] X');
+        expect(filter).toMatchTaskFromLine('- [/] X');
+        expect(filter).toMatchTaskFromLine('- [-] X');
+        expect(filter).toMatchTaskFromLine('- [!] X');
     });
 
     it('not done', () => {
@@ -29,8 +32,12 @@ describe('status', () => {
         const filter = new StatusField().createFilterOrErrorMessage('not done');
 
         // Assert
-        testStatusFilter(filter, Status.TODO, true);
-        testStatusFilter(filter, Status.DONE, false);
+        expect(filter).toMatchTaskFromLine('- [ ] X');
+        expect(filter).not.toMatchTaskFromLine('- [x] X');
+        expect(filter).not.toMatchTaskFromLine('- [X] X');
+        expect(filter).not.toMatchTaskFromLine('- [/] X');
+        expect(filter).not.toMatchTaskFromLine('- [-] X');
+        expect(filter).not.toMatchTaskFromLine('- [!] X');
     });
 });
 
@@ -49,8 +56,18 @@ describe('sorting by status', () => {
 
         // Assert
         expectTaskComparesAfter(sorter, doneTask, todoTask);
+
         expectTaskComparesBefore(sorter, todoTask, doneTask);
+        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, 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' }));
     });
 
     it('sort by status reverse', () => {