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

Issue:
If toggling TO an unknown status, Tasks makes the task invalid, with an empty status character
### 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

When the custom status settings say that a task's next symbol is a character that is not any of the custom statuses, and the user toggles the task, Tasks should still honour the request for the next symbol.

### Current behaviour

Tasks sets the task's symbol to the empty string, so the task line becomes `- []`, which is not recognised as a task, and is effectively broken.

### Steps to reproduce

### Setup

1. Download and open the build of 84b433bd9dffd72a24f507f826c39a6bff117267 
    - by go here https://github.com/obsidian-tasks-group/obsidian-tasks/actions/runs/3868443498
    - downloading the Tasks-Demo zip file
    - expanding it
    - opening the folder in Obsidian
    - trusting the plugins
1. Go to Tasks settings
2. Scroll down to **Custom Status Types**
3. Click **Add New Task Status**
5. Click the Pencil icon
    - Set the boxes to `!`, `Important`, `D`
    - Click the Checkmark to save

### Reproduction

6. Create this new task line in any markdown file:
    `- [!] #task I was important`
7. In Reading mode, click on the checkbox for the new task

### Expected behaviour:
- Task becomes:
    `- [D] #task I was important`

### Actual behaviour:
- Task becomes this - with no character between the square brackets:
    `- [] #task I was important`
and is not longer recognised as a task.




### Which Operating Systems are you using?

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

### Obsidian Version

1.1.9

### Tasks Plugin Version

Commit 84b433bd9dffd72a24f507f826c39a6bff117267

### Checks

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

### Possible solution

Don't put empty [], do put the requested character inside the [].
The task parsing code already copes with this situation, creating a Status called something like `Unknown (D)` or whatever the unknown symbol was.

Pull request:
fix: toggling to unknown status now gives valid task
# Description

<!--- Describe your changes in detail -->

If toggling TO an unknown status, Tasks now honours the requested next character, instead of breaking the task with an empty `[]`.

## Motivation and Context

Fixes #1499

## How has this been tested?

- Adding unit test which failed before the bug was fixed
- Testing the new behaviour manually in Obsidian.

## 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] **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`.
- [ ] My change requires a change to the documentation.
- [ ] 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/src/Status.ts b/src/Status.ts
index f3fc325f..88911d56 100644
--- a/src/Status.ts
+++ b/src/Status.ts
@@ -185,6 +185,17 @@ export class Status {
         this.configuration = configuration;
     }
 
+    /**
+     * Create a Status representing the given, unknown indicator.
+     *
+     * This can be useful when StatusRegistry does not recognise an indicator,
+     * and we do not want to expose the user's data to the Status.EMPTY status.
+     * @param unknownIndicator
+     */
+    static createUnknownStatus(unknownIndicator: string) {
+        return new Status(new StatusConfiguration(unknownIndicator, 'Unknown', 'x', false));
+    }
+
     /**
      * Returns the completion status for a task, this is only supported
      * when the task is done/x.
diff --git a/src/Task.ts b/src/Task.ts
index f8e2c7ec..31c085e0 100644
--- a/src/Task.ts
+++ b/src/Task.ts
@@ -4,7 +4,7 @@ import type { TaskLayoutComponent } from './TaskLayout';
 import { Recurrence } from './Recurrence';
 import { getSettings } from './Config/Settings';
 import { StatusRegistry } from './StatusRegistry';
-import { Status, StatusConfiguration } from './Status';
+import { Status } from './Status';
 import { Urgency } from './Urgency';
 import { DateField } from './Query/Filter/DateField';
 import { renderTaskLine } from './TaskLineRenderer';
@@ -265,7 +265,7 @@ export class Task {
         const statusString = regexMatch[3];
         let status = StatusRegistry.getInstance().byIndicator(statusString);
         if (status === Status.EMPTY) {
-            status = new Status(new StatusConfiguration(statusString, 'Unknown', 'x', false));
+            status = Status.createUnknownStatus(statusString);
         }
 
         // Match for block link and remove if found. Always expected to be
@@ -516,7 +516,10 @@ export class Task {
      * task is not recurring, it will return `[toggled]`.
      */
     public toggle(): Task[] {
-        const newStatus = StatusRegistry.getInstance().getNextStatus(this.status);
+        let newStatus = StatusRegistry.getInstance().getNextStatus(this.status);
+        if (newStatus === Status.EMPTY) {
+            newStatus = Status.createUnknownStatus(this.status.nextStatusIndicator);
+        }
 
         let newDoneDate = null;
 


Test patch:
diff --git a/tests/Status.test.ts b/tests/Status.test.ts
index 260c4023..7c993411 100644
--- a/tests/Status.test.ts
+++ b/tests/Status.test.ts
@@ -41,4 +41,19 @@ describe('Status', () => {
         expect(status!.nextStatusIndicator).toEqual(next);
         expect(status!.isCompleted()).toEqual(true);
     });
+
+    it('should construct a Status for unknown symbol', () => {
+        // Arrange
+        const indicator = 'U';
+
+        // Act
+        const status = Status.createUnknownStatus(indicator);
+
+        // Assert
+        expect(status).not.toBeNull();
+        expect(status!.indicator).toEqual(indicator);
+        expect(status!.name).toEqual('Unknown');
+        expect(status!.nextStatusIndicator).toEqual('x');
+        expect(status!.isCompleted()).toEqual(false);
+    });
 });
