User attribution (#5827)

* initial changes adding modified and created by fields to domain objects and updating properties inspector
* adding created date to object creation
* added a test for created timestamp
* updating remove action to hold the transaction and disregard edit state when handling transactions, also updated object api to return transaction when starting and ignore edit state when determining if transaction is active
* updating docs for object api "startTransaction"
* updating incorrect use of edit and transaction in our appActions for testing

Co-authored-by: Andrew Henry <akhenry@gmail.com>
Co-authored-by: Shefali <simplyrender@gmail.com>

8 files changed, 195 insertions, 63 deletions

diff --git a/e2e/appActions.js b/e2e/appActions.js
index 3064ecf07..7b84080bb 100644
--- a/e2e/appActions.js
+++ b/e2e/appActions.js
@@ -225,15 +225,14 @@ async function getHashUrlToDomainObject(page, uuid) {
 }
 
 /**
- * Utilizes the OpenMCT API to detect if the given object has an active transaction (is in Edit mode).
+ * Utilizes the OpenMCT API to detect if the UI is in Edit mode.
  * @private
  * @param {import('@playwright/test').Page} page
- * @param {string | import('../src/api/objects/ObjectAPI').Identifier} identifier
- * @return {Promise<boolean>} true if the object has an active transaction, false otherwise
+ * @return {Promise<boolean>} true if the Open MCT is in Edit Mode
  */
 async function _isInEditMode(page, identifier) {
     // eslint-disable-next-line no-return-await
-    return await page.evaluate((objectIdentifier) => window.openmct.objects.isTransactionActive(objectIdentifier), identifier);
+    return await page.evaluate(() => window.openmct.editor.isEditing());
 }
 
 /**
diff --git a/src/api/annotation/AnnotationAPISpec.js b/src/api/annotation/AnnotationAPISpec.js
index cc362d8a1..b8464a35d 100644
--- a/src/api/annotation/AnnotationAPISpec.js
+++ b/src/api/annotation/AnnotationAPISpec.js
@@ -94,7 +94,6 @@ describe("The Annotation API", () => {
         openmct.startHeadless();
     });
     afterEach(async () => {
-        openmct.objects.providers = {};
         await resetApplicationState(openmct);
     });
     it("is defined", () => {
diff --git a/src/api/objects/ObjectAPI.js b/src/api/objects/ObjectAPI.js
index 1d0ccfe25..5c22765f9 100644
--- a/src/api/objects/ObjectAPI.js
+++ b/src/api/objects/ObjectAPI.js
@@ -96,7 +96,7 @@ export default class ObjectAPI {
         this.cache = {};
         this.interceptorRegistry = new InterceptorRegistry();
 
-        this.SYNCHRONIZED_OBJECT_TYPES = ['notebook', 'plan', 'annotation'];
+        this.SYNCHRONIZED_OBJECT_TYPES = ['notebook', 'restricted-notebook', 'plan', 'annotation'];
 
         this.errors = {
             Conflict: ConflictError
@@ -204,13 +204,13 @@ export default class ObjectAPI {
         }
 
         identifier = utils.parseKeyString(identifier);
-        let dirtyObject;
+
         if (this.isTransactionActive()) {
-            dirtyObject = this.transaction.getDirtyObject(identifier);
-        }
+            let dirtyObject = this.transaction.getDirtyObject(identifier);
 
-        if (dirtyObject) {
-            return Promise.resolve(dirtyObject);
+            if (dirtyObject) {
+                return Promise.resolve(dirtyObject);
+            }
         }
 
         const provider = this.getProvider(identifier);
@@ -354,10 +354,8 @@ export default class ObjectAPI {
      * @returns {Promise} a promise which will resolve when the domain object
      *          has been saved, or be rejected if it cannot be saved
      */
-    save(domainObject) {
-        let provider = this.getProvider(domainObject.identifier);
-        let savedResolve;
-        let savedReject;
+    async save(domainObject) {
+        const provider = this.getProvider(domainObject.identifier);
         let result;
 
         if (!this.isPersistable(domainObject.identifier)) {
@@ -366,27 +364,37 @@ export default class ObjectAPI {
             result = Promise.resolve(true);
         } else {
             const persistedTime = Date.now();
-            if (domainObject.persisted === undefined) {
-                result = new Promise((resolve, reject) => {
-                    savedResolve = resolve;
-                    savedReject = reject;
+            const username = await this.#getCurrentUsername();
+            const isNewObject = domainObject.persisted === undefined;
+            let savedResolve;
+            let savedReject;
+            let savedObjectPromise;
+
+            result = new Promise((resolve, reject) => {
+                savedResolve = resolve;
+                savedReject = reject;
+            });
+
+            this.#mutate(domainObject, 'persisted', persistedTime);
+            this.#mutate(domainObject, 'modifiedBy', username);
+
+            if (isNewObject) {
+                this.#mutate(domainObject, 'created', persistedTime);
+                this.#mutate(domainObject, 'createdBy', username);
+
+                savedObjectPromise = provider.create(domainObject);
+            } else {
+                savedObjectPromise = provider.update(domainObject);
+            }
+
+            if (savedObjectPromise) {
+                savedObjectPromise.then(response => {
+                    savedResolve(response);
+                }).catch((error) => {
+                    savedReject(error);
                 });
-                domainObject.persisted = persistedTime;
-                const newObjectPromise = provider.create(domainObject);
-                if (newObjectPromise) {
-                    newObjectPromise.then(response => {
-                        this.mutate(domainObject, 'persisted', persistedTime);
-                        savedResolve(response);
-                    }).catch((error) => {
-                        savedReject(error);
-                    });
-                } else {
-                    result = Promise.reject(`[ObjectAPI][save] Object provider returned ${newObjectPromise} when creating new object.`);
-                }
             } else {
-                domainObject.persisted = persistedTime;
-                this.mutate(domainObject, 'persisted', persistedTime);
-                result = provider.update(domainObject);
+                result = Promise.reject(`[ObjectAPI][save] Object provider returned ${savedObjectPromise} when ${isNewObject ? 'creating new' : 'updating'} object.`);
             }
         }
 
@@ -399,8 +407,21 @@ export default class ObjectAPI {
         });
     }
 
+    async #getCurrentUsername() {
+        const user = await this.openmct.user.getCurrentUser();
+        let username;
+
+        if (user !== undefined) {
+            username = user.getName();
+        }
+
+        return username;
+    }
+
     /**
      * After entering into edit mode, creates a new instance of Transaction to keep track of changes in Objects
+     *
+     * @returns {Transaction} a new Transaction that was just created
      */
     startTransaction() {
         if (this.isTransactionActive()) {
@@ -408,6 +429,8 @@ export default class ObjectAPI {
         }
 
         this.transaction = new Transaction(this);
+
+        return this.transaction;
     }
 
     /**
@@ -480,14 +503,16 @@ export default class ObjectAPI {
     }
 
     /**
-     * Modify a domain object.
+     * Modify a domain object. Internal to ObjectAPI, won't call save after.
+     * @private
+     *
      * @param {module:openmct.DomainObject} object the object to mutate
      * @param {string} path the property to modify
      * @param {*} value the new value for this property
      * @method mutate
      * @memberof module:openmct.ObjectAPI#
      */
-    mutate(domainObject, path, value) {
+    #mutate(domainObject, path, value) {
         if (!this.supportsMutation(domainObject.identifier)) {
             throw `Error: Attempted to mutate immutable object ${domainObject.name}`;
         }
@@ -508,6 +533,18 @@ export default class ObjectAPI {
             //Destroy temporary mutable object
             this.destroyMutable(mutableDomainObject);
         }
+    }
+
+    /**
+     * Modify a domain object and save.
+     * @param {module:openmct.DomainObject} object the object to mutate
+     * @param {string} path the property to modify
+     * @param {*} value the new value for this property
+     * @method mutate
+     * @memberof module:openmct.ObjectAPI#
+     */
+    mutate(domainObject, path, value) {
+        this.#mutate(domainObject, path, value);
 
         if (this.isTransactionActive()) {
             this.transaction.add(domainObject);
@@ -684,7 +721,7 @@ export default class ObjectAPI {
     }
 
     isTransactionActive() {
-        return Boolean(this.transaction && this.openmct.editor.isEditing());
+        return this.transaction !== undefined && this.transaction !== null;
     }
 
     #hasAlreadyBeenPersisted(domainObject) {
diff --git a/src/api/objects/ObjectAPISpec.js b/src/api/objects/ObjectAPISpec.js
index 950b356be..c15b3c7db 100644
--- a/src/api/objects/ObjectAPISpec.js
+++ b/src/api/objects/ObjectAPISpec.js
@@ -8,13 +8,27 @@ describe("The Object API", () => {
     let mockDomainObject;
     const TEST_NAMESPACE = "test-namespace";
     const TEST_KEY = "test-key";
+    const USERNAME = 'Joan Q Public';
     const FIFTEEN_MINUTES = 15 * 60 * 1000;
 
     beforeEach((done) => {
         typeRegistry = jasmine.createSpyObj('typeRegistry', [
             'get'
         ]);
+        const userProvider = {
+            isLoggedIn() {
+                return true;
+            },
+            getCurrentUser() {
+                return Promise.resolve({
+                    getName() {
+                        return USERNAME;
+                    }
+                });
+            }
+        };
         openmct = createOpenMct();
+        openmct.user.setProvider(userProvider);
         objectAPI = openmct.objects;
 
         openmct.editor = {};
@@ -63,19 +77,34 @@ describe("The Object API", () => {
                 mockProvider.update.and.returnValue(Promise.resolve(true));
                 objectAPI.addProvider(TEST_NAMESPACE, mockProvider);
             });
-            it("Calls 'create' on provider if object is new", () => {
-                objectAPI.save(mockDomainObject);
+            it("Adds a 'created' timestamp to new objects", async () => {
+                await objectAPI.save(mockDomainObject);
+                expect(mockDomainObject.created).not.toBeUndefined();
+            });
+            it("Calls 'create' on provider if object is new", async () => {
+                await objectAPI.save(mockDomainObject);
                 expect(mockProvider.create).toHaveBeenCalled();
                 expect(mockProvider.update).not.toHaveBeenCalled();
             });
-            it("Calls 'update' on provider if object is not new", () => {
+            it("Calls 'update' on provider if object is not new", async () => {
                 mockDomainObject.persisted = Date.now() - FIFTEEN_MINUTES;
                 mockDomainObject.modified = Date.now();
 
-                objectAPI.save(mockDomainObject);
+                await objectAPI.save(mockDomainObject);
                 expect(mockProvider.create).not.toHaveBeenCalled();
                 expect(mockProvider.update).toHaveBeenCalled();
             });
+            it("Sets the current user for 'createdBy' on new objects", async () => {
+                await objectAPI.save(mockDomainObject);
+                expect(mockDomainObject.createdBy).toBe(USERNAME);
+            });
+            it("Sets the current user for 'modifedBy' on existing objects", async () => {
+                mockDomainObject.persisted = Date.now() - FIFTEEN_MINUTES;
+                mockDomainObject.modified = Date.now();
+
+                await objectAPI.save(mockDomainObject);
+                expect(mockDomainObject.modifiedBy).toBe(USERNAME);
+            });
 
             it("Does not persist if the object is unchanged", () => {
                 mockDomainObject.persisted =
diff --git a/src/plugins/plan/pluginSpec.js b/src/plugins/plan/pluginSpec.js
index d981ac265..781ae7710 100644
--- a/src/plugins/plan/pluginSpec.js
+++ b/src/plugins/plan/pluginSpec.js
@@ -264,7 +264,7 @@ describe('the plugin', function () {
         it('provides an inspector view with the version information if available', () => {
             componentObject = component.$root.$children[0];
             const propertiesEls = componentObject.$el.querySelectorAll('.c-inspect-properties__row');
-            expect(propertiesEls.length).toEqual(4);
+            expect(propertiesEls.length).toEqual(6);
             const found = Array.from(propertiesEls).some((propertyEl) => {
                 return (propertyEl.children[0].innerHTML.trim() === 'Version'
                     && propertyEl.children[1].innerHTML.trim() === 'v1');
diff --git a/src/plugins/remove/RemoveAction.js b/src/plugins/remove/RemoveAction.js
index 8a0b9ec4c..fd6700bde 100644
--- a/src/plugins/remove/RemoveAction.js
+++ b/src/plugins/remove/RemoveAction.js
@@ -19,8 +19,12 @@
  * this source code distribution or the Licensing information page available
  * at runtime from the About dialog for additional information.
  *****************************************************************************/
+
 export default class RemoveAction {
+    #transaction;
+
     constructor(openmct) {
+
         this.name = 'Remove';
         this.key = 'remove';
         this.description = 'Remove this object from its containing object.';
@@ -29,17 +33,25 @@ export default class RemoveAction {
         this.priority = 1;
 
         this.openmct = openmct;
+
+        this.removeFromComposition = this.removeFromComposition.bind(this); // for access to private transaction variable
     }
 
-    invoke(objectPath) {
+    async invoke(objectPath) {
         let object = objectPath[0];
         let parent = objectPath[1];
-        this.showConfirmDialog(object).then(() => {
-            this.removeFromComposition(parent, object);
-            if (this.inNavigationPath(object)) {
-                this.navigateTo(objectPath.slice(1));
-            }
-        }).catch(() => {});
+
+        try {
+            await this.showConfirmDialog(object);
+        } catch (error) {
+            return; // form canceled, exit invoke
+        }
+
+        await this.removeFromComposition(parent, object);
+
+        if (this.inNavigationPath(object)) {
+            this.navigateTo(objectPath.slice(1));
+        }
     }
 
     showConfirmDialog(object) {
@@ -81,20 +93,21 @@ export default class RemoveAction {
         this.openmct.router.navigate('#/browse/' + urlPath);
     }
 
-    removeFromComposition(parent, child) {
-        let composition = parent.composition.filter(id =>
-            !this.openmct.objects.areIdsEqual(id, child.identifier)
-        );
+    async removeFromComposition(parent, child) {
+        this.startTransaction();
 
-        this.openmct.objects.mutate(parent, 'composition', composition);
+        const composition = this.openmct.composition.get(parent);
+        composition.remove(child);
+
+        if (!this.isAlias(child, parent)) {
+            this.openmct.objects.mutate(child, 'location', null);
+        }
 
         if (this.inNavigationPath(child) && this.openmct.editor.isEditing()) {
             this.openmct.editor.save();
         }
 
-        if (!this.isAlias(child, parent)) {
-            this.openmct.objects.mutate(child, 'location', null);
-        }
+        await this.saveTransaction();
     }
 
     isAlias(child, parent) {
@@ -132,4 +145,23 @@ export default class RemoveAction {
             && parentType.definition.creatable
             && Array.isArray(parent.composition);
     }
+
+    startTransaction() {
+        if (!this.openmct.objects.isTransactionActive()) {
+            this.#transaction = this.openmct.objects.startTransaction();
+        }
+    }
+
+    saveTransaction() {
+        if (!this.#transaction) {
+            return;
+        }
+
+        return this.#transaction.commit()
+            .catch(error => {
+                throw error;
+            }).finally(() => {
+                this.openmct.objects.endTransaction();
+            });
+    }
 }
diff --git a/src/ui/inspector/InspectorDetailsSpec.js b/src/ui/inspector/InspectorDetailsSpec.js
index 24553c5c4..208069fb0 100644
--- a/src/ui/inspector/InspectorDetailsSpec.js
+++ b/src/ui/inspector/InspectorDetailsSpec.js
@@ -38,6 +38,8 @@ describe('the inspector', () => {
         folderItem = {
             name: 'folder',
             type: 'folder',
+            createdBy: 'John Q',
+            modifiedBy: 'Public',
             id: 'mock-folder-key',
             identifier: {
                 namespace: '',
@@ -74,6 +76,8 @@ describe('the inspector', () => {
         const [
             title,
             type,
+            createdBy,
+            modifiedBy,
             notes,
             timestamp
         ] = details;
@@ -87,6 +91,14 @@ describe('the inspector', () => {
             .toEqual('Type');
         expect(type.value.toLowerCase())
             .toEqual(folderItem.type);
+        expect(createdBy.name)
+            .toEqual('Created By');
+        expect(createdBy.value)
+            .toEqual(folderItem.createdBy);
+        expect(modifiedBy.name)
+            .toEqual('Modified By');
+        expect(modifiedBy.value)
+            .toEqual(folderItem.modifiedBy);
         expect(notes.value)
             .toEqual('This object should have some notes');
 
diff --git a/src/ui/inspector/details/Properties.vue b/src/ui/inspector/details/Properties.vue
index fe0edda9e..d14a2b4ed 100644
--- a/src/ui/inspector/details/Properties.vue
+++ b/src/ui/inspector/details/Properties.vue
@@ -90,10 +90,13 @@ export default {
                 return;
             }
 
+            const UNKNOWN_USER = 'Unknown';
             const title = this.domainObject.name;
             const typeName = this.type ? this.type.definition.name : `Unknown: ${this.domainObject.type}`;
-            const timestampLabel = this.domainObject.modified ? 'Modified' : 'Created';
-            const timestamp = this.domainObject.modified ? this.domainObject.modified : this.domainObject.created;
+            const createdTimestamp = this.domainObject.created;
+            const createdBy = this.domainObject.createdBy ? this.domainObject.createdBy : UNKNOWN_USER;
+            const modifiedBy = this.domainObject.modifiedBy ? this.domainObject.modifiedBy : UNKNOWN_USER;
+            const modifiedTimestamp = this.domainObject.modified ? this.domainObject.modified : this.domainObject.created;
             const notes = this.domainObject.notes;
             const version = this.domainObject.version;
 
@@ -105,6 +108,14 @@ export default {
                 {
                     name: 'Type',
                     value: typeName
+                },
+                {
+                    name: 'Created By',
+                    value: createdBy
+                },
+                {
+                    name: 'Modified By',
+                    value: modifiedBy
                 }
             ];
 
@@ -115,15 +126,28 @@ export default {
                 });
             }
 
-            if (timestamp !== undefined) {
-                const formattedTimestamp = Moment.utc(timestamp)
+            if (createdTimestamp !== undefined) {
+                const formattedCreatedTimestamp = Moment.utc(createdTimestamp)
+                    .format('YYYY-MM-DD[\n]HH:mm:ss')
+                    + ' UTC';
+
+                details.push(
+                    {
+                        name: 'Created',
+                        value: formattedCreatedTimestamp
+                    }
+                );
+            }
+
+            if (modifiedTimestamp !== undefined) {
+                const formattedModifiedTimestamp = Moment.utc(modifiedTimestamp)
                     .format('YYYY-MM-DD[\n]HH:mm:ss')
                     + ' UTC';
 
                 details.push(
                     {
-                        name: timestampLabel,
-                        value: formattedTimestamp
+                        name: 'Modified',
+                        value: formattedModifiedTimestamp
                     }
                 );
             }