fix(id): unclaim id on element remove

Closes #421

Fixing incorrect test+implementation for ID property update

lib/features/modeling/Modeling.js

@@ -9,7 +9,8 @@ var UpdatePropertiesHandler = require('./cmd/UpdatePropertiesHandler'),
     AddLaneHandler = require('./cmd/AddLaneHandler'),
     SplitLaneHandler = require('./cmd/SplitLaneHandler'),
     ResizeLaneHandler = require('./cmd/ResizeLaneHandler'),
-    UpdateFlowNodeRefsHandler = require('./cmd/UpdateFlowNodeRefsHandler');
+    UpdateFlowNodeRefsHandler = require('./cmd/UpdateFlowNodeRefsHandler'),
+    IdClaimHandler = require('./cmd/IdClaimHandler');
 
 
 /**
@@ -42,6 +43,7 @@ Modeling.prototype.getHandlers = function() {
   handlers['lane.resize'] = ResizeLaneHandler;
   handlers['lane.split'] = SplitLaneHandler;
   handlers['lane.updateRefs'] = UpdateFlowNodeRefsHandler;
+  handlers['id.updateClaim'] = IdClaimHandler;
 
   return handlers;
 };
@@ -145,3 +147,20 @@ Modeling.prototype.makeProcess = function() {
 
   this._commandStack.execute('canvas.updateRoot', context);
 };
+
+
+Modeling.prototype.claimId = function(id, moddleElement) {
+  this._commandStack.execute('id.updateClaim', {
+    id: id,
+    element: moddleElement,
+    claiming: true
+  });
+};
+
+
+Modeling.prototype.unclaimId = function(id, moddleElement) {
+  this._commandStack.execute('id.updateClaim', {
+    id: id,
+    element: moddleElement
+  });
+};

lib/features/modeling/behavior/UnclaimIdBehavior.js

@@ -0,0 +1,28 @@
+'use strict';
+
+var forEach = require('lodash/collection/forEach');
+
+var inherits = require('inherits');
+
+var CommandInterceptor = require('diagram-js/lib/command/CommandInterceptor');
+
+function UnclaimIdBehavior(eventBus, modeling) {
+
+  CommandInterceptor.call(this, eventBus);
+
+  this.preExecute('elements.delete', function(event) {
+    var context = event.context,
+        elements = context.elements;
+
+    forEach(elements, function(element) {
+      modeling.unclaimId(element.businessObject.id, element.businessObject);
+    });
+
+  });
+}
+
+inherits(UnclaimIdBehavior, CommandInterceptor);
+
+UnclaimIdBehavior.$inject = [ 'eventBus', 'modeling' ];
+
+module.exports = UnclaimIdBehavior;

lib/features/modeling/behavior/index.js

@@ -15,7 +15,8 @@ module.exports = {
     'resizeLaneBehavior',
     'unsetDefaultFlowBehavior',
     'updateFlowNodeRefsBehavior',
-    'removeElementBehavior'
+    'removeElementBehavior',
+    'unclaimIdBehavior'
   ],
   appendBehavior: [ 'type', require('./AppendBehavior') ],
   createBoundaryEventBehavior: [ 'type', require('./CreateBoundaryEventBehavior') ],
@@ -32,5 +33,6 @@ module.exports = {
   resizeLaneBehavior: [ 'type', require('./ResizeLaneBehavior') ],
   unsetDefaultFlowBehavior: [ 'type', require('./UnsetDefaultFlowBehavior') ],
   updateFlowNodeRefsBehavior: [ 'type', require('./UpdateFlowNodeRefsBehavior') ],
-  removeElementBehavior: [ 'type', require('./RemoveElementBehavior') ]
+  removeElementBehavior: [ 'type', require('./RemoveElementBehavior') ],
+  unclaimIdBehavior: [ 'type', require('./UnclaimIdBehavior') ]
 };

lib/features/modeling/cmd/IdClaimHandler.js

@@ -0,0 +1,41 @@
+'use strict';
+
+
+function IdClaimHandler(moddle) {
+  this._moddle = moddle;
+}
+
+IdClaimHandler.$inject = [ 'moddle' ];
+
+module.exports = IdClaimHandler;
+
+
+IdClaimHandler.prototype.execute = function(context) {
+  var ids = this._moddle.ids,
+      id = context.id,
+      element = context.element,
+      claiming = context.claiming;
+
+  if (claiming){
+    ids.claim(id, element);
+  } else {
+    ids.unclaim(id);
+  }
+};
+
+/**
+ * Command revert implementation.
+ */
+IdClaimHandler.prototype.revert = function(context) {
+  var ids = this._moddle.ids,
+      id = context.id,
+      element = context.element,
+      claiming = context.claiming;
+
+  if (claiming){
+    ids.unclaim(id);
+  } else {
+    ids.claim(id, element);
+  }
+};
+

