From 60f20c64d5250c9ea3915412b177861b11c7a2ed Mon Sep 17 00:00:00 2001 From: Andrew Henry Date: Fri, 29 Jul 2022 02:14:09 -0700 Subject: [PATCH] Fix multi user notebook (#5563) * Detect remote changes to notebook object and re-render entries. Detect changes to tags as well * Do not throw an error when getCurrentUser is called, just return undefined. Code needs a way of testing whether there is a valid user * Support remote sync of annotations for notebook entries * Fix bug in notebook spec that prevented multi-user notebook regression being detected * Fixes edge case where an annotation does not initially exist * Use structuredClone instead of JSON functions. Fix logical error in entry modification attribution. Address magical value Co-authored-by: Scott Bell --- src/api/objects/ObjectAPI.js | 2 +- src/api/user/UserAPI.js | 8 ++-- src/plugins/notebook/components/Notebook.vue | 5 +++ .../notebook/components/NotebookEntry.vue | 23 ++++++++-- src/plugins/notebook/pluginSpec.js | 37 +++++++++------- .../persistence/couch/CouchObjectProvider.js | 2 +- src/ui/components/tags/TagEditor.vue | 42 +++++++++++++++---- 7 files changed, 86 insertions(+), 33 deletions(-) diff --git a/src/api/objects/ObjectAPI.js b/src/api/objects/ObjectAPI.js index a0aae34ce9..9f5a681510 100644 --- a/src/api/objects/ObjectAPI.js +++ b/src/api/objects/ObjectAPI.js @@ -88,7 +88,7 @@ export default class ObjectAPI { this.cache = {}; this.interceptorRegistry = new InterceptorRegistry(); - this.SYNCHRONIZED_OBJECT_TYPES = ['notebook', 'plan']; + this.SYNCHRONIZED_OBJECT_TYPES = ['notebook', 'plan', 'annotation']; this.errors = { Conflict: ConflictError diff --git a/src/api/user/UserAPI.js b/src/api/user/UserAPI.js index 0ab2d91569..5d5e45476e 100644 --- a/src/api/user/UserAPI.js +++ b/src/api/user/UserAPI.js @@ -83,9 +83,11 @@ class UserAPI extends EventEmitter { * @throws Will throw an error if no user provider is set */ getCurrentUser() { - this.noProviderCheck(); - - return this._provider.getCurrentUser(); + if (!this.hasProvider()) { + return Promise.resolve(undefined); + } else { + return this._provider.getCurrentUser(); + } } /** diff --git a/src/plugins/notebook/components/Notebook.vue b/src/plugins/notebook/components/Notebook.vue index 4d493525ac..cf7492bd58 100644 --- a/src/plugins/notebook/components/Notebook.vue +++ b/src/plugins/notebook/components/Notebook.vue @@ -296,12 +296,17 @@ export default { window.addEventListener('orientationchange', this.formatSidebar); window.addEventListener('hashchange', this.setSectionAndPageFromUrl); this.filterAndSortEntries(); + this.unobserveEntries = this.openmct.objects.observe(this.domainObject, '*', this.filterAndSortEntries); }, beforeDestroy() { if (this.unlisten) { this.unlisten(); } + if (this.unobserveEntries) { + this.unobserveEntries(); + } + window.removeEventListener('orientationchange', this.formatSidebar); window.removeEventListener('hashchange', this.setSectionAndPageFromUrl); }, diff --git a/src/plugins/notebook/components/NotebookEntry.vue b/src/plugins/notebook/components/NotebookEntry.vue index f90aecb030..947d1b4ffd 100644 --- a/src/plugins/notebook/components/NotebookEntry.vue +++ b/src/plugins/notebook/components/NotebookEntry.vue @@ -88,6 +88,7 @@ :annotation-type="openmct.annotation.ANNOTATION_TYPES.NOTEBOOK" :annotation-search-type="openmct.objects.SEARCH_TYPES.NOTEBOOK_ANNOTATIONS" :target-specific-details="{entryId: entry.id}" + @tags-updated="timestampAndUpdate" />
@@ -146,6 +147,8 @@ import { saveNotebookImageDomainObject, updateNamespaceOfDomainObject } from '.. import Moment from 'moment'; +const UNKNOWN_USER = 'Unknown'; + export default { components: { NotebookEmbed, @@ -206,7 +209,8 @@ export default { return { targetKeyString, - entryId: this.entry.id + entryId: this.entry.id, + modified: this.entry.modified }; }, createdOnTime() { @@ -283,7 +287,7 @@ export default { await this.addNewEmbed(objectPath); } - this.$emit('updateEntry', this.entry); + this.timestampAndUpdate(); }, findPositionInArray(array, id) { let position = -1; @@ -321,7 +325,7 @@ export default { // TODO: remove notebook snapshot object using object remove API this.entry.embeds.splice(embedPosition, 1); - this.$emit('updateEntry', this.entry); + this.timestampAndUpdate(); }, updateEmbed(newEmbed) { this.entry.embeds.some(e => { @@ -333,6 +337,17 @@ export default { return found; }); + this.timestampAndUpdate(); + }, + async timestampAndUpdate() { + const user = await this.openmct.user.getCurrentUser(); + + if (user === undefined) { + this.entry.modifiedBy = UNKNOWN_USER; + } + + this.entry.modified = Date.now(); + this.$emit('updateEntry', this.entry); }, editingEntry() { @@ -342,7 +357,7 @@ export default { const value = $event.target.innerText; if (value !== this.entry.text && value.match(/\S/)) { this.entry.text = value; - this.$emit('updateEntry', this.entry); + this.timestampAndUpdate(); } else { this.$emit('cancelEdit'); } diff --git a/src/plugins/notebook/pluginSpec.js b/src/plugins/notebook/pluginSpec.js index ba83a4f4bc..4fa3e87704 100644 --- a/src/plugins/notebook/pluginSpec.js +++ b/src/plugins/notebook/pluginSpec.js @@ -211,10 +211,17 @@ describe("Notebook plugin:", () => { describe("synchronization", () => { + let objectCloneToSyncFrom; + + beforeEach(() => { + objectCloneToSyncFrom = structuredClone(notebookViewObject); + objectCloneToSyncFrom.persisted = notebookViewObject.modified + 1; + }); + it("updates an entry when another user modifies it", () => { expect(getEntryText(0).innerText).toBe("First Test Entry"); - notebookViewObject.configuration.entries["test-section-1"]["test-page-1"][0].text = "Modified entry text"; - objectProviderObserver(notebookViewObject); + objectCloneToSyncFrom.configuration.entries["test-section-1"]["test-page-1"][0].text = "Modified entry text"; + objectProviderObserver(objectCloneToSyncFrom); return Vue.nextTick().then(() => { expect(getEntryText(0).innerText).toBe("Modified entry text"); @@ -223,13 +230,13 @@ describe("Notebook plugin:", () => { it("shows new entry when another user adds one", () => { expect(allNotebookEntryElements().length).toBe(2); - notebookViewObject.configuration.entries["test-section-1"]["test-page-1"].push({ + objectCloneToSyncFrom.configuration.entries["test-section-1"]["test-page-1"].push({ "id": "entry-3", "createdOn": 0, "text": "Third Test Entry", "embeds": [] }); - objectProviderObserver(notebookViewObject); + objectProviderObserver(objectCloneToSyncFrom); return Vue.nextTick().then(() => { expect(allNotebookEntryElements().length).toBe(3); @@ -237,9 +244,9 @@ describe("Notebook plugin:", () => { }); it("removes an entry when another user removes one", () => { expect(allNotebookEntryElements().length).toBe(2); - let entries = notebookViewObject.configuration.entries["test-section-1"]["test-page-1"]; - notebookViewObject.configuration.entries["test-section-1"]["test-page-1"] = entries.splice(0, 1); - objectProviderObserver(notebookViewObject); + let entries = objectCloneToSyncFrom.configuration.entries["test-section-1"]["test-page-1"]; + objectCloneToSyncFrom.configuration.entries["test-section-1"]["test-page-1"] = entries.splice(0, 1); + objectProviderObserver(objectCloneToSyncFrom); return Vue.nextTick().then(() => { expect(allNotebookEntryElements().length).toBe(1); @@ -256,8 +263,8 @@ describe("Notebook plugin:", () => { }; expect(allNotebookPageElements().length).toBe(2); - notebookViewObject.configuration.sections[0].pages.push(newPage); - objectProviderObserver(notebookViewObject); + objectCloneToSyncFrom.configuration.sections[0].pages.push(newPage); + objectProviderObserver(objectCloneToSyncFrom); return Vue.nextTick().then(() => { expect(allNotebookPageElements().length).toBe(3); @@ -267,8 +274,8 @@ describe("Notebook plugin:", () => { it("updates the notebook when a user removes a page", () => { expect(allNotebookPageElements().length).toBe(2); - notebookViewObject.configuration.sections[0].pages.splice(0, 1); - objectProviderObserver(notebookViewObject); + objectCloneToSyncFrom.configuration.sections[0].pages.splice(0, 1); + objectProviderObserver(objectCloneToSyncFrom); return Vue.nextTick().then(() => { expect(allNotebookPageElements().length).toBe(1); @@ -291,8 +298,8 @@ describe("Notebook plugin:", () => { }; expect(allNotebookSectionElements().length).toBe(2); - notebookViewObject.configuration.sections.push(newSection); - objectProviderObserver(notebookViewObject); + objectCloneToSyncFrom.configuration.sections.push(newSection); + objectProviderObserver(objectCloneToSyncFrom); return Vue.nextTick().then(() => { expect(allNotebookSectionElements().length).toBe(3); @@ -301,8 +308,8 @@ describe("Notebook plugin:", () => { it("updates the notebook when a user removes a section", () => { expect(allNotebookSectionElements().length).toBe(2); - notebookViewObject.configuration.sections.splice(0, 1); - objectProviderObserver(notebookViewObject); + objectCloneToSyncFrom.configuration.sections.splice(0, 1); + objectProviderObserver(objectCloneToSyncFrom); return Vue.nextTick().then(() => { expect(allNotebookSectionElements().length).toBe(1); diff --git a/src/plugins/persistence/couch/CouchObjectProvider.js b/src/plugins/persistence/couch/CouchObjectProvider.js index 754fb6d5da..1c5e76129a 100644 --- a/src/plugins/persistence/couch/CouchObjectProvider.js +++ b/src/plugins/persistence/couch/CouchObjectProvider.js @@ -287,7 +287,7 @@ class CouchObjectProvider { this.objectQueue[key] = new CouchObjectQueue(undefined, response[REV]); } - if (isNotebookType(object)) { + if (isNotebookType(object) || object.type === 'annotation') { //Temporary measure until object sync is supported for all object types //Always update notebook revision number because we have realtime sync, so always assume it's the latest. this.objectQueue[key].updateRevision(response[REV]); diff --git a/src/ui/components/tags/TagEditor.vue b/src/ui/components/tags/TagEditor.vue index 4f581d9964..2a40680091 100644 --- a/src/ui/components/tags/TagEditor.vue +++ b/src/ui/components/tags/TagEditor.vue @@ -97,14 +97,17 @@ export default { this.tagsChanged(this.annotation.tags); }, deep: true + }, + annotationQuery: { + handler() { + this.unloadAnnotation(); + this.loadAnnotation(); + }, + deep: true } }, - async mounted() { - this.annotation = await this.openmct.annotation.getAnnotation(this.annotationQuery, this.annotationSearchType); - this.addAnnotationListener(this.annotation); - if (this.annotation && this.annotation.tags) { - this.tagsChanged(this.annotation.tags); - } + mounted() { + this.loadAnnotation(); }, destroyed() { if (this.removeTagsListener) { @@ -114,7 +117,23 @@ export default { methods: { addAnnotationListener(annotation) { if (annotation && !this.removeTagsListener) { - this.removeTagsListener = this.openmct.objects.observe(annotation, 'tags', this.tagsChanged); + this.removeTagsListener = this.openmct.objects.observe(annotation, '*', (newAnnotation) => { + this.tagsChanged(newAnnotation.tags); + this.annotation = newAnnotation; + }); + } + }, + async loadAnnotation() { + this.annotation = await this.openmct.annotation.getAnnotation(this.annotationQuery, this.annotationSearchType); + this.addAnnotationListener(this.annotation); + if (this.annotation && this.annotation.tags) { + this.tagsChanged(this.annotation.tags); + } + }, + unloadAnnotation() { + if (this.removeTagsListener) { + this.removeTagsListener(); + this.removeTagsListener = undefined; } }, tagsChanged(newTags) { @@ -133,8 +152,11 @@ export default { this.addedTags.push(newTagValue); this.userAddingTag = true; }, - tagRemoved(tagToRemove) { - return this.openmct.annotation.removeAnnotationTag(this.annotation, tagToRemove); + async tagRemoved(tagToRemove) { + const result = await this.openmct.annotation.removeAnnotationTag(this.annotation, tagToRemove); + this.$emit('tags-updated'); + + return result; }, async tagAdded(newTag) { const annotationWasCreated = this.annotation === null || this.annotation === undefined; @@ -146,6 +168,8 @@ export default { this.tagsChanged(this.annotation.tags); this.userAddingTag = false; + + this.$emit('tags-updated'); } } };