-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: add transformer to files downloaded event #35
Conversation
14846ba
to
b672668
Compare
filesmanager/processors/xapi/event_transformers/filesmanager_events.py
Outdated
Show resolved
Hide resolved
filesmanager/processors/xapi/event_transformers/filesmanager_events.py
Outdated
Show resolved
Hide resolved
`Activity` | ||
""" | ||
return Activity( | ||
id=self.get_object_iri("xblock", self.get_data("data.block_id", True)), |
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.
data.something
makes reference to a field inside the event, so this line is looking for the block_id
field, however the event doesn't have any block_id
, there are course_id
and xblock_id
.
IMO this should be the S3 or the static url since this is the identifier of the object which is the downloaded file in this case
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.
data.something
makes reference to a field inside the event, so this line is looking for theblock_id
field, however the event doesn't have anyblock_id
, there arecourse_id
andxblock_id
.
Okay, it was not clear to me, thanks!
IMO this should be the S3 or the static url since this is the identifier of the object which is the downloaded file in this case
Yes, you are right. The problem I found is that the zip file that is downloaded is generated in runtime, then I don't know what value the id could have. Does it make sense for the id to be a list of URLs for each file?
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.
that means that the user can download multiple files at the same time ??
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.
Yes, the student can choose to download one file or download multiple files. In case want to download multiple files, or maybe download a folder, a .zip file is generated in runtime.
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'm not an expert of XApi but I think the list of ids is not possible, my advice is to emit one event per file
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.
@andrey-canon, talking to @Ian2012, told me that it's only important to know that X student downloaded something for the event, and thus have the download metrics. That's why I would leave the xblock_id as the id.
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.
Probably that arguments is just valid in your context, and depends on what you want to achieve, if you want to measure the amount of files per xblock it's ok, however if you have an implementation where you are using the xblock with two files (lecture and answers) and the client wants to know how many students downloaded the answers sheet without downloading the lecture file, this won't be valid or a simple case the client just wants to measure the amount of a specific file... My recommendation is to think out of the box and consider more implementation cases since this a public repository however if this xblock is designed for specific client and they just need that keep the simple implementation
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.
For the moment, we will handle the event like this, but we will take into account what you say in case it is necessary to have a more detailed event. Thanks for the review, it was a great help!
BTW, this error was happening because the event/
endpoint was not sending the courserun_key
, when it was sent it was solved.
filesmanager/processors/xapi/event_transformers/filesmanager_events.py
Outdated
Show resolved
Hide resolved
Hi @andrey-canon, thanks for the review! I'm checking the logs to confirm that the transformer is working properly, as I have not been able to install aspects at my local. These are the logs I get in the 2023-12-29 19:12:58,905 INFO 1 [celery.worker.strategy] [user None] [ip None] strategy.py:161 - Task filesmanager.tasks.create_zip_file_task[b0edf829-3c43-4c9b-8011-81b057460773] received
2023-12-29 19:12:58,956 INFO 37 [celery.app.trace] [user None] [ip None] trace.py:131 - Task filesmanager.tasks.create_zip_file_task[b0edf829-3c43-4c9b-8011-81b057460773] succeeded in 0.049137711997900624s: '/media/downloads/b0edf829-3c43-4c9b-8011-81b057460773.zip'
2023-12-29 19:12:59,134 INFO 1 [celery.worker.strategy] [user None] [ip None] strategy.py:161 - Task eventtracking.tasks.send_event[5c10fb38-7b9d-4adf-9b5f-cef01c738b59] received
2023-12-29 19:12:59,170 WARNING 37 [event_routing_backends.processors.mixins.base_transformer] [user None] [ip None] base_transformer.py:179 - Could not get value for context.course_id in event "edunext.xblock.filesmanager.files.downloaded"
2023-12-29 19:12:59,171 INFO 37 [xapi_tracking] [user None] [ip None] transformer_processor.py:61 - {"id": "239bf073-3549-59b4-99ff-b4966627c5c2", "version": "1.0.3", "actor": {"objectType": "Agent", "account": {"name": "15cb54db-fdd6-4c0b-9b95-3a5fd52781e3", "homePage": "http://local.edly.io"}}, "verb": {"id": "http://id.tincanapi.com/verb/downloaded", "display": {"en": "downloaded"}}, "object": {"id": "http://local.edly.io/xblock/block-v1:edX+DemoX+Demo_Course+type@filesmanager+block@6bc9ede2e4234f1b90c4f3f383f9dde4", "objectType": "Activity", "definition": {"type": "http://id.tincanapi.com/verb/downloaded"}}, "timestamp": "2023-12-29T19:12:59.100905+00:00", "context": {"extensions": {"https://w3id.org/xapi/openedx/extension/transformer-version": "event-routing-backends@8.0.0", "https://w3id.org/xapi/openedx/extensions/session-id": "1299db65fa91988cd048b847c5e5965e"}}}
2023-12-29 19:12:59,182 INFO 1 [celery.worker.strategy] [user None] [ip None] strategy.py:161 - Task event_routing_backends.tasks.dispatch_event[ed8849e7-a3e3-4de6-ae9e-54b209c5ee9d] received
2023-12-29 19:12:59,183 INFO 37 [celery.app.trace] [user None] [ip None] trace.py:131 - Task eventtracking.tasks.send_event[5c10fb38-7b9d-4adf-9b5f-cef01c738b59] succeeded in 0.046917900999687845s: None
2023-12-29 19:12:59,542 INFO 38 [celery.app.trace] [user None] [ip None] trace.py:131 - Task event_routing_backends.tasks.dispatch_event[ed8849e7-a3e3-4de6-ae9e-54b209c5ee9d] succeeded in 0.3582683710010315s: None I get an error when getting |
I'm facing a similar error, in your case I'm not sure the reason however probably this won't affect your implementation the difference is that won't have the following field |
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.
LGTM
Description
This PR adds a transformer for files downloaded event added here
Testing Instructions
Set this setting in your environment using a YML plugin
Install this XBlock in your environment and enable it in your course (see more)
Checkout to this branch
Add files and folders to the files manager component (see more)
From LMS download any file or folder (event)
Check the log of the event.
Check that data was published. You can use Aspects.