-
Notifications
You must be signed in to change notification settings - Fork 3
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
285 file sharing #59
285 file sharing #59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work. Left some comments, some of them constructive.
Of course, I should complain that sonarcloud says your test coverage is too low on the new code, but I don't know how easy it is to add a test for this?
@@ -37,3 +37,48 @@ async def run(self, dispatcher, tracker, domain): | |||
logging.info("no celery error") | |||
|
|||
return [] | |||
|
|||
|
|||
class SendMetadata(Action): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be overkill, but a few unrelated actions could all end up together in actions_common
, so I would add a short doctring here making clear that the SendMetadata
relates to the UploadFile
and SetFilePath
actions (at least, I assume they do)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a good suggestion. It will help in understanding how to use such actions in the future.
dispatcher.utter_message(response="utter_no_attachment") | ||
return {"received_file_text": None} | ||
|
||
# At this point the file ID should be saved in the DB for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we the ones sending these images, or will the user ever send images to us? Will the images ever be sensitive data that we need to be extra careful with in the db?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what I'm wondering is, can anyone with the file ID fetch the file? Is it stored on Niceday servers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the files are stored in the NiceDay server, and for accessing them the id and the access token are needed.
The images that we will send will not contain sensitive data, and the files sent by the users will not be stored in anyway on our system, so we should be on the safe side.
async def run(self, dispatcher, tracker, domain): | ||
|
||
# This is hardcoded for testing. Needs to be set according to the use case | ||
filepath = '/app/tst.PNG' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a random choice XD
if e['event'] == 'user': | ||
user_events.append(e) | ||
# we defined the metadata key name 'attachmentIds' is defined in the broker | ||
received_file_ids_list = user_events[-1]['metadata']['attachmentIds'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the last event always contains the list of received files? Is there an async problem possible where two images are sent simultaneously and the user_events list contains a mess of events for the two sends? (apologies if I've misunderstood what's happening here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a validation action that runs on user text input and the new message structure created in the custom channels always contains the metadata (even if the files list is empty). So, in theory we always get this. But theory and reality do not always match. It is a good idea to surround this with some verification.
This action runs on a single message, so the is no possibility to mess with this action.
Rasa_Bot/custom_channels.py
Outdated
from sanic import Blueprint, response | ||
from sanic.request import Request | ||
from sanic.response import HTTPResponse | ||
|
||
from niceday_client import NicedayClient | ||
|
||
NICEDAY_API_URL = 'http://niceday_api:8080/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this really be hard coded here? There's a NICEDAY_API_ENDPOINT
in the .env
configuration files that seems to be the place to set this. I'm worried this only works currently because those happen to be set the same way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. I will change this to use the .env file as the rest of the components.
- text: "Volgens mij heb je een probleem waar een mens je beter mee kan helpen. Ik raad je aan om contact op te nemen met iemand die je vertrouwt, zoals je huisarts." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for when the user shares a really horrible image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could absolutely use it in this way!
@@ -57,18 +74,22 @@ async def health(request: Request) -> HTTPResponse: # pylint: disable=unused-ar | |||
async def receive(request: Request) -> HTTPResponse: | |||
sender_id = request.json.get("sender") # method to get sender_id | |||
text = request.json.get("message") # method to fetch text | |||
|
|||
metadata = request.json.get("metadata") | |||
collector = self.get_output_channel() | |||
await on_new_message( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this ever timeout (or fail in some way)? Does something cause the app to retry if that happens? (this is just out of curiosity)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This contacts the Rasa server, that has a timeout. But if it fails for whatever reason, at the moment we do nothing, you are right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This contacts the Rasa server, that has a timeout. But if it fails for whatever reason, at the moment we do nothing, you are right
Co-authored-by: Robin Richardson <raar1@users.noreply.github.com>
SonarCloud Quality Gate failed. |
In this pull request the Rasa modifications needed to use the file sharing functionality have been implemented, together with example stories.
A detailed explanation of the workflow is reported here.
Fixes: PerfectFit-project/virtual-coach-issues#285