diff --git a/tests/StatusRegistry.test.ts b/tests/StatusRegistry.test.ts
index a62bc2a3..83981b36 100644
--- a/tests/StatusRegistry.test.ts
+++ b/tests/StatusRegistry.test.ts
@@ -5,6 +5,7 @@ import moment from 'moment';
 import { StatusRegistry } from '../src/StatusRegistry';
 import { Status, StatusConfiguration } from '../src/Status';
 import { Task } from '../src/Task';
+import * as TestHelpers from './TestHelpers';
 
 jest.mock('obsidian');
 window.moment = moment;
@@ -43,69 +44,6 @@ describe('StatusRegistry', () => {
         expect(statusRegistry.byIndicator('?').indicator).toEqual(Status.EMPTY.indicator);
     });
 
-    it('should allow task to toggle through standard transitions', () => {
-        // Arrange
-        // Global statusRegistry instance - which controls toggling - will have been reset
-        // in beforeEach() above.
-        const line = '- [ ] this is a task starting at A';
-        const path = 'file.md';
-        const sectionStart = 1337;
-        const sectionIndex = 1209;
-        const precedingHeader = 'Eloquent Section';
-        const task = Task.fromLine({
-            line,
-            path,
-            sectionStart,
-            sectionIndex,
-            precedingHeader,
-            fallbackDate: null,
-        });
-
-        // Act
-
-        // Assert
-        expect(task).not.toBeNull();
-        expect(task!.status.indicator).toEqual(Status.TODO.indicator);
-
-        // In Tasks, TODO toggles to DONE, for consistency with earlier releases.
-        // const toggledInProgress = task?.toggle()[0];
-        // expect(toggledInProgress?.status.indicator).toEqual(Status.IN_PROGRESS.indicator);
-
-        const toggledDone = task?.toggle()[0];
-        expect(toggledDone?.status.indicator).toEqual(Status.DONE.indicator);
-
-        const toggledTodo = toggledDone?.toggle()[0];
-        expect(toggledTodo?.status.indicator).toEqual(Status.TODO.indicator);
-    });
-
-    it('should allow task to toggle from cancelled to todo', () => {
-        // Arrange
-        // Global statusRegistry instance - which controls toggling - will have been reset
-        // in beforeEach() above.
-        const line = '- [-] This is a cancelled task';
-        const path = 'file.md';
-        const sectionStart = 1337;
-        const sectionIndex = 1209;
-        const precedingHeader = 'Eloquent Section';
-        const task = Task.fromLine({
-            line,
-            path,
-            sectionStart,
-            sectionIndex,
-            precedingHeader,
-            fallbackDate: null,
-        });
-
-        // Act
-
-        // Assert
-        expect(task).not.toBeNull();
-        expect(task!.status.indicator).toEqual(Status.CANCELLED.indicator);
-
-        const toggledTodo = task?.toggle()[0];
-        expect(toggledTodo?.status.indicator).toEqual(Status.TODO.indicator);
-    });
-
     it('should allow lookup of next status for a status', () => {
         // Arrange
         const statusRegistry = new StatusRegistry();
@@ -150,49 +88,134 @@ describe('StatusRegistry', () => {
         expect(status2).toStrictEqual(status);
     });
 
-    it('should allow task to toggle through custom transitions', () => {
-        // Arrange
-        // Toggling code uses the global status registry, so we must explicitly modify that instance.
-        // It will already have been reset in beforeEach() above.
-        const globalStatusRegistry = StatusRegistry.getInstance();
-        const statusA = new Status(new StatusConfiguration('a', 'A', 'b', false));
-        const statusB = new Status(new StatusConfiguration('b', 'B', 'c', false));
-        const statusC = new Status(new StatusConfiguration('c', 'C', 'd', false));
-        const statusD = new Status(new StatusConfiguration('d', 'D', 'a', false));
-        globalStatusRegistry.add(statusA);
-        globalStatusRegistry.add(statusB);
-        globalStatusRegistry.add(statusC);
-        globalStatusRegistry.add(statusD);
-        const line = '- [a] this is a task starting at A';
-        const path = 'file.md';
-        const sectionStart = 1337;
-        const sectionIndex = 1209;
-        const precedingHeader = 'Eloquent Section';
-        const task = Task.fromLine({
-            line,
-            path,
-            sectionStart,
-            sectionIndex,
-            precedingHeader,
-            fallbackDate: null,
+    describe('toggling', () => {
+        it('should allow task to toggle through standard transitions', () => {
+            // Arrange
+            // Global statusRegistry instance - which controls toggling - will have been reset
+            // in beforeEach() above.
+            const line = '- [ ] this is a task starting at A';
+            const path = 'file.md';
+            const sectionStart = 1337;
+            const sectionIndex = 1209;
+            const precedingHeader = 'Eloquent Section';
+            const task = Task.fromLine({
+                line,
+                path,
+                sectionStart,
+                sectionIndex,
+                precedingHeader,
+                fallbackDate: null,
+            });
+
+            // Act
+
+            // Assert
+            expect(task).not.toBeNull();
+            expect(task!.status.indicator).toEqual(Status.TODO.indicator);
+
+            // In Tasks, TODO toggles to DONE, for consistency with earlier releases.
+            // const toggledInProgress = task?.toggle()[0];
+            // expect(toggledInProgress?.status.indicator).toEqual(Status.IN_PROGRESS.indicator);
+
+            const toggledDone = task?.toggle()[0];
+            expect(toggledDone?.status.indicator).toEqual(Status.DONE.indicator);
+
+            const toggledTodo = toggledDone?.toggle()[0];
+            expect(toggledTodo?.status.indicator).toEqual(Status.TODO.indicator);
         });
 
-        // Act
+        it('should allow task to toggle from cancelled to todo', () => {
+            // Arrange
+            // Global statusRegistry instance - which controls toggling - will have been reset
+            // in beforeEach() above.
+            const line = '- [-] This is a cancelled task';
+            const path = 'file.md';
+            const sectionStart = 1337;
+            const sectionIndex = 1209;
+            const precedingHeader = 'Eloquent Section';
+            const task = Task.fromLine({
+                line,
+                path,
+                sectionStart,
+                sectionIndex,
+                precedingHeader,
+                fallbackDate: null,
+            });
+
+            // Act
+
+            // Assert
+            expect(task).not.toBeNull();
+            expect(task!.status.indicator).toEqual(Status.CANCELLED.indicator);
+
+            const toggledTodo = task?.toggle()[0];
+            expect(toggledTodo?.status.indicator).toEqual(Status.TODO.indicator);
+        });
 
-        // Assert
-        expect(task).not.toBeNull();
-        expect(task!.status.indicator).toEqual(statusA.indicator);
+        it('should allow task to toggle through custom transitions', () => {
+            // Arrange
+            // Toggling code uses the global status registry, so we must explicitly modify that instance.
+            // It will already have been reset in beforeEach() above.
+            const globalStatusRegistry = StatusRegistry.getInstance();
+            const statusA = new Status(new StatusConfiguration('a', 'A', 'b', false));
+            const statusB = new Status(new StatusConfiguration('b', 'B', 'c', false));
+            const statusC = new Status(new StatusConfiguration('c', 'C', 'd', false));
+            const statusD = new Status(new StatusConfiguration('d', 'D', 'a', false));
+            globalStatusRegistry.add(statusA);
+            globalStatusRegistry.add(statusB);
+            globalStatusRegistry.add(statusC);
+            globalStatusRegistry.add(statusD);
+            const line = '- [a] this is a task starting at A';
+            const path = 'file.md';
+            const sectionStart = 1337;
+            const sectionIndex = 1209;
+            const precedingHeader = 'Eloquent Section';
+            const task = Task.fromLine({
+                line,
+                path,
+                sectionStart,
+                sectionIndex,
+                precedingHeader,
+                fallbackDate: null,
+            });
+
+            // Act
+
+            // Assert
+            expect(task).not.toBeNull();
+            expect(task!.status.indicator).toEqual(statusA.indicator);
+
+            const toggledA = task?.toggle()[0];
+            expect(toggledA?.status.indicator).toEqual(statusB.indicator);
+
+            const toggledB = toggledA?.toggle()[0];
+            expect(toggledB?.status.indicator).toEqual(statusC.indicator);
+
+            const toggledC = toggledB?.toggle()[0];
+            expect(toggledC?.status.indicator).toEqual(statusD.indicator);
+
+            const toggledD = toggledC?.toggle()[0];
+            expect(toggledD?.status.indicator).toEqual(statusA.indicator);
+        });
 
-        const toggledA = task?.toggle()[0];
-        expect(toggledA?.status.indicator).toEqual(statusB.indicator);
+        it('should leave task valid if toggling to unknown status', () => {
+            // Arrange
+            const globalStatusRegistry = StatusRegistry.getInstance();
+            const important = new StatusConfiguration('!', 'Important', 'D', false);
+            globalStatusRegistry.add(important);
 
-        const toggledB = toggledA?.toggle()[0];
-        expect(toggledB?.status.indicator).toEqual(statusC.indicator);
+            const line = '- [!] this is an important task';
+            const task = TestHelpers.fromLine({ line });
+            expect(task).not.toBeNull();
+            expect(task!.status.indicator).toEqual(important.indicator);
 
-        const toggledC = toggledB?.toggle()[0];
-        expect(toggledC?.status.indicator).toEqual(statusD.indicator);
+            // Act
+            const toggled = task?.toggle()[0];
 
-        const toggledD = toggledC?.toggle()[0];
-        expect(toggledD?.status.indicator).toEqual(statusA.indicator);
+            // Assert
+            // Issue https://github.com/obsidian-tasks-group/obsidian-tasks/issues/1499
+            // Ensure that the next status was applied, even though it is an unrecognised status
+            expect(toggled?.status.indicator).toEqual('D');
+        });
     });
 });