Mutables for the Tree 🎄 + clean up TreeItem observers and mutables properly (#6032)

* fix: refresh object after conflict error

* fix: recover from error thrown during create

- Ensure that the "Saving" modal dialog is closed

- Notify user of the error, and also print to console to catch in e2e

* fix: default selector tree item to 'mine' folder

- If create fails due to a conflict or otherwise, and the user immediately tries to "Create" again, default the selector tree's selected item to the "mine" folder (which we know exists).

* fix: don't listen to composition if Selector Tree

* refactor: remove dead code

* fix: use MutableDomainObjects in the tree

- Only use mutables and observers if NOT a SelectorTree

- Properly clean up observers and mutables when a parent item is removed from the tree

* test: verify conflicts don't break object creation

* test: verify dialog closes and object is created

* refactor(e2e): update test

- Error notification on 'My Items' folder missing was removed, so don't check for it

* test: increase timeout

* refactor(e2e): use Promise.any()

* refactor(e2e): use Promise instead of polling

* test: add 2p annotation

* test: use `waitForRequest` instead of promise

- tidy up test, add comments describing our pattern

* docs(e2e): add best practices for network tests

* refactor(e2e): avoid using Promise.any

* fix: de-reactify observer and mutable maps

* fix: destroy by path on treeItem close

* fix: don't refresh for synchronized objects

* docs: fix a typo 🔥

* fix: remove existing mutable before adding

* fix: fail fast if these aren't functions

- Remove check for typeof 'function' to not hide any potential coding errors

* fix: walk up navigationPath if item not found

* chore: fix lint errors

* fix: parse conflicted object name correctly

* fix: re-throw conflict error

* fix: Cancel edit mode on conflict

e2e/README.md

@@ -276,14 +276,36 @@ Skipping based on browser version (Rarely used): <https://github.com/microsoft/p
   - Leverage `await page.goto('./', { waitUntil: 'networkidle' });`
   - Avoid repeated setup to test to test a single assertion. Write longer tests with multiple soft assertions.
 
-### How to write a great test (TODO)
+### How to write a great test (WIP)
+
+- Use our [App Actions](./appActions.js) for performing common actions whenever applicable.
+- If you create an object outside of using the `createDomainObjectWithDefaults` App Action, make sure to fill in the 'Notes' section of your object with `page.testNotes`:
+
+  ```js
+  // Fill the "Notes" section with information about the
+  // currently running test and its project.
+  const { testNotes } = page;
+  const notesInput = page.locator('form[name="mctForm"] #notes-textarea');
+  await notesInput.fill(testNotes);
+  ```
 
 #### How to write a great visual test (TODO)
 
+#### How to write a great network test
+
+- Where possible, it is best to mock out third-party network activity to ensure we are testing application behavior of Open MCT.
+- It is best to be as specific as possible about the expected network request/response structures in creating your mocks.
+- Make sure to only mock requests which are relevant to the specific behavior being tested.
+- Where possible, network requests and responses should be treated in an order-agnostic manner, as the order in which certain requests/responses happen is dynamic and subject to change.
+
+Some examples of mocking network responses in regards to CouchDB can be found in our [couchdb.e2e.spec.js](./tests/functional/couchdb.e2e.spec.js) test file.
+
 ### Best Practices
 
 For now, our best practices exist as self-tested, living documentation in our [exampleTemplate.e2e.spec.js](./tests/framework/exampleTemplate.e2e.spec.js) file.
 
+For best practices with regards to mocking network responses, see our [couchdb.e2e.spec.js](./tests/functional/couchdb.e2e.spec.js) file.
+
 ### Tips & Tricks (TODO)
 
 The following contains a list of tips and tricks which don't exactly fit into a FAQ or Best Practices doc.

e2e/tests/functional/couchdb.e2e.spec.js

@@ -27,7 +27,7 @@
 
 const { test, expect } = require('../../pluginFixtures');
 
-test.describe("CouchDB Status Indicator @couchdb", () => {
+test.describe("CouchDB Status Indicator with mocked responses @couchdb", () => {
     test.use({ failOnConsoleError: false });
     //TODO BeforeAll Verify CouchDB Connectivity with APIContext
     test('Shows green if connected', async ({ page }) => {
@@ -71,38 +71,41 @@ test.describe("CouchDB Status Indicator @couchdb", () => {
     });
 });
 
-test.describe("CouchDB initialization @couchdb", () => {
+test.describe("CouchDB initialization with mocked responses @couchdb", () => {
     test.use({ failOnConsoleError: false });
     test("'My Items' folder is created if it doesn't exist", async ({ page }) => {
-        // Store any relevant PUT requests that happen on the page
-        const createMineFolderRequests = [];
-        page.on('request', req => {
-            // eslint-disable-next-line playwright/no-conditional-in-test
-            if (req.method() === 'PUT' && req.url().endsWith('openmct/mine')) {
-                createMineFolderRequests.push(req);
-            }
-        });
+        const mockedMissingObjectResponsefromCouchDB = {
+            status: 404,
+            contentType: 'application/json',
+            body: JSON.stringify({})
+        };
 
-        // Override the first request to GET openmct/mine to return a 404
-        await page.route('**/openmct/mine', route => {
-            route.fulfill({
-                status: 404,
-                contentType: 'application/json',
-                body: JSON.stringify({})
-            });
+        // Override the first request to GET openmct/mine to return a 404.
+        // This simulates the case of starting Open MCT with a fresh database
+        // and no "My Items" folder created yet.
+        await page.route('**/mine', route => {
+            route.fulfill(mockedMissingObjectResponsefromCouchDB);
         }, { times: 1 });
 
-        // Go to baseURL
+        // Set up promise to verify that a PUT request to create "My Items"
+        // folder was made.
+        const putMineFolderRequest = page.waitForRequest(req =>
+            req.url().endsWith('/mine')
+            && req.method() === 'PUT');
+
+        // Set up promise to verify that a GET request to retrieve "My Items"
+        // folder was made.
+        const getMineFolderRequest = page.waitForRequest(req =>
+            req.url().endsWith('/mine')
+            && req.method() === 'GET');
+
+        // Go to baseURL.
         await page.goto('./', { waitUntil: 'networkidle' });
 
-        // Verify that error banner is displayed
-        const bannerMessage = await page.locator('.c-message-banner__message').innerText();
-        expect(bannerMessage).toEqual('Failed to retrieve object mine');
-
-        // Verify that a PUT request to create "My Items" folder was made
-        await expect.poll(() => createMineFolderRequests.length, {
-            message: 'Verify that PUT request to create "mine" folder was made',
-            timeout: 1000
-        }).toBeGreaterThanOrEqual(1);
+        // Wait for both requests to resolve.
+        await Promise.all([
+            putMineFolderRequest,
+            getMineFolderRequest
+        ]);
     });
 });

e2e/tests/functional/forms.e2e.spec.js

@@ -24,8 +24,9 @@
 This test suite is dedicated to tests which verify form functionality in isolation
 */
 
-const { test, expect } = require('../../baseFixtures');
+const { test, expect } = require('../../pluginFixtures');
 const { createDomainObjectWithDefaults } = require('../../appActions');
+const genUuid = require('uuid').v4;
 const path = require('path');
 
 const TEST_FOLDER = 'test folder';
@@ -128,6 +129,108 @@ test.describe('Persistence operations @couchdb', () => {
             timeout: 1000
         }).toEqual(1);
     });
+    test('Can create an object after a conflict error @couchdb @2p', async ({ page }) => {
+        test.info().annotations.push({
+            type: 'issue',
+            description: 'https://github.com/nasa/openmct/issues/5982'
+        });
+
+        const page2 = await page.context().newPage();
+
+        // Both pages: Go to baseURL
+        await Promise.all([
+            page.goto('./', { waitUntil: 'networkidle' }),
+            page2.goto('./', { waitUntil: 'networkidle' })
+        ]);
+
+        // Both pages: Click the Create button
+        await Promise.all([
+            page.click('button:has-text("Create")'),
+            page2.click('button:has-text("Create")')
+        ]);
+
+        // Both pages: Click "Clock" in the Create menu
+        await Promise.all([
+            page.click(`li[role='menuitem']:text("Clock")`),
+            page2.click(`li[role='menuitem']:text("Clock")`)
+        ]);
+
+        // Generate unique names for both objects
+        const nameInput = page.locator('form[name="mctForm"] .first input[type="text"]');
+        const nameInput2 = page2.locator('form[name="mctForm"] .first input[type="text"]');
+
+        // Both pages: Fill in the 'Name' form field.
+        await Promise.all([
+            nameInput.fill(""),
+            nameInput.fill(`Clock:${genUuid()}`),
+            nameInput2.fill(""),
+            nameInput2.fill(`Clock:${genUuid()}`)
+        ]);
+
+        // Both pages: Fill the "Notes" section with information about the
+        // currently running test and its project.
+        const testNotes = page.testNotes;
+        const notesInput = page.locator('form[name="mctForm"] #notes-textarea');
+        const notesInput2 = page2.locator('form[name="mctForm"] #notes-textarea');
+        await Promise.all([
+            notesInput.fill(testNotes),
+            notesInput2.fill(testNotes)
+        ]);
+
+        // Page 2: Click "OK" to create the domain object and wait for navigation.
+        // This will update the composition of the parent folder, setting the
+        // conditions for a conflict error from the first page.
+        await Promise.all([
+            page2.waitForLoadState(),
+            page2.click('[aria-label="Save"]'),
+            // Wait for Save Banner to appear
+            page2.waitForSelector('.c-message-banner__message')
+        ]);
+
+        // Close Page 2, we're done with it.
+        await page2.close();
+
+        // Page 1: Click "OK" to create the domain object and wait for navigation.
+        // This will trigger a conflict error upon attempting to update
+        // the composition of the parent folder.
+        await Promise.all([
+            page.waitForLoadState(),
+            page.click('[aria-label="Save"]'),
+            // Wait for Save Banner to appear
+            page.waitForSelector('.c-message-banner__message')
+        ]);
+
+        // Page 1: Verify that the conflict has occurred and an error notification is displayed.
+        await expect(page.locator('.c-message-banner__message', {
+            hasText: "Conflict detected while saving mine"
+        })).toBeVisible();
+
+        // Page 1: Start logging console errors from this point on
+        let errors = [];
+        page.on('console', (msg) => {
+            if (msg.type() === 'error') {
+                errors.push(msg.text());
+            }
+        });
+
+        // Page 1: Try to create a clock with the page that received the conflict.
+        const clockAfterConflict = await createDomainObjectWithDefaults(page, {
+            type: 'Clock'
+        });
+
+        // Page 1: Wait for save progress dialog to appear/disappear
+        await page.locator('.c-message-banner__message', {
+            hasText: 'Do not navigate away from this page or close this browser tab while this message is displayed.',
+            state: 'visible'
+        }).waitFor({ state: 'hidden' });
+
+        // Page 1: Navigate to 'My Items' and verify that the second clock was created
+        await page.goto('./#/browse/mine');
+        await expect(page.locator(`.c-grid-item__name[title="${clockAfterConflict.name}"]`)).toBeVisible();
+
+        // Verify no console errors occurred
+        expect(errors).toHaveLength(0);
+    });
 });
 
 test.describe('Form Correctness by Object Type', () => {

src/api/Editor.js

@@ -73,6 +73,10 @@ export default class Editor extends EventEmitter {
 
         return new Promise((resolve, reject) => {
             const transaction = this.openmct.objects.getActiveTransaction();
+            if (!transaction) {
+                return resolve();
+            }
+
             transaction.cancel()
                 .then(resolve)
                 .catch(reject)

src/api/objects/ObjectAPI.js

@@ -410,9 +410,19 @@ export default class ObjectAPI {
             }
         }
 
-        return result.catch((error) => {
+        return result.catch(async (error) => {
             if (error instanceof this.errors.Conflict) {
                 this.openmct.notifications.error(`Conflict detected while saving ${this.makeKeyString(domainObject.identifier)}`);
+
+                // Synchronized objects will resolve their own conflicts, so
+                // bypass the refresh here and throw the error.
+                if (!this.SYNCHRONIZED_OBJECT_TYPES.includes(domainObject.type)) {
+                    if (this.isTransactionActive()) {
+                        this.endTransaction();
+                    }
+
+                    await this.refresh(domainObject);
+                }
             }
 
             throw error;

src/plugins/formActions/CreateAction.js

@@ -73,19 +73,21 @@ export default class CreateAction extends PropertiesAction {
             title: 'Saving'
         });
 
-        const success = await this.openmct.objects.save(this.domainObject);
-        if (success) {
+        try {
+            await this.openmct.objects.save(this.domainObject);
             const compositionCollection = await this.openmct.composition.get(parentDomainObject);
             compositionCollection.add(this.domainObject);
 
             this._navigateAndEdit(this.domainObject, parentDomainObjectPath);
 
             this.openmct.notifications.info('Save successful');
-        } else {
-            this.openmct.notifications.error('Error saving objects');
+        } catch (err) {
+            console.error(err);
+            this.openmct.notifications.error(`Error saving objects: ${err}`);
+        } finally {
+            dialog.dismiss();
         }
 
-        dialog.dismiss();
     }
 
     /**

src/plugins/persistence/couch/CouchObjectProvider.js

@@ -234,7 +234,8 @@ class CouchObjectProvider {
     #handleResponseCode(status, json, fetchOptions) {
         this.indicator.setIndicatorToState(this.#statusCodeToIndicatorState(status));
         if (status === CouchObjectProvider.HTTP_CONFLICT) {
-            throw new this.openmct.objects.errors.Conflict(`Conflict persisting ${fetchOptions.body.name}`);
+            const objectName = JSON.parse(fetchOptions.body)?.model?.name;
+            throw new this.openmct.objects.errors.Conflict(`Conflict persisting "${objectName}"`);
         } else if (status >= CouchObjectProvider.HTTP_BAD_REQUEST) {
             if (!json.error || !json.reason) {
                 throw new Error(`CouchDB Error ${status}`);

src/ui/layout/BrowseBar.vue

@@ -335,6 +335,7 @@ export default {
                 dialog.dismiss();
                 this.openmct.notifications.error('Error saving objects');
                 console.error(error);
+                this.openmct.editor.cancel();
             });
         },
         saveAndContinueEditing() {

src/ui/layout/mct-tree.vue

@@ -174,8 +174,7 @@ export default {
             itemOffset: 0,
             activeSearch: false,
             mainTreeTopMargin: undefined,
-            selectedItem: {},
-            observers: {}
+            selectedItem: {}
         };
     },
     computed: {
@@ -277,10 +276,13 @@ export default {
             this.treeResizeObserver.disconnect();
         }
 
-        this.destroyObservers(this.observers);
+        this.destroyObservers();
+        this.destroyMutables();
     },
     methods: {
         async initialize() {
+            this.observers = {};
+            this.mutables = {};
             this.isLoading = true;
             this.getSavedOpenItems();
             this.treeResizeObserver = new ResizeObserver(this.handleTreeResize);
@@ -355,8 +357,15 @@ export default {
             }
 
             this.treeItems = this.treeItems.filter((checkItem) => {
-                return checkItem.navigationPath === path
-                    || !checkItem.navigationPath.includes(path);
+                if (checkItem.navigationPath !== path
+                    && checkItem.navigationPath.includes(path)) {
+                    this.destroyObserverByPath(checkItem.navigationPath);
+                    this.destroyMutableByPath(checkItem.navigationPath);
+
+                    return false;
+                }
+
+                return true;
             });
             this.openTreeItems.splice(pathIndex, 1);
             this.removeCompositionListenerFor(path);
@@ -436,7 +445,17 @@ export default {
 
             }, Promise.resolve()).then(() => {
                 if (this.isSelectorTree) {
-                    this.treeItemSelection(this.getTreeItemByPath(navigationPath));
+                    // If item is missing due to error in object creation,
+                    // walk up the navigationPath until we find an item
+                    let item = this.getTreeItemByPath(navigationPath);
+                    while (!item) {
+                        const startIndex = 0;
+                        const endIndex = navigationPath.lastIndexOf('/');
+                        navigationPath = navigationPath.substring(startIndex, endIndex);
+                        item = this.getTreeItemByPath(navigationPath);
+                    }
+
+                    this.treeItemSelection(item);
                 }
             });
         },
@@ -537,7 +556,7 @@ export default {
                 composition = sortedComposition;
             }
 
-            if (parentObjectPath.length) {
+            if (parentObjectPath.length && !this.isSelectorTree) {
                 let navigationPath = this.buildNavigationPath(parentObjectPath);
 
                 if (this.compositionCollections[navigationPath]) {
@@ -556,7 +575,15 @@ export default {
             }
 
             return composition.map((object) => {
-                this.addTreeItemObserver(object, parentObjectPath);
+                // Only add observers and mutables if this is NOT a selector tree
+                if (!this.isSelectorTree) {
+                    if (this.openmct.objects.supportsMutation(object.identifier)) {
+                        object = this.openmct.objects.toMutable(object);
+                        this.addMutable(object, parentObjectPath);
+                    }
+
+                    this.addTreeItemObserver(object, parentObjectPath);
+                }
 
                 return this.buildTreeItem(object, parentObjectPath);
             });
@@ -574,6 +601,15 @@ export default {
                 navigationPath
             };
         },
+        addMutable(mutableDomainObject, parentObjectPath) {
+            const objectPath = [mutableDomainObject].concat(parentObjectPath);
+            const navigationPath = this.buildNavigationPath(objectPath);
+
+            // If the mutable already exists, destroy it.
+            this.destroyMutableByPath(navigationPath);
+
+            this.mutables[navigationPath] = () => this.openmct.objects.destroyMutable(mutableDomainObject);
+        },
         addTreeItemObserver(domainObject, parentObjectPath) {
             const objectPath = [domainObject].concat(parentObjectPath);
             const navigationPath = this.buildNavigationPath(objectPath);
@@ -588,30 +624,6 @@ export default {
                 this.sortTreeItems.bind(this, parentObjectPath)
             );
         },
-        async updateTreeItems(parentObjectPath) {
-            let children;
-
-            if (parentObjectPath.length) {
-                const parentItem = this.treeItems.find(item => item.objectPath === parentObjectPath);
-                const descendants = this.getChildrenInTreeFor(parentItem, true);
-                const parentIndex = this.treeItems.map(e => e.object).indexOf(parentObjectPath[0]);
-
-                children = await this.loadAndBuildTreeItemsFor(parentItem.object, parentItem.objectPath);
-
-                this.treeItems.splice(parentIndex + 1, descendants.length, ...children);
-            } else {
-                const root = await this.openmct.objects.get('ROOT');
-                children = await this.loadAndBuildTreeItemsFor(root, []);
-
-                this.treeItems = [...children];
-            }
-
-            for (let item of children) {
-                if (this.isTreeItemOpen(item)) {
-                    this.openTreeItem(item);
-                }
-            }
-        },
         sortTreeItems(parentObjectPath) {
             const navigationPath = this.buildNavigationPath(parentObjectPath);
             const parentItem = this.getTreeItemByPath(navigationPath);
@@ -662,6 +674,10 @@ export default {
                 const descendants = this.getChildrenInTreeFor(parentItem, true);
                 const directDescendants = this.getChildrenInTreeFor(parentItem);
 
+                if (domainObject.isMutable) {
+                    this.addMutable(domainObject, parentItem.objectPath);
+                }
+
                 this.addTreeItemObserver(domainObject, parentItem.objectPath);
 
                 if (directDescendants.length === 0) {
@@ -692,13 +708,15 @@ export default {
         },
         compositionRemoveHandler(navigationPath) {
             return (identifier) => {
-                let removeKeyString = this.openmct.objects.makeKeyString(identifier);
-                let parentItem = this.getTreeItemByPath(navigationPath);
-                let directDescendants = this.getChildrenInTreeFor(parentItem);
-                let removeItem = directDescendants.find(item => item.id === removeKeyString);
+                const removeKeyString = this.openmct.objects.makeKeyString(identifier);
+                const parentItem = this.getTreeItemByPath(navigationPath);
+                const directDescendants = this.getChildrenInTreeFor(parentItem);
+                const removeItem = directDescendants.find(item => item.id === removeKeyString);
 
+                // Remove the item from the tree, unobserve it, and clean up any mutables
                 this.removeItemFromTree(removeItem);
-                this.removeItemFromObservers(removeItem);
+                this.destroyObserverByPath(removeItem.navigationPath);
+                this.destroyMutableByPath(removeItem.navigationPath);
             };
         },
         removeCompositionListenerFor(navigationPath) {
@@ -720,13 +738,6 @@ export default {
             const removeIndex = this.getTreeItemIndex(item.navigationPath);
             this.treeItems.splice(removeIndex, 1);
         },
-        removeItemFromObservers(item) {
-            if (this.observers[item.id]) {
-                this.observers[item.id]();
-
-                delete this.observers[item.id];
-            }
-        },
         addItemToTreeBefore(addItem, beforeItem) {
             const addIndex = this.getTreeItemIndex(beforeItem.navigationPath);
 
@@ -964,13 +975,46 @@ export default {
         handleTreeResize() {
             this.calculateHeights();
         },
-        destroyObservers(observers) {
-            Object.entries(observers).forEach(([keyString, unobserve]) => {
-                if (typeof unobserve === 'function') {
+        /**
+         * Destroy an observer for the given navigationPath.
+         */
+        destroyObserverByPath(navigationPath) {
+            if (this.observers[navigationPath]) {
+                this.observers[navigationPath]();
+                delete this.observers[navigationPath];
+            }
+        },
+        /**
+         * Destroy all observers.
+         */
+        destroyObservers() {
+            Object.entries(this.observers).forEach(([key, unobserve]) => {
+                if (unobserve) {
                     unobserve();
                 }
 
-                delete observers[keyString];
+                delete this.observers[key];
+            });
+        },
+        /**
+         * Destroy a mutable for the given navigationPath.
+         */
+        destroyMutableByPath(navigationPath) {
+            if (this.mutables[navigationPath]) {
+                this.mutables[navigationPath]();
+                delete this.mutables[navigationPath];
+            }
+        },
+        /**
+         * Destroy all mutables.
+         */
+        destroyMutables() {
+            Object.entries(this.mutables).forEach(([key, destroyMutable]) => {
+                if (destroyMutable) {
+                    destroyMutable();
+                }
+
+                delete this.mutables[key];
             });
         }
     }