Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1455 (2023-01-01 22:33:00)

fix: Fix an unreleased error in StatusRegistry
<!--- Provide a general summary of your changes in the Title above -->

# Description

This fixes some confusing nested Status objects returned from StatusRegistry.

I have found equality tests on `Status` objects obtained from the `StatusRegistry` were failing confusingly because `Status` objects returned from `StatusRegistry` had a duplicate nested `configuration` field, and I didn't understand why.

It turned out that the code in obsidian-tasks-x was offering the convenience of adding either `StatusConfiguration` or `Status` to the `StatusRegistry`, but was not checking the type passed in. This meant that if a Status was added, what was saved was `Status(Status(StatusConfiguration))`.

This is why the code from obsidian-tasks-x which I added in 3d67b00ceca17842a31c86e8a7f1db33d0bbba9d had to do all the converting from:

```ts
    expect(task!.status).toStrictEqual(Status.DONE);
```

to:

```ts
    expect(task!.status.indicator).toStrictEqual(Status.DONE.indicator);
```

This is now fixed, and I have re-simplified the status tests in `tests/Task.test.ts`.

## Motivation and Context

Make it easier to write tests of `Status` values - and make it easier to debug the code.

## How has this been tested?

Writing new tests and running existing ones.

## Screenshots (if appropriate)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

Internal changes:

It was a refactor in that the original code was not broken, just confusing - and it wasn't a bug, in that it did not fix externally visible behaviour, due to the code being unreleased.

- [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`.
- [ ] 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/StatusRegistry.ts b/src/StatusRegistry.ts
index 86d7fb77..70858ce4 100644
--- a/src/StatusRegistry.ts
+++ b/src/StatusRegistry.ts
@@ -44,7 +44,11 @@ export class StatusRegistry {
      */
     public add(status: StatusConfiguration | Status): void {
         if (!this.hasIndicator(status.indicator)) {
-            this._registeredStatuses.push(new Status(status));
+            if (status instanceof Status) {
+                this._registeredStatuses.push(status);
+            } else {
+                this._registeredStatuses.push(new Status(status));
+            }
         }
     }
 


Test patch:
diff --git a/tests/StatusRegistry.test.ts b/tests/StatusRegistry.test.ts
index 2314aab7..6bc3bb80 100644
--- a/tests/StatusRegistry.test.ts
+++ b/tests/StatusRegistry.test.ts
@@ -10,6 +10,14 @@ jest.mock('obsidian');
 window.moment = moment;
 
 describe('StatusRegistry', () => {
+    beforeEach(() => {
+        StatusRegistry.getInstance().clearStatuses();
+    });
+
+    afterEach(() => {
+        StatusRegistry.getInstance().clearStatuses();
+    });
+
     it('should create a new instance and populate default status indicators', () => {
         // Arrange
 
@@ -52,6 +60,28 @@ describe('StatusRegistry', () => {
         expect(statusRegistry.getNextStatus(statusD).indicator).toEqual('a');
     });
 
+    it('should handle adding custom StatusConfiguration', () => {
+        // Arrange
+        const statusRegistry = StatusRegistry.getInstance();
+        const statusConfiguration = new StatusConfiguration('a', 'A', 'b', false);
+        statusRegistry.add(statusConfiguration);
+
+        // Assert
+        const status2 = statusRegistry.byIndicator('a');
+        expect(status2).toStrictEqual(new Status(statusConfiguration));
+    });
+
+    it('should not modify an added custom Status', () => {
+        // Arrange
+        const statusRegistry = StatusRegistry.getInstance();
+        const status = new Status(new StatusConfiguration('a', 'A', 'b', false));
+        statusRegistry.add(status);
+
+        // Assert
+        const status2 = statusRegistry.byIndicator('a');
+        expect(status2).toStrictEqual(status);
+    });
+
     it('should allow task to toggle through custom transitions', () => {
         // Arrange
         const statusRegistry = StatusRegistry.getInstance();
diff --git a/tests/Task.test.ts b/tests/Task.test.ts
index 94f90bf6..5524f406 100644
--- a/tests/Task.test.ts
+++ b/tests/Task.test.ts
@@ -27,7 +27,7 @@ describe('parsing', () => {
         expect(task).not.toBeNull();
         expect(task!.listMarker).toEqual('-');
         expect(task!.description).toEqual('this is a done task');
-        expect(task!.status.indicator).toStrictEqual(Status.DONE.indicator);
+        expect(task!.status).toStrictEqual(Status.DONE);
         expect(task!.dueDate).not.toBeNull();
         expect(task!.dueDate!.isSame(moment('2021-09-12', 'YYYY-MM-DD'))).toStrictEqual(true);
         expect(task!.doneDate).not.toBeNull();
@@ -63,7 +63,7 @@ describe('parsing', () => {
         expect(task).not.toBeNull();
         expect(task!.listMarker).toEqual('1.');
         expect(task!.description).toEqual('this is a done task');
-        expect(task!.status.indicator).toStrictEqual(Status.DONE.indicator);
+        expect(task!.status).toStrictEqual(Status.DONE);
         expect(task!.originalMarkdown).toStrictEqual(line);
     });
 
@@ -80,7 +80,7 @@ describe('parsing', () => {
         expect(task).not.toBeNull();
         expect(task!.listMarker).toEqual('909999.');
         expect(task!.description).toEqual('this is a todo task');
-        expect(task!.status.indicator).toStrictEqual(Status.TODO.indicator);
+        expect(task!.status).toStrictEqual(Status.TODO);
         expect(task!.originalMarkdown).toStrictEqual(line);
     });
 
@@ -124,7 +124,7 @@ describe('parsing', () => {
         // Assert
         expect(task).not.toBeNull();
         expect(task!.description).toEqual('this is a ✅ done task');
-        expect(task!.status.indicator).toStrictEqual(Status.DONE.indicator);
+        expect(task!.status).toStrictEqual(Status.DONE);
         expect(task!.dueDate).not.toBeNull();
         expect(task!.dueDate!.isSame(moment('2021-09-12', 'YYYY-MM-DD'))).toStrictEqual(true);
         expect(task!.doneDate).not.toBeNull();
@@ -143,7 +143,7 @@ describe('parsing', () => {
         // Assert
         expect(task).not.toBeNull();
         expect(task!.description).toEqual('this is a ✅ done task');
-        expect(task!.status.indicator).toStrictEqual(Status.DONE.indicator);
+        expect(task!.status).toStrictEqual(Status.DONE);
         expect(task!.dueDate).not.toBeNull();
         expect(task!.dueDate!.isSame(moment('2021-09-12', 'YYYY-MM-DD'))).toStrictEqual(true);
         expect(task!.doneDate).not.toBeNull();
@@ -473,7 +473,7 @@ describe('toggle done', () => {
 
         // Assert
         expect(toggled).not.toBeNull();
-        expect(toggled!.status.indicator).toStrictEqual(Status.DONE.indicator);
+        expect(toggled!.status).toStrictEqual(Status.DONE);
         expect(toggled!.doneDate).not.toBeNull();
         expect(toggled!.status.indicator).toStrictEqual('x');
         expect(toggled!.blockLink).toEqual(' ^my-precious');
@@ -493,7 +493,7 @@ describe('toggle done', () => {
 
         // Assert
         expect(toggled).not.toBeNull();
-        expect(toggled!.status.indicator).toStrictEqual(Status.TODO.indicator);
+        expect(toggled!.status).toStrictEqual(Status.TODO);
         expect(toggled!.status.indicator).toStrictEqual(' ');
         expect(toggled!.doneDate).toBeNull();
     });