-
Notifications
You must be signed in to change notification settings - Fork 21
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
refactor: update ora submission data #341
refactor: update ora submission data #341
Conversation
Thanks for the pull request, @BryanttV! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
7b8e7bf
to
46c0b97
Compare
openedx_events/learning/data.py
Outdated
uuid = attr.ib(type=str) | ||
anonymous_user_id = attr.ib(type=str) | ||
location = attr.ib(type=str) | ||
attempt_number = attr.ib(type=int) | ||
created_at = attr.ib(type=datetime) | ||
submitted_at = attr.ib(type=datetime) | ||
answer = attr.ib(type=dict) |
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.
A few comments:
- I have enough context to know we didn't use this event before, so we can update the data without much trouble. Could we add that to the PR cover letter?
- Should all this fields be required?
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 all this fields be required?
I would think that the location
and anonymous_user_id
fields could be optional. The file_downloads
field is already optional by having a factory.
The other fields being part of the submission data I think they should be required, what do you think?
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 would think that the location and anonymous_user_id fields could be optional.
Is there a scenario in which this will not be sent by the event, or where those fields are none/empty? That's a good way of knowing if they should be optional. I agree the rest of them should be required, although, is the answer always present?
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 there a scenario in which this will not be sent by the event, or where those fields are none/empty?
In the submission these fields are not found, so they may not always be available.
is the answer always present?
Yes, in a submission there will always be a dictionary with the user's response, either with the response text, or information from the attached files.
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.
In the submission these fields are not found, so they may not always be available.
But they should always be available when sending the event, correct? Since we always have the location and the user anonymous id. So they are always non-empty. An example of an optional ID would be name
for the user, since it's not always available. I think it's fine for them to be required.
Yes, in a submission there will always be a dictionary with the user's response, either with the response text, or information from the attached files.
Mmm. Now I wonder, what's the difference between answer and https://github.com/openedx/openedx-events/pull/341/files#diff-5cf3a0b26961626ce777acf38a3cd803cb0ab982f8f35a35d078cb9c62438fa3R450-R464?
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.
If answer and FileDownloads overlap, should we create a class for answer (ORASubmissionAnswer or something similar) and include the FileDownloads there? So we don't send two fields with the same information. Correct me if I get it wrong.
I imagine something like:
class ORASubmissionAnswer:
... answer specific info like:
text: ...
file_downloads: ... FileDownloads specific fields
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 followed your suggestion and updated the data classes, could you check it?
46c0b97
to
d6bf843
Compare
d6bf843
to
3bbb712
Compare
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.
Thank you for addressing all my comments! Could you update the lib versions so we can merge?
@BryanttV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR updates the
ORASubmissionData
class to send the same data as the legacy analytics event. This will facilitate the transition from one event to another if necessary.Note
As this is an event that has not been used yet, there will be no problem in adding these new fields.
The new class attributes are:
id
->uuid
(Renamed to maintain consistency with the legacy event)anonymous_user_id
location
attempt_number
created_at
submitted_at
answer
(ORASubmissionAnswer
)parts
file_keys
file_descriptions
file_names
file_sizes
file_urls
The new fields that are not in the legacy event are:
anonymous_user_id
: Anonymous user ID of the user who submitted the ORA submission.location
: Location of the ORA submission.This new data is important as it allows us to have more complete and detailed information on ORA submission at the event.