Repository: obsidian-tasks-group/obsidian-tasks (ts)

Issue:
'group by status' is confusing users who have set up custom statuses, and differs from the docs
### Please check that this issue hasn't been reported before.

- [X] I searched previous [Bug Reports](https://github.com/obsidian-tasks-group/obsidian-tasks/issues?q=is%3Aopen+is%3Aissue+label%3A%22type%3A+bug%22+sort%3Acomments-desc) didn't find any similar reports.

### Expected Behavior

_Reporting on behalf of Discord user Mold, from [discussion starting at this message](https://discord.com/channels/686053708261228577/1257646566999588874/1273365314083487794)._


That  `group by status` behaves like it is documented:

<img width="839" alt="image" src="https://github.com/user-attachments/assets/2af84375-5a0b-4a40-85a2-a10dd8298ddc">


### Current behaviour

The code says it only says it only uses `todo` for `space`, however:


https://github.com/obsidian-tasks-group/obsidian-tasks/blob/b954d241221463a5f2a361891e27ee148ab2869e/src/Query/Filter/StatusField.ts#L63-L70

and

https://github.com/obsidian-tasks-group/obsidian-tasks/blob/b954d241221463a5f2a361891e27ee148ab2869e/src/Query/Filter/StatusField.ts#L51-L57

### Steps to reproduce

In a vault with Tasks enabled:

1. Create a new note - the name does not matter
2. In the Tasks settings, add this custom status:

<img width="774" alt="image" src="https://github.com/user-attachments/assets/275574cf-288b-4d91-87cc-075e607cc978">

3. Optionally install and enable the Minimal theme for nice display of the task line that you are about to add...
4. Paste in this markdown:


~~~

- ["] Make dentist appt #life/health  ⏳  2024-08-13

it should show as TODO but is showing under DONE

```tasks
group by status
group by status.name
group by status.type
path includes {{query.file.path}}
```
~~~
5. View the note in Reading mode

**Actual result**

The first heading in the results  is `DONE`

<img width="548" alt="image" src="https://github.com/user-attachments/assets/715094e8-1f07-4a7e-ac43-1f1e054485c5">


**Expected result**

The first heading should be `DONE`.


### Which Operating Systems are you using?

- [ ] Android
- [ ] iPhone/iPad
- [ ] Linux
- [X] macOS
- [ ] Windows

### Obsidian Version

v1.7.0

### Tasks Plugin Version

7.8.0

### Checks

- [ ] I have tried it with all other plugins disabled and the error still occurs

### Possible solution

I think it's time to make a breaking change and drop backwards compatibility with the pre-custom-statuses behaviour, and make `StatusField` respect custom statuses.

To put it another way, if the user has gone to the trouble of setting of custom statuses, we should honour them.

Pull request:
fix: Improve usefulness of 'group by status' and 'sort by status'
# 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)
  - Issue/discussion: #3030
- [x] **Documentation** (prefix: `docs` - improvements to any documentation content **for users**)

Internal changes:

- [x] **Tests** (prefix: `test` - additions and improvements to unit tests and the smoke tests)

## Description


- Make `group by status` and `sort by status` use StatusType, instead of just caring whether the status symbol was a space or not.
- This means that the `group by status` behaviour now matches that stated in the documentation.
- `sort by status` docs was never very specific, but the behaviour is an improvement, and still fits with the docs.

## Motivation and Context

- Fix #3030 

## How has this been tested?

- Automated tests
- Manual testing in demo vault

## 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://publish.obsidian.md/tasks-contributing/Documentation/About+Documentation) accordingly.
    - Just the updating of a snippet 
- [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 follows 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/docs/Getting Started/Statuses/Status Types.md b/docs/Getting Started/Statuses/Status Types.md
index c5f5446c..290a469b 100644
--- a/docs/Getting Started/Statuses/Status Types.md
+++ b/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 |
 
diff --git a/src/Query/Filter/StatusField.ts b/src/Query/Filter/StatusField.ts
index 7715539d..18407854 100644
--- a/src/Query/Filter/StatusField.ts
+++ b/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)];
         };
     }


Test patch:
diff --git a/tests/DocumentationSamples/DocsSamplesForStatuses.test.Status_Transitions_status-types.approved.md b/tests/DocumentationSamples/DocsSamplesForStatuses.test.Status_Transitions_status-types.approved.md
index d0b80318..f2002977 100644
--- a/tests/DocumentationSamples/DocsSamplesForStatuses.test.Status_Transitions_status-types.approved.md
+++ b/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 |
 
diff --git a/tests/Query/Filter/StatusField.test.ts b/tests/Query/Filter/StatusField.test.ts
index 565669cf..a62831a7 100644
--- a/tests/Query/Filter/StatusField.test.ts
+++ b/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);
     });