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

feat: send event when ora submission is created #2203

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

BryanttV
Copy link
Contributor

@BryanttV BryanttV commented Apr 11, 2024

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:

  1. Install this xblock with changes in this branch in the LMS/CMS services.

  2. Install openedx-events in the LMS/CMS services with these changes.

  3. Install platform-plugin-turnitin in the LMS/CMS services with the changes in the ora event branch.

  4. Add the Turnitin credentials to your environment and restart the services. You can use this inline plugin:

    name: events-settings
    version: 0.1.0
    patches:
      openedx-common-settings: |
        TURNITIN_TII_API_URL = "https://xyz-sandbox.com"
        TURNITIN_TCA_INTEGRATION_FAMILY = "xyz"
        TURNITIN_TCA_INTEGRATION_VERSION = "X.Y.Z"
        TURNITIN_TCA_API_KEY = "xyzw"
        TURNITIN_API_TIMEOUT = 30
  5. Create a new ORA assessment in a course unit, maybe Staff Assessment Only.

  6. In the settings of the component > File Uploads Response = Required and in File Types = pdf,doc,docx

  7. In the LMS send a submission with some files.

  8. Consumes this endpoint:

    {lms_domain}/platform-plugin-turnitin/{course_id}/api/v1/viewer-url/{ora_submission_id}/
    
  9. 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.:

    [
        {
    	    "url": "https://xyz-sandbox.com/viewer/submissions/1",
    	    "file_name": "Students' Text Response.txt"
        },
        {
    	    "url": "https://xyz-sandbox.com/viewer/submissions/2",
    	    "file_name": "second-file.pdf"
        }
    ]

@BryanttV BryanttV requested a review from a team as a code owner April 11, 2024 02:42
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 11, 2024
@openedx-webhooks
Copy link

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@BryanttV BryanttV force-pushed the bav/add-created-submission-event branch 2 times, most recently from 0e49f33 to 4547106 Compare April 11, 2024 18:50
@BryanttV BryanttV requested a review from mariajgrimaldi April 11, 2024 21:03
@BryanttV
Copy link
Contributor Author

Hi @pomegranited, this PR is part of the implementation of the ADR for lightweight extension points, could you take a look at it? Thanks!

@BryanttV BryanttV force-pushed the bav/add-created-submission-event branch from 1bc8e20 to de5209c Compare April 15, 2024 13:47
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a 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.

@pomegranited
Copy link
Contributor

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?

@mariajgrimaldi
Copy link
Member

@pomegranited: some of the failures are related to the Coverage setup https://github.com/openedx/edx-ora2/actions/runs/8692487174/job/23837223009?pr=2203

Copy link
Contributor

@pomegranited pomegranited left a 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
"""
Copy link
Contributor

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.

Copy link
Member

@mariajgrimaldi mariajgrimaldi Apr 17, 2024

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:

  1. Maintain the current data sent
  2. 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!

Copy link
Contributor

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 !

Copy link
Contributor

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).

Copy link
Member

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!

Copy link
Member

@mariajgrimaldi mariajgrimaldi Apr 18, 2024

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.

Copy link
Contributor

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.

@BryanttV BryanttV force-pushed the bav/add-created-submission-event branch 2 times, most recently from 2053983 to 8a2d520 Compare April 17, 2024 15:00
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.05%. Comparing base (d8eeddf) to head (a3c821b).

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           
Flag Coverage Δ
unittests 95.05% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BryanttV BryanttV force-pushed the bav/add-created-submission-event branch from 8a2d520 to dcc1e06 Compare April 17, 2024 15:10
@BryanttV BryanttV force-pushed the bav/add-created-submission-event branch 2 times, most recently from d96237a to 0851e36 Compare April 18, 2024 19:55
@BryanttV BryanttV force-pushed the bav/add-created-submission-event branch from 0851e36 to a3c821b Compare April 18, 2024 20:00
Copy link
Contributor

@pomegranited pomegranited left a 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 issues N/A backend only
  • Includes documentation
  • User-facing strings are extracted for translation

@pomegranited pomegranited merged commit 43ae22a into openedx:master Apr 19, 2024
11 checks passed
@openedx-webhooks
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants