sartography / spiff-arena

SpiffWorkflow is a software development platform for building, running, and monitoring executable diagrams
https://www.spiffworkflow.org/
GNU Lesser General Public License v2.1
62 stars 42 forks source link

Issues with Messages #1626

Closed danfunk closed 2 weeks ago

danfunk commented 3 months ago

Encountered the following problems the first time I tried to use the message UI (I didn't go looking for these problems, I just hit them trying to create the video)

Items I would call critical

1) Close button deletes all entered information about message and just closes window without notification. It took several minutes for me to realize I hadn't hit save. This alone makes the new "user friendly messages" actually less user friendly.

> -- Changed close text to "Close without saving" and preventing closing the modal by clicking outside of it

1) When an invalid json schema is provided, you get the following error message and lose all of your work: image You have to start all over again.

> -- Added rsjf validation to make sure it is valid json

1) Frequent errors about "message_name" must be a string, when it is clearly a string - maybe it doesn't like underscores? image

> -- Cannot repro but I did add pattern and validation message to make sure its a valid format - did same with Location

1) Dropdown does not populate with an option after creating a message - at least not always. The first message required a hard refresh. Perhaps it refreshes if you click off and back on again - but very disorienting - I thought I'd failed to click save again.

> -- Fixed with https://github.com/sartography/spiff-arena/commit/c9df8bec8bd268e15ea89909e70f2d239ce1c214

1) On running for the first time, I get this error message: image

> -- Fixed in SpiffWorkflow

1) A missing payload in a send message fails with the following error that does not provide an explanation image

> -- This bug exists in the current system - recommend deferring it

1) Correlation keys seem to be getting set under the message name or under some default "Main Correlation Key" heading that is causing the return message to fail. I don't know what the thought process behind this is - maybe we can talk about it tomorrow. image

> -- Fixed with new bpmn-js-spiffworkflow

Dreams of a better future that I don't expect to see fixed in this ticket

1) Any way we could have even a basic json editor for the schema? 1) The ability to save and reuse a schema comes up for me frequently in trying to build these examples. I'd love to re-use the schema for the form, the send, and the receive messages. Having all these connected would make it far easier to explain to people.

madhurrya commented 3 months ago

~EDIT: Should be fixed with https://github.com/sartography/spiff-arena/commit/c7c04b8c1538c82ce0872ce13ed017dc14f03258~

~8. I was typing things in these 2 boxes and then went to another tab to copy something and when I came back these fields are empty. Same happens for the correlation fields~ image

madhurrya commented 3 months ago

EDIT: I believe this is a duplicate of number 4

  1. Even if I save the data, when I click on the Message editor again Json schema and Message Name is empty. Whether I use the Close button or the x to close or even if I click outside the popup same happens. image

After opening the Message Editor again image

UPDATE : later realized that I have to select the message here first. It is not auto selected after I add it. image

madhurrya commented 3 months ago

EDIT: Seems fixed now -- jasquat

  1. I added a matching condition and saved it. image

But when I open the diagram next time the matching condition is missing. image

madhurrya commented 3 months ago

~EDIT: Seems fixed now -- jasquat~

~11 The text here seems to be getting cut~ image

jasquat commented 3 months ago

@theaubmov for number 10 above, I'm not having an issue setting the matching condition to true however, if I uncheck it then the xml sets it to "false" but the checkbox is always checked from that point on. The conditional may need to check for strings as well rather than a boolean only.

Also, is there anything you can do for number 11 with the cut off text?

jasquat commented 3 months ago

@madhurrya is number 8 still happening? I cannot reproduce that.

madhurrya commented 3 months ago

@jasquat yes it still happens. https://drive.google.com/file/d/1sEXf22IK5QakN9Na28rnljfNcZ2tnDWi/view?usp=sharing Please try with a new message

jasquat commented 3 months ago

Okay, I think number 8 is mostly fixed although I do think the state does get in an odd state when moving between tabs however this state isn't always reflected within the form. Will have to look more into it.

jasquat commented 3 months ago

To sum up the state of this. All issues seem to have been addressed except for issues 5-7. I'm not sure what the best way to handle those errors in 5 and 6 are and I do not fully understand issue 7.

jasquat commented 3 months ago

@danfunk for number 7 do you mean the fact that it sets a correlation key? In the xml we fabricate correlation keys and assign the property to it. This way the xml remains to spec. It shows up in that table since the rest of the system is expecting to use correlation keys throughout which helps to keep backwards compatibility with older messages. I'm not sure we really need it but I believe other parts of the system expect it.

