Remove duplicate policy (#2399)

* Removed policy preventing duplicate composition, and implemented no-op in composition provider instead

* Change order of edit on drop event listener

* Add mutation listener to CompositionCollection even if nothing listening to collection

* Updated test specs

* Address review comments

src/api/composition/CompositionAPISpec.js

@@ -22,8 +22,20 @@ define([
             publicAPI = {};
             publicAPI.objects = jasmine.createSpyObj('ObjectAPI', [
                 'get',
-                'mutate'
+                'mutate',
+                'observe',
+                'areIdsEqual'
             ]);
+
+            publicAPI.objects.areIdsEqual.and.callFake(function (id1, id2) {
+                return id1.namespace === id2.namespace && id1.key === id2.key;
+            });
+
+            publicAPI.composition = jasmine.createSpyObj('CompositionAPI', [
+                'checkPolicy'
+            ]);
+            publicAPI.composition.checkPolicy.and.returnValue(true);
+
             publicAPI.objects.eventEmitter = jasmine.createSpyObj('eventemitter', [
                 'on'
             ]);
@@ -91,7 +103,7 @@ define([
                 beforeEach(function () {
                     listener = jasmine.createSpy('reorderListener');
                     composition.on('reorder', listener);
-                    
+
                     return composition.load();
                 });
                 it('', function () {
@@ -119,49 +131,16 @@ define([
                     expect(newComposition[2].key).toEqual('a');
                 })
             });
-
+            it('supports adding an object to composition', function () {
-            // TODO: Implement add/removal in new default provider.
+                let addListener = jasmine.createSpy('addListener');
-            xit('synchronizes changes between instances', function () {
+                let mockChildObject = {
-                var otherComposition = compositionAPI.get(domainObject);
+                    identifier: {key: 'mock-key', namespace: ''}
-                var addListener = jasmine.createSpy('addListener');
+                };
-                var removeListener = jasmine.createSpy('removeListener');
-                var otherAddListener = jasmine.createSpy('otherAddListener');
-                var otherRemoveListener = jasmine.createSpy('otherRemoveListener');
                 composition.on('add', addListener);
-                composition.on('remove', removeListener);
+                composition.add(mockChildObject);
-                otherComposition.on('add', otherAddListener);
-                otherComposition.on('remove', otherRemoveListener);
 
-                return Promise.all([composition.load(), otherComposition.load()])
+                expect(domainObject.composition.length).toBe(4);
-                    .then(function () {
+                expect(domainObject.composition[3]).toEqual(mockChildObject.identifier);
-                        expect(addListener).toHaveBeenCalled();
-                        expect(otherAddListener).toHaveBeenCalled();
-                        expect(removeListener).not.toHaveBeenCalled();
-                        expect(otherRemoveListener).not.toHaveBeenCalled();
-
-                        var object = addListener.calls.mostRecent().args[0];
-                        composition.remove(object);
-                        expect(removeListener).toHaveBeenCalled();
-                        expect(otherRemoveListener).toHaveBeenCalled();
-
-                        addListener.reset();
-                        otherAddListener.reset();
-                        composition.add(object);
-                        expect(addListener).toHaveBeenCalled();
-                        expect(otherAddListener).toHaveBeenCalled();
-
-                        removeListener.reset();
-                        otherRemoveListener.reset();
-                        otherComposition.remove(object);
-                        expect(removeListener).toHaveBeenCalled();
-                        expect(otherRemoveListener).toHaveBeenCalled();
-
-                        addListener.reset();
-                        otherAddListener.reset();
-                        otherComposition.add(object);
-                        expect(addListener).toHaveBeenCalled();
-                        expect(otherAddListener).toHaveBeenCalled();
-                    });
             });
         });
 
@@ -184,7 +163,9 @@ define([
                                 key: 'thing'
                             }
                         ]);
-                    }
+                    },
+                    add: jasmine.createSpy('add'),
+                    remove: jasmine.createSpy('remove')
                 };
                 domainObject = {
                     identifier: {
@@ -214,6 +195,25 @@ define([
                     });
                 });
             });
+            describe('Calling add or remove', function () {
+                let mockChildObject;
+
+                beforeEach(function () {
+                    mockChildObject = {
+                        identifier: {key: 'mock-key', namespace: ''}
+                    };
+                    composition.add(mockChildObject);
+                });
+
+                it('calls add on the provider', function () {
+                    expect(customProvider.add).toHaveBeenCalledWith(domainObject, mockChildObject.identifier);
+                });
+
+                it('calls remove on the provider', function () {
+                    composition.remove(mockChildObject);
+                    expect(customProvider.remove).toHaveBeenCalledWith(domainObject, mockChildObject.identifier);
+                });
+            });
         });
 
         describe('dynamic custom composition', function () {

src/api/composition/CompositionCollection.js

@@ -75,9 +75,7 @@ define([
             throw new Error('Event not supported by composition: ' + event);
         }
         if (!this.mutationListener) {
-            this.mutationListener = this.publicAPI.objects.observe(this.domainObject, '*', (newDomainObject) => {
+            this._synchronize();
-                this.domainObject = newDomainObject;
-            })
         }
         if (this.provider.on && this.provider.off) {
             if (event === 'add') {
@@ -134,10 +132,8 @@ define([
 
         this.listeners[event].splice(index, 1);
         if (this.listeners[event].length === 0) {
-            if (this.mutationListener) {
+            this._destroy();
-                this.mutationListener();
+
-                delete this.mutationListener;
-            }
             // Remove provider listener if this is the last callback to
             // be removed.
             if (this.provider.off && this.provider.on) {
@@ -181,6 +177,9 @@ define([
      */
     CompositionCollection.prototype.add = function (child, skipMutate) {
         if (!skipMutate) {
+            if (!this.publicAPI.composition.checkPolicy(this.domainObject, child)) {
+                throw `Object of type ${child.type} cannot be added to object of type ${this.domainObject.type}`;
+            }
             this.provider.add(this.domainObject, child.identifier);
         } else {
             this.emit('add', child);
@@ -272,6 +271,19 @@ define([
         this.remove(child, true);
     };
 
+    CompositionCollection.prototype._synchronize = function () {
+        this.mutationListener = this.publicAPI.objects.observe(this.domainObject, '*', (newDomainObject) => {
+            this.domainObject = JSON.parse(JSON.stringify(newDomainObject));
+        });
+    };
+
+    CompositionCollection.prototype._destroy = function () {
+        if (this.mutationListener) {
+            this.mutationListener();
+            delete this.mutationListener;
+        }
+    };
+
     /**
      * Emit events.
      * @private

src/api/composition/DefaultCompositionProvider.js

@@ -48,24 +48,11 @@ define([
         this.listeningTo = {};
         this.onMutation = this.onMutation.bind(this);
 
-        this.cannotContainDuplicates = this.cannotContainDuplicates.bind(this);
         this.cannotContainItself = this.cannotContainItself.bind(this);
 
-        compositionAPI.addPolicy(this.cannotContainDuplicates);
         compositionAPI.addPolicy(this.cannotContainItself);
     }
 
-    /**
-     * @private
-     */
-    DefaultCompositionProvider.prototype.cannotContainDuplicates = function (parent, child) {
-        return this.appliesTo(parent) &&
-            parent.composition.findIndex((composeeId) => {
-                return composeeId.namespace === child.identifier.namespace &&
-                    composeeId.key === child.identifier.key;
-            }) === -1;
-    }
-
     /**
      * @private
      */
@@ -199,9 +186,18 @@ define([
      * @memberof module:openmct.CompositionProvider#
      * @method add
      */
-    DefaultCompositionProvider.prototype.add = function (domainObject, child) {
+    DefaultCompositionProvider.prototype.add = function (parent, childId) {
-        throw new Error('Default Provider does not implement adding.');
+        if (!this.includes(parent, childId)) {
-        // TODO: this needs to be synchronized via mutation
+            parent.composition.push(childId);
+            this.publicAPI.objects.mutate(parent, 'composition', parent.composition);
+        }
+    };
+    /**
+     * @private
+     */
+    DefaultCompositionProvider.prototype.includes = function (parent, childId) {
+        return parent.composition.findIndex(composee =>
+            this.publicAPI.objects.areIdsEqual(composee, childId)) !== -1;
     };
 
     DefaultCompositionProvider.prototype.reorder = function (domainObject, oldIndex, newIndex) {

src/ui/components/ObjectView.vue

@@ -34,10 +34,10 @@ export default {
         this.currentObject = this.object;
         this.updateView();
         this.$el.addEventListener('dragover', this.onDragOver);
-        this.$el.addEventListener('drop', this.addObjectToParent);
         this.$el.addEventListener('drop', this.editIfEditable, {
             capture: true
         });
+        this.$el.addEventListener('drop', this.addObjectToParent);
     },
     methods: {
         clear() {
@@ -57,6 +57,10 @@ export default {
                 this.removeSelectable();
                 delete this.removeSelectable;
             }
+
+            if (this.composition) {
+                this.composition._destroy();
+            }
         },
         invokeEditModeHandler(editMode) {
             this.currentView.onEditModeChange(editMode);
@@ -112,14 +116,27 @@ export default {
                 delete this.removeSelectable;
             }
 
+            if (this.composition) {
+                this.composition._destroy();
+            }
+
             this.currentObject = object;
             this.unlisten = this.openmct.objects.observe(this.currentObject, '*', (mutatedObject) => {
                 this.currentObject = mutatedObject;
             });
 
+            this.composition = this.openmct.composition.get(this.currentObject);
+            if (this.composition) {
+                this.composition._synchronize();
+                this.loadComposition();
+            }
+
             this.viewKey = viewKey;
             this.updateView(immediatelySelect);
         },
+        loadComposition() {
+            return this.composition.load();
+        },
         getSelectionContext() {
             if (this.currentView.getSelectionContext) {
                 return this.currentView.getSelectionContext();
@@ -133,10 +150,12 @@ export default {
             }
         },
         addObjectToParent(event) {
-            if (this.hasComposableDomainObject(event)) {
+            if (this.hasComposableDomainObject(event) && this.composition) {
                 let composableDomainObject = this.getComposableDomainObject(event);
-                this.currentObject.composition.push(composableDomainObject.identifier);
+                this.loadComposition().then(() => {
-                this.openmct.objects.mutate(this.currentObject, 'composition', this.currentObject.composition);
+                    this.composition.add(composableDomainObject);
+                });
+
                 event.preventDefault();
                 event.stopPropagation();
             }
@@ -155,6 +174,7 @@ export default {
         editIfEditable(event) {
             let provider = this.getViewProvider();
             if (provider && 
+                provider.canEdit &&
                 provider.canEdit(this.currentObject) &&
                 !this.openmct.editor.isEditing()) {
                     this.openmct.editor.edit();