-
Notifications
You must be signed in to change notification settings - Fork 196
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: send event when ora submission is created #2203
feat: send event when ora submission is created #2203
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. |
0e49f33
to
4547106
Compare
Hi @pomegranited, this PR is part of the implementation of the ADR for lightweight extension points, could you take a look at it? Thanks! |
1bc8e20
to
de5209c
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.
This looks really good, thank you! I just left a few comments for you to address.
de5209c
to
d3047c4
Compare
Hi @BryanttV ! I'm on leave from 20-29 April, so will do my best to review your PRs this week. Could you address the test failures here? |
@pomegranited: some of the failures are related to the Coverage setup https://github.com/openedx/edx-ora2/actions/runs/8692487174/job/23837223009?pr=2203 |
45f6daa
to
00f9f65
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.
This is working perfectly, great work @BryanttV :)
I have one small concern noted inline, but I'm open to discussion.
|
||
Args: | ||
submission (dict): The submission data | ||
""" |
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 change means we're throwing 2 "events" on ORA submission, this new one plus the legacy analytics event.
I'm a little worried that this will be confusing to people trying to consume these events, since you can't get everything you need from either event (this one has file urls but not the text answer, the analytics event has the text answer and other useful fields, but no file urls).
But I'm not sure how best to resolve this.. Any thoughts @BryanttV @mariajgrimaldi ?
Maybe instead of having your plugin re-fetch the submission in when it receives this new event, you could include all the extra submission data in the ORASubmissionData
? Then we could say in a comment here that ORA_SUBMISSION_CREATED
should be used instead of the old openassessmentblock.create_submission
analytics event.
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.
@pomegranited: thank you for raising this!
Currently, these events are not interchangeable and they're used for different things. As far as I'm aware, we're not replacing one with the other, so the pattern of adding a new Open edX Event, even though there's already a tracking log event, repeats itself in a few places across the project.
That's why I don't find consuming one or the other confusing. However, that might change. So it doesn't make a lot of sense to send completely different data, making a transition from one to the other more difficult in the future. So, after discussing it with Bryann, we decided to:
- Maintain the current data sent
- Additionally, add the analytics event data to the current event definition
So, the analytics event sends a subset of what the Open edX Event will send. I think that would make it easier in case of a transition or replacement.
@BryanttV, let us know if this sums up our conversation. @pomegranited: let us know what 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.
That sounds great thank you @mariajgrimaldi and @BryanttV !
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.
FYI, I am on leave from 20 - 29 April, so if we need to merge this now to make the redwood cut, it's fine to make this data change part of a separate PR.
If that's the way you'd rather go, please let me know. You'll just need to resolve the conflicts after we merge #2204, and I can merge this first thing tomorrow (around 00:00 UTC).
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'll update this PR with the new data class today. We'll let you know how it goes before 00:00 UTC!
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.
Since the idea is to merge this soon, we updated this PR with your latest suggestions @pomegranited. Now the data sent by the Open edX Event is a superset of what the legacy analytics event sends:
https://github.com/openedx/openedx-events/blob/main/openedx_events/learning/data.py#L450C1-L495C47
So, enough information is available for the events' users, and if, in the foreseeable future, the analytics event is replaced by this one, then the same data will be available.
Thank you, @BryanttV, for all the hard work! And to you, Jill, for the detailed review. Please, let us know what 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.
Your changes look great, thank you! Will just run some quick manual tests and get this merged.
2053983
to
8a2d520
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2203 +/- ##
=======================================
Coverage 95.05% 95.05%
=======================================
Files 191 191
Lines 21083 21098 +15
Branches 1903 1904 +1
=======================================
+ Hits 20040 20055 +15
Misses 779 779
Partials 264 264
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8a2d520
to
dcc1e06
Compare
d96237a
to
0851e36
Compare
0851e36
to
a3c821b
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.
👍 This is working great, thank you @BryanttV !
I'll get this merged, and then do a release/version bump so you can update the version used on edx-platform.
- I tested this using the PR test instructions. (The Turnitin plugin needs to be updated to use the new data structure, but the event comes through fine.)
- I read through the code
-
I checked for accessibility issuesN/A backend only - Includes documentation
- User-facing strings are extracted for translation
@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 adds the dispatch of an event when an ORA submission is created.
For now, this event will be used to obtain a similarity report generated by Turnitin for each of the files (including the student's response) in an ORA submission.
Dependencies
This PR needs the new ORA submission created event:
Supporting Information
These changes are based on those proposed in the following ADR
How to Test
Using Tutor:
Install this xblock with changes in this branch in the LMS/CMS services.
Install
openedx-events
in the LMS/CMS services with these changes.Install
platform-plugin-turnitin
in the LMS/CMS services with the changes in the ora event branch.Add the Turnitin credentials to your environment and restart the services. You can use this inline plugin:
Create a new ORA assessment in a course unit, maybe Staff Assessment Only.
In the settings of the component > File Uploads Response = Required and in File Types = pdf,doc,docx
In the LMS send a submission with some files.
Consumes this endpoint:
You should see a list with the URL to access the similarity report and the filename for each file in the ORA submission. e.g.: