Deleting messages and changing message properties was not possible if…

… correlation key reused between messages (#125)

* avoid error when message ref doesn't exist on correlation property (rare, but ran into it).  When deleting an object with a message, be aware that you might be deeply nested in other things.

* MatchingConditionArray, TaskEventMessageProvider.js - fixing the ids of div elements to make them easier to find in tests.

MessageHelpers - And added a deleteMessage function that will remove a message AND it's correlation properties. (The reason I came here)

MessageHelpers, MessageInterceptor.js - fixing the misspelled name of syncCorrelationProperties.

MessageCorrSpec tests that when you update a message through a message editor, it will correctly update the correlation properties, and not leave invalid properties lying around.

MessageSelect - will now remove the old message definition (including correlation properties), and replace it with the new message definition when a message is returned from the message editor.  As we do this when we receive a new message,there is no need for the cleanupOldMessage function when selecting a different message.

MessageSpec - a hell of a time with this.  "spiffExtensionOptions" is a constant that I could not seem to clear out between differest spec file tests.  I finally resorted to creating a "clearMessages" function that would force the list of messages to get cleared out when needed.

Co-authored-by: burnettk
Co-authored-by: jasquat

* forgot a test file in last commit.

* fixing a minor error.

* Update test/spec/MessagesSpec.js

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Update test/spec/MessagesSpec.js

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

---------

Co-authored-by: Kevin Burnett <18027+burnettk@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

app/app.js

@@ -303,6 +303,16 @@ bpmnModeler.on('import.parse.complete', (event) => {
 
 bpmnModeler.importXML(diagramXML).then(() => {});
 
+// Use this to simulate an add/update from the message editor.
+// bpmnModeler.on('spiff.message.edit', (event) => {
+//   event.eventBus.fire('spiff.add_message.returned', {
+//     elementId: "ActivityA",
+//     name: "messageA",
+//     correlation_properties:
+//       { "new_name": { retrieval_expression: "new_exp" }}
+//   });
+// });
+
 // This handles the download and upload buttons - it isn't specific to
 // the BPMN modeler or these extensions, just a quick way to allow you to
 // create and save files, so keeping it outside the example.

app/spiffworkflow/messages/MessageHelpers.js

@@ -182,7 +182,6 @@ export function findCorrelationPropertiesByMessage(element) {
 
   const { businessObject } = element;
   const root = getRoot(businessObject);
-  const correlationProperties = [];
 
   if (isMessageEvent(element)) {
     if (
@@ -197,27 +196,29 @@ export function findCorrelationPropertiesByMessage(element) {
     if (!businessObject.messageRef) return;
     messageId = businessObject.messageRef.id;
   }
+  return findCorrelationPropertiesByMessageId(messageId, root)
+}
 
-  if (isIterable(root.rootElements)) {
-    for (const rootElement of root.rootElements) {
-      if (rootElement.$type === 'bpmn:CorrelationProperty') {
-        rootElement.correlationPropertyRetrievalExpression =
-          rootElement.correlationPropertyRetrievalExpression
-            ? rootElement.correlationPropertyRetrievalExpression
-            : [];
-        const existingExpressionIndex =
-          rootElement.correlationPropertyRetrievalExpression.findIndex(
-            (retrievalExpr) =>
-              retrievalExpr.messageRef &&
-              retrievalExpr.messageRef.id === messageId,
-          );
-        existingExpressionIndex !== -1
-          ? correlationProperties.push(rootElement)
-          : null;
-      }
+export function findCorrelationPropertiesByMessageId(messageId, root) {
+  const correlationProperties = [];
+
+  for (const rootElement of root.rootElements) {
+    if (rootElement.$type === 'bpmn:CorrelationProperty') {
+      rootElement.correlationPropertyRetrievalExpression =
+        rootElement.correlationPropertyRetrievalExpression
+          ? rootElement.correlationPropertyRetrievalExpression
+          : [];
+      const existingExpressionIndex =
+        rootElement.correlationPropertyRetrievalExpression.findIndex(
+          (retrievalExpr) =>
+            retrievalExpr.messageRef &&
+            retrievalExpr.messageRef.id === messageId,
+        );
+      existingExpressionIndex !== -1
+        ? correlationProperties.push(rootElement)
+        : null;
     }
   }
-
   return correlationProperties;
 }
 
@@ -819,7 +820,7 @@ function findOrCreateMainCorrelationKey(definitions, bpmnFactory, moddle) {
   return mainCorrelationKey;
 }
 
-export function synCorrleationProperties(
+export function syncCorrelationProperties(
   element,
   definitions,
   moddle,
@@ -832,11 +833,9 @@ export function synCorrleationProperties(
   for (let cProperty of correlationProps) {
     let isUsed = false;
     for (const cpExpression of cProperty.correlationPropertyRetrievalExpression) {
-      const msgRef = findMessageElement(
-        businessObject,
-        cpExpression.messageRef.id,
-        definitions,
-      );
+      const msgRef = cpExpression.messageRef
+        ? findMessageElement(businessObject, cpExpression.messageRef.id, definitions)
+        : undefined;
       isUsed =
         msgRef &&
         msgObject &&
@@ -878,3 +877,31 @@ export function synCorrleationProperties(
     }
   }
 }
+
+export function deleteMessage(definitions, messageId) {
+  const rootElements = definitions.rootElements;
+
+  // Delete correlation retrieval expressions (Which live outside the message)
+  const corProps = findCorrelationPropertiesByMessage
+
+  // Delete the message object
+  const index = rootElements.findIndex(
+    (element) =>
+      element.$type === 'bpmn:Message' && element.id === messageId
+  );
+  if (rootElements && index !== -1) {
+    rootElements.splice(index, 1);
+    definitions.rootElements = rootElements;
+  }
+
+  // Delete retrieval expressions related to message
+  const props = findCorrelationPropertiesByMessageId(messageId, definitions)
+  for (let prop of props) {
+    let propIndex = prop.correlationPropertyRetrievalExpression.findIndex(
+      (retrievalExpr) =>
+        retrievalExpr.messageRef &&
+        retrievalExpr.messageRef.id === messageId,
+    );
+    prop.correlationPropertyRetrievalExpression.splice(propIndex, 1);
+  }
+}

app/spiffworkflow/messages/MessageInterceptor.js

@@ -1,5 +1,5 @@
 import CommandInterceptor from 'diagram-js/lib/command/CommandInterceptor';
-import { isMessageElement, isMessageRefUsed, setParentCorrelationKeys, synCorrleationProperties } from './MessageHelpers';
+import { isMessageElement, isMessageRefUsed, setParentCorrelationKeys, syncCorrelationProperties, getRoot} from './MessageHelpers';
 
 const HIGH_PRIORITY = 90500;
 
@@ -16,8 +16,8 @@ export default class MessageInterceptor extends CommandInterceptor {
             if (isMessageElement(shape)) {
 
                 let oldMessageRef = (businessObject.eventDefinitions) ? businessObject.eventDefinitions[0].messageRef : businessObject.messageRef;
-                
-                let definitions = rootElement.businessObject.$parent;
+
+                let definitions = getRoot(rootElement, moddle)
                 if (!definitions.get('rootElements')) {
                     definitions.set('rootElements', []);
                 }
@@ -38,7 +38,7 @@ export default class MessageInterceptor extends CommandInterceptor {
                     }
 
                     // Automatic deletion of previous message correlation properties
-                    synCorrleationProperties(shape, definitions, moddle);
+                    syncCorrelationProperties(shape, definitions, moddle);
                 }
 
                 // Update Correlation key if Process has collaboration

app/spiffworkflow/messages/propertiesPanel/elementLevelProvider/MatchingConditionArray.js

@@ -26,7 +26,7 @@ export function MatchingCorrelationEntries(props) {
     const entries = (correlationPropertyArray && correlationPropertyArray.length !== 0) ? correlationPropertyArray.map(
         (correlationPropertyModdleElement, index) => {
             return {
-                id: `${idPrefix}-correlation-property-name`,
+                id: `${idPrefix}-correlation-property-`,
                 component: MatchingConditionTextField,
                 correlationPropertyModdleElement,
                 element,

app/spiffworkflow/messages/propertiesPanel/elementLevelProvider/MessageSelect.jsx

@@ -10,7 +10,8 @@ import {
   isMessageEvent,
   isMessageRefUsed,
   setParentCorrelationKeys,
-  synCorrleationProperties,
+  syncCorrelationProperties,
+  deleteMessage
 } from '../../MessageHelpers';
 import { SPIFF_ADD_MESSAGE_RETURNED_EVENT } from '../../../constants';
 
@@ -80,8 +81,7 @@ export function MessageSelect(props) {
     }
 
     if (oldMessageRef) {
-      cleanupOldMessage(definitions, oldMessageRef.id);
-      synCorrleationProperties(element, definitions, moddle, messageObject);
+      syncCorrelationProperties(element, definitions, moddle, messageObject);
     }
 
     try {
@@ -111,6 +111,16 @@ export function MessageSelect(props) {
       correlation_properties: cProperties,
     };
 
+    // Delete the original message object if one exists, so we can replace it with the new definition.
+    const { businessObject } = element;
+    const definitions = getRoot(businessObject);
+    let oldMessage = findMessageById(definitions, newMsg.identifier);
+    if(oldMessage) {
+      deleteMessage(definitions, oldMessage.id);
+    }
+
+
+    // Update the list of options to display
     spiffExtensionOptions['spiff.messages'] =
       Array.isArray(spiffExtensionOptions['spiff.messages']) &&
       spiffExtensionOptions['spiff.messages']
@@ -243,17 +253,3 @@ function findMessageObject(messageId) {
     return null;
   }
 }
-
-function cleanupOldMessage(definitions, oldMessageId) {
-  if (!isMessageRefUsed(definitions, oldMessageId)) {
-    const rootElements = definitions.rootElements;
-    const oldMessageIndex = rootElements.findIndex(
-      (element) =>
-        element.$type === 'bpmn:Message' && element.id === oldMessageId
-    );
-    if (rootElements && oldMessageIndex !== -1) {
-      rootElements.splice(oldMessageIndex, 1);
-      definitions.rootElements = rootElements;
-    }
-  }
-}

app/spiffworkflow/messages/propertiesPanel/elementLevelProvider/TaskEventMessageProvider.js

@@ -142,11 +142,13 @@ export function createMessageGroup(
 
   // Adding Correlation Conditions Section
   const isMatchingCorrelation = businessObject.get('spiffworkflow:isMatchingCorrelation');
+  const id = businessObject.get('messageRef')?.id ?? "undefined";
   if (isMatchingCorrelation && isMatchingCorrelation !== "false" && canReceiveMessage(element)) {
     results.push({
       id: "correlationConditions",
       label: translate('Matching Conditions'),
       ...MatchingCorrelationEntries({
+        idPrefix: id,
         element,
         moddle,
         commandStack,

test/spec/MessagesCorrSpec.js

@@ -0,0 +1,92 @@
+import TestContainer from 'mocha-test-container-support';
+import {
+  query as domQuery,
+  queryAll as domQueryAll,
+} from 'min-dom';
+import {
+  BpmnPropertiesPanelModule,
+  BpmnPropertiesProviderModule,
+} from 'bpmn-js-properties-panel';
+import {
+  bootstrapPropertiesPanel,
+  expectSelected,
+  findEntry,
+  findGroupEntry,
+  findSelect,
+  findTextarea,
+  findInput,
+  pressButton,
+  findButtonByClass,
+  getPropertiesPanel,
+  changeInput,
+  findDivByClass
+} from './helpers';
+import spiffModdleExtension from '../../app/spiffworkflow/moddle/spiffworkflow.json';
+import messages from '../../app/spiffworkflow/messages';
+import { fireEvent } from '@testing-library/preact';
+import { getBpmnJS, inject } from 'bpmn-js/test/helper';
+import { findCorrelationProperties, findMessageModdleElements } from '../../app/spiffworkflow/messages/MessageHelpers';
+import {spiffExtensionOptions} from "../../app/spiffworkflow/extensions/propertiesPanel/SpiffExtensionSelect";
+
+describe('Multiple messages should work', function () {
+  const xml = require('./bpmn/two_messages.bpmn').default;
+  let container;
+
+  beforeEach(function () {
+    container = TestContainer.get(this);
+  });
+
+  beforeEach(
+    bootstrapPropertiesPanel(xml, {
+      container,
+      debounceInput: false,
+      additionalModules: [
+        messages,
+        BpmnPropertiesPanelModule,
+        BpmnPropertiesProviderModule,
+      ],
+      moddleExtensions: {
+        spiffworkflow: spiffModdleExtension,
+      },
+    })
+  );
+
+
+  const new_message_event = (eventBus) => {
+    eventBus.fire('spiff.add_message.returned', {
+      elementId: "ActivityA",
+      name: "messageA",
+      correlation_properties:
+        { "new_name": { retrieval_expression: "new_exp" }}
+    });
+  };
+
+
+  it('and it should be possible to change a correlation property name', async function () {
+    const modeler = getBpmnJS();
+
+    const sendShape = await expectSelected('ActivityA');
+    expect(sendShape, "Can't find Send Task").to.exist;
+
+    const oldName = "old_name"
+    const newName = "new_name"
+
+    const labels = domQueryAll(`.bio-properties-panel-label`, container);
+    const oldNameFound = Array.from(labels).some(label => label.textContent.includes(oldName));
+    expect(oldNameFound).to.be.true;
+
+    //Update message
+    new_message_event(modeler.get('eventBus'))
+    const sendShape2 = await expectSelected('ActivityA');
+
+    // The old name should no longer be there, but the new name should exist.
+    const labels2 = domQueryAll(`.bio-properties-panel-label`, container);
+    const oldNameFound2 = Array.from(labels2).some(label => label.textContent.includes(oldName));
+    expect(oldNameFound2).to.be.false;
+    const newNameFound2 = Array.from(labels2).some(label => label.textContent.includes(newName));
+    expect(newNameFound2).to.be.true;
+
+
+  });
+
+});