Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): disallow transitions to same node (resolve #900) #962

Merged
merged 1 commit into from Oct 1, 2018

Conversation

epaminond
Copy link
Contributor

No description provided.

@epaminond epaminond self-assigned this Sep 30, 2018
@dmk23
Copy link

dmk23 commented Sep 30, 2018

@epaminond Why should this be disallowed rather than fixed?

There is an obvious use case for this - any user input that must get validated and might be re-tried

The usage workaround (of adding separate nodes for input capture vs. validation) makes flow diagrams clunkier... I'd much rather prefer to have fewer nodes with stronger functionality

I suppose the real fix should involve some way to kick the dialog back to onReceive?

@epaminond
Copy link
Contributor Author

@dmk23 ,

  1. I can see this is actually disallowed
  2. This transition looks ugly and you won't be able to select/delete it from the UI
  3. This way it's easy to cause infinite loop

@dmk23
Copy link

dmk23 commented Oct 1, 2018

  1. That's why validating/retrying user input is a problem now
  2. Most transitions (and node boxes) look ugly now, Flow UX will need major improvements in any case (yes, I have gotten this exact feedback showing Botpress UX to potential non-technical users)
  3. Indeed, and that's why there should be clear warnings. Also, same kind of loops are possible with any workarounds (e.g. using 2 nodes for this kind of validation workflows)

@epaminond
Copy link
Contributor Author

Merging this for now to keep things consistent. At a later point we may always return and implement self-pointing feature.

@epaminond epaminond merged commit cdfcd26 into master Oct 1, 2018
@epaminond epaminond deleted the disallow-self-transitioning branch October 6, 2018 22:40
rndlaine added a commit that referenced this pull request Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants