Repository: obsidian-tasks-group/obsidian-tasks
Pull request #1606 (2023-01-29 19:27:57)

chore: Improve internal validation of theme status collections
<!--- Provide a general summary of your changes in the Title above -->

# Description

There is a certain amount of manual editing involved in creating the data that goes in to the data behind each of the 'Add all Status Types for ... Theme' buttons in Custom Status Settings.

I was checking it by eye, but now the checks are built in to the tests that generate the per-theme files that get embedded in to the documentation, so any inconsistencies will be found before release.

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

## How has this been tested?

- Adding more tests
- Running it on the un-released files for adding support for Ebullientworks theme. It found the errors very quickly, making the process much quicker.

These do not affect the validation that users see in the 'Edit status' modal - that is unchanged.

## 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:

- [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)
- [] **Infrastructure** (prefix: `chore` - examples include GitHub Actions, issue templates)
    - Sort of... It adds checks that only benefit the maintenance of the project.

## 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/Config/SettingsTab.ts b/src/Config/SettingsTab.ts
index 13953f98..f1f825ce 100644
--- a/src/Config/SettingsTab.ts
+++ b/src/Config/SettingsTab.ts
@@ -4,18 +4,13 @@ import type TasksPlugin from '../main';
 import { StatusRegistry } from '../StatusRegistry';
 import { Status } from '../Status';
 import type { StatusCollection } from '../StatusCollection';
+import * as Themes from './Themes';
 import type { HeadingState } from './Settings';
 import { getSettings, isFeatureEnabled, updateGeneralSetting, updateSettings } from './Settings';
 import { StatusSettings } from './StatusSettings';
 import settingsJson from './settingsConfiguration.json';
 
 import { CustomStatusModal } from './CustomStatusModal';
-import {
-    auraSupportedStatuses,
-    itsSupportedStatuses,
-    minimalSupportedStatuses,
-    thingsSupportedStatuses,
-} from './Themes';
 
 export class SettingsTab extends PluginSettingTab {
     // If the UI needs a more complex setting you can create a
@@ -432,10 +427,10 @@ export class SettingsTab extends PluginSettingTab {
         /* -------------------- Add all Status types supported by ... buttons -------------------- */
         type NamedTheme = [string, StatusCollection];
         const themes: NamedTheme[] = [
-            ['Minimal Theme', minimalSupportedStatuses()],
-            ['ITS Theme', itsSupportedStatuses()],
-            ['Things Theme', thingsSupportedStatuses()],
-            ['Aura Theme (Dark mode only)', auraSupportedStatuses()],
+            ['Minimal Theme', Themes.minimalSupportedStatuses()],
+            ['ITS Theme', Themes.itsSupportedStatuses()],
+            ['Things Theme', Themes.thingsSupportedStatuses()],
+            ['Aura Theme (Dark mode only)', Themes.auraSupportedStatuses()],
         ];
         for (const [name, collection] of themes) {
             const addStatusesSupportedByThisTheme = new Setting(containerEl).addButton((button) => {
diff --git a/src/StatusValidator.ts b/src/StatusValidator.ts
index b92a4377..0ad5f1e1 100644
--- a/src/StatusValidator.ts
+++ b/src/StatusValidator.ts
@@ -1,4 +1,8 @@
 import type { StatusConfiguration } from './StatusConfiguration';
+import { StatusType } from './StatusConfiguration';
+import type { StatusCollectionEntry } from './StatusCollection';
+import { Status } from './Status';
+import { StatusRegistry } from './StatusRegistry';
 
 export class StatusValidator {
     /**
@@ -15,6 +19,44 @@ export class StatusValidator {
         return errors;
     }
 
+    /**
+     * Validate data in StatusCollection lists. These are the descriptions of statuses in various themes,
+     * that are imported via one-click buttons in the Custom Status settings.
+     *
+     * This does a few checks to guard against human error when creating the lists, and then
+     * also calls {@link validate} too.
+     * @param entry
+     */
+    public validateStatusCollectionEntry(entry: StatusCollectionEntry) {
+        const [symbol, _name, nextStatusSymbol, typeAsString] = entry;
+
+        const errors: string[] = [];
+
+        // Checks that can only be done on the raw data.
+        // Status.createFromImportedValue() falls back to StatusType.TODO if the
+        // type string is not recognised, so we have to test that first.
+        errors.push(...this.validateType(typeAsString));
+
+        // For users, it is valid to have a status that toggles to itself.
+        // For imported data for themes, it seems worth preventing that situation,
+        // to guard against human error when setting up the status collections.
+        if (symbol === nextStatusSymbol) {
+            errors.push(`Status symbol '${symbol}' toggles to itself`);
+        }
+
+        // If the raw data was not valid, return now, to avoid potentially misleading
+        // errors from later checks.
+        if (errors.length > 0) {
+            return errors;
+        }
+
+        const configuration = Status.createFromImportedValue(entry).configuration;
+        errors.push(...this.validateSymbolTypeConventions(configuration));
+        errors.push(...this.validate(configuration));
+
+        return errors;
+    }
+
     public validateSymbol(statusConfiguration: StatusConfiguration): string[] {
         return StatusValidator.validateOneSymbol(statusConfiguration.symbol, 'Task Status Symbol');
     }
@@ -31,6 +73,41 @@ export class StatusValidator {
         return errors;
     }
 
+    public validateType(symbolName: string): string[] {
+        const statusTypeElement = StatusType[symbolName as keyof typeof StatusType];
+        const errors: string[] = [];
+        if (!statusTypeElement) {
+            errors.push(`Status Type "${symbolName}" is not a valid type`);
+        }
+        if (statusTypeElement == StatusType.EMPTY) {
+            errors.push('Status Type "EMPTY" is not permitted in user data');
+        }
+        return errors;
+    }
+
+    public validateSymbolTypeConventions(configuration: StatusConfiguration): string[] {
+        const errors: string[] = [];
+
+        const symbol = configuration.symbol;
+        const registry = new StatusRegistry();
+        const symbolToSearchFor = symbol === 'X' ? 'x' : symbol;
+        const defaultStatusFromRegistry = registry.bySymbol(symbolToSearchFor);
+        if (defaultStatusFromRegistry.type !== StatusType.EMPTY) {
+            if (configuration.nextStatusSymbol !== defaultStatusFromRegistry.nextStatusSymbol) {
+                errors.push(
+                    `Next Status Symbol for symbol '${symbol}': '${configuration.nextStatusSymbol}' is inconsistent with convention '${defaultStatusFromRegistry.nextStatusSymbol}'`,
+                );
+            }
+            if (configuration.type !== defaultStatusFromRegistry.type) {
+                errors.push(
+                    `Status Type for symbol '${symbol}': '${configuration.type}' is inconsistent with convention '${defaultStatusFromRegistry.type}'`,
+                );
+            }
+        }
+
+        return errors;
+    }
+
     private static validateOneSymbol(symbol: string, symbolName: string): string[] {
         const errors: string[] = [];
         if (symbol.length === 0) {


Test patch:
diff --git a/tests/DocsSamplesForStatuses.test.ts b/tests/DocsSamplesForStatuses.test.ts
index 2b83e837..ba90ddf5 100644
--- a/tests/DocsSamplesForStatuses.test.ts
+++ b/tests/DocsSamplesForStatuses.test.ts
@@ -10,9 +10,7 @@ import { Group } from '../src/Query/Group';
 import { StatusNameField } from '../src/Query/Filter/StatusNameField';
 import { StatusTypeField } from '../src/Query/Filter/StatusTypeField';
 import type { StatusCollection } from '../src/StatusCollection';
-import { auraSupportedStatuses, itsSupportedStatuses } from '../src/Config/Themes';
-import { minimalSupportedStatuses } from '../src/Config/Themes';
-import { thingsSupportedStatuses } from '../src/Config/Themes';
+import * as Themes from '../src/Config/Themes';
 import { TaskBuilder } from './TestingTools/TaskBuilder';
 
 function verifyMarkdown(markdown: string) {
@@ -171,10 +169,10 @@ describe('Theme', () => {
     type NamedTheme = [string, StatusCollection];
     const themes: NamedTheme[] = [
         // Alphabetical order by name:
-        ['Aura', auraSupportedStatuses()],
-        ['ITS', itsSupportedStatuses()],
-        ['Minimal', minimalSupportedStatuses()],
-        ['Things', thingsSupportedStatuses()],
+        ['Aura', Themes.auraSupportedStatuses()],
+        ['ITS', Themes.itsSupportedStatuses()],
+        ['Minimal', Themes.minimalSupportedStatuses()],
+        ['Things', Themes.thingsSupportedStatuses()],
     ];
 
     describe.each(themes)('%s', (_, statuses) => {
diff --git a/tests/StatusValidator.test.ts b/tests/StatusValidator.test.ts
index 5de98f83..32b80a54 100644
--- a/tests/StatusValidator.test.ts
+++ b/tests/StatusValidator.test.ts
@@ -1,5 +1,6 @@
-import { StatusConfiguration } from '../src/StatusConfiguration';
+import { StatusConfiguration, StatusType } from '../src/StatusConfiguration';
 import { StatusValidator } from '../src/StatusValidator';
+import type { StatusCollectionEntry } from '../src/StatusCollection';
 
 describe('StatusValidator', () => {
     const statusValidator = new StatusValidator();
@@ -9,7 +10,7 @@ describe('StatusValidator', () => {
         expect(errors).toEqual(expectedMessages);
     }
 
-    describe('validation', () => {
+    describe('validate StatusConfiguration', () => {
         it('should handle valid input correctly', () => {
             const config = new StatusConfiguration('X', 'Completed', ' ', false);
             checkValidation(config, []);
@@ -51,33 +52,139 @@ describe('StatusValidator', () => {
             const config = new StatusConfiguration('X', 'Completed', 'yyy', false);
             checkValidation(config, ['Task Next Status Symbol ("yyy") must be a single character.']);
         });
+    });
+
+    describe('validate StatusCollectionEntry', () => {
+        it('should produce no messages for valid entry', () => {
+            expect(statusValidator.validateStatusCollectionEntry(['x', 'Name', ' ', 'DONE'])).toStrictEqual([]);
+            expect(statusValidator.validateStatusCollectionEntry(['X', 'Name', ' ', 'DONE'])).toStrictEqual([]);
+        });
+
+        it('should validate type', () => {
+            const entry: StatusCollectionEntry = ['!', 'Name', ' ', 'Done'];
+            expect(statusValidator.validateStatusCollectionEntry(entry)).toStrictEqual([
+                'Status Type "Done" is not a valid type',
+            ]);
+        });
+
+        it('should recognise inconsistent symbol and next symbol', () => {
+            const entry: StatusCollectionEntry = ['-', 'cancelled', 'x', 'CANCELLED'];
+            expect(statusValidator.validateStatusCollectionEntry(entry)).toStrictEqual([
+                "Next Status Symbol for symbol '-': 'x' is inconsistent with convention ' '",
+            ]);
+        });
+
+        it('should recognise inconsistent symbol and type', () => {
+            const entry: StatusCollectionEntry = ['x', 'Done', ' ', 'TODO'];
+            expect(statusValidator.validateStatusCollectionEntry(entry)).toStrictEqual([
+                "Status Type for symbol 'x': 'TODO' is inconsistent with convention 'DONE'",
+            ]);
+        });
+
+        it('should recognise symbol toggling to itself', () => {
+            const entry: StatusCollectionEntry = ['!', 'Name', '!', 'TODO'];
+            expect(statusValidator.validateStatusCollectionEntry(entry)).toStrictEqual([
+                "Status symbol '!' toggles to itself",
+            ]);
+        });
+
+        it('should recognise an error in created StatusConfiguration', () => {
+            const entry: StatusCollectionEntry = ['!', 'Name', 'cc', 'DONE'];
+            expect(statusValidator.validateStatusCollectionEntry(entry)).toStrictEqual([
+                'Task Next Status Symbol ("cc") must be a single character.',
+            ]);
+        });
+    });
+
+    describe('validate symbol', () => {
+        it('valid symbol', () => {
+            const config = new StatusConfiguration('X', 'Completed', 'c', false);
+            expect(statusValidator.validateSymbol(config)).toStrictEqual([]);
+        });
+
+        it('invalid symbol', () => {
+            const config = new StatusConfiguration('XYZ', 'Completed', 'c', false);
+            expect(statusValidator.validateSymbol(config)).toStrictEqual([
+                'Task Status Symbol ("XYZ") must be a single character.',
+            ]);
+        });
+    });
+
+    describe('validate next symbol', () => {
+        it('valid symbol', () => {
+            const config = new StatusConfiguration('c', 'Completed', 'X', false);
+            expect(statusValidator.validateNextSymbol(config)).toStrictEqual([]);
+        });
+
+        it('invalid next symbol', () => {
+            const config = new StatusConfiguration('c', 'Completed', 'XYZ', false);
+            expect(statusValidator.validateNextSymbol(config)).toStrictEqual([
+                'Task Next Status Symbol ("XYZ") must be a single character.',
+            ]);
+        });
+    });
+
+    describe('validate type as raw string', () => {
+        it('valid symbol', () => {
+            expect(statusValidator.validateType('TODO')).toStrictEqual([]);
+            expect(statusValidator.validateType('IN_PROGRESS')).toStrictEqual([]);
+        });
+
+        it('invalidate type as raw string', () => {
+            expect(statusValidator.validateType('in_progress')).toStrictEqual([
+                'Status Type "in_progress" is not a valid type',
+            ]);
+            expect(statusValidator.validateType('CANCELED')).toStrictEqual([
+                'Status Type "CANCELED" is not a valid type',
+            ]);
+        });
+
+        it('should forbid type EMPTY', () => {
+            expect(statusValidator.validateType('EMPTY')).toStrictEqual([
+                'Status Type "EMPTY" is not permitted in user data',
+            ]);
+        });
+    });
+
+    describe('validate symbol/type consistency for convention', () => {
+        it('matches convention', () => {
+            expect(
+                statusValidator.validateSymbolTypeConventions(
+                    new StatusConfiguration(' ', 'Any old name', 'x', false, StatusType.TODO),
+                ),
+            ).toEqual([]);
 
-        describe('validate symbol', () => {
-            it('valid symbol', () => {
-                const config = new StatusConfiguration('X', 'Completed', 'c', false);
-                expect(statusValidator.validateSymbol(config)).toStrictEqual([]);
-            });
-
-            it('invalid symbol', () => {
-                const config = new StatusConfiguration('XYZ', 'Completed', 'c', false);
-                expect(statusValidator.validateSymbol(config)).toStrictEqual([
-                    'Task Status Symbol ("XYZ") must be a single character.',
-                ]);
-            });
-        });
-
-        describe('validate next symbol', () => {
-            it('valid symbol', () => {
-                const config = new StatusConfiguration('c', 'Completed', 'X', false);
-                expect(statusValidator.validateNextSymbol(config)).toStrictEqual([]);
-            });
-
-            it('invalid next symbol', () => {
-                const config = new StatusConfiguration('c', 'Completed', 'XYZ', false);
-                expect(statusValidator.validateNextSymbol(config)).toStrictEqual([
-                    'Task Next Status Symbol ("XYZ") must be a single character.',
-                ]);
-            });
+            expect(
+                statusValidator.validateSymbolTypeConventions(
+                    new StatusConfiguration('X', 'Any old name', ' ', false, StatusType.DONE),
+                ),
+            ).toEqual([]);
+        });
+
+        it('does not match convention', () => {
+            expect(
+                statusValidator.validateSymbolTypeConventions(
+                    new StatusConfiguration(' ', 'Any old name', '!', false, StatusType.IN_PROGRESS),
+                ),
+            ).toEqual([
+                "Next Status Symbol for symbol ' ': '!' is inconsistent with convention 'x'",
+                "Status Type for symbol ' ': 'IN_PROGRESS' is inconsistent with convention 'TODO'",
+            ]);
+
+            expect(
+                statusValidator.validateSymbolTypeConventions(
+                    new StatusConfiguration(' ', 'Any old name', 'x', false, StatusType.NON_TASK),
+                ),
+            ).toEqual(["Status Type for symbol ' ': 'NON_TASK' is inconsistent with convention 'TODO'"]);
+
+            expect(
+                statusValidator.validateSymbolTypeConventions(
+                    new StatusConfiguration('X', 'Name', '-', false, StatusType.TODO),
+                ),
+            ).toEqual([
+                "Next Status Symbol for symbol 'X': '-' is inconsistent with convention ' '",
+                "Status Type for symbol 'X': 'TODO' is inconsistent with convention 'DONE'",
+            ]);
         });
     });
 });