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.

Sign up for GitHub

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
Merged

feat: send event when ora submission is created #2203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab302bf
feat: send event when ora submission is created
BryanttV Feb 2, 2024
52a4471
chore: add openedx-events in base requirements
BryanttV Apr 11, 2024
515a2a2
test: update unit test
BryanttV Apr 12, 2024
8ea89ff
chore: address PR review
BryanttV Apr 15, 2024
69b0051
chore: update event data
BryanttV Apr 18, 2024
a3c821b
chore: update openedx-events
BryanttV Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 25 additions & 22 deletions openassessment/xblock/test/test_submission.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
from openassessment.xblock.utils.data_conversion import create_submission_dict, prepare_submission_for_serialization
from openassessment.xblock.openassessmentblock import OpenAssessmentBlock
from openassessment.xblock.ui_mixins.legacy.views.submission import get_team_submission_context, render_submission
from openassessment.xblock.ui_mixins.legacy.handlers_mixin import LegacyHandlersMixin
from openassessment.xblock.workflow_mixin import WorkflowMixin
from openassessment.xblock.test.test_team import MockTeamsService, MOCK_TEAM_ID

Expand Down Expand Up @@ -361,28 +362,30 @@ def test_delete_and_submit(self, xblock):

with patch('submissions.api.create_submission') as mocked_submit:
with patch.object(WorkflowMixin, 'create_workflow'):
mocked_submit.return_value = {
"uuid": '1111',
"attempt_number": 1,
"created_at": dt.datetime.now(),
"submitted_at": dt.datetime.now(),
"answer": {},
}
resp = self.request(xblock, 'submit', self.SUBMISSION, response_format='json')
mocked_submit.assert_called_once()
student_sub_dict = mocked_submit.call_args[0][1]
self.assertEqual(
student_sub_dict['files_descriptions'],
[meta['description'] for meta in expected_file_metadata]
)
self.assertEqual(
student_sub_dict['files_names'],
[meta['fileName'] for meta in expected_file_metadata]
)
self.assertEqual(
student_sub_dict['files_sizes'],
[meta['fileSize'] for meta in expected_file_metadata]
)
with patch.object(LegacyHandlersMixin, 'send_ora_submission_created_event') as mock_send_event:
mocked_submit.return_value = {
"uuid": '1111',
"attempt_number": 1,
"created_at": dt.datetime.now(),
"submitted_at": dt.datetime.now(),
"answer": {},
}
resp = self.request(xblock, 'submit', self.SUBMISSION, response_format='json')
mock_send_event.assert_called_once()
mocked_submit.assert_called_once()
student_sub_dict = mocked_submit.call_args[0][1]
self.assertEqual(
student_sub_dict['files_descriptions'],
[meta['description'] for meta in expected_file_metadata]
)
self.assertEqual(
student_sub_dict['files_names'],
[meta['fileName'] for meta in expected_file_metadata]
)
self.assertEqual(
student_sub_dict['files_sizes'],
[meta['fileSize'] for meta in expected_file_metadata]
)

