From d6e80447ab742a345784b9101907a0fa51d1c8c4 Mon Sep 17 00:00:00 2001 From: Jesse Mazzella Date: Tue, 20 Dec 2022 13:27:51 -0800 Subject: [PATCH] =?UTF-8?q?Mutables=20for=20the=20Tree=20=F0=9F=8E=84=20+?= =?UTF-8?q?=20clean=20up=20TreeItem=20observers=20and=20mutables=20properl?= =?UTF-8?q?y=20(#6032)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 24 ++- e2e/tests/functional/couchdb.e2e.spec.js | 57 +++---- e2e/tests/functional/forms.e2e.spec.js | 105 ++++++++++++- src/api/Editor.js | 4 + src/api/objects/ObjectAPI.js | 12 +- src/plugins/formActions/CreateAction.js | 12 +- .../persistence/couch/CouchObjectProvider.js | 3 +- src/ui/layout/BrowseBar.vue | 1 + src/ui/layout/mct-tree.vue | 140 ++++++++++++------ 9 files changed, 274 insertions(+), 84 deletions(-) diff --git a/e2e/README.md b/e2e/README.md index 6cfd604a20..11de7615da 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -276,14 +276,36 @@ Skipping based on browser version (Rarely used): { +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 + ]); }); }); diff --git a/e2e/tests/functional/forms.e2e.spec.js b/e2e/tests/functional/forms.e2e.spec.js index dff694a949..3e37e5c015 100644 --- a/e2e/tests/functional/forms.e2e.spec.js +++ b/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', () => { diff --git a/src/api/Editor.js b/src/api/Editor.js index 53c7310246..c0c4459942 100644 --- a/src/api/Editor.js +++ b/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) diff --git a/src/api/objects/ObjectAPI.js b/src/api/objects/ObjectAPI.js index 13ce738efb..f5031ec6df 100644 --- a/src/api/objects/ObjectAPI.js +++ b/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; diff --git a/src/plugins/formActions/CreateAction.js b/src/plugins/formActions/CreateAction.js index b0b8a0a800..baf5e1b89b 100644 --- a/src/plugins/formActions/CreateAction.js +++ b/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(); } /** diff --git a/src/plugins/persistence/couch/CouchObjectProvider.js b/src/plugins/persistence/couch/CouchObjectProvider.js index e43dfefb0b..0b98d713a2 100644 --- a/src/plugins/persistence/couch/CouchObjectProvider.js +++ b/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}`); diff --git a/src/ui/layout/BrowseBar.vue b/src/ui/layout/BrowseBar.vue index ba427d9c5c..dd376218d4 100644 --- a/src/ui/layout/BrowseBar.vue +++ b/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() { diff --git a/src/ui/layout/mct-tree.vue b/src/ui/layout/mct-tree.vue index b2a8020987..9c2151d58a 100644 --- a/src/ui/layout/mct-tree.vue +++ b/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]; }); } }