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

Forward bot messages received from /tracker/events route to output channel #6273

Merged
merged 33 commits into from
Sep 24, 2020
Merged

Forward bot messages received from /tracker/events route to output channel #6273

merged 33 commits into from
Sep 24, 2020

Conversation

pheel
Copy link
Contributor

@pheel pheel commented Jul 24, 2020

Proposed changes:

  • Events posted to /conversations/<conversation_id>/tracker/events do not trigger a new prediction, and that is fine. But a use case for this route is not currently supported: it could be used to push bot messages back to an output channel for the user to see. This is a minimal PR to add this feature. It adds the output_channel query param to the route.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@sara-tagger sara-tagger requested a review from Ghostvv July 27, 2020 06:00
@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @Ghostvv will take a look at it as soon as possible ✨

@federicotdn
Copy link
Contributor

Hi @pheel, could you give a bit more context on why you feel this change is needed? Do you need this in order to get something else working?

@pheel
Copy link
Contributor Author

pheel commented Jul 27, 2020

@federicotdn This need came up in the context of working out a handoff/handover flow: NLG is put on pause, but the user still needs to receive messages as if they were the result of actions. The most convenient way I found to do this is to append those replies to the tracker in the form of BotUttered events using the /tracker/events route.

To me the use case for where BotUttered events are forwarded to output is more obvious than the use case where they aren't, but on the face of it both seem legitimate?

@federicotdn
Copy link
Contributor

Hey @pheel, thanks for the explanation. I'm trying to think, just in case, if there's already an existing way of achieving what you need. In theory, you could use the POST /conversations/<id>/execute endpoint to execute an action (which could be a bot response with the desired text defined in the domain), which will also send the events into the output channel. Would that work for you or do you need a more fine-grained control?

@pheel
Copy link
Contributor Author

pheel commented Jul 30, 2020

The problem with using /execute is that the text content of the BotUttered event is by definition not predictable in a handoff scenario. During a handoff, both BotUttered and UserUttered messages come from humans. I might have missed something researching this, but I did not find an example of a design allowing true human-to-human conversations through the tracker. Thanks.

@Ghostvv Ghostvv requested review from federicotdn and removed request for Ghostvv August 10, 2020 09:13
Copy link
Contributor

@federicotdn federicotdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your use case, and I have some thoughts:

  • Another way of potentially doing the human hand-off is simply bypassing Rasa entirely. This could potentially be simpler than trying to use the Rasa tracker as a middleman for sending and receiving messages between two humans.
  • We need to have a test for the new functionality.

rasa/server.py Outdated Show resolved Hide resolved
@pheel pheel requested a review from federicotdn August 29, 2020 20:15
Copy link
Contributor

@federicotdn federicotdn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! Apologies for the delay. Please update the changelog and then we'll merge this.

I also saw that you removed an unused arg from handle_reminder. I recommend you add a comment on the PR after doing this, so that the changes are easier to read. You can also do these types of changes on a separate PR. But thanks anyways!

changelog/6273.improvement.md Outdated Show resolved Hide resolved
@federicotdn federicotdn self-assigned this Sep 8, 2020
@rasabot
Copy link
Collaborator

rasabot commented Sep 17, 2020

Could not update branch. Most likely this is due to a merge conflict. Please update the branch manually and fix any issues.

@pheel
Copy link
Contributor Author

pheel commented Sep 17, 2020

@rasabot ???

@federicotdn federicotdn merged commit 957c4b7 into RasaHQ:master Sep 24, 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.

4 participants