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

Handle callback queries in conversations #23

Merged
merged 3 commits into from
Dec 22, 2020
Merged

Handle callback queries in conversations #23

merged 3 commits into from
Dec 22, 2020

Conversation

sergix44
Copy link
Collaborator

This, should partially fix the issue #22, BUT it's also true that if the cbquery match some listener, the conversation is discarded. As I written in the issue, if for example we want force the user to follow conversation/step by step wizard it can simply escape by typing any other command.
@cheeghi Feel free to edit this branch if you have a better solution 😄

@cheeghi
Copy link
Collaborator

cheeghi commented Dec 19, 2020

Hi, maybe I'm missing something, but I didn't get the reason to clear the conversation state if a callback query listener is found, we can call both the listeners and the conversation handler. Instead for text messages, you're proposing to prioritize the conversation instead of the listener, but this means that if someone in the middle of a conversation wants to escape and sends /mycommand, we should call the conversation handler and ignore the command listener. How botman deals with this? (I'm just wondering, to find the best solution)

@sergix44
Copy link
Collaborator Author

sergix44 commented Dec 19, 2020

Botman ignores other handlers if it's in the middle of a conversation, but you set some handlers that allows to "escape" if you are in a conversation: https://botman.io/2.0/conversations (Skip or Stop Conversations section). Maybe here, we can try to evaluate the conversation before calling a listener, and maybe add a parameter on the nextStep method like nextStep($callback, bool $skipListeners = false) that allow the next step to be called without searching for handlers. In this way we can enforce a step or keep the user free of interrupting a coversation when he want.
What do you think?

@cheeghi
Copy link
Collaborator

cheeghi commented Dec 19, 2020

It may be a good solution actually, I'm a bit worried about retrieving the conversation. Right now ListenerResolver::resolve() resolves the listeners looking inside the in-memory array ListenerCollector::$listeners, having to check inside the cache for possible conversation before returning the resolved listeners means waiting for the promise response that would make the whole method promise-based. So, it may be tricky, but we can see how it goes.

…n conversation step; now "resolve" method returns a promise whose solved value is the listeners array; moved conversation methods inside a new class "ConversationManager"
@cheeghi
Copy link
Collaborator

cheeghi commented Dec 20, 2020

I made the implementation, I also moved the conversation methods in a dedicated class. If you may take a look and give me a feedback I would really appreciate

@sergix44
Copy link
Collaborator Author

sergix44 commented Dec 22, 2020

Looks good! I didn't tested on a real world bot (that I am doing, as you can imagine from the pr 😃 ) but tests and logic seems fine

@cheeghi cheeghi merged commit c18356e into badfarm:develop Dec 22, 2020
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.

2 participants