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

Implemented get_output_channel() fn for SocketIO Channel #5578

Merged

Conversation

alfredfrancis
Copy link
Contributor

@alfredfrancis alfredfrancis commented Apr 4, 2020

Proposed changes:

  • Implemented get_output_channel() fn for SocketIO Channel
  • Changed Socketio Output message handlers to use recepient_id instead of self.sid

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)

@CLAassistant
Copy link

CLAassistant commented Apr 4, 2020

CLA assistant check
All committers have signed the CLA.

@sara-tagger
Copy link
Collaborator

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

@sara-tagger
Copy link
Collaborator

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

@chkoss
Copy link
Contributor

chkoss commented Apr 8, 2020

Here is some context as to why this feature was implemented: https://forum.rasa.com/t/integrate-rasa-with-live-chat/22314/6

@degiz degiz added this to the Rasa 1.10 milestone Apr 8, 2020
@erohmensing erohmensing removed their request for review April 15, 2020 12:54
degiz
degiz previously requested changes Apr 15, 2020
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 👌

I've added few comments. Also, do you think we can have a unit test for that change?

@wochinge could you take a look as well? I know you had some concerns. I've tested that change locally and it seems to work ok. What I tried:

  • Create two conversation in different browsers
  • Inject intents via REST to both conversations, and both did received responses

rasa/core/channels/socketio.py Show resolved Hide resolved
rasa/core/channels/socketio.py Outdated Show resolved Hide resolved
rasa/core/channels/socketio.py Outdated Show resolved Hide resolved
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Thanks for pulling me in @degiz .
Cool work @alfredfrancis . I didn't know that socketio handles a mapping from sid to socket in the background 💯 🚀

To be honest. I am still not entirely convinced since this won't work with multiple Rasa Open Source instances or in case we are using multiple Sanic workers (the latter is not the case at the moment). However, since it probably covers 90% of all use cases, I'm fine with merging it.

  • let's add a note that this only works with a single Rasa Open Source instance
  • We should also update the API spec here that Socketio works now

rasa/core/channels/socketio.py Show resolved Hide resolved
rasa/core/channels/socketio.py Outdated Show resolved Hide resolved
rasa/core/channels/socketio.py Outdated Show resolved Hide resolved
@m-vdb
Copy link
Collaborator

m-vdb commented Apr 23, 2020

@wochinge @alfredfrancis 💡 Rasa Open Source release is scheduled for next Tuesday April 28th. Any chance we can ship this as part of the milestone?

@alfredfrancis
Copy link
Contributor Author

@wochinge @alfredfrancis Rasa Open Source release is scheduled for next Tuesday April 28th. Any chance we can ship this as part of the milestone?

I'm waiting for the comments from @wochinge

@wochinge
Copy link
Contributor

@alfredfrancis I have to do another review first, then I'm back on this 👍

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Great work @alfredcogniwide 🎉 Left two small comments, but otherwise ready to go.
Can you please make sure that the CI tests here pass https://github.com/RasaHQ/rasa/actions/runs/81645737 ? If any results there are wrong, please let me know. We / GitHub recently had some problems with GitHub actions 😞

rasa/core/channels/socketio.py Show resolved Hide resolved
rasa/core/channels/socketio.py Outdated Show resolved Hide resolved
rasa/core/channels/socketio.py Outdated Show resolved Hide resolved
@alfredfrancis
Copy link
Contributor Author

Great work @alfredcogniwide Left two small comments, but otherwise ready to go.
Can you please make sure that the CI tests here pass https://github.com/RasaHQ/rasa/actions/runs/81645737 ? If any results there are wrong, please let me know. We / GitHub recently had some problems with GitHub actions

Ok, let me try

@degiz degiz requested a review from wochinge April 23, 2020 14:02
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

@alfredfrancis Can you please also add a changelog entry? 🙏

@wochinge
Copy link
Contributor

@alfredfrancis Could you please also sign the CLA? Thanks 🎉

@alfredfrancis
Copy link
Contributor Author

alfredfrancis commented Apr 28, 2020

@alfredfrancis Can you please also add a changelog entry?

Could you help me do this ? Shall I add an entry under [1.9.7] - 2020-04-23

@wochinge
Copy link
Contributor

Awesome! I quickly added changelog and a small docs change, because we want to include this in the release today 🎉

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @alfredfrancis 🙏

@alfredfrancis
Copy link
Contributor Author

Awesome! I quickly added changelog and a small docs change, because we want to include this in the release today

Thank you

@alfredfrancis
Copy link
Contributor Author

Thanks for the contribution @alfredfrancis

You are most welcome ❤️

@wochinge wochinge merged commit 84a262e into RasaHQ:master Apr 28, 2020
@alfredfrancis alfredfrancis deleted the feature/get_output_channel_for_socketio branch May 12, 2020 16:36
pheel pushed a commit to botfront/rasa-addons that referenced this pull request May 18, 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.

8 participants