Repository: ProcessMaker/modeler (js)

Issue:
Highlight element automatically when dropped onto the grid
When an element is dragged onto the grid, it should be highlighted by default.

Acceptance criteria:
* Highlighted by default without having to select it manually.
* The crown config should be open.
* The inspector panel should have the element's config loaded.
* Clicking away should de-select the element as normal.

Pull request description:
Highlight dropped element automatically
Fixes #783.


Patch:
diff --git a/src/components/MiniPaper.vue b/src/components/MiniPaper.vue
index acb918dd..94b84f22 100644
--- a/src/components/MiniPaper.vue
+++ b/src/components/MiniPaper.vue
@@ -59,6 +59,7 @@ export default {
 <style lang="scss">
 $mini-paper-container-top-position: 1rem;
 $mini-paper-container-right-position: 17.5rem;
+$mini-paper-container-right-expanded-position: 2rem;
 
 .mini-paper-container {
   position: absolute;
@@ -73,4 +74,7 @@ $mini-paper-container-right-position: 17.5rem;
     pointer-events: none;
   }
 }
+.mini-paper-expanded {
+  right: $mini-paper-container-right-expanded-position;
+}
 </style>
diff --git a/src/components/Modeler.vue b/src/components/Modeler.vue
index d9c6c277..4bcbc34b 100644
--- a/src/components/Modeler.vue
+++ b/src/components/Modeler.vue
@@ -12,7 +12,6 @@
         :controls="controls"
         :panelsCompressed="panelsCompressed"
         :style="{ height: parentHeight }"
-        :invalidDrop="validateDropTarget"
         :allowDrop="allowDrop"
         @drag="validateDropTarget"
         @handleDrop="handleDrop"
@@ -57,7 +56,7 @@
       <div ref="paper" data-test="paper" class="main-paper" />
     </b-col>
 
-    <mini-paper :isOpen="miniMapOpen" :paperManager="paperManager" :graph="graph" />
+    <mini-paper :isOpen="miniMapOpen" :paperManager="paperManager" :graph="graph" :class="{ 'mini-paper-expanded' : panelsCompressed }" />
 
     <b-col
       v-show="!panelsCompressed"
@@ -127,7 +126,7 @@ import runningInCypressTest from '@/runningInCypressTest';
 import getValidationProperties from '@/targetValidationUtils';
 import MiniPaper from '@/components/MiniPaper';
 
-import { faMapMarked, faMinus, faPlus, faExpand, faCompress } from '@fortawesome/free-solid-svg-icons';
+import { faCompress, faExpand, faMapMarked, faMinus, faPlus } from '@fortawesome/free-solid-svg-icons';
 import { FontAwesomeIcon } from '@fortawesome/vue-fontawesome';
 
 import { id as poolId } from './nodes/pool';
@@ -140,6 +139,7 @@ import PaperManager from './paperManager';
 import registerInspectorExtension from '@/components/InspectorExtensionManager';
 
 import initAnchor from '@/mixins/linkManager.js';
+import { addIdToNodeAndSetUpDiagramReference, addNodeToProcess, getTargetProcess } from '@/components/nodeManager';
 
 const version = '1.0';
 
@@ -659,10 +659,7 @@ export default {
         return;
       }
 
-      // Add to our processNode
       const definition = this.nodeRegistry[control.type].definition(this.moddle, this.$t);
-
-      // Now, let's modify planeElement
       const diagram = this.nodeRegistry[control.type].diagram(this.moddle);
 
       const { x, y } = this.paperManager.clientToGridPoint(clientX, clientY);
@@ -673,9 +670,9 @@ export default {
         this.setShapeCenterUnderCursor(diagram);
       }
 
-      // Our BPMN models are updated, now add to our nodes
-      // @todo come up with random id
-      this.addNode({ type: control.type, definition, diagram });
+      const node = {type: control.type, definition, diagram};
+      this.highlightNode(node);
+      this.addNode(node);
     },
     isBoundaryEvent(definition) {
       return definition.$type === 'bpmn:BoundaryEvent';
@@ -684,51 +681,18 @@ export default {
       diagram.bounds.x = diagram.bounds.x - (diagram.bounds.width / 2);
       diagram.bounds.y = diagram.bounds.y - (diagram.bounds.height / 2);
     },
-    addNode({ type, definition, diagram }) {
-      /*
-       * If we are adding a pool, first, create a bpmn:Collaboration, or get the current bpmn:Collaboration,
-       * if one exists.
-       *
-       * For each process, bpmn:Collaboration will contain a bpmn:participant (a pool is a graphical represnetation of a participant).
-       * If there are currently no pools, don't create a new process, use the current one instead, and add (embed) all current flow
-       * elements to it.
-       */
-      if (type !== poolId) {
-        /* Check if this.poolTarget is set, and if so, add to appropriate process. */
-        const targetProcess = this.poolTarget
-          ? this.processes.find(({ id }) => id === this.poolTarget.component.node.definition.get('processRef').id)
-          : this.processNode.definition;
-
-        if (type === laneId) {
-          targetProcess
-            .get('laneSets')[0]
-            .get('lanes')
-            .push(definition);
-        } else if (definition.$type === 'bpmn:TextAnnotation' || definition.$type === 'bpmn:Association') {
-          targetProcess.get('artifacts').push(definition);
-        } else if (definition.$type !== 'bpmn:MessageFlow') {
-          targetProcess.get('flowElements').push(definition);
-        }
-      }
+    addNode(node) {
+      node.pool = this.poolTarget;
 
-      const id = definition.id || this.nodeIdGenerator.generateUniqueNodeId();
-      definition.id = id;
-
-      if (diagram) {
-        diagram.id = `${id}_di`;
-        diagram.bpmnElement = definition;
-      }
+      const targetProcess = getTargetProcess(node, this.processes, this.processNode);
+      addNodeToProcess(node, targetProcess);
+      addIdToNodeAndSetUpDiagramReference(node, this.nodeIdGenerator);
 
-      this.planeElements.push(diagram);
+      this.planeElements.push(node.diagram);
 
-      store.commit('addNode', {
-        type,
-        definition,
-        diagram,
-        pool: this.poolTarget,
-      });
+      store.commit('addNode', node);
 
-      if (![sequenceFlowId, laneId, associationId, messageFlowId].includes(type)) {
+      if (![sequenceFlowId, laneId, associationId, messageFlowId].includes(node.type)) {
         setTimeout(() => this.pushToUndoStack());
       }
 
diff --git a/src/components/nodeManager.js b/src/components/nodeManager.js
new file mode 100644
index 00000000..ad5e99ea
--- /dev/null
+++ b/src/components/nodeManager.js
@@ -0,0 +1,40 @@
+import { id as poolId } from './nodes/pool';
+import { id as laneId } from '@/components/nodes/poolLane';
+
+export function addIdToNodeAndSetUpDiagramReference(node, nodeIdGenerator) {
+  const id = node.definition.id || nodeIdGenerator.generateUniqueNodeId();
+
+  node.definition.id = id;
+
+  if (node.diagram) {
+    node.diagram.id = `${id}_di`;
+    node.diagram.bpmnElement = node.definition;
+  }
+}
+
+export function addNodeToProcess(node, targetProcess) {
+  if (node.type === poolId || node.definition.$type === 'bpmn:MessageFlow') {
+    return;
+  }
+
+  if (node.type === laneId) {
+    targetProcess
+      .get('laneSets')[0]
+      .get('lanes')
+      .push(node.definition);
+    return;
+  }
+
+  if (node.definition.$type === 'bpmn:TextAnnotation' || node.definition.$type === 'bpmn:Association') {
+    targetProcess.get('artifacts').push(node.definition);
+    return;
+  }
+
+  targetProcess.get('flowElements').push(node.definition);
+}
+
+export function getTargetProcess(node, processes, processNode) {
+  return node.pool
+    ? processes.find(({id}) => id === node.pool.component.node.definition.get('processRef').id)
+    : processNode.definition;
+}
diff --git a/src/mixins/crownConfig.js b/src/mixins/crownConfig.js
index bf44899f..cbc0539c 100644
--- a/src/mixins/crownConfig.js
+++ b/src/mixins/crownConfig.js
@@ -58,14 +58,8 @@ export default {
     };
   },
   watch: {
-    highlighted(highlighted) {
-      if (highlighted) {
-        this.shapeView.highlight(this.shapeBody, { highlighter: defaultHighlighter });
-        this.addCrown();
-        return;
-      }
-      this.shapeView.unhighlight(this.shapeBody, { highlighter: defaultHighlighter });
-      this.removeCrown();
+    highlighted() {
+      this.setHighlight();
     },
     hasError() {
       if (this.isRendering) {
@@ -93,6 +87,16 @@ export default {
     },
   },
   methods: {
+    setHighlight() {
+      if (this.highlighted) {
+        this.shapeView.highlight(this.shapeBody, {highlighter: defaultHighlighter});
+        this.addCrown();
+        return;
+      }
+
+      this.shapeView.unhighlight(this.shapeBody, {highlighter: defaultHighlighter});
+      this.removeCrown();
+    },
     setErrorHighlight() {
       if (!this.shapeView) {
         return;
@@ -231,9 +235,11 @@ export default {
         });
 
         this.shape.embed(button);
-        button.addTo(this.graph);
       });
 
+      this.paper.once('render:done', () => this.setHighlight());
+      this.graph.addCells(this.buttons);
+
       this.shape.listenTo(this.paper, 'cell:mouseenter', cellView => {
         if (this.buttons.includes(cellView.model)) {
           this.$emit('setTooltip', cellView);
@@ -320,9 +326,7 @@ export default {
       this.$emit('save-state');
     },
     setUpCrownConfig() {
-      this.$once('click', () => {
-        this.configureCrown();
-      });
+      this.configureCrown();
 
       this.shape.on('change:position', (element, newPosition) => {
         this.node.diagram.bounds.x = newPosition.x;
diff --git a/src/mixins/hideLabelOnDrag.js b/src/mixins/hideLabelOnDrag.js
index e172181d..366c1748 100644
--- a/src/mixins/hideLabelOnDrag.js
+++ b/src/mixins/hideLabelOnDrag.js
@@ -22,7 +22,9 @@ export default {
   async mounted() {
     await this.$nextTick();
 
-    this.shape.once('change:position', this.hideLabel);
+    this.$once('click', () => {
+      this.shape.once('change:position', this.hideLabel);
+    });
     this.shape.listenTo(this.paper, 'cell:pointerup blank:pointerup', this.showLabel);
     this.shape.once('change:parent', this.showLabel);
   },


Test patch:
diff --git a/tests/e2e/specs/BoundaryEventCommonBehaviour.spec.js b/tests/e2e/specs/BoundaryEventCommonBehaviour.spec.js
index 1fa954ae..14be7e3d 100644
--- a/tests/e2e/specs/BoundaryEventCommonBehaviour.spec.js
+++ b/tests/e2e/specs/BoundaryEventCommonBehaviour.spec.js
@@ -259,8 +259,8 @@ boundaryEventData.forEach(({ type, nodeType, eventXMLSnippet, taskType, invalidT
         cy.get('@boundaryEvent').should($boundaryEvent => {
           const { left, top } = $boundaryEvent.position();
 
-          expect(left).to.equal(boundaryEventPosition.left);
-          expect(top).to.equal(boundaryEventPosition.top);
+          expect(left).to.be.closeTo(boundaryEventPosition.left, 2);
+          expect(top).to.be.closeTo(boundaryEventPosition.top, 2);
         });
       });
 
@@ -287,8 +287,8 @@ boundaryEventData.forEach(({ type, nodeType, eventXMLSnippet, taskType, invalidT
         cy.get('@boundaryEvent').should($boundaryEvent => {
           const { left, top } = $boundaryEvent.position();
 
-          expect(left).to.equal(boundaryEventPosition.left);
-          expect(top).to.equal(boundaryEventPosition.top);
+          expect(left).to.closeTo(boundaryEventPosition.left, 2);
+          expect(top).to.closeTo(boundaryEventPosition.top, 2);
         });
       });
     });
diff --git a/tests/e2e/specs/BoundaryTimerEvent.spec.js b/tests/e2e/specs/BoundaryTimerEvent.spec.js
index 0ab51f07..f7fc5488 100644
--- a/tests/e2e/specs/BoundaryTimerEvent.spec.js
+++ b/tests/e2e/specs/BoundaryTimerEvent.spec.js
@@ -156,7 +156,7 @@ describe('Boundary Timer Event', () => {
     dragFromSourceToDest(nodeTypes.task, task2Position);
 
     const boundaryTimerEventSelector = '.main-paper ' +
-      '[data-type="processmaker.components.nodes.task.Shape"] + ' +
+      '[data-type="processmaker.components.nodes.task.Shape"] ~ ' +
       '[data-type="processmaker.components.nodes.boundaryEvent.Shape"]';
 
     getPositionInPaperCoords(task2Position).then(newPosition => {
@@ -205,7 +205,7 @@ describe('Boundary Timer Event', () => {
     dragFromSourceToDest(nodeTypes.task, task2Position);
 
     const boundaryTimerEventSelector = '.main-paper ' +
-      '[data-type="processmaker.components.nodes.task.Shape"] + ' +
+      '[data-type="processmaker.components.nodes.task.Shape"] ~ ' +
       '[data-type="processmaker.components.nodes.boundaryEvent.Shape"]';
 
     getPositionInPaperCoords(task2Position).then(newPosition => {
diff --git a/tests/unit/nodeManager.spec.js b/tests/unit/nodeManager.spec.js
new file mode 100644
index 00000000..66f1ba84
--- /dev/null
+++ b/tests/unit/nodeManager.spec.js
@@ -0,0 +1,143 @@
+import { addIdToNodeAndSetUpDiagramReference, addNodeToProcess, getTargetProcess } from '@/components/nodeManager';
+import { id as poolId } from '@/components/nodes/pool';
+import { id as laneId } from '@/components/nodes/poolLane';
+
+describe('nodeManager', () => {
+  describe('addIdToNodeAndSetUpDiagramReference', () => {
+    it('should not change diagram ID if it already has an ID set', () => {
+      const nodeId = 'foobar';
+      const node = {
+        definition: { id: nodeId },
+        diagram: {},
+      };
+
+      addIdToNodeAndSetUpDiagramReference(node);
+
+      expect(node.definition.id).toBe(nodeId);
+      expect(node.diagram.id).toBe(`${nodeId}_di`);
+      expect(node.diagram.bpmnElement).toBe(node.definition);
+    });
+
+    it('should not throw if node does not have a diagram', () => {
+      const node = {
+        definition: { id: 123 },
+      };
+
+      expect(() => addIdToNodeAndSetUpDiagramReference(node)).not.toThrow();
+    });
+
+    it('should use ID generator if definition does not have ID', () => {
+      const nodeId = 'foobar';
+      const nodeIdGenerator = { generateUniqueNodeId: () => nodeId };
+      const node = {
+        definition: {},
+        diagram: {},
+      };
+
+      addIdToNodeAndSetUpDiagramReference(node, nodeIdGenerator);
+
+      expect(node.definition.id).toBe(nodeId);
+      expect(node.diagram.id).toBe(`${nodeId}_di`);
+    });
+  });
+
+  describe('addNodeToProcess', () => {
+    it('should not modify target process for pool nodes', () => {
+      const node = { type: poolId };
+      const targetProcess = {};
+
+      addNodeToProcess(node, targetProcess);
+
+      expect(targetProcess).toEqual({});
+    });
+
+    it('should not modify target process for node definition $type "bpmn:MessageFlow"', () => {
+      const node = { definition: { $type: 'bpmn:MessageFlow' } };
+      const targetProcess = {};
+
+      addNodeToProcess(node, targetProcess);
+
+      expect(targetProcess).toEqual({});
+    });
+
+    it('should set lane definition on target process lanes', () => {
+      const node = {type: laneId, definition: {}};
+      const targetProcess = {
+        laneSets: [{
+          lanes: [],
+          get(key) {
+            return this[key];
+          },
+        }],
+        get(key) {
+          return this[key];
+        },
+      };
+
+      addNodeToProcess(node, targetProcess);
+
+      expect(targetProcess.laneSets[0].lanes).toHaveLength(1);
+      expect(targetProcess.laneSets[0].lanes[0]).toBe(node.definition);
+    });
+
+    it.each([
+      'bpmn:TextAnnotation',
+      'bpmn:Association',
+    ])('should set definition on target process artifacts for node definition type %s', $type => {
+      const node = { definition: { $type } };
+      const targetProcess = {
+        artifacts: [],
+        get(key) {
+          return this[key];
+        },
+      };
+
+      addNodeToProcess(node, targetProcess);
+
+      expect(targetProcess.artifacts).toHaveLength(1);
+      expect(targetProcess.artifacts[0]).toBe(node.definition);
+    });
+
+    it('should set definition on target process flowElements for non-message-flow node', () => {
+      const node = { definition: { $type: 'bpmn:NotMessageFlow' } };
+      const targetProcess = {
+        flowElements: [],
+        get(key) {
+          return this[key];
+        },
+      };
+
+      addNodeToProcess(node, targetProcess);
+
+      expect(targetProcess.flowElements).toHaveLength(1);
+      expect(targetProcess.flowElements[0]).toBe(node.definition);
+    });
+  });
+
+  describe('getTargetProcess', () => {
+    it('should return processNode definition for node that does not have pool', () => {
+      const node = {};
+      const processNode = { definition: {} };
+
+      const targetProcess = getTargetProcess(node, [], processNode);
+
+      expect(targetProcess).toBe(processNode.definition);
+    });
+
+    it('should return matching pool process for node that has pool', () => {
+      const targetProcessId = 'foobar';
+      const process = { id: targetProcessId };
+      const node = { pool: { component: { node: {definition: {
+        processRef: { id: targetProcessId },
+        get(key) {
+          return this[key];
+        },
+      }}}}};
+      const processes = [{}, {}, process, {}, {}];
+
+      const targetProcess = getTargetProcess(node, processes, {});
+
+      expect(targetProcess).toBe(process);
+    });
+  });
+});