@mock_s3
@override_settings(
Expand Down
37 changes: 37 additions & 0 deletions openassessment/xblock/ui_mixins/legacy/handlers_mixin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import logging

from openedx_events.learning.data import ORASubmissionAnswer, ORASubmissionData
from openedx_events.learning.signals import ORA_SUBMISSION_CREATED
from xblock.core import XBlock
from submissions import api as submissions_api
from openassessment.assessment.errors.peer import (
Expand Down Expand Up @@ -42,6 +44,7 @@
from openassessment.xblock.staff_area_mixin import require_course_staff
from openassessment.xblock.ui_mixins.legacy.serializers import SaveFilesDescriptionRequestSerializer
from openassessment.xblock.utils.data_conversion import verify_assessment_parameters
from openassessment.xblock.utils.user_data import get_anonymous_user_id

logger = logging.getLogger(__name__)

Expand All @@ -61,6 +64,39 @@ class LegacyHandlersMixin:
Exposes actions (@XBlock.json_handlers) used in our legacy ORA UI
"""

def send_ora_submission_created_event(self, submission: dict) -> None:
"""
Send an event when a submission is created.

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 •
edited
Loading

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 •
edited
Loading

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.

# This import is here to avoid a circular import
from openassessment.xblock.openassessmentblock import OpenAssessmentBlock
BryanttV marked this conversation as resolved.
Show resolved Hide resolved

file_downloads = OpenAssessmentBlock.get_download_urls_from_submission(submission)
file_urls = [file_download.get("download_url") for file_download in file_downloads]
answer = submission.get("answer", {})
# .. event_implemented_name: ORA_SUBMISSION_CREATED
ORA_SUBMISSION_CREATED.send_event(
BryanttV marked this conversation as resolved.
Show resolved Hide resolved
submission=ORASubmissionData(
uuid=submission.get("uuid"),
anonymous_user_id=get_anonymous_user_id(self.runtime.service(self, "user")),
location=str(self.location),
attempt_number=submission.get("attempt_number"),
created_at=submission.get("created_at"),
submitted_at=submission.get("submitted_at"),
answer=ORASubmissionAnswer(
parts=answer.get("parts"),
file_keys=answer.get("file_keys"),
file_descriptions=answer.get("files_descriptions"),
file_names=answer.get("files_names"),
file_sizes=answer.get("files_sizes"),
file_urls=file_urls,
),
)
)

# Submissions

@XBlock.json_handler
Expand All @@ -79,6 +115,7 @@ def submit(self, data, suffix=""): # pylint: disable=unused-argument
self.submission_data,
self.workflow_data
)
self.send_ora_submission_created_event(submission)
return (
True,
submission.get("student_item"),
Expand Down
9 changes: 9 additions & 0 deletions openassessment/xblock/utils/user_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,12 @@ def get_user_preferences(user_service):
user_preferences['user_language'] = retrieved_preferences.get('pref-lang')

return user_preferences


def get_anonymous_user_id(user_service):
"""
Returns the anonymous user id for the current user.

:param user_service: XblockUserService
"""
return user_service.get_current_user().opt_attrs.get('edx-platform.anonymous_user_id')
1 change: 1 addition & 0 deletions requirements/base.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ djangorestframework
Xblock
edx-opaque-keys
openedx-filters
openedx-events

django
django-simple-history
Expand Down
22 changes: 16 additions & 6 deletions requirements/base.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@ appdirs==1.4.4
# via fs
asgiref==3.8.1
# via django
attrs==23.2.0
# via openedx-events
backports-zoneinfo==0.2.1 ; python_version < "3.9"
# via
# -c requirements/constraints.txt
# django
# djangorestframework
bleach==6.1.0
# via -r requirements/base.in
boto3==1.34.83
boto3==1.34.87
# via -r requirements/base.in
botocore==1.34.83
botocore==1.34.87
# via
# boto3
# s3transfer
Expand Down Expand Up @@ -48,6 +50,7 @@ django==4.2.11
# edx-submissions
# edx-toggles
# jsonfield
# openedx-events
# openedx-filters
django-crum==0.7.9
# via
Expand All @@ -73,14 +76,19 @@ edx-django-utils==5.12.0
# via
# -r requirements/base.in
# edx-toggles
# openedx-events
edx-i18n-tools==1.5.0
# via -r requirements/base.in
edx-opaque-keys==2.5.1
# via -r requirements/base.in
edx-opaque-keys[django]==2.5.1
# via
# -r requirements/base.in
# openedx-events
edx-submissions==3.7.0
# via -r requirements/base.in
edx-toggles==5.2.0
# via -r requirements/base.in
fastavro==1.9.4
# via openedx-events
fs==2.0.18
# via
# -c requirements/constraints.txt
Expand Down Expand Up @@ -122,7 +130,9 @@ markupsafe==2.1.5
# xblock
newrelic==9.8.0
# via edx-django-utils
openedx-filters==1.8.0
openedx-events==9.9.2
# via -r requirements/base.in
openedx-filters==1.8.1
# via -r requirements/base.in
path==13.1.0
# via
Expand Down Expand Up @@ -178,7 +188,7 @@ six==1.16.0
# html5lib
# python-dateutil
# python-swiftclient
sqlparse==0.4.4
sqlparse==0.5.0
# via django
stevedore==5.2.0
# via
Expand Down
2 changes: 1 addition & 1 deletion requirements/ci.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ tox==4.14.2
# via -r requirements/tox.txt
urllib3==2.2.1
# via requests
virtualenv==20.25.1
virtualenv==20.25.3
# via
# -r requirements/tox.txt
# tox
32 changes: 23 additions & 9 deletions requirements/quality.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ astroid==3.1.0
# via
# pylint
# pylint-celery
attrs==23.2.0
# via
# -r requirements/test.txt
# openedx-events
backports-zoneinfo[tzdata]==0.2.1 ; python_version < "3.9"
# via
# -c requirements/constraints.txt
Expand All @@ -42,12 +46,12 @@ binaryornot==0.4.4
# cookiecutter
bleach==6.1.0
# via -r requirements/test.txt
boto3==1.34.83
boto3==1.34.87
# via
# -r requirements/test.txt
# fs-s3fs
# moto
botocore==1.34.83
botocore==1.34.87
# via
# -r requirements/test.txt
# boto3
Expand All @@ -57,7 +61,7 @@ cachetools==5.3.3
# via
# -r requirements/test.txt
# tox
celery==5.3.6
celery==5.4.0
# via -r requirements/test.txt
certifi==2024.2.2
# via
Expand Down Expand Up @@ -149,6 +153,7 @@ django==4.2.11
# edx-submissions
# edx-toggles
# jsonfield
# openedx-events
# openedx-filters
# xblock-sdk
django-crum==0.7.9
Expand Down Expand Up @@ -177,12 +182,15 @@ edx-django-utils==5.12.0
# via
# -r requirements/test.txt
# edx-toggles
# openedx-events
edx-i18n-tools==1.5.0
# via -r requirements/test.txt
edx-lint==5.3.6
# via -r requirements/quality.in
edx-opaque-keys==2.5.1
# via -r requirements/test.txt
edx-opaque-keys[django]==2.5.1
# via
# -r requirements/test.txt
# openedx-events
edx-submissions==3.7.0
# via -r requirements/test.txt
edx-toggles==5.2.0
Expand All @@ -193,10 +201,14 @@ exceptiongroup==1.2.0
# pytest
factory-boy==3.3.0
# via -r requirements/test.txt
faker==24.8.0
faker==24.11.0
# via
# -r requirements/test.txt
# factory-boy
fastavro==1.9.4
# via
# -r requirements/test.txt
# openedx-events
filelock==3.13.4
# via
# -r requirements/test.txt
Expand Down Expand Up @@ -295,7 +307,9 @@ newrelic==9.8.0
# via
# -r requirements/test.txt
# edx-django-utils
openedx-filters==1.8.0
openedx-events==9.9.2
# via -r requirements/test.txt
openedx-filters==1.8.1
# via -r requirements/test.txt
packaging==24.0
# via
Expand Down Expand Up @@ -456,7 +470,7 @@ six==1.16.0
# html5lib
# python-dateutil
# python-swiftclient
sqlparse==0.4.4
sqlparse==0.5.0
# via
# -r requirements/test.txt
# django
Expand Down Expand Up @@ -515,7 +529,7 @@ vine==5.1.0
# amqp
# celery
# kombu
virtualenv==20.25.1
virtualenv==20.25.3
# via
# -r requirements/test.txt
# tox
Expand Down
Loading
Loading