Add error message on attempt to create circular references

[marthacryan]

Fixes #256 and #660. Adds an error message that appears when users try to create circular references. The error message is structured to provide flexibility for future validation needs - from PipelineEditor, use this.setState({ showValidationError: true, errorMessage: 'YOUR ERROR MESSAGE' }) to open the error message.

Notes

• Should it close on it's own after a certain timeout? How long?
• This will need to be revisited when the canvas version is updated because there is a handler that allows you to check edits before they're added to the canvas, which would be a cleaner way of doing this.

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

[ajbozarth]

A couple initial code comments, I'll be finding time to test this locally and do a more though code review soon

[kevin-bates]

I can't really comment on the UI changes, but think this is really great and elegantly done - nice work.

As for the error message timeout, I think forcing the user to dismiss is correct, but will also defer to the UX experts on that. Is the reason we don't just pop up a dialog because that isn't the "canvas way" in this particular scenario?

[vabarbosa]

@marthacryan i like this, a couple comments:

• on the idea of flexibility for future validation needs, should it be possible to allow the caller to specify an optional severity level (e.g., info, warning, etc) and if none is provided it defaults to error?

  this.setState({ showValidationError: true, errorMessage: 'YOUR ERROR MESSAGE', severity: 'error' })
  

• forcing the user to dismiss the alert is a good idea, but in conjunction to that i would also suggest if the user does not dismiss the alert but adds a new valid link or adds a new node to the canvas, i think the alert should at that point go away.

[ajbozarth]

I checked this out and tried it and it works great. I made a couple code comments below including one refactor idea.

I did notice in my testing that canvas prevents connecting a node to itself by default, but this doesn't show our error. Do we want to try to tie our new error messaging into that behavior as well for consistency?

Also a larger question: why did you use material-ui for this? I ran a quick check and it's not currently used in lab, lumino, or canvas.

[marthacryan]

Also a larger question: why did you use material-ui for this? I ran a quick check and it's not currently used in lab, lumino, or canvas.

@ajbozarth Someone in a jupyterlab community meeting mentioned using material-ui when jupyterlab doesn't have a feature already implemented, and I thought this sort of alert would be better than a dialog for what the error was, so I thought I'd give material-ui a try. It looked good and was relatively simple to add, so I went with it!

[ajbozarth]

Tested locally and everything looks good. A couple minor comments, once those are addressed this should be good to go.

[marthacryan]

Last commit moves the link validation code into beforeEditActionHandler because the updated version of canvas allows using that handler to verify an edit before going through with the edit. Doesn't change behavior of error handling or validation, just makes the code cleaner. @ajbozarth If you want to make sure that last commit looks good, that might be a good idea, otherwise @lresende this is ready to be merged

[ajbozarth]

Still looks good with the latest changes

[karlaspuldaro]

Very nice add-on :)

[vabarbosa]

looks good to me!

there is an edge case which i am not sure if it is worth addressing. if i have a saved pipeline which already has a circular reference when i open/load this pipeline the circular reference is not detected. if this scenario is valid then perhaps an initial check should be done when opening a pipeline?

[marthacryan]

@vabarbosa Good point! That kind of error might be better as a decoration though (it might be confusing if we go in and delete links then show an error right when users open the pipeline) - I could include it in #752 as part of the validation that I'm adding for nodes, but the one issue that comes to mind is that there isn't an existing tooltip for links, so I'm not sure how we'd indicate what the issue is with the link. Thoughts?

[vabarbosa]

@vabarbosa Good point! That kind of error might be better as a decoration though (it might be confusing if we go in and delete links then show an error right when users open the pipeline) - I could include it in #752 as part of the validation that I'm adding for nodes, but the one issue that comes to mind is that there isn't an existing tooltip for links, so I'm not sure how we'd indicate what the issue is with the link. Thoughts?

yeah, i don't believe deleting the link would be the way to go either. maybe showing the notification and also changing the link to red dashed line. for example

[marthacryan]

@vabarbosa I like that! I'll add that to #752 because it already has validation code that runs on startup.