From b4554d2fc1fbc7a8c6ed17171e37ffac724e780b Mon Sep 17 00:00:00 2001 From: Jamie V Date: Fri, 21 Oct 2022 17:05:59 -0700 Subject: [PATCH] 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 Co-authored-by: Shefali --- e2e/appActions.js | 7 +- src/api/annotation/AnnotationAPISpec.js | 1 - src/api/objects/ObjectAPI.js | 105 +++++++++++++++-------- src/api/objects/ObjectAPISpec.js | 37 +++++++- src/plugins/plan/pluginSpec.js | 2 +- src/plugins/remove/RemoveAction.js | 62 +++++++++---- src/ui/inspector/InspectorDetailsSpec.js | 12 +++ src/ui/inspector/details/Properties.vue | 36 ++++++-- 8 files changed, 197 insertions(+), 65 deletions(-) diff --git a/e2e/appActions.js b/e2e/appActions.js index 3064ecf079..7b84080bbb 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} true if the object has an active transaction, false otherwise + * @return {Promise} 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 cc362d8a18..b8464a35d7 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 1d0ccfe25c..5c22765f91 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); - } - if (dirtyObject) { - return Promise.resolve(dirtyObject); + if (this.isTransactionActive()) { + let dirtyObject = this.transaction.getDirtyObject(identifier); + + 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; - }); - 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.`); - } + 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 { - domainObject.persisted = persistedTime; - this.mutate(domainObject, 'persisted', persistedTime); - result = provider.update(domainObject); + savedObjectPromise = provider.update(domainObject); + } + + if (savedObjectPromise) { + savedObjectPromise.then(response => { + savedResolve(response); + }).catch((error) => { + savedReject(error); + }); + } else { + 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 950b356be0..c15b3c7db5 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 d981ac2653..781ae7710f 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 8a0b9ec4ca..fd6700bde9 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 24553c5c4d..208069fb08 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 fe0edda9e0..d14a2b4ed9 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: timestampLabel, - value: formattedTimestamp + name: 'Created', + value: formattedCreatedTimestamp + } + ); + } + + if (modifiedTimestamp !== undefined) { + const formattedModifiedTimestamp = Moment.utc(modifiedTimestamp) + .format('YYYY-MM-DD[\n]HH:mm:ss') + + ' UTC'; + + details.push( + { + name: 'Modified', + value: formattedModifiedTimestamp } ); }