fix: autoscale turned off could cause errors (#5040)

* fix: autoscale turned off could cause errors

* remove commented code

* add tests for plot ticks

* make sure autoscale tests use a certain window size so they work consistently

* add commented code to use once playwright snapshot testing is fixed

* default the user selected range to the current range prior to when they turn off autoscale

* add snapshot tests for plots autoscale turned off test

e2e/tests/plugins/plot/autoscale.e2e.spec.js

@@ -0,0 +1,190 @@
+/*****************************************************************************
+ * Open MCT, Copyright (c) 2014-2022, United States Government
+ * as represented by the Administrator of the National Aeronautics and Space
+ * Administration. All rights reserved.
+ *
+ * Open MCT is licensed under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0.
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ * Open MCT includes source code licensed under additional open source
+ * licenses. See the Open Source Licenses file (LICENSES.md) included with
+ * this source code distribution or the Licensing information page available
+ * at runtime from the About dialog for additional information.
+ *****************************************************************************/
+
+/*
+Test for plot autoscale.
+*/
+
+const { test: _test, expect } = require('@playwright/test');
+
+// create a new `test` API that will not append platform details to snapshot
+// file names, only for the tests in this file, so that the same snapshots will
+// be used for all platforms.
+const test = _test.extend({
+    _autoSnapshotSuffix: [
+        async ({}, use, testInfo) => {
+            testInfo.snapshotSuffix = '';
+            await use();
+        },
+        { auto: true }
+    ]
+});
+
+test.use({
+    viewport: {
+        width: 1280,
+        height: 720
+    }
+});
+
+test.describe('ExportAsJSON', () => {
+    test.only('autoscale off causes no error from undefined user range', async ({ page }) => {
+        await page.goto('/', { waitUntil: 'networkidle' });
+
+        await setTimeRange(page);
+
+        await createSinewaveOverlayPlot(page);
+
+        await testYTicks(page, ['-1.00', '-0.50', '0.00', '0.50', '1.00']);
+
+        await turnOffAutoscale(page);
+
+        const canvas = page.locator('canvas').nth(1);
+
+        // Make sure that after turning off autoscale, the user selected range values start at the same values the plot had prior.
+        await Promise.all([
+            testYTicks(page, ['-1.00', '-0.50', '0.00', '0.50', '1.00']),
+            new Promise(r => setTimeout(r, 100))
+                .then(() => canvas.screenshot())
+                .then(shot => expect(shot).toMatchSnapshot('autoscale-canvas-prepan.png', { maxDiffPixels: 40 }))
+        ]);
+
+        let errorCount = 0;
+
+        function onError() {
+            errorCount++;
+        }
+
+        page.on('pageerror', onError);
+
+        await page.keyboard.down('Alt');
+
+        await canvas.dragTo(canvas, {
+            sourcePosition: {
+                x: 200,
+                y: 200
+            },
+            targetPosition: {
+                x: 400,
+                y: 400
+            }
+        });
+
+        await page.keyboard.up('Alt');
+
+        page.off('pageerror', onError);
+
+        // There would have been an error at this point. So if there isn't, then
+        // we fixed it.
+        expect(errorCount).toBe(0);
+
+        // Ensure the drag worked.
+        await Promise.all([
+            testYTicks(page, ['0.00', '0.50', '1.00', '1.50', '2.00']),
+            new Promise(r => setTimeout(r, 100))
+                .then(() => canvas.screenshot())
+                .then(shot => expect(shot).toMatchSnapshot('autoscale-canvas-panned.png', { maxDiffPixels: 20 }))
+        ]);
+    });
+});
+
+/**
+ * @param {import('@playwright/test').Page} page
+ * @param {string} start
+ * @param {string} end
+ */
+async function setTimeRange(page, start = '2022-03-29 22:00:00.000Z', end = '2022-03-29 22:00:30.000Z') {
+    // Set a specific time range for consistency, otherwise it will change
+    // on every test to a range based on the current time.
+
+    const timeInputs = page.locator('input.c-input--datetime');
+    await timeInputs.first().click();
+    await timeInputs.first().fill(start);
+
+    await timeInputs.nth(1).click();
+    await timeInputs.nth(1).fill(end);
+}
+
+/**
+ * @param {import('@playwright/test').Page} page
+ */
+async function createSinewaveOverlayPlot(page) {
+    // click create button
+    await page.locator('button:has-text("Create")').click();
+
+    // add overlay plot with defaults
+    await page.locator('li:has-text("Overlay Plot")').click();
+    await Promise.all([
+        page.waitForNavigation(/*{ url: 'http://localhost:8080/#/browse/mine/a9268c6f-45cc-4bcd-a6a0-50ac4036e396?tc.mode=fixed&tc.startBound=1649305424163&tc.endBound=1649307224163&tc.timeSystem=utc&view=plot-overlay' }*/),
+        page.locator('text=OK').click()
+    ]);
+
+    // save (exit edit mode)
+    await page.locator('text=Snapshot Save and Finish Editing Save and Continue Editing >> button').nth(1).click();
+    await page.locator('text=Save and Finish Editing').click();
+
+    // click create button
+    await page.locator('button:has-text("Create")').click();
+
+    // add sine wave generator with defaults
+    await page.locator('li:has-text("Sine Wave Generator")').click();
+    await Promise.all([
+        page.waitForNavigation(/*{ url: 'http://localhost:8080/#/browse/mine/a9268c6f-45cc-4bcd-a6a0-50ac4036e396/5cfa5c69-17bc-4a99-9545-4da8125380c5?tc.mode=fixed&tc.startBound=1649305424163&tc.endBound=1649307224163&tc.timeSystem=utc&view=plot-single' }*/),
+        page.locator('text=OK').click()
+    ]);
+
+    // focus the overlay plot
+    await page.locator('text=Open MCT My Items >> span').nth(3).click();
+    await Promise.all([
+        page.waitForNavigation(/*{ url: 'http://localhost:8080/#/browse/mine/a9268c6f-45cc-4bcd-a6a0-50ac4036e396?tc.mode=fixed&tc.startBound=1649305424163&tc.endBound=1649307224163&tc.timeSystem=utc&view=plot-overlay' }*/),
+        page.locator('text=Unnamed Overlay Plot').first().click()
+    ]);
+}
+
+/**
+ * @param {import('@playwright/test').Page} page
+ */
+async function turnOffAutoscale(page) {
+    // enter edit mode
+    await page.locator('text=Unnamed Overlay Plot Snapshot >> button').nth(3).click();
+
+    // uncheck autoscale
+    await page.locator('text=Y Axis Scaling Auto scale Padding >> input[type="checkbox"]').uncheck();
+
+    // save
+    await page.locator('text=Snapshot Save and Finish Editing Save and Continue Editing >> button').nth(1).click();
+    await page.locator('text=Save and Finish Editing').click();
+}
+
+/**
+ * @param {import('@playwright/test').Page} page
+ */
+async function testYTicks(page, values) {
+    const yTicks = page.locator('.gl-plot-y-tick-label');
+    let promises = [yTicks.count().then(c => expect(c).toBe(values.length))];
+
+    for (let i = 0, l = values.length; i < l; i += 1) {
+        promises.push(expect(yTicks.nth(i)).toHaveText(values[i])); // eslint-disable-line
+    }
+
+    await Promise.all(promises);
+}

src/plugins/plot/configuration/PlotConfigurationModel.js

@@ -157,7 +157,8 @@ export default class PlotConfigurationModel extends Model {
 @typedef {{
     configuration: {
         series: import('./PlotSeries').PlotSeriesModelType[]
-    }
+        yAxis: import('./YAxisModel').YAxisModelType
+    },
 }} SomeDomainObject_NeedsName
 */
 

src/plugins/plot/configuration/XAxisModel.js

@@ -19,7 +19,7 @@
  * this source code distribution or the Licensing information page available
  * at runtime from the About dialog for additional information.
  *****************************************************************************/
-import Model from "./Model";
+import Model from './Model';
 
 /**
  * @extends {Model<XAxisModelType, XAxisModelOptions>}
@@ -49,11 +49,11 @@ export default class XAxisModel extends Model {
             }
         });
 
-        this.on('change:frozen', ((frozen) => {
+        this.on('change:frozen', (frozen) => {
             if (!frozen) {
                 this.set('range', this.get('range'));
             }
-        }));
+        });
 
         if (this.get('range')) {
             this.set('range', this.get('range'));
@@ -126,7 +126,7 @@ export default class XAxisModel extends Model {
 
 /**
 @typedef {import("./Model").ModelType<{
-    range: NumberRange
+    range?: NumberRange
     displayRange: NumberRange
     frozen: boolean
     label: string

src/plugins/plot/configuration/YAxisModel.js

@@ -19,7 +19,6 @@
  * this source code distribution or the Licensing information page available
  * at runtime from the About dialog for additional information.
  *****************************************************************************/
-import _ from 'lodash';
 import Model from './Model';
 
 /**
@@ -63,14 +62,14 @@ export default class YAxisModel extends Model {
      */
     listenToSeriesCollection(seriesCollection) {
         this.seriesCollection = seriesCollection;
-        this.listenTo(this.seriesCollection, 'add', (series => {
+        this.listenTo(this.seriesCollection, 'add', series => {
             this.trackSeries(series);
             this.updateFromSeries(this.seriesCollection);
-        }), this);
-        this.listenTo(this.seriesCollection, 'remove', (series => {
+        }, this);
+        this.listenTo(this.seriesCollection, 'remove', series => {
             this.untrackSeries(series);
             this.updateFromSeries(this.seriesCollection);
-        }), this);
+        }, this);
         this.seriesCollection.forEach(this.trackSeries, this);
         this.updateFromSeries(this.seriesCollection);
     }
@@ -140,11 +139,11 @@ export default class YAxisModel extends Model {
     }
     resetStats() {
         this.unset('stats');
-        this.seriesCollection.forEach(function (series) {
+        this.seriesCollection.forEach(series => {
             if (series.has('stats')) {
                 this.updateStats(series.get('stats'));
             }
-        }, this);
+        });
     }
     /**
      * @param {import('./PlotSeries').default} series
@@ -170,7 +169,18 @@ export default class YAxisModel extends Model {
         if (autoscale && this.has('stats')) {
             this.set('displayRange', this.applyPadding(this.get('stats')));
         } else {
-            this.set('displayRange', this.get('range'));
+            const range = this.get('range');
+
+            if (range) {
+                // If we already have a user-defined range, make sure it maps to the
+                // range we'll actually use for the ticks.
+                this.set('displayRange', range);
+            } else {
+                // Otherwise use the last known displayRange as the initial
+                // values for the user-defined range, so that we don't end up
+                // with any error from an undefined user range.
+                this.set('range', this.get('displayRange'));
+            }
         }
     }
     /**
@@ -179,7 +189,7 @@ export default class YAxisModel extends Model {
      */
     updateFromSeries(seriesCollection) {
         const plotModel = this.plot.get('domainObject');
-        const label = _.get(plotModel, 'configuration.yAxis.label');
+        const label = plotModel?.configuration?.yAxis?.label;
         const sampleSeries = seriesCollection.first();
         if (!sampleSeries) {
             if (!label) {
@@ -195,19 +205,19 @@ export default class YAxisModel extends Model {
         this.set('format', yFormat.format.bind(yFormat));
         this.set('values', yMetadata.values);
         if (!label) {
-            const labelName = seriesCollection.map(function (s) {
-                return s.metadata ? s.metadata.value(s.get('yKey')).name : '';
-            }).reduce(function (a, b) {
-                if (a === undefined) {
-                    return b;
-                }
+            const labelName = seriesCollection
+                .map(s => (s.metadata ? s.metadata.value(s.get('yKey')).name : ''))
+                .reduce((a, b) => {
+                    if (a === undefined) {
+                        return b;
+                    }
 
-                if (a === b) {
-                    return a;
-                }
+                    if (a === b) {
+                        return a;
+                    }
 
-                return '';
-            }, undefined);
+                    return '';
+                }, undefined);
 
             if (labelName) {
                 this.set('label', labelName);
@@ -215,19 +225,19 @@ export default class YAxisModel extends Model {
                 return;
             }
 
-            const labelUnits = seriesCollection.map(function (s) {
-                return s.metadata ? s.metadata.value(s.get('yKey')).units : '';
-            }).reduce(function (a, b) {
-                if (a === undefined) {
-                    return b;
-                }
+            const labelUnits = seriesCollection
+                .map(s => (s.metadata ? s.metadata.value(s.get('yKey')).units : ''))
+                .reduce((a, b) => {
+                    if (a === undefined) {
+                        return b;
+                    }
 
-                if (a === b) {
-                    return a;
-                }
+                    if (a === b) {
+                        return a;
+                    }
 
-                return '';
-            }, undefined);
+                    return '';
+                }, undefined);
 
             if (labelUnits) {
                 this.set('label', labelUnits);
@@ -239,14 +249,17 @@ export default class YAxisModel extends Model {
     /**
      * @override
      * @param {import('./Model').ModelOptions<YAxisModelType, YAxisModelOptions>} options
-     * @returns {YAxisModelType}
+     * @returns {Partial<YAxisModelType>}
      */
     defaultModel(options) {
-        // @ts-ignore incomplete YAxisModelType object used for default value.
         return {
             frozen: false,
             autoscale: true,
             autoscalePadding: 0.1
+
+            // 'range' is not specified here, it is undefined at first. When the
+            // user turns off autoscale, the current 'displayRange' is used for
+            // the initial value of 'range'.
         };
     }
 }
@@ -257,7 +270,7 @@ export default class YAxisModel extends Model {
 @typedef {import('./XAxisModel').AxisModelType & {
     autoscale: boolean
     autoscalePadding: number
-    stats: import('./XAxisModel').NumberRange
+    stats?: import('./XAxisModel').NumberRange
     values: Array<TODO>
 }} YAxisModelType
 */

src/plugins/plot/inspector/forms/YAxisForm.vue

@@ -52,10 +52,10 @@
         class="l-inspector-part"
     >
         <div
-            v-show="!autoscale && validation.range"
+            v-show="!autoscale && validationErrors.range"
             class="grid-span-all form-error"
         >
-            {{ validation.range }}
+            {{ validationErrors.range }}
         </div>
         <li class="grid-row force-border">
             <div
@@ -88,7 +88,7 @@
 </template>
 
 <script>
-import { objectPath, validate, coerce } from "./formUtil";
+import { objectPath } from "./formUtil";
 import _ from "lodash";
 
 export default {
@@ -108,7 +108,7 @@ export default {
             autoscalePadding: '',
             rangeMin: '',
             rangeMax: '',
-            validation: {}
+            validationErrors: {}
         };
     },
     mounted() {
@@ -178,12 +178,6 @@ export default {
                         if (Number(range.min) > Number(range.max)) {
                             return 'Minimum must be less than Maximum.';
                         }
-
-                        if (model.get('autoscale')) {
-                            return false;
-                        }
-
-                        return true;
                     }
                 }
             ];
@@ -192,14 +186,9 @@ export default {
             this.label = this.yAxis.get('label');
             this.autoscale = this.yAxis.get('autoscale');
             this.autoscalePadding = this.yAxis.get('autoscalePadding');
-            const range = this.yAxis.get('range');
-            if (!range) {
-                this.rangeMin = undefined;
-                this.rangeMax = undefined;
-            } else {
-                this.rangeMin = range.min;
-                this.rangeMax = range.max;
-            }
+            const range = this.yAxis.get('range') ?? this.yAxis.get('displayRange');
+            this.rangeMin = range?.min;
+            this.rangeMax = range?.max;
         },
         updateForm(formKey) {
             let newVal;
@@ -212,26 +201,27 @@ export default {
                 newVal = this[formKey];
             }
 
-            const oldVal = this.yAxis.get(formKey);
+            let oldVal = this.yAxis.get(formKey);
             const formField = this.fields.find((field) => field.modelProp === formKey);
 
-            const path = objectPath(formField.objectPath);
-            const validationResult = validate(newVal, this.yAxis, formField.validate);
-            if (validationResult === true) {
-                delete this.validation[formKey];
-            } else {
-                this.validation[formKey] = validationResult;
-
+            const validationError = formField.validate?.(newVal, this.yAxis);
+            this.validationErrors[formKey] = validationError;
+            if (validationError) {
                 return;
             }
 
-            if (!_.isEqual(coerce(newVal, formField.coerce), coerce(oldVal, formField.coerce))) {
-                this.yAxis.set(formKey, coerce(newVal, formField.coerce));
+            newVal = formField.coerce?.(newVal) ?? newVal;
+            oldVal = formField.coerce?.(oldVal) ?? oldVal;
+
+            const path = objectPath(formField.objectPath);
+            if (!_.isEqual(newVal, oldVal)) {
+                // TODO: Why do we mutate yAxis twice, once directly, once via objects.mutate? Or are they different objects?
+                this.yAxis.set(formKey, newVal);
                 if (path) {
                     this.openmct.objects.mutate(
                         this.domainObject,
                         path(this.domainObject, this.yAxis),
-                        coerce(newVal, formField.coerce)
+                        newVal
                     );
                 }
             }

src/plugins/plot/inspector/forms/formUtil.js

@@ -15,15 +15,5 @@ export function validate(value, model, validateFunc) {
 }
 
 export function objectPath(path) {
-    if (path) {
-        if (typeof path !== "function") {
-            const staticObjectPath = path;
-
-            return function (object, model) {
-                return staticObjectPath;
-            };
-        }
-
-        return path;
-    }
+    return path && typeof path !== 'function' ? () => path : path;
 }