madhurrya commented 3 months ago

~12. The variable name is still missing from the old models~

Image

madhurrya commented 3 months ago

~13. It might be better to move the Save button before the Close button and move the close button to the right a bit~

Image

madhurrya commented 3 months ago
  1. If I define the payload in the Prescript of the Send task, it doesn't recognize it. But if I define it in a script task before that then it works. Is that the expected behaviour?

Image

Image

madhurrya commented 3 months ago
  1. When I try to add a matching condition it says 'No matching conditions can be established since the selected message has no correlation properties'. But it has a correlation property.

Model : https://qa5.spiffcrm.com/editor/process-models/misc:test:madhurya-testing:message-testing:simple-receive-message/files/simple-receive-message.bpmn

Image

Image

madhurrya commented 3 months ago

~16. I got a database integrity error when trying to change the 'Retrieval expression' here from name to age. But in the other model and this ones where I use that message it has been automatically updated to age even though I got that error. I don't have a screenshot of that error, But I think it's the same error as in the error 17 below.~

Image

~17 It seems I can't create any new messages in new models, as I keep getting this error~

Image

jasquat commented 3 months ago

For issue: 12 - @theaubmov - is this an issue with bpmn-js-spiiffworkflow. It looks like the variable names used to be in a different place where it was on the message itself and now it's on a definition. Can it look into both and move appropriately? 13 - yep agreed. there are other changes we want to do with this editor and we can look into at that time. 14 - that is a good question. @essweine do you know if something like that works in SpiffWorkflow? 15 - @theaubmov bpmn-js-spiffowrkflow issue? 16 - @madhurrya if you get it again let us know 17 - I can't see the error since i do not have access to the screenshot and it's pixelated in its current form.

Also, the message improvements branch has been merged to main now so maybe let's create new tickets for new issues instead of using this one. After these last few are addressed we should close this ticket.

madhurrya commented 3 months ago

@jasquat I updated the message for 17. As I can remember I got the same error in 16. I think after that I keep getting the same error when I try to add a new message. If you try to save a new message, you might also get the same error. I can't do anything due to this error.

jasquat commented 3 months ago

oohh I think i understand number 17. when you changed the retrieval expression, instead of changing the name it added a new entry so the original message looks like:

        "Madhu_message": {
            "correlation_properties": {
                "username": {
                    "retrieval_expressions": [
                        "name",
                        "age"
                    ]
                }
            },
            "schema": {}
        },

So when you save the message it's also trying to save all messages associated with the process group - which includes the one above - and that causes an error since it's trying to create new entries for each retrieval expression but the way the db is set up where there is a uniqueness constraint between correlation property identifier and message identitfier, only one can succeed.

@madhurrya i think you can continue testing, just not in that one process group and you cannot update retrieval expressions at the moment.

madhurrya commented 3 months ago

As per Jasosn's comment above, created this new ticket for new issues https://github.com/sartography/spiff-arena/issues/1716

theaubmov commented 3 months ago

@jasquat @madhurrya In issue 15, the correlation properties are present in the process group, but the process XML is not updated. This is the reason why the XML does not have any correlation properties, which explains why the matching conditions show nothing. The issue can be resolved by opening the message editor and clicking on "save," which will update the XML with new values. Also we can consider If the XML and group values are not synchronized with xml, we can consider displaying a message in the message editor to inform the user to click "save" again.

madhurrya commented 3 months ago

@theaubmov I tried by saving again and then it was able to add the Matching condition. But when I saved the model and came back, then again it is missing. image

madhurrya commented 2 months ago

The last issue I mentioned above is still there. When I come back to a task later and check the matching condition, it shows as no matching condition. (It is there in the xml) image

madhurrya commented 2 months ago

I am still noticing the issue no. 15 for this model. Even though it has a correlation it doesn't show the option to add the Matching condition. (But for some other models/messages it worked) (In this model I changed the message from the one I initially had, not sure whether it's the reason for that) https://qa5.spiffcrm.com/editor/process-models/misc:test:madhurya-testing:message-testing4:receive-message/files/receive-message.bpmn

madhurrya commented 2 months ago

~Issue no 16 is still there. Got this error when I changed the 'Retrieval expression' from name to age and saved~ image

image

madhurrya commented 2 months ago

~Issue No 12 is also not fixed. Still the Variable name is missing in old models.~

jasquat commented 2 months ago

Latest fixes have been deployed to qa5.

madhurrya commented 2 months ago

Closing this ticket as most of these issues and fixed and new issues have been added for existing ones.