lib/features/modeling/cmd/UpdatePropertiesHandler.js

@@ -65,6 +65,8 @@ UpdatePropertiesHandler.prototype.execute = function(context) {
     ids.unclaim(businessObject[ID]);
 
     elementRegistry.updateId(element, properties[ID]);
+
+    ids.claim(properties[ID], businessObject);
   }
 
   // correctly indicate visual changes on default flow updates
@@ -120,6 +122,8 @@ UpdatePropertiesHandler.prototype.revert = function(context) {
     ids.unclaim(properties[ID]);
 
     elementRegistry.updateId(element, oldProperties[ID]);
+
+    ids.claim(oldProperties[ID], businessObject);
   }
 
   return context.changed;

test/spec/features/modeling/IdClaimSpec.bpmn

@@ -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:di="http://www.omg.org/spec/DD/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" id="Definitions_1" targetNamespace="http://bpmn.io/schema/bpmn">
+  <bpmn:process id="Process_1" isExecutable="false">
+    <bpmn:startEvent id="StartEvent_2" />
+  </bpmn:process>
+  <bpmndi:BPMNDiagram id="BPMNDiagram_1">
+    <bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_1">
+      <bpmndi:BPMNShape id="_BPMNShape_StartEvent_2" bpmnElement="StartEvent_2">
+        <dc:Bounds x="173" y="102" width="36" height="36" />
+      </bpmndi:BPMNShape>
+    </bpmndi:BPMNPlane>
+  </bpmndi:BPMNDiagram>
+</bpmn:definitions>

test/spec/features/modeling/IdClaimSpec.js

@@ -0,0 +1,50 @@
+'use strict';
+
+/* global bootstrapModeler, inject */
+
+var modelingModule = require('../../../../lib/features/modeling'),
+    coreModule = require('../../../../lib/core');
+
+
+describe('features/modeling - id claim management', function() {
+
+  var testModules = [ coreModule, modelingModule ];
+
+  var processDiagramXML = require('./IdClaimSpec.bpmn');
+
+  beforeEach(bootstrapModeler(processDiagramXML, { modules: testModules }));
+
+  var element, moddleElement, id;
+
+  beforeEach(inject(function(elementRegistry, moddle) {
+    id = 'StartEvent_2';
+    element = elementRegistry.get(id);
+    moddleElement = element.businessObject;
+  }));
+
+
+  describe('unclaim', function() {
+
+    it('should unclaim id when removing element', inject(function(modeling, moddle) {
+      // when
+      modeling.removeElements([ element ]);
+
+      // then
+      expect(moddle.ids.assigned(id)).to.be.false;
+    }));
+
+
+    it('should revert unclaim action on restoring element', inject(function(modeling, moddle, commandStack) {
+      // given
+      modeling.removeElements([ element ]);
+
+      // when
+      commandStack.undo();
+
+      // then
+      expect(moddle.ids.assigned(id)).to.eql(moddleElement);
+    }));
+
+  });
+
+});

test/spec/features/modeling/UpdatePropertiesSpec.js

@@ -146,7 +146,7 @@ describe('features/modeling - update properties', function() {
       modeling.updateProperties(flowConnection, { id: 'FOO_BAR' });
 
       // then
-      expect(ids.assigned('FOO_BAR')).to.exist;
+      expect(ids.assigned('FOO_BAR')).to.eql(flowConnection.businessObject);
       expect(ids.assigned('SequenceFlow_1')).to.be.false;
 
       expect(flowConnection.businessObject.id).to.equal('FOO_BAR');
@@ -275,7 +275,7 @@ describe('features/modeling - update properties', function() {
 
       // then
       expect(ids.assigned('FOO_BAR')).to.be.false;
-      expect(ids.assigned('SequenceFlow_1')).to.exist;
+      expect(ids.assigned('SequenceFlow_1')).to.eql(flowConnection.businessObject);
 
       expect(flowConnection.businessObject.id).to.equal('SequenceFlow_1');
       expect(flowConnection.id).to.equal('SequenceFlow_1');
@@ -409,7 +409,7 @@ describe('features/modeling - update properties', function() {
       modeling.updateProperties(flowConnection, { id: 'SequenceFlow_1' });
 
       // then
-      expect(ids.assigned('SequenceFlow_1')).to.exist;
+      expect(ids.assigned('SequenceFlow_1')).to.eql(flowConnection.businessObject);
 
       expect(flowConnection.businessObject.id).to.equal('SequenceFlow_1');
     }));