Fix plot composition (#6206)

* Fixed composition

* Remove unnecessary guard code

* Removing deprecated code

* Use valid key for stacked plot v-for

* Fixed object API specs to expect old values as well as new values in mutation callbacks

* Fixed existing tests

* Added E2E test

* Fixed linting error

---------

Co-authored-by: Shefali Joshi <simplyrender@gmail.com>

e2e/tests/functional/plugins/plot/stackedPlot.e2e.spec.js

@@ -0,0 +1,79 @@
+/*****************************************************************************
+ * 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.
+ *****************************************************************************/
+
+/*
+Tests to verify log plot functionality. Note this test suite if very much under active development and should not
+necessarily be used for reference when writing new tests in this area.
+*/
+
+const { test, expect } = require('../../../../pluginFixtures');
+const { createDomainObjectWithDefaults } = require('../../../../appActions');
+
+test.describe('Stacked Plot', () => {
+
+    test('Using the remove action removes the correct plot', async ({ page }) => {
+        await page.goto('/', { waitUntil: 'networkidle' });
+        const overlayPlot = await createDomainObjectWithDefaults(page, {
+            type: "Overlay Plot"
+        });
+
+        await createDomainObjectWithDefaults(page, {
+            type: "Sine Wave Generator",
+            name: 'swg a',
+            parent: overlayPlot.uuid
+        });
+        await createDomainObjectWithDefaults(page, {
+            type: "Sine Wave Generator",
+            name: 'swg b',
+            parent: overlayPlot.uuid
+        });
+        await createDomainObjectWithDefaults(page, {
+            type: "Sine Wave Generator",
+            name: 'swg c',
+            parent: overlayPlot.uuid
+        });
+        await page.goto(overlayPlot.url);
+        await page.click('button[title="Edit"]');
+
+        // Expand the elements pool vertically
+        await page.locator('.l-pane__handle').nth(2).hover({ trial: true });
+        await page.mouse.down();
+        await page.mouse.move(0, 100);
+        await page.mouse.up();
+
+        await page.locator('.js-elements-pool__tree >> text=swg b').click({ button: 'right' });
+        await page.locator('li[role="menuitem"]:has-text("Remove")').click();
+        await page.locator('.js-overlay .js-overlay__button >> text=OK').click();
+
+        // Wait until the number of elements in the elements pool has changed, and then confirm that the correct children were retained
+        // await page.waitForFunction(() => {
+        //     return Array.from(document.querySelectorAll('.js-elements-pool__tree .js-elements-pool__item')).length === 2;
+        // });
+        // Wait until there are only two items in the elements pool (ie the remove action has completed)
+        await expect(page.locator('.js-elements-pool__tree .js-elements-pool__item')).toHaveCount(2);
+
+        // Confirm that the elements pool contains the items we expect
+        await expect(page.locator('.js-elements-pool__tree >> text=swg a')).toHaveCount(1);
+        await expect(page.locator('.js-elements-pool__tree >> text=swg b')).toHaveCount(0);
+        await expect(page.locator('.js-elements-pool__tree >> text=swg c')).toHaveCount(1);
+    });
+});

src/api/composition/CompositionAPISpec.js

@@ -1,47 +1,35 @@
-import CompositionAPI from './CompositionAPI';
+import { createOpenMct, resetApplicationState } from '../../utils/testing';
 import CompositionCollection from './CompositionCollection';
 
 describe('The Composition API', function () {
     let publicAPI;
     let compositionAPI;
-    let topicService;
-    let mutationTopic;
 
-    beforeEach(function () {
+    beforeEach(function (done) {
+        publicAPI = createOpenMct();
+        compositionAPI = publicAPI.composition;
 
-        mutationTopic = jasmine.createSpyObj('mutationTopic', [
-            'listen'
-        ]);
-        topicService = jasmine.createSpy('topicService');
-        topicService.and.returnValue(mutationTopic);
-        publicAPI = {};
-        publicAPI.objects = jasmine.createSpyObj('ObjectAPI', [
-            'get',
-            'mutate',
-            'observe',
-            'areIdsEqual'
+        const mockObjectProvider = jasmine.createSpyObj("mock provider", [
+            "create",
+            "update",
+            "get"
         ]);
 
-        publicAPI.objects.areIdsEqual.and.callFake(function (id1, id2) {
-            return id1.namespace === id2.namespace && id1.key === id2.key;
+        mockObjectProvider.create.and.returnValue(Promise.resolve(true));
+        mockObjectProvider.update.and.returnValue(Promise.resolve(true));
+        mockObjectProvider.get.and.callFake((identifier) => {
+            return Promise.resolve({identifier});
         });
 
-        publicAPI.composition = jasmine.createSpyObj('CompositionAPI', [
-            'checkPolicy'
-        ]);
-        publicAPI.composition.checkPolicy.and.returnValue(true);
+        publicAPI.objects.addProvider('test', mockObjectProvider);
+        publicAPI.objects.addProvider('custom', mockObjectProvider);
 
-        publicAPI.objects.eventEmitter = jasmine.createSpyObj('eventemitter', [
-            'on'
-        ]);
-        publicAPI.objects.get.and.callFake(function (identifier) {
-            return Promise.resolve({identifier: identifier});
-        });
-        publicAPI.$injector = jasmine.createSpyObj('$injector', [
-            'get'
-        ]);
-        publicAPI.$injector.get.and.returnValue(topicService);
-        compositionAPI = new CompositionAPI(publicAPI);
+        publicAPI.on('start', done);
+        publicAPI.startHeadless();
+    });
+
+    afterEach(() => {
+        return resetApplicationState(publicAPI);
     });
 
     it('returns falsy if an object does not support composition', function () {
@@ -106,6 +94,9 @@ describe('The Composition API', function () {
             let listener;
             beforeEach(function () {
                 listener = jasmine.createSpy('reorderListener');
+                spyOn(publicAPI.objects, 'mutate');
+                publicAPI.objects.mutate.and.callThrough();
+
                 composition.on('reorder', listener);
 
                 return composition.load();
@@ -136,18 +127,20 @@ describe('The Composition API', function () {
             });
         });
         it('supports adding an object to composition', function () {
-            let addListener = jasmine.createSpy('addListener');
             let mockChildObject = {
                 identifier: {
                     key: 'mock-key',
                     namespace: ''
                 }
             };
-            composition.on('add', addListener);
-            composition.add(mockChildObject);
 
-            expect(domainObject.composition.length).toBe(4);
-            expect(domainObject.composition[3]).toEqual(mockChildObject.identifier);
+            return new Promise((resolve) => {
+                composition.on('add', resolve);
+                composition.add(mockChildObject);
+            }).then(() => {
+                expect(domainObject.composition.length).toBe(4);
+                expect(domainObject.composition[3]).toEqual(mockChildObject.identifier);
+            });
         });
     });
 

src/api/composition/CompositionProvider.js

@@ -224,7 +224,7 @@ export default class CompositionProvider {
       * @private
       * @param {DomainObject} oldDomainObject
       */
-    #onMutation(oldDomainObject) {
+    #onMutation(newDomainObject, oldDomainObject) {
         const id = objectUtils.makeKeyString(oldDomainObject.identifier);
         const listeners = this.#listeningTo[id];
 
@@ -232,8 +232,8 @@ export default class CompositionProvider {
             return;
         }
 
-        const oldComposition = listeners.composition.map(objectUtils.makeKeyString);
-        const newComposition = oldDomainObject.composition.map(objectUtils.makeKeyString);
+        const oldComposition = oldDomainObject.composition.map(objectUtils.makeKeyString);
+        const newComposition = newDomainObject.composition.map(objectUtils.makeKeyString);
 
         const added = _.difference(newComposition, oldComposition).map(objectUtils.parseKeyString);
         const removed = _.difference(oldComposition, newComposition).map(objectUtils.parseKeyString);
@@ -248,8 +248,6 @@ export default class CompositionProvider {
             };
         }
 
-        listeners.composition = newComposition.map(objectUtils.parseKeyString);
-
         added.forEach(function (addedChild) {
             listeners.add.forEach(notify(addedChild));
         });

src/api/composition/DefaultCompositionProvider.js

@@ -99,8 +99,7 @@ export default class DefaultCompositionProvider extends CompositionProvider {
             objectListeners = this.listeningTo[keyString] = {
                 add: [],
                 remove: [],
-                reorder: [],
-                composition: [].slice.apply(domainObject.composition)
+                reorder: []
             };
         }
 
@@ -172,8 +171,9 @@ export default class DefaultCompositionProvider extends CompositionProvider {
      */
     add(parent, childId) {
         if (!this.includes(parent, childId)) {
-            parent.composition.push(childId);
-            this.publicAPI.objects.mutate(parent, 'composition', parent.composition);
+            const composition = structuredClone(parent.composition);
+            composition.push(childId);
+            this.publicAPI.objects.mutate(parent, 'composition', composition);
         }
     }
 

src/api/objects/MutableDomainObject.js

@@ -75,21 +75,23 @@ class MutableDomainObject {
         return eventOff;
     }
     $set(path, value) {
+        const oldModel = structuredClone(this);
+        const oldValue = _.get(oldModel, path);
         MutableDomainObject.mutateObject(this, path, value);
 
         //Emit secret synchronization event first, so that all objects are in sync before subsequent events fired.
         this._globalEventEmitter.emit(qualifiedEventName(this, '$_synchronize_model'), this);
 
         //Emit a general "any object" event
-        this._globalEventEmitter.emit(ANY_OBJECT_EVENT, this);
+        this._globalEventEmitter.emit(ANY_OBJECT_EVENT, this, oldModel);
         //Emit wildcard event, with path so that callback knows what changed
-        this._globalEventEmitter.emit(qualifiedEventName(this, '*'), this, path, value);
+        this._globalEventEmitter.emit(qualifiedEventName(this, '*'), this, path, value, oldModel, oldValue);
 
         //Emit events specific to properties affected
         let parentPropertiesList = path.split('.');
         for (let index = parentPropertiesList.length; index > 0; index--) {
             let parentPropertyPath = parentPropertiesList.slice(0, index).join('.');
-            this._globalEventEmitter.emit(qualifiedEventName(this, parentPropertyPath), _.get(this, parentPropertyPath));
+            this._globalEventEmitter.emit(qualifiedEventName(this, parentPropertyPath), _.get(this, parentPropertyPath), _.get(oldModel, parentPropertyPath));
         }
 
         //TODO: Emit events for listeners of child properties when parent changes.
@@ -124,7 +126,7 @@ class MutableDomainObject {
         Object.assign(mutable, object);
 
         mutable.$observe('$_synchronize_model', (updatedObject) => {
-            let clone = JSON.parse(JSON.stringify(updatedObject));
+            let clone = structuredClone(updatedObject);
             utils.refresh(mutable, clone);
         });
 

src/api/objects/ObjectAPI.js

@@ -648,7 +648,7 @@ export default class ObjectAPI {
      * @param {module:openmct.DomainObject} object the object to observe
      * @param {string} path the property to observe
      * @param {Function} callback a callback to invoke when new values for
-     *        this property are observed
+     *        this property are observed.
      * @method observe
      * @memberof module:openmct.ObjectAPI#
      */

src/api/objects/ObjectAPISpec.js

@@ -399,7 +399,7 @@ describe("The Object API", () => {
                     unlisten = objectAPI.observe(mutableSecondInstance, 'otherAttribute', mutationCallback);
                     objectAPI.mutate(mutable, 'otherAttribute', 'some-new-value');
                 }).then(function () {
-                    expect(mutationCallback).toHaveBeenCalledWith('some-new-value');
+                    expect(mutationCallback).toHaveBeenCalledWith('some-new-value', 'other-attribute-value');
                     unlisten();
                 });
             });
@@ -419,14 +419,20 @@ describe("The Object API", () => {
 
                     objectAPI.mutate(mutable, 'objectAttribute.embeddedObject.embeddedKey', 'updated-embedded-value');
                 }).then(function () {
-                    expect(embeddedKeyCallback).toHaveBeenCalledWith('updated-embedded-value');
+                    expect(embeddedKeyCallback).toHaveBeenCalledWith('updated-embedded-value', 'embedded-value');
                     expect(embeddedObjectCallback).toHaveBeenCalledWith({
                         embeddedKey: 'updated-embedded-value'
+                    }, {
+                        embeddedKey: 'embedded-value'
                     });
                     expect(objectAttributeCallback).toHaveBeenCalledWith({
                         embeddedObject: {
                             embeddedKey: 'updated-embedded-value'
                         }
+                    }, {
+                        embeddedObject: {
+                            embeddedKey: 'embedded-value'
+                        }
                     });
 
                     listeners.forEach(listener => listener());

src/api/overlays/components/OverlayComponent.vue

@@ -1,5 +1,5 @@
 <template>
-<div class="c-overlay">
+<div class="c-overlay js-overlay">
     <div
         class="c-overlay__blocker"
         @click="destroy"
@@ -26,7 +26,7 @@
                 v-for="(button, index) in buttons"
                 ref="buttons"
                 :key="index"
-                class="c-button"
+                class="c-button js-overlay__button"
                 tabindex="0"
                 :class="{'c-button--major': focusIndex===index}"
                 @focus="focusIndex=index"

src/plugins/formActions/CreateAction.js

@@ -59,7 +59,7 @@ export default class CreateAction extends PropertiesAction {
             _.set(this.domainObject, key, value);
         });
 
-        const parentDomainObject = parentDomainObjectPath[0];
+        const parentDomainObject = this.openmct.objects.toMutable(parentDomainObjectPath[0]);
 
         this.domainObject.modified = Date.now();
         this.domainObject.location = this.openmct.objects.makeKeyString(parentDomainObject.identifier);
@@ -85,6 +85,7 @@ export default class CreateAction extends PropertiesAction {
             console.error(err);
             this.openmct.notifications.error(`Error saving objects: ${err}`);
         } finally {
+            this.openmct.objects.destroyMutable(parentDomainObject);
             dialog.dismiss();
         }
 
@@ -142,18 +143,21 @@ export default class CreateAction extends PropertiesAction {
             }
         };
 
-        this.domainObject = domainObject;
+        this.domainObject = this.openmct.objects.toMutable(domainObject);
 
         if (definition.initialize) {
-            definition.initialize(domainObject);
+            definition.initialize(this.domainObject);
         }
 
-        const createWizard = new CreateWizard(this.openmct, domainObject, this.parentDomainObject);
+        const createWizard = new CreateWizard(this.openmct, this.domainObject, this.parentDomainObject);
         const formStructure = createWizard.getFormStructure(true);
         formStructure.title = 'Create a New ' + definition.name;
 
         this.openmct.forms.showForm(formStructure)
             .then(this._onSave.bind(this))
-            .catch(this._onCancel.bind(this));
+            .catch(this._onCancel.bind(this))
+            .finally(() => {
+                this.openmct.objects.destroyMutable(this.domainObject);
+            });
     }
 }

src/plugins/plot/stackedPlot/StackedPlot.vue

@@ -35,10 +35,10 @@
     />
     <div class="l-view-section">
         <stacked-plot-item
-            v-for="object in compositionObjects"
-            :key="object.id"
+            v-for="objectWrapper in compositionObjects"
+            :key="objectWrapper.keyString"
             class="c-plot--stacked-container"
-            :child-object="object"
+            :child-object="objectWrapper.object"
             :options="options"
             :grid-lines="gridLines"
             :color-palette="colorPalette"
@@ -169,7 +169,10 @@ export default {
 
             this.$set(this.tickWidthMap, id, 0);
 
-            this.compositionObjects.push(child);
+            this.compositionObjects.push({
+                object: child,
+                keyString: id
+            });
         },
 
         removeChild(childIdentifier) {
@@ -180,6 +183,7 @@ export default {
             const configIndex = this.domainObject.configuration.series.findIndex((seriesConfig) => {
                 return this.openmct.objects.areIdsEqual(seriesConfig.identifier, childIdentifier);
             });
+
             if (configIndex > -1) {
                 this.domainObject.configuration.series.splice(configIndex, 1);
             }
@@ -188,15 +192,11 @@ export default {
                 keyString: id
             });
 
-            const childObj = this.compositionObjects.filter((c) => {
-                const identifier = this.openmct.objects.makeKeyString(c.identifier);
+            this.compositionObjects = this.compositionObjects.filter((c) => {
+                const identifier = c.keyString;
 
-                return identifier === id;
-            })[0];
-            if (childObj) {
-                const index = this.compositionObjects.indexOf(childObj);
-                this.compositionObjects.splice(index, 1);
-            }
+                return identifier !== id;
+            });
         },
 
         compositionReorder(reorderPlan) {

src/ui/inspector/ElementItem.vue

@@ -30,7 +30,7 @@
     @drop="emitDropEvent"
 >
     <div
-        class="c-tree__item c-elements-pool__item"
+        class="c-tree__item c-elements-pool__item js-elements-pool__item"
         :class="{
             'is-context-clicked': contextClickActive,
             'hover': hover,

src/ui/inspector/PlotElementsPool.vue

@@ -34,7 +34,7 @@
         <ul
             v-if="hasElements"
             id="inspector-elements-tree"
-            class="c-tree c-elements-pool__tree"
+            class="c-tree c-elements-pool__tree js-elements-pool__tree"
         >
             <div class="c-elements-pool__instructions"> Select and drag an element to move it into a different axis. </div>
             <element-item-group

src/ui/layout/CreateButton.vue

@@ -14,7 +14,6 @@
 
 <script>
 import CreateAction from '@/plugins/formActions/CreateAction';
-import objectUtils from 'objectUtils';
 
 export default {
     inject: ['openmct'],
@@ -74,23 +73,9 @@ export default {
             this.openmct.menus.showSuperMenu(x, y, this.sortedItems, menuOptions);
         },
         create(key) {
-            // Hack for support.  TODO: rewrite create action.
-            // 1. Get contextual object from navigation
-            // 2. Get legacy type from legacy api
-            // 3. Instantiate create action with type, parent, context
-            // 4. perform action.
-            return this.openmct.objects.get(this.openmct.router.path[0].identifier)
-                .then((currentObject) => {
-                    const createAction = new CreateAction(this.openmct, key, currentObject);
+            const createAction = new CreateAction(this.openmct, key, this.openmct.router.path[0]);
 
-                    createAction.invoke();
-                });
-        },
-        convertToLegacy(domainObject) {
-            let keyString = objectUtils.makeKeyString(domainObject.identifier);
-            let oldModel = objectUtils.toOldFormat(domainObject);
-
-            return this.openmct.$injector.get('instantiate')(oldModel, keyString);
+            createAction.invoke();
         }
     }
 };