Repository: ProcessMaker/modeler (js)

Issue:
Add new crown to Call Activity
**Acceptance Criteria:**
Should only have a trash icon and a sequence flow.

![Screen Shot 2019-11-13 at 9 44 06 AM](https://user-images.githubusercontent.com/6653340/68773814-3822f780-05fa-11ea-8b0f-f964f682a190.png)

Related: #789 

Pull request:
Added new crown to Call Activity
Closes #802 

Patch:
diff --git a/src/MessageEventIdGenerator.js b/src/MessageEventIdGenerator.js
deleted file mode 100644
index c1191c4c..00000000
--- a/src/MessageEventIdGenerator.js
+++ /dev/null
@@ -1,25 +0,0 @@
-import  NodeIdGenerator from './NodeIdGenerator';
-import store from '@/store';
-import get from 'lodash/get';
-
-export default class MessageEventIdGenerator extends NodeIdGenerator {
-  generateNewMessageEventId() {
-    let id = super.generateNewNodeId('message_event_');
-
-    while (!this.isIdUnique(id)) {
-      id = this.generateNewMessageEventId();
-    }
-
-    return id;
-  }
-
-  isIdUnique(id) {
-    const nodes = store.getters.nodes;
-
-    const eventDefinitionIdList = nodes
-      .map(node => get(node, 'definition.eventDefinitions[0].id'))
-      .filter(id => id !== undefined);
-
-    return !eventDefinitionIdList.includes(id);
-  }
-}
diff --git a/src/NodeIdGenerator.js b/src/NodeIdGenerator.js
index 9c14cafd..518b0f3c 100644
--- a/src/NodeIdGenerator.js
+++ b/src/NodeIdGenerator.js
@@ -1,33 +1,55 @@
-const nodeIdPrefix = 'node_';
-
 export default class NodeIdGenerator {
+  static prefix = 'node_';
+
+  #counter = 1;
+  #diagramCounter = 1;
+
   constructor(definitions) {
-    this.counter = 1;
     this.definitions = definitions;
   }
 
-  generateUniqueNodeId() {
-    let id = this.generateNewNodeId();
+  generate() {
+    let definitionId = this.#generateDefinitionId();
+    let diagramId = this.#generateDiagramId();
 
-    while (!this.isIdUnique(id)) {
-      id = this.generateNewNodeId();
+    while (!this.#isDefinitionIdUnique(definitionId)) {
+      definitionId = this.#generateDefinitionId();
     }
 
-    return id;
+    while (!this.#isDiagramIdUnique(diagramId)) {
+      diagramId = this.#generateDiagramId();
+    }
+
+    return [definitionId, diagramId];
   }
 
-  generateNewNodeId(prefix = nodeIdPrefix) {
-    const id = prefix + this.counter;
-    this.counter++;
+  #generateDefinitionId = () => {
+    const id = NodeIdGenerator.prefix + this.#counter;
+    this.#counter++;
 
     return id;
-  }
+  };
 
-  isIdUnique(id) {
+  #generateDiagramId = () => {
+    const id = NodeIdGenerator.prefix + this.#diagramCounter + '_di';
+    this.#diagramCounter++;
+
+    return id;
+  };
+
+  #isDefinitionIdUnique = id => {
     const planeElementIds = this.definitions.diagrams[0].plane
       .get('planeElement')
       .map(planeElement => planeElement.get('bpmnElement').id);
 
     return !planeElementIds.includes(id);
-  }
+  };
+
+  #isDiagramIdUnique = id => {
+    const diagramElementIds = this.definitions.diagrams[0].plane
+      .get('planeElement')
+      .map(planeElement => planeElement.id);
+
+    return !diagramElementIds.includes(id);
+  };
 }
diff --git a/src/components/crownConfig.vue b/src/components/crownConfig.vue
index be42dacf..7da3e198 100644
--- a/src/components/crownConfig.vue
+++ b/src/components/crownConfig.vue
@@ -6,6 +6,7 @@
       :title="$t('Sequence Flow')"
     />
     <message-flow-button
+      v-if="isValidMessageFlowSource"
       @click="addMessageFlow"
       class="crown-config__icon"
       :title="$t('Message Flow')"
@@ -59,6 +60,13 @@ export default {
       /* allowSetNodePosition is used to prevent setting a node position outside of a pool */
       allowSetNodePosition: true,
       savePositionOnPointerupEventSet: false,
+      validMessageFlowSources: [
+        'processmaker-modeler-start-event',
+        'processmaker-modeler-end-event',
+        'processmaker-modeler-task',
+        'processmaker-modeler-pool',
+        'processmaker-modeler-intermediate-message-catch-event',
+      ],
     };
   },
   created() {
@@ -84,6 +92,9 @@ export default {
       const width = (this.node.diagram.bounds.width * this.paper.scale().sx);
       return 'top:' + (y - 45) + 'px;left:' + (x + width - 20) + 'px;cursor:pointer';
     },
+    isValidMessageFlowSource() {
+      return this.validMessageFlowSources.includes(this.node.type);
+    },
   },
   methods: {
     addSequence(cellView, evt, x, y) {
diff --git a/src/components/nodeManager.js b/src/components/nodeManager.js
index ad5e99ea..fc3a871a 100644
--- a/src/components/nodeManager.js
+++ b/src/components/nodeManager.js
@@ -2,12 +2,14 @@ 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();
+  const [nodeId, diagramId] = nodeIdGenerator.generate();
 
-  node.definition.id = id;
+  if (!node.definition.id) {
+    node.definition.id = nodeId;
+  }
 
   if (node.diagram) {
-    node.diagram.id = `${id}_di`;
+    node.diagram.id = diagramId;
     node.diagram.bpmnElement = node.definition;
   }
 }
diff --git a/src/components/nodes/callActivity/callActivity.vue b/src/components/nodes/callActivity/callActivity.vue
index 33c71e3b..fae83a98 100644
--- a/src/components/nodes/callActivity/callActivity.vue
+++ b/src/components/nodes/callActivity/callActivity.vue
@@ -1,11 +1,25 @@
 <template>
-  <div />
+  <crown-config
+    :highlighted="highlighted"
+    :paper="paper"
+    :graph="graph"
+    :shape="shape"
+    :node="node"
+    :nodeRegistry="nodeRegistry"
+    :moddle="moddle"
+    :collaboration="collaboration"
+    :process-node="processNode"
+    :plane-elements="planeElements"
+    :is-rendering="isRendering"
+    @remove-node="$emit('remove-node', $event)"
+    @add-node="$emit('add-node', $event)"
+    @save-state="$emit('save-state', $event)"
+  />
 </template>
 
 <script>
 import { util } from 'jointjs';
 import connectIcon from '@/assets/connect-elements.svg';
-import crownConfig from '@/mixins/crownConfig';
 import portsConfig from '@/mixins/portsConfig';
 import TaskShape from '@/components/nodes/task/shape';
 import { taskHeight } from '@/components/nodes/task/taskConfig';
@@ -14,13 +28,30 @@ import uniqBy from 'lodash/uniqBy';
 import hasMarkers, { markerSize } from '@/mixins/hasMarkers';
 import hideLabelOnDrag from '@/mixins/hideLabelOnDrag';
 import { elementIdParser } from '@/components/nodes/callActivity/elementIdParser';
+import CrownConfig from '@/components/crownConfig';
+import highlightConfig from '@/mixins/highlightConfig';
 
 const labelPadding = 15;
 const topAndBottomMarkersSpace = 2 * markerSize;
 
 export default {
-  props: ['graph', 'node', 'id'],
-  mixins: [crownConfig, portsConfig, hasMarkers, hideLabelOnDrag],
+  components: {
+    CrownConfig,
+  },
+  props: [
+    'graph',
+    'node',
+    'id',
+    'highlighted',
+    'nodeRegistry',
+    'moddle',
+    'paper',
+    'collaboration',
+    'processNode',
+    'planeElements',
+    'isRendering',
+  ],
+  mixins: [highlightConfig, portsConfig, hasMarkers, hideLabelOnDrag],
   data() {
     return {
       crownConfig: [
@@ -78,7 +109,7 @@ export default {
   },
   mounted() {
     this.shape = new TaskShape();
-
+    this.shape.set('type', 'newCrown');
     let bounds = this.node.diagram.bounds;
     this.shape.position(bounds.x, bounds.y);
     this.shape.resize(bounds.width, bounds.height);
diff --git a/src/components/nodes/task/task.vue b/src/components/nodes/task/task.vue
index 2b4e52c6..80a67014 100644
--- a/src/components/nodes/task/task.vue
+++ b/src/components/nodes/task/task.vue
@@ -1,22 +1,20 @@
 <template>
-  <div>
-    <crown-config
-      :highlighted="highlighted"
-      :paper="paper"
-      :graph="graph"
-      :shape="shape"
-      :node="node"
-      :nodeRegistry="nodeRegistry"
-      :moddle="moddle"
-      :collaboration="collaboration"
-      :process-node="processNode"
-      :plane-elements="planeElements"
-      :is-rendering="isRendering"
-      @remove-node="$emit('remove-node', $event)"
-      @add-node="$emit('add-node', $event)"
-      @save-state="$emit('save-state', $event)"
-    />
-  </div>
+  <crown-config
+    :highlighted="highlighted"
+    :paper="paper"
+    :graph="graph"
+    :shape="shape"
+    :node="node"
+    :nodeRegistry="nodeRegistry"
+    :moddle="moddle"
+    :collaboration="collaboration"
+    :process-node="processNode"
+    :plane-elements="planeElements"
+    :is-rendering="isRendering"
+    @remove-node="$emit('remove-node', $event)"
+    @add-node="$emit('add-node', $event)"
+    @save-state="$emit('save-state', $event)"
+  />
 </template>
 
 <script>


Test patch:
diff --git a/tests/e2e/fixtures/setUpForDuplicateDiagramId.xml b/tests/e2e/fixtures/setUpForDuplicateDiagramId.xml
new file mode 100644
index 00000000..152f3ace
--- /dev/null
+++ b/tests/e2e/fixtures/setUpForDuplicateDiagramId.xml
@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" id="Definitions_03dabax" targetNamespace="http://bpmn.io/schema/bpmn" exporter="ProcessMaker Modeler" exporterVersion="1.0">
+  <bpmn:process id="Process_1" isExecutable="true">
+    <bpmn:task id="node_2" name="Task" />
+  </bpmn:process>
+  <bpmndi:BPMNDiagram id="BPMNDiagram_1">
+    <bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_1">
+      <bpmndi:BPMNShape id="node_1_di" bpmnElement="node_2">
+        <dc:Bounds x="310" y="170" width="116" height="76" />
+      </bpmndi:BPMNShape>
+    </bpmndi:BPMNPlane>
+  </bpmndi:BPMNDiagram>
+</bpmn:definitions>
diff --git a/tests/e2e/specs/BoundaryEventCommonBehaviour.spec.js b/tests/e2e/specs/BoundaryEventCommonBehaviour.spec.js
index 7933807d..95d42468 100644
--- a/tests/e2e/specs/BoundaryEventCommonBehaviour.spec.js
+++ b/tests/e2e/specs/BoundaryEventCommonBehaviour.spec.js
@@ -21,32 +21,24 @@ const boundaryEventData = [{
   eventXMLSnippet: '<bpmn:boundaryEvent id="node_3" name="New Boundary Timer Event" attachedToRef="node_2"><bpmn:timerEventDefinition><bpmn:timeDuration>PT1H</bpmn:timeDuration></bpmn:timerEventDefinition></bpmn:boundaryEvent>',
   taskType: nodeTypes.task,
   invalidTargets: [{ type: nodeTypes.startEvent }],
-  taskSelector: '.main-paper ' +
-    '[data-type="newCrown"]',
 }, {
   type: 'Boundary Error Event',
   nodeType: nodeTypes.boundaryErrorEvent,
   eventXMLSnippet: '<bpmn:boundaryEvent id="node_3" name="New Boundary Error Event" attachedToRef="node_2"><bpmn:errorEventDefinition /></bpmn:boundaryEvent>',
   taskType: nodeTypes.task,
   invalidTargets: [{ type: nodeTypes.startEvent }],
-  taskSelector: '.main-paper ' +
-    '[data-type="newCrown"]',
 }, {
   type: 'Boundary Escalation Event',
   nodeType: nodeTypes.boundaryEscalationEvent,
   eventXMLSnippet: '<bpmn:boundaryEvent id="node_3" name="New Boundary Escalation Event" attachedToRef="node_2"><bpmn:escalationEventDefinition /></bpmn:boundaryEvent>',
   taskType: nodeTypes.callActivity,
   invalidTargets: [{ type: nodeTypes.startEvent }, { type: nodeTypes.task, color: defaultNodeColor }],
-  taskSelector: '.main-paper ' +
-    '[data-type="processmaker.components.nodes.task.Shape"]',
 }, {
   type: 'Boundary Message Event',
   nodeType: nodeTypes.boundaryMessageEvent,
   eventXMLSnippet: '<bpmn:boundaryEvent id="node_3" name="New Boundary Message Event" attachedToRef="node_2"><bpmn:messageEventDefinition /></bpmn:boundaryEvent>',
   taskType: nodeTypes.callActivity,
   invalidTargets: [{ type: nodeTypes.startEvent }],
-  taskSelector: '.main-paper ' +
-    '[data-type="processmaker.components.nodes.task.Shape"]',
 }];
 
 function testThatBoundaryEventIsCloseToTask(boundaryEvent, task) {
@@ -69,7 +61,7 @@ function configurePool(poolPosition, nodeType, taskType) {
   dragFromSourceToDest(nodeTypes.pool, poolPosition);
 }
 
-boundaryEventData.forEach(({ type, nodeType, eventXMLSnippet, taskType, invalidTargets, taskSelector }) => {
+boundaryEventData.forEach(({ type, nodeType, eventXMLSnippet, taskType, invalidTargets }) => {
   describe(`Common behaviour test for boundary event type ${type}`, () => {
     it('can render a boundary event of this type', function() {
       dragFromSourceToDest(taskType, taskPosition);
@@ -109,6 +101,8 @@ boundaryEventData.forEach(({ type, nodeType, eventXMLSnippet, taskType, invalidT
 
     it('can stay anchored to task when moving pool', function() {
       configurePool({ x: 300, y: 300 }, nodeType, taskType);
+      const taskSelector = '.main-paper ' +
+        '[data-type="newCrown"]';
 
       cy.get(boundaryEventSelector).then($boundaryEvent => {
         cy.get(taskSelector).then($task => {
diff --git a/tests/e2e/specs/Modeler.spec.js b/tests/e2e/specs/Modeler.spec.js
index dba8ad19..0338b8bb 100644
--- a/tests/e2e/specs/Modeler.spec.js
+++ b/tests/e2e/specs/Modeler.spec.js
@@ -5,6 +5,7 @@ import {
   getElementAtPosition,
   getGraphElements,
   getLinksConnectedToElement,
+  getXml,
   isElementCovered,
   removeIndentationAndLinebreaks,
   typeIntoTextInput,
@@ -444,4 +445,13 @@ describe('Modeler', () => {
       getType().
       should('equal', nodeTypes.task);
   });
+
+  it('should not generate duplicate diagram IDs', function() {
+    uploadXml('setUpForDuplicateDiagramId.xml');
+    dragFromSourceToDest(nodeTypes.task, { x: 300, y: 300 });
+    getXml().then(xml => {
+      expect(xml.match(/node_1_di/g)).to.have.length(1);
+      expect(xml.match(/node_2_di/g)).to.have.length(1);
+    });
+  });
 });
diff --git a/tests/e2e/support/utils.js b/tests/e2e/support/utils.js
index 5030921c..f0201262 100644
--- a/tests/e2e/support/utils.js
+++ b/tests/e2e/support/utils.js
@@ -248,22 +248,21 @@ export function getNumberOfLinks() {
     .then(({ graph }) => graph.getLinks().length);
 }
 
-export function assertDownloadedXmlContainsExpected(xmlString) {
+export function getXml() {
   cy.get('[data-test=downloadXMLBtn]').click();
-  cy.window()
+  return cy.window()
     .its('xml')
-    .then(removeIndentationAndLinebreaks)
-    .then(xml => {
-      expect(xml).to.contain(removeIndentationAndLinebreaks(xmlString));
-    });
+    .then(removeIndentationAndLinebreaks);
+}
+
+export function assertDownloadedXmlContainsExpected(xmlString) {
+  getXml().then(xml => {
+    expect(xml).to.contain(removeIndentationAndLinebreaks(xmlString));
+  });
 }
 
 export function assertDownloadedXmlDoesNotContainExpected(xmlString) {
-  cy.get('[data-test=downloadXMLBtn]').click();
-  cy.window()
-    .its('xml')
-    .then(removeIndentationAndLinebreaks)
-    .then(xml => {
-      expect(xml).to.not.contain(removeIndentationAndLinebreaks(xmlString));
-    });
+  getXml().then(xml => {
+    expect(xml).to.not.contain(removeIndentationAndLinebreaks(xmlString));
+  });
 }
diff --git a/tests/unit/nodeIdGenerator.spec.js b/tests/unit/nodeIdGenerator.spec.js
new file mode 100644
index 00000000..966263e4
--- /dev/null
+++ b/tests/unit/nodeIdGenerator.spec.js
@@ -0,0 +1,81 @@
+import NodeIdGenerator from '@/NodeIdGenerator';
+import zip from 'lodash/zip';
+
+function definitionsFactory(definitionIds = [], diagramIds = []) {
+  const planeElement = zip(definitionIds, diagramIds).map(([definitionId, diagramId]) => ({
+    id: diagramId,
+    bpmnElement: { id: definitionId },
+    get(key) {
+      return this[key];
+    },
+  }));
+
+  return {
+    diagrams: [{
+      plane: {
+        planeElement,
+        get(key) {
+          return this[key];
+        },
+      },
+    }],
+  };
+}
+
+describe('NodeIdGenerator', () => {
+  it.each([['definition', 0], ['diagram', 1]])('should generate unique %s IDs', (name, idIndex) => {
+    const generator = new NodeIdGenerator(definitionsFactory());
+    const ids = [];
+    const numberOfIdsToGenerate = 10;
+
+    for (let i = 0; i < numberOfIdsToGenerate; i++) {
+      ids.push(generator.generate()[idIndex]);
+    }
+
+    while (ids.length > 0) {
+      expect(ids).not.toContain(ids.pop());
+    }
+  });
+
+  it('should not generate definition IDs that already exist in definitions', () => {
+    const duplicateGenerator = new NodeIdGenerator(definitionsFactory());
+    const definitionIds = [
+      duplicateGenerator.generate()[0],
+      duplicateGenerator.generate()[0],
+      duplicateGenerator.generate()[0],
+    ];
+    const generator = new NodeIdGenerator(definitionsFactory(definitionIds));
+
+    const newDefinitionIds = [
+      generator.generate()[0],
+      generator.generate()[0],
+      generator.generate()[0],
+      generator.generate()[0],
+    ];
+
+    while (newDefinitionIds.length > 0) {
+      expect(definitionIds).not.toContain(newDefinitionIds.pop());
+    }
+  });
+
+  it('should not generate diagram IDs that already exist in definitions', () => {
+    const duplicateGenerator = new NodeIdGenerator(definitionsFactory());
+    const diagramIds = [
+      duplicateGenerator.generate()[1],
+      duplicateGenerator.generate()[1],
+      duplicateGenerator.generate()[1],
+    ];
+    const generator = new NodeIdGenerator(definitionsFactory([], diagramIds));
+
+    const newDefinitionIds = [
+      generator.generate()[1],
+      generator.generate()[1],
+      generator.generate()[1],
+      generator.generate()[1],
+    ];
+
+    while (newDefinitionIds.length > 0) {
+      expect(diagramIds).not.toContain(newDefinitionIds.pop());
+    }
+  });
+});
diff --git a/tests/unit/nodeManager.spec.js b/tests/unit/nodeManager.spec.js
index 66f1ba84..ab9d275e 100644
--- a/tests/unit/nodeManager.spec.js
+++ b/tests/unit/nodeManager.spec.js
@@ -4,17 +4,22 @@ 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', () => {
+    it('should not change node ID if it already has an ID set', () => {
       const nodeId = 'foobar';
+      const newNodeId = 'baz';
+      const diagramId = 'diagram_id';
       const node = {
         definition: { id: nodeId },
         diagram: {},
       };
+      const nodeIdGenerator = {
+        generate: () => ([newNodeId, diagramId]),
+      };
 
-      addIdToNodeAndSetUpDiagramReference(node);
+      addIdToNodeAndSetUpDiagramReference(node, nodeIdGenerator);
 
       expect(node.definition.id).toBe(nodeId);
-      expect(node.diagram.id).toBe(`${nodeId}_di`);
+      expect(node.diagram.id).toBe(diagramId);
       expect(node.diagram.bpmnElement).toBe(node.definition);
     });
 
@@ -22,13 +27,17 @@ describe('nodeManager', () => {
       const node = {
         definition: { id: 123 },
       };
+      const nodeIdGenerator = {
+        generate: () => ([]),
+      };
 
-      expect(() => addIdToNodeAndSetUpDiagramReference(node)).not.toThrow();
+      expect(() => addIdToNodeAndSetUpDiagramReference(node, nodeIdGenerator)).not.toThrow();
     });
 
     it('should use ID generator if definition does not have ID', () => {
       const nodeId = 'foobar';
-      const nodeIdGenerator = { generateUniqueNodeId: () => nodeId };
+      const diagramId = 'baz';
+      const nodeIdGenerator = { generate: () => [nodeId, diagramId] };
       const node = {
         definition: {},
         diagram: {},
@@ -37,7 +46,7 @@ describe('nodeManager', () => {
       addIdToNodeAndSetUpDiagramReference(node, nodeIdGenerator);
 
       expect(node.definition.id).toBe(nodeId);
-      expect(node.diagram.id).toBe(`${nodeId}_di`);
+      expect(node.diagram.id).toBe(diagramId);
     });
   });