Repository: obsidian-tasks-group/obsidian-tasks
Pull request #970 (2022-08-02 22:24:50)

feat: add feature toggle framework
# Description

Adds in the feature framework so it can be used to manage new features for user testing but not impact all users by default by having a new feature on by default.

## Motivation and Context
New changes have a higher chance of impact until initial issues are resolved, being able to deploy with them off by default allows changes to get out faster to users to test and use. 

## How has this been tested?

Used heavily in fork with no issues and added basic unit tests. 

## Screenshots (if appropriate)

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Refactor (non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

## 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 accordingly.
- [x] My change has adequate Unit Test coverage.

By creating a Pull Request you agree to our [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md). For further guidance on contributing please see [contributing guide](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CONTRIBUTING.md)


claremacrae: Hey, @sytone

I was thinking about this over night...  If you haven't started the renaming of symbols - and the suggested/requested would cause a problem for you in tasks-x - then feel free to instead improve the docs for the various - to clearly state their purpose and use - so that at least anyone hovering over the symbols in a good IDE will immediately know what they do.
sytone: When I make changes to variables and parameters on this side I backport to my repo so they stay in sync, renames are not usually a issue. Search and replace is your friend.

Been away for a while and will hopefully get time on the weekend to catch up.
claremacrae: > When I make changes to variables and parameters on this side I backport to my repo so they stay in sync, renames are not usually a issue. 

Thanks.

> Search and replace is your friend.

Erm. I can't tell if you're joking! 😀 

Maybe 95% of the time, the rest of the time it's a great way to introduce bugs!

I would always recommend using an IDE that has proper rename refactoring.

> Been away for a while and will hopefully get time on the weekend to catch up.

Awesome. Many thanks.

Patch:
diff --git a/src/Commands/ToggleDone.ts b/src/Commands/ToggleDone.ts
index bc14ef8b..acbd4907 100644
--- a/src/Commands/ToggleDone.ts
+++ b/src/Commands/ToggleDone.ts
@@ -51,7 +51,7 @@ export const toggleDone = (checking: boolean, editor: Editor, view: View) => {
     });
 };
 
-const toggleLine = (line: string, path: string) => {
+export const toggleLine = (line: string, path: string) => {
     let toggledLine = line;
 
     const task = Task.fromLine({
@@ -113,7 +113,7 @@ const toggleTask = (task: Task): string => {
 
 So cursor should be reset if 0, which includes being moved to new end if got shorter. Then might need to move right 2 or 3.
 */
-const calculateCursorOffset = (
+export const calculateCursorOffset = (
     origCursorCh: number,
     line: string,
     toggledLine: string,
diff --git a/src/config/Feature.ts b/src/config/Feature.ts
new file mode 100644
index 00000000..739e16bf
--- /dev/null
+++ b/src/config/Feature.ts
@@ -0,0 +1,92 @@
+import featuresJson from './featureConfiguration.json';
+
+export type FeatureFlag = {
+    [internalName: string]: boolean;
+};
+
+/**
+ * The Feature class tracks all the possible features that users can enabled that are in development. This allows
+ * new features to be added to the platform but not enabled by default. This reduces the complications when it
+ * comes to adding new features and a large cascade of dependent branches.
+ *
+ * When you add a new feature you need to add it to the featureConfiguration.json file. It then needs to be added to
+ * settings so a user can enable it. If you want it hidden you will need to manually update the data.json file. In the
+ * plugin folder.
+ *
+ * @since 2022-05-29
+ */
+export class Feature {
+    private constructor(
+        public readonly internalName: string,
+        public readonly index: number,
+        public readonly description: string,
+        public readonly displayName: string,
+        public readonly enabledByDefault: boolean,
+        public readonly stable: boolean,
+    ) {}
+
+    /**
+     * Returns the list of all available features.
+     *
+     * @readonly
+     * @static
+     * @type {Feature[]}
+     * @memberof Feature
+     */
+    static get values(): Feature[] {
+        let availableFeatures: Feature[] = [];
+
+        featuresJson.forEach((feature) => {
+            availableFeatures = [
+                ...availableFeatures,
+                new Feature(
+                    feature.internalName,
+                    feature.index,
+                    feature.description,
+                    feature.displayName,
+                    feature.enabledByDefault,
+                    feature.stable,
+                ),
+            ];
+        });
+        return availableFeatures;
+    }
+
+    /**
+     * Returns the enabled state of the feature.
+     *
+     * @readonly
+     * @static
+     * @type {FeatureFlag}
+     * @memberof Feature
+     */
+    static get settingsFlags(): FeatureFlag {
+        const featureFlags: { [internalName: string]: boolean } = {};
+
+        Feature.values.forEach((feature) => {
+            featureFlags[feature.internalName] = feature.enabledByDefault;
+        });
+        return featureFlags;
+    }
+
+    /**
+     * Converts a name to its corresponding default Feature instance.
+     *
+     * @param name the name to convert to Feature
+     * @throws RangeError, if a string that has no corresponding Feature value was passed.
+     * @returns the matching Feature
+     */
+    static fromString(name: string): Feature {
+        for (const feature of Feature.values) {
+            if (name === feature.internalName) {
+                return feature;
+            }
+        }
+
+        throw new RangeError(
+            `Illegal argument passed to fromString(): ${name} does not correspond to any available Feature ${
+                (this as any).prototype.constructor.name
+            }`,
+        );
+    }
+}
diff --git a/src/config/Settings.ts b/src/config/Settings.ts
index 95aa4155..244d4109 100644
--- a/src/config/Settings.ts
+++ b/src/config/Settings.ts
@@ -1,3 +1,6 @@
+import { Feature } from './Feature';
+import type { FeatureFlag } from './Feature';
+
 export interface Settings {
     globalFilter: string;
     removeGlobalFilter: boolean;
@@ -5,6 +8,9 @@ export interface Settings {
     autoSuggestInEditor: boolean;
     autoSuggestMinMatch: number;
     autoSuggestMaxItems: number;
+
+    // Collection of feature flag IDs and their state.
+    features: FeatureFlag;
 }
 
 const defaultSettings: Settings = {
@@ -14,11 +20,27 @@ const defaultSettings: Settings = {
     autoSuggestInEditor: true,
     autoSuggestMinMatch: 0,
     autoSuggestMaxItems: 6,
+    features: Feature.settingsFlags,
 };
 
 let settings: Settings = { ...defaultSettings };
 
+/**
+ * Returns the current settings as a object, it will also check and
+ * update the flags to make sure they are all shown in the data.json
+ * file. Exposure via the settings UI is optional.
+ *
+ * @export
+ * @returns true if the feature is enabled.
+ */
 export const getSettings = (): Settings => {
+    // Check to see if there is a new flag and if so add it to the users settings.
+    for (const flag in Feature.settingsFlags) {
+        if (settings.features[flag] === undefined) {
+            settings.features[flag] = Feature.settingsFlags[flag];
+        }
+    }
+
     return { ...settings };
 };
 
@@ -31,3 +53,30 @@ export const updateSettings = (newSettings: Partial<Settings>): Settings => {
 export const resetSettings = (): Settings => {
     return updateSettings(defaultSettings);
 };
+
+/**
+ * Returns the enabled state of the feature from settings.
+ *
+ * @export
+ * @param internalName the internal name of the feature.
+ * @returns true if the feature is enabled.
+ */
+export const isFeatureEnabled = (internalName: string): boolean => {
+    return settings.features[internalName] ?? false;
+};
+
+/**
+ * enables toggling the feature and returning the current collection with state.
+ *
+ * @export
+ * @param internalName the internal name of the feature.
+ * @param enabled the expected state of the feature.
+ * @returns the features with the specified feature toggled.
+ */
+export const toggleFeature = (
+    internalName: string,
+    enabled: boolean,
+): FeatureFlag => {
+    settings.features[internalName] = enabled;
+    return settings.features;
+};
diff --git a/src/config/featureConfiguration.json b/src/config/featureConfiguration.json
new file mode 100644
index 00000000..f8a06d56
--- /dev/null
+++ b/src/config/featureConfiguration.json
@@ -0,0 +1,10 @@
+[
+  {
+    "index": 9999,
+    "internalName": "INTERNAL_TESTING_ENABLED_BY_DEFAULT",
+    "displayName": "Test Item. Used to validate the Feature Framework.",
+    "description": "Description",
+    "enabledByDefault": true,
+    "stable": false
+  }
+]
diff --git a/tsconfig.json b/tsconfig.json
index 68a1c866..13cab6c1 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -3,6 +3,7 @@
   "compilerOptions": {
     "allowSyntheticDefaultImports": true,
     "baseUrl": "./src",
+    "resolveJsonModule": true,
     "esModuleInterop": true,
     "inlineSources": true,
     "sourceMap": true,
@@ -16,7 +17,10 @@
     "moduleResolution": "node",
     "importHelpers": true,
     "paths": {
-      "src": ["src/*", "tests/*"]
+      "src": [
+        "src/*",
+        "tests/*"
+      ]
     },
     "types": [
       "jest",
@@ -35,6 +39,6 @@
     "tests/**/*"
   ],
   "exclude": [
-      "node_modules/*"
+    "node_modules/*"
   ]
-}
+}
\ No newline at end of file


Test patch:
diff --git a/tests/Commands/ToggleDone.test.ts b/tests/Commands/ToggleDone.test.ts
new file mode 100644
index 00000000..feb67a34
--- /dev/null
+++ b/tests/Commands/ToggleDone.test.ts
@@ -0,0 +1,131 @@
+/**
+ * @jest-environment jsdom
+ */
+
+import moment from 'moment';
+import {
+    calculateCursorOffset,
+    toggleLine,
+} from '../../src/Commands/ToggleDone';
+
+window.moment = moment;
+
+/**
+ * Test that toggling the task on the given input line generates the correct
+ * text content, and also gives the correct expected new cursor position.
+ *
+ * Cursor positions are indicated by a | (pipe) character for easy visualisation
+ * of the cursor position.
+ *
+ * @param inputWithCursorMark
+ * @param expectedWithCursorMark
+ */
+function testToggleLine(
+    inputWithCursorMark: string,
+    expectedWithCursorMark: string,
+) {
+    const cursorMarker = '|';
+    const cursorMarkerRegex = /\|/g;
+
+    const input = inputWithCursorMark.replace(cursorMarkerRegex, '');
+    const expected = expectedWithCursorMark.replace(cursorMarkerRegex, '');
+
+    // Check that the cursor marker appears exactly once in each input string:
+    expect(input.length).toEqual(inputWithCursorMark.length - 1);
+    expect(expected.length).toEqual(expectedWithCursorMark.length - 1);
+
+    testToggleLineForOutOfRangeCursorPositions(
+        input,
+        inputWithCursorMark.indexOf(cursorMarker),
+        expected,
+        expectedWithCursorMark.indexOf(cursorMarker),
+    );
+}
+
+/**
+ * Test that toggling the task on the given input line generates the correct
+ * text content, and also gives the correct expected new cursor position.
+ *
+ * Cursor positions are given by indices, where 0 is the start of the line.
+ *
+ * Call this version to represent a cursor position outside of the allowed
+ * range of character positions in the input or expected strings.
+ *
+ * @param input
+ * @param initialCursorOffset
+ * @param expected
+ * @param expectedCursorOffset
+ */
+function testToggleLineForOutOfRangeCursorPositions(
+    input: string,
+    initialCursorOffset: number,
+    expected: string,
+    expectedCursorOffset: number,
+) {
+    const result = toggleLine(input, 'x.md');
+    expect(result).toStrictEqual(expected);
+    const actualCursorOffset = calculateCursorOffset(
+        initialCursorOffset,
+        input,
+        result,
+    );
+    expect(actualCursorOffset).toEqual(expectedCursorOffset);
+}
+
+describe('ToggleDone', () => {
+    const todaySpy = jest
+        .spyOn(Date, 'now')
+        .mockReturnValue(moment('2022-09-04').valueOf());
+
+    // The | (pipe) indicates the calculated position where the cursor should be displayed.
+    // Note that prior to the #1103 fix, this position was sometimes ignored.
+
+    it('should add hyphen and space to empty line', () => {
+        testToggleLine('|', '- |');
+    });
+
+    it('should add checkbox to hyphen and space', () => {
+        testToggleLine('|- ', '- [ |] ');
+        testToggleLine('- |', '- [ ] |');
+    });
+
+    it('should complete a task', () => {
+        testToggleLine('|- [ ] ', '|- [x]  ✅ 2022-09-04');
+        testToggleLine('- [ ] |', '- [x] | ✅ 2022-09-04');
+
+        // Issue #449 - cursor jumped 13 characters to the right on completion
+        testToggleLine(
+            '- [ ] I have a |proper description',
+            '- [x] I have a |proper description ✅ 2022-09-04',
+        );
+    });
+
+    it('should un-complete a completed task', () => {
+        testToggleLine('|- [x]  ✅ 2022-09-04', '|- [ ] ');
+        testToggleLine('- [x]  ✅ 2022-09-04|', '- [ ] |');
+
+        // Issue #449 - cursor jumped 13 characters to the left on un-completion
+        testToggleLine(
+            '- [x] I have a proper description| ✅ 2022-09-04',
+            '- [ ] I have a proper description|',
+        );
+    });
+
+    it('should complete a recurring task', () => {
+        testToggleLine(
+            '- [ ] I am a recurring task| 🔁 every day 📅 2022-09-04',
+            `- [ ] I am a recurring task 🔁 every day 📅 2022-09-05
+- [x] I am a recurring task| 🔁 every day 📅 2022-09-04 ✅ 2022-09-04`,
+        );
+
+        // With a trailing space at the end of the initial line, which is deleted
+        // when the task lines are regenerated, the cursor moves one character to the left:
+        testToggleLine(
+            '- [ ] I am a recurring task| 🔁 every day 📅 2022-09-04 ',
+            `- [ ] I am a recurring task 🔁 every day 📅 2022-09-05
+- [x] I am a recurring tas|k 🔁 every day 📅 2022-09-04 ✅ 2022-09-04`,
+        );
+    });
+
+    todaySpy.mockClear();
+});
diff --git a/tests/config/Feature.test.ts b/tests/config/Feature.test.ts
new file mode 100644
index 00000000..002eab5c
--- /dev/null
+++ b/tests/config/Feature.test.ts
@@ -0,0 +1,49 @@
+/**
+ * @jest-environment jsdom
+ */
+import { Feature } from '../../src/config/Feature';
+
+describe('feature-usage', () => {
+    it('load current features', () => {
+        const currentFeatures = Feature.values;
+
+        expect(currentFeatures.length).toBeGreaterThan(0);
+    });
+
+    it('load current feature states', () => {
+        for (const flag in Feature.settingsFlags) {
+            expect(Feature.settingsFlags[flag]).not.toBeNull();
+        }
+    });
+
+    it('load and validate enabled by default', () => {
+        const name = 'INTERNAL_TESTING_ENABLED_BY_DEFAULT';
+
+        expect(Feature.settingsFlags[name]).toBe(true);
+        expect(Feature.fromString(name).enabledByDefault).toBe(true);
+    });
+
+    it('load and validate feature properties', () => {
+        const name = 'INTERNAL_TESTING_ENABLED_BY_DEFAULT';
+        const feature = Feature.fromString(name);
+
+        expect(feature.internalName).toBe(name);
+        expect(feature.index).toBe(9999);
+        expect(feature.description).toBe('Description');
+        expect(feature.displayName).toBe(
+            'Test Item. Used to validate the Feature Framework.',
+        );
+        expect(feature.enabledByDefault).toBe(true);
+        expect(feature.stable).toBe(false);
+    });
+
+    it('load and access invalid feature name', () => {
+        function getFeature() {
+            const name = 'INVALID_FEATURE_NAME';
+            Feature.fromString(name);
+        }
+
+        expect(getFeature).toThrowError('Illegal argument passed');
+        expect(getFeature).toThrowError(RangeError);
+    });
+});
diff --git a/tests/config/Settings.test.ts b/tests/config/Settings.test.ts
new file mode 100644
index 00000000..f8efc6e6
--- /dev/null
+++ b/tests/config/Settings.test.ts
@@ -0,0 +1,44 @@
+/**
+ * @jest-environment jsdom
+ */
+import {
+    getSettings,
+    isFeatureEnabled,
+    toggleFeature,
+} from '../../src/config/Settings';
+
+describe('settings-usage', () => {
+    it('load default settings and validate features', () => {
+        const currentSettings = getSettings();
+
+        expect(Object.entries(currentSettings.features).length).toBeGreaterThan(
+            0,
+        );
+        expect(
+            currentSettings.features['INTERNAL_TESTING_ENABLED_BY_DEFAULT'],
+        ).toBe(true);
+    });
+
+    it('returns true if feature enabled', () => {
+        const currentSettings = isFeatureEnabled(
+            'INTERNAL_TESTING_ENABLED_BY_DEFAULT',
+        );
+
+        expect(currentSettings).toBe(true);
+    });
+
+    it('toggles a feature', () => {
+        const updatedFeatures = toggleFeature(
+            'INTERNAL_TESTING_ENABLED_BY_DEFAULT',
+            false,
+        );
+        expect(updatedFeatures['INTERNAL_TESTING_ENABLED_BY_DEFAULT']).toBe(
+            false,
+        );
+
+        const currentSettings = getSettings();
+        expect(
+            currentSettings.features['INTERNAL_TESTING_ENABLED_BY_DEFAULT'],
+        ).toBe(false);
+    });
+});