fix: set loop characteristics in a command

Closes #1960

Release-As: 14.0.1

Author: barmac

lib/features/popup-menu/ReplaceMenuProvider.js

@@ -15,19 +15,19 @@ import {
 import {
   forEach,
   filter,
-  isArray,
-  isUndefined
+  isArray
 } from 'min-dash';
 
 import * as replaceOptions from '../replace/ReplaceOptions';
 
 /**
- * @typedef {import('../features/BpmnFactory').default} BpmnFactory
+ * @typedef {import('../modeling/BpmnFactory').default} BpmnFactory
  * @typedef {import('diagram-js/lib/features/popup-menu/PopupMenu').default} PopupMenu
- * @typedef {import('../features/Modeling').default} Modeling
- * @typedef {import('../features/BpmnReplace').default} BpmnReplace
+ * @typedef {import('../modeling/Modeling').default} Modeling
+ * @typedef {import('../replace/BpmnReplace').default} BpmnReplace
  * @typedef {import('diagram-js/lib/features/Rules').default} Rules
  * @typedef {import('diagram-js/lib/i18n/translate/translate').default} Translate
+ * @typedef {import('../copy-paste/ModdleCopy').default} ModdleCopy
  *
  * @typedef {import('../../model/Types').Element} Element
  * @typedef {import('../../model/Types').Moddle} Moddle
@@ -54,10 +54,11 @@ import * as replaceOptions from '../replace/ReplaceOptions';
  * @param {BpmnReplace} bpmnReplace
  * @param {Rules} rules
  * @param {Translate} translate
+ * @param {ModdleCopy} moddleCopy
  */
 export default function ReplaceMenuProvider(
     bpmnFactory, popupMenu, modeling, moddle,
-    bpmnReplace, rules, translate) {
+    bpmnReplace, rules, translate, moddleCopy) {
 
   this._bpmnFactory = bpmnFactory;
   this._popupMenu = popupMenu;
@@ -66,6 +67,7 @@ export default function ReplaceMenuProvider(
   this._bpmnReplace = bpmnReplace;
   this._rules = rules;
   this._translate = translate;
+  this._moddleCopy = moddleCopy;
 
   this._register();
 }
@@ -77,7 +79,8 @@ ReplaceMenuProvider.$inject = [
   'moddle',
   'bpmnReplace',
   'rules',
-  'translate'
+  'translate',
+  'moddleCopy'
 ];
 
 ReplaceMenuProvider.prototype._register = function() {
@@ -461,18 +464,24 @@ ReplaceMenuProvider.prototype._getLoopCharacteristicsHeaderEntries = function(ta
   var translate = this._translate;
 
   function toggleLoopEntry(event, entry) {
-    var newLoopCharacteristics = getBusinessObject(target).loopCharacteristics;
 
+    // remove
     if (entry.active) {
-      newLoopCharacteristics = undefined;
-    } else {
-      if (isUndefined(entry.options.isSequential) || !newLoopCharacteristics
-      || !is(newLoopCharacteristics, entry.options.loopCharacteristics)) {
-        newLoopCharacteristics = self._moddle.create(entry.options.loopCharacteristics);
-      }
+      self._modeling.updateProperties(target, { loopCharacteristics: undefined });
+      return;
+    }
+
+    const currentLoopCharacteristics = target.businessObject.get('loopCharacteristics'),
+          newLoopCharacteristics = self._moddle.create(entry.options.loopCharacteristics);
 
-      newLoopCharacteristics.isSequential = entry.options.isSequential;
+    // copy old properties
+    if (currentLoopCharacteristics) {
+      self._moddleCopy.copyElement(currentLoopCharacteristics, newLoopCharacteristics);
     }
+
+    // update `isSequential` property
+    newLoopCharacteristics.set('isSequential', entry.options.isSequential);
+
     self._modeling.updateProperties(target, { loopCharacteristics: newLoopCharacteristics });
   }
 

test/fixtures/bpmn/draw/activity-markers-simple.bpmn

@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<bpmn2:definitions xmlns:bpmn2="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" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" id="_opd4cBZiEeWgh4rX9Ivzlg" targetNamespace="http://activiti.org/bpmn" exporter="Camunda Modeler" exporterVersion="1.0.0" xsi:schemaLocation="http://www.omg.org/spec/BPMN/20100524/MODEL BPMN20.xsd">
+<bpmn2:definitions xmlns:bpmn2="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" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:camunda="http://camunda.org/schema/1.0/bpmn" id="_opd4cBZiEeWgh4rX9Ivzlg" targetNamespace="http://activiti.org/bpmn" exporter="Camunda Modeler" exporterVersion="1.0.0" xsi:schemaLocation="http://www.omg.org/spec/BPMN/20100524/MODEL BPMN20.xsd">
   <bpmn2:process id="Process_1" isExecutable="false">
     <bpmn2:task id="ParallelTask" name="ParallelTask">
       <bpmn2:multiInstanceLoopCharacteristics camunda:collection="foo" camunda:elementVariable="bar">

test/spec/features/popup-menu/ReplaceMenuProviderSpec.js

@@ -779,6 +779,72 @@ describe('features/popup-menu - replace menu provider', function() {
       }));
     });
 
+
+    describe('integration', function() {
+
+      it('should toggle sequential -> undo to parallel', inject(function(elementRegistry, commandStack) {
+
+        // given
+        var task = elementRegistry.get('ParallelTask');
+
+        openPopup(task);
+
+        // when
+        triggerAction('toggle-sequential-mi');
+
+        commandStack.undo();
+
+        // then
+        const bo = getBusinessObject(task),
+              loopCharacteristics = bo.get('loopCharacteristics');
+
+        expect(is(loopCharacteristics, 'bpmn:MultiInstanceLoopCharacteristics')).to.be.true;
+        expect(loopCharacteristics.isSequential).to.be.false;
+      }));
+
+
+      it('should toggle parallel -> undo to parallel', inject(function(elementRegistry, commandStack) {
+
+        // given
+        var task = elementRegistry.get('ParallelTask');
+
+        openPopup(task);
+
+        // when
+        triggerAction('toggle-parallel-mi');
+
+        commandStack.undo();
+
+        // then
+        const bo = getBusinessObject(task),
+              loopCharacteristics = bo.get('loopCharacteristics');
+
+        expect(is(loopCharacteristics, 'bpmn:MultiInstanceLoopCharacteristics')).to.be.true;
+        expect(loopCharacteristics.isSequential).to.be.false;
+      }));
+
+
+      it('should toggle loop -> undo to parallel', inject(function(elementRegistry, commandStack) {
+
+        // given
+        var task = elementRegistry.get('ParallelTask');
+
+        openPopup(task);
+
+        // when
+        triggerAction('toggle-loop');
+
+        commandStack.undo();
+
+        // then
+        const bo = getBusinessObject(task),
+              loopCharacteristics = bo.get('loopCharacteristics');
+
+        expect(is(loopCharacteristics, 'bpmn:MultiInstanceLoopCharacteristics')).to.be.true;
+        expect(loopCharacteristics.isSequential).to.be.false;
+      }));
+    });
+
   });