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

Issue:
Fix the existing validation facility when editing custom statuses
### The Problem

As of 5c5d28850045d38dca8a7fe90b9ab7c175077bdf 9c8bfe1fa7ea2dc05795fdd9d7198a60dbf8d248, the modal for editing custom statuses looks good:


![Image](https://user-images.githubusercontent.com/4840096/211218703-793541bb-86fa-44bb-8ac3-ab3d2fcf4df0.png)

There is some validation code in it that I do not yet understand - it's pulled directly from obsidian-tasks-x. It works by changing CSS classes, which is an area I am not very familiar with.

An example of the problem behaviour is that if I delete the status character, the display changes to this and it is no longer possible to edit the status character. So the modal has to be closed and then re-opened.


![Image](https://user-images.githubusercontent.com/4840096/211218783-99158d50-5785-47e6-b255-7f69182887c5.png)

I observed the same in the original code - https://github.com/sytone/obsidian-tasks-x/commit/2c0b659457cc80806ff18585c955496c76861b87 - so it seems not to have been a porting problem.


### The desired behaviour

The intention of the code is to prevent the user from saving the edit values if there are any invalid fields.

The existing checks are:

https://github.com/obsidian-tasks-group/obsidian-tasks/blob/4721d4d468e5e1ecc35c1c2c9ccbc86fc1dd8667/src/Config/CustomStatusModal.ts#L46-L63

This issue is to fix the existing checks, so that:

- if the value in the Task Status Symbol field is invalid, the error message is displayed
- and the user is able to correct the value
- and then save the new value

I will later log a separate issue for add some missing checks.



Pull request description:
Fix edit status error validation
<!--- Provide a general summary of your changes in the Title above -->

# Description

Essential fix for first release of custom statuses feature.

Fixes problem that if the status symbol was invalid, the modal removed the status symbol text field.
I was unable to prevent this, so now, colouring is used to indicate if there is an error.

Summary of changes:

- `StatusConfiguration` class now provides methods for validating individual fields and the whole object
- `CustomStatusModal` now:
    - uses the above validating code to highlight any incorrect values in red as the user edits the status
    - and when user clicks the checkbox, any errors are displayed in a notice.

Thanks to @Fevol for help with the red colours to show errors.

## Motivation and Context

Fixes #1498

## How has this been tested?

- Unit tests added for the actual validation messages for the status
- Extensive manual testing of the behaviour of the edit dialog

## Screenshots (if appropriate)

![image](https://user-images.githubusercontent.com/4840096/212378235-dc3f1093-0bac-4600-b2fb-23bec7ca7d5a.png)

![image](https://user-images.githubusercontent.com/4840096/212378267-cd06e563-744e-4826-a7e9-4a81105a4901.png)


## 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 ] **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`.
- [x] 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/CustomStatusModal.ts b/src/Config/CustomStatusModal.ts
index 109831db..e4a3e2cd 100644
--- a/src/Config/CustomStatusModal.ts
+++ b/src/Config/CustomStatusModal.ts
@@ -1,4 +1,4 @@
-import { Modal, Setting, TextComponent } from 'obsidian';
+import { Modal, Notice, Setting, TextComponent } from 'obsidian';
 import { Status, StatusConfiguration } from '../Status';
 import type TasksPlugin from '../main';
 
@@ -43,42 +43,44 @@ export class CustomStatusModal extends Modal {
             .setDesc('This is the character between the square braces.')
             .addText((text) => {
                 statusSymbolText = text;
-                statusSymbolText.setValue(this.statusSymbol).onChange((v) => {
-                    if (!v.length) {
-                        CustomStatusModal.setValidationError(text, 'Task status type cannot be empty.');
-                        return;
-                    }
-
-                    if (v.includes(' ')) {
-                        CustomStatusModal.setValidationError(text, 'Task status type cannot include spaces.');
-                        return;
-                    }
-
-                    if (v.length > 1) {
-                        CustomStatusModal.setValidationError(text, 'Task status must be a single character.');
-                        return;
-                    }
-                    CustomStatusModal.removeValidationError(text);
+                text.setValue(this.statusSymbol).onChange((v) => {
                     this.statusSymbol = v;
+                    CustomStatusModal.setValid(text, this.statusConfiguration().validateIndicator());
                 });
+            })
+            .then((_setting) => {
+                // Show any error if the initial value loaded is incorrect.
+                CustomStatusModal.setValid(statusSymbolText, this.statusConfiguration().validateIndicator());
             });
 
+        let statusNameText: TextComponent;
         new Setting(settingDiv)
             .setName('Task Status Name')
             .setDesc('This is the friendly name of the task status.')
             .addText((text) => {
+                statusNameText = text;
                 text.setValue(this.statusName).onChange((v) => {
                     this.statusName = v;
+                    CustomStatusModal.setValid(text, this.statusConfiguration().validateName());
                 });
+            })
+            .then((_setting) => {
+                CustomStatusModal.setValid(statusNameText, this.statusConfiguration().validateName());
             });
 
+        let statusNextSymbolText: TextComponent;
         new Setting(settingDiv)
             .setName('Task Next Status Symbol')
             .setDesc('When clicked on this is the symbol that should be used next.')
             .addText((text) => {
+                statusNextSymbolText = text;
                 text.setValue(this.statusNextSymbol).onChange((v) => {
                     this.statusNextSymbol = v;
+                    CustomStatusModal.setValid(text, this.statusConfiguration().validateNextIndicator());
                 });
+            })
+            .then((_setting) => {
+                CustomStatusModal.setValid(statusNextSymbolText, this.statusConfiguration().validateNextIndicator());
             });
 
         if (Status.tasksPluginCanCreateCommandsForStatuses()) {
@@ -100,17 +102,13 @@ export class CustomStatusModal extends Modal {
             b.setTooltip('Save')
                 .setIcon('checkmark')
                 .onClick(async () => {
-                    // let error = false;
-                    // if (!this.statusSymbol.length) {
-                    //     SettingsModal.setValidationError(this.statusSymbol, 'Task status type cannot be empty.');
-                    //     error = true;
-                    //     return;
-                    // }
-
-                    // if (error) {
-                    //     new Notice('Fix errors before saving.');
-                    //     return;
-                    // }
+                    const errors = this.statusConfiguration().validate();
+                    if (errors.length > 0) {
+                        const message = errors.join('\n') + '\n\n' + 'Fix errors before saving.';
+                        // console.debug(message);
+                        new Notice(message);
+                        return;
+                    }
                     this.saved = true;
                     this.close();
                 });
@@ -137,28 +135,20 @@ export class CustomStatusModal extends Modal {
         this.display();
     }
 
-    static setValidationError(textInput: TextComponent, message?: string) {
-        textInput.inputEl.addClass('is-invalid');
-        if (message) {
-            textInput.inputEl.parentElement?.addClasses(['has-invalid-message', 'unset-align-items']);
-            textInput.inputEl.parentElement?.parentElement?.addClass('.unset-align-items');
-            let mDiv = textInput.inputEl.parentElement?.querySelector('.invalid-feedback') as HTMLDivElement;
-
-            if (!mDiv) {
-                mDiv = createDiv({ cls: 'invalid-feedback' });
-            }
-            mDiv.innerText = message;
-            mDiv.insertAfter(textInput.inputEl);
-        }
+    static setValidationError(textInput: TextComponent) {
+        textInput.inputEl.addClass('tasks-settings-is-invalid');
     }
+
     static removeValidationError(textInput: TextComponent) {
-        textInput.inputEl.removeClass('is-invalid');
-        textInput.inputEl.parentElement?.removeClasses(['has-invalid-message', 'unset-align-items']);
-        textInput.inputEl.parentElement?.parentElement?.removeClass('.unset-align-items');
+        textInput.inputEl.removeClass('tasks-settings-is-invalid');
+    }
 
-        const invalidFeedback = textInput.inputEl.parentElement?.querySelector('.invalid-feedback');
-        if (invalidFeedback) {
-            textInput.inputEl.parentElement?.removeChild(invalidFeedback);
+    private static setValid(text: TextComponent, messages: string[]) {
+        const valid = messages.length === 0;
+        if (valid) {
+            CustomStatusModal.removeValidationError(text);
+        } else {
+            CustomStatusModal.setValidationError(text);
         }
     }
 }
diff --git a/src/Status.ts b/src/Status.ts
index 88911d56..18f6e62b 100644
--- a/src/Status.ts
+++ b/src/Status.ts
@@ -54,6 +54,48 @@ export class StatusConfiguration {
         this.nextStatusIndicator = nextStatusIndicator;
         this.availableAsCommand = availableAsCommand;
     }
+
+    /**
+     * Determine whether the date in this object is valid, and return error message(s) for display if not.
+     */
+    public validate(): string[] {
+        const errors: string[] = [];
+
+        // Messages are added in the order fields are shown when editing statuses.
+        errors.push(...this.validateIndicator());
+        errors.push(...this.validateName());
+        errors.push(...this.validateNextIndicator());
+
+        return errors;
+    }
+
+    public validateIndicator(): string[] {
+        return StatusConfiguration.validateOneIndicator(this.indicator, 'Task Status Symbol');
+    }
+
+    public validateNextIndicator(): string[] {
+        return StatusConfiguration.validateOneIndicator(this.nextStatusIndicator, 'Task Next Status Symbol');
+    }
+
+    public validateName() {
+        const errors: string[] = [];
+        if (this.name.length === 0) {
+            errors.push('Task Status Name cannot be empty.');
+        }
+        return errors;
+    }
+
+    private static validateOneIndicator(indicator: string, indicatorName: string): string[] {
+        const errors: string[] = [];
+        if (indicator.length === 0) {
+            errors.push(`${indicatorName} cannot be empty.`);
+        }
+
+        if (indicator.length > 1) {
+            errors.push(`${indicatorName} ("${indicator}") must be a single character.`);
+        }
+        return errors;
+    }
 }
 
 /**
diff --git a/styles.css b/styles.css
index 4808f765..996d3fd5 100644
--- a/styles.css
+++ b/styles.css
@@ -211,6 +211,12 @@
  **                            SETTINGS
  *------------------------------------------------------------------------**/
 
+.tasks-settings-is-invalid {
+    /* Dark red text on pale background*/
+    color: var(--text-error) !important;
+    background-color: rgba(var(--background-modifier-error-rgb), 0.2) !important;
+}
+
 
  .tasks-settings .additional {
     margin: 6px 12px;


Test patch:
diff --git a/tests/Status.test.ts b/tests/Status.test.ts
index 7c993411..74f5a92f 100644
--- a/tests/Status.test.ts
+++ b/tests/Status.test.ts
@@ -7,6 +7,85 @@ import { Status, StatusConfiguration } from '../src/Status';
 jest.mock('obsidian');
 window.moment = moment;
 
+describe('StatusConfiguration', () => {
+    function checkValidation(statusConfiguration: StatusConfiguration, expectedMessages: string[]) {
+        const errors = statusConfiguration.validate();
+        expect(errors).toEqual(expectedMessages);
+    }
+
+    describe('validation', () => {
+        it('should handle valid input correctly', () => {
+            const config = new StatusConfiguration('X', 'Completed', ' ', false);
+            checkValidation(config, []);
+        });
+
+        it('should handle totally invalid input correctly', () => {
+            const config = new StatusConfiguration('Xxx', '', '', false);
+            checkValidation(config, [
+                'Task Status Symbol ("Xxx") must be a single character.',
+                'Task Status Name cannot be empty.',
+                'Task Next Status Symbol cannot be empty.',
+            ]);
+        });
+
+        // Check status name
+        it('should detect empty name', () => {
+            const config = new StatusConfiguration('X', '', ' ', false);
+            checkValidation(config, ['Task Status Name cannot be empty.']);
+        });
+
+        // Check Symbol
+        it('should detect empty symbol', () => {
+            const config = new StatusConfiguration('', 'Completed', ' ', false);
+            checkValidation(config, ['Task Status Symbol cannot be empty.']);
+        });
+
+        it('should detect too-long symbol', () => {
+            const config = new StatusConfiguration('yyy', 'Completed', ' ', false);
+            checkValidation(config, ['Task Status Symbol ("yyy") must be a single character.']);
+        });
+
+        // Check Next symbol
+        it('should detect next empty symbol', () => {
+            const config = new StatusConfiguration('X', 'Completed', '', false);
+            checkValidation(config, ['Task Next Status Symbol cannot be empty.']);
+        });
+
+        it('should detect too-long next symbol', () => {
+            const config = new StatusConfiguration('X', 'Completed', 'yyy', false);
+            checkValidation(config, ['Task Next Status Symbol ("yyy") must be a single character.']);
+        });
+
+        describe('validate indicator', () => {
+            it('valid indicator', () => {
+                const config = new StatusConfiguration('X', 'Completed', 'c', false);
+                expect(config.validateIndicator()).toStrictEqual([]);
+            });
+
+            it('invalid indicator', () => {
+                const config = new StatusConfiguration('XYZ', 'Completed', 'c', false);
+                expect(config.validateIndicator()).toStrictEqual([
+                    'Task Status Symbol ("XYZ") must be a single character.',
+                ]);
+            });
+        });
+
+        describe('validate next indicator', () => {
+            it('valid indicator', () => {
+                const config = new StatusConfiguration('c', 'Completed', 'X', false);
+                expect(config.validateNextIndicator()).toStrictEqual([]);
+            });
+
+            it('invalid next indicator', () => {
+                const config = new StatusConfiguration('c', 'Completed', 'XYZ', false);
+                expect(config.validateNextIndicator()).toStrictEqual([
+                    'Task Next Status Symbol ("XYZ") must be a single character.',
+                ]);
+            });
+        });
+    });
+});
+
 describe('Status', () => {
     it('should initialize with valid properties', () => {
         // Arrange