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

Emit the ExamAttemptSubmitted Open edX event when an exam is submitted. #178

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions edx_exams/apps/core/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
ExamIllegalStatusTransition
)
from edx_exams.apps.core.models import Exam, ExamAttempt
from edx_exams.apps.core.signals.signals import emit_exam_attempt_submitted_event
from edx_exams.apps.core.statuses import ExamAttemptStatus

log = logging.getLogger(__name__)
Expand Down Expand Up @@ -96,6 +97,15 @@ def update_attempt_status(attempt_id, to_status):
if to_status == ExamAttemptStatus.submitted:
attempt_obj.end_time = datetime.now(pytz.UTC)

course_key = CourseKey.from_string(attempt_obj.exam.course_id)
usage_key = UsageKey.from_string(attempt_obj.exam.content_id)
emit_exam_attempt_submitted_event(
attempt_obj.user,
course_key,
usage_key,
attempt_obj.exam.exam_type
)

attempt_obj.status = to_status
attempt_obj.save()

Expand Down
18 changes: 18 additions & 0 deletions edx_exams/apps/core/apps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
"""
Core Application Configuration
"""

from django.apps import AppConfig


class CoreConfig(AppConfig):
"""
Application configuration for core application.
"""
name = 'edx_exams.apps.core'

def ready(self):
"""
Connect handlers to signals.
"""
from .signals import handlers # pylint: disable=unused-import,import-outside-toplevel
Empty file.
20 changes: 20 additions & 0 deletions edx_exams/apps/core/signals/handlers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
"""
Signal handlers for the edx-exams application.
"""
from django.dispatch import receiver
from openedx_events.event_bus import get_producer
from openedx_events.learning.signals import EXAM_ATTEMPT_SUBMITTED


@receiver(EXAM_ATTEMPT_SUBMITTED)
def listen_for_exam_attempt_submitted(sender, signal, **kwargs): # pylint: disable=unused-argument
"""
Publish EXAM_ATTEMPT_SUBMITTED signals onto the event bus.
"""
get_producer().send(
signal=EXAM_ATTEMPT_SUBMITTED,
topic='exam-attempt-submitted',
event_key_field='exam_attempt.course_key',
event_data={'exam_attempt': kwargs['exam_attempt']},
event_metadata=kwargs['metadata'],
)
32 changes: 32 additions & 0 deletions edx_exams/apps/core/signals/signals.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
"""
Signal definitions and functions to send those signals for the edx-exams application.
"""

from openedx_events.learning.data import ExamAttemptData, UserData, UserPersonalData
from openedx_events.learning.signals import EXAM_ATTEMPT_SUBMITTED


def emit_exam_attempt_submitted_event(user, course_key, usage_key, exam_type):
"""
Emit the EXAM_ATTEMPT_SUBMITTED Open edX event.
"""
user_data = UserData(
id=user.id,
is_active=user.is_active,
pii=UserPersonalData(
username=user.username,
email=user.email,
name=user.full_name
)
)

# .. event_implemented_name: EXAM_ATTEMPT_SUBMITTED
EXAM_ATTEMPT_SUBMITTED.send_event(
exam_attempt=ExamAttemptData(
student_user=user_data,
course_key=course_key,
usage_key=usage_key,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I guess it defaults to None, and only a learner can submit their own exam

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The plan was to reserve this attribute for cases where a user other than the learner has made the change, like an instructor resetting an exam in the Instructor Dashboard or overriding an exam attempt status on manual review, for example. I don't believe that submission can be triggered by anyone but the learner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back on this and decided to add in the requesting_user whenever there is one. I would prefer to be explicit about the requesting_user than having an implicit understanding that it should be empty whenever the learner is the requester.

exam_type=exam_type,
requesting_user=user_data
)
)
1 change: 1 addition & 0 deletions edx_exams/apps/core/test_utils/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class Meta:
password = factory.PostGenerationMethodCall('set_password', _DEFAULT_PASSWORD)
first_name = factory.Sequence('User{}'.format)
last_name = 'Test'
full_name = "{} {}".format(first_name, last_name)
is_superuser = False
is_staff = False

Expand Down
32 changes: 31 additions & 1 deletion edx_exams/apps/core/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from django.utils import timezone
from freezegun import freeze_time
from opaque_keys.edx.keys import CourseKey, UsageKey
from openedx_events.learning.data import ExamAttemptData, UserData, UserPersonalData

from edx_exams.apps.api.test_utils import ExamsAPITestCase
from edx_exams.apps.core.api import (
Expand Down Expand Up @@ -212,7 +213,7 @@ def setUp(self):
resource_id=str(uuid.uuid4()),
course_id=self.course_id,
provider=self.test_provider,
content_id='abcd1234',
content_id='block-v1:edX+test+2023+type@sequential+block@1111111111',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because the content_id must be able to be parsed into a valid UsageKey.

exam_name='test_exam',
exam_type='proctored',
time_limit_mins=30,
Expand Down Expand Up @@ -298,6 +299,35 @@ def test_submit_attempt(self):
self.assertEqual(updated_attempt.status, ExamAttemptStatus.submitted)
self.assertEqual(updated_attempt.end_time, timezone.now())

@patch('edx_exams.apps.core.signals.signals.EXAM_ATTEMPT_SUBMITTED.send_event')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in a separate file for testing signals specifically?

Copy link
Member Author

@MichaelRoytman MichaelRoytman Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signal is emitted from the API, so I think that this test makes the most sense in this file, because that's what I'm testing. I could move it into a separate class specifically for testing events emitted by the API functions, if you'd prefer?

If I were to have tests in signals.py, they would be to test that calling emit_exam_attempt_submitted_event results in an emitted signal. Do you think that's a useful test to have? I could add that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what you're saying, yeah it makes sense to do that test on the API level.

I guess I'm moreso trying to figure out if we need to do any testing on the level of the signal itself. I'm doing some digging now to see if that's necessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know what you find! I don't think we need to test the actual Django signal API. All the plumbing that funnels Django signals to the event bus is all behind the scenes and not something we could test, I believe. But we could test whether calling emit_exam_attempt_submitted_event results in an emitted signal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I found an example, there's a test here in edx-platform certificates that seems similar to yours.

That tests seems to go about as deep as yours does so I think it's good as is.

def test_submit_attempt_event_emitted(self, mock_event_send):
"""
Test that when an exam is submitted, the EXAM_ATTEMPT_SUBMITED Open edX event is emitted.
"""
update_attempt_status(self.exam_attempt.id, ExamAttemptStatus.submitted)
self.assertEqual(mock_event_send.call_count, 1)

user_data = UserData(
id=self.user.id,
is_active=self.user.is_active,
pii=UserPersonalData(
username=self.user.username,
email=self.user.email,
name=self.user.full_name
)
)
course_key = CourseKey.from_string(self.exam.course_id)
usage_key = UsageKey.from_string(self.exam.content_id)

expected_data = ExamAttemptData(
student_user=user_data,
course_key=course_key,
usage_key=usage_key,
exam_type=self.exam.exam_type,
requesting_user=user_data,
)
mock_event_send.assert_called_with(exam_attempt=expected_data)

def test_illegal_start(self):
"""
Test that an already started exam cannot be started
Expand Down
24 changes: 12 additions & 12 deletions requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,23 @@ attrs==23.1.0
# via
# lti-consumer-xblock
# openedx-events
avro==1.11.2
avro==1.11.3
# via confluent-kafka
bleach==6.0.0
# via lti-consumer-xblock
boto3==1.28.52
boto3==1.28.57
# via fs-s3fs
botocore==1.31.52
botocore==1.31.57
# via
# boto3
# s3transfer
certifi==2023.7.22
# via requests
cffi==1.15.1
cffi==1.16.0
# via
# cryptography
# pynacl
charset-normalizer==3.2.0
charset-normalizer==3.3.0
# via requests
click==8.1.7
# via
Expand All @@ -48,7 +48,7 @@ cryptography==41.0.4
# via
# pyjwt
# social-auth-core
defusedxml==0.7.1
defusedxml==0.8.0rc2
# via
# python3-openid
# social-auth-core
Expand Down Expand Up @@ -208,7 +208,7 @@ markupsafe==2.1.3
# xblock
mysqlclient==2.2.0
# via -r requirements/base.in
newrelic==9.0.0
newrelic==9.1.0
# via edx-django-utils
oauthlib==3.2.2
# via
Expand All @@ -221,13 +221,13 @@ openedx-django-pyfs==3.4.0
# via
# lti-consumer-xblock
# xblock
openedx-events==8.6.0
openedx-events==8.8.0
# via
# -r requirements/base.in
# edx-event-bus-kafka
openedx-filters==1.6.0
# via lti-consumer-xblock
packaging==23.1
packaging==23.2
# via drf-yasg
pbr==5.11.1
# via stevedore
Expand Down Expand Up @@ -287,7 +287,7 @@ requests==2.31.0
# social-auth-core
requests-oauthlib==1.3.1
# via social-auth-core
s3transfer==0.6.2
s3transfer==0.7.0
# via boto3
semantic-version==2.10.0
# via edx-drf-extensions
Expand Down Expand Up @@ -330,7 +330,7 @@ uritemplate==4.1.1
# via
# coreapi
# drf-yasg
urllib3==1.26.16
urllib3==1.26.17
# via
# botocore
# requests
Expand All @@ -342,7 +342,7 @@ webencodings==0.5.1
# via bleach
webob==1.8.7
# via xblock
xblock[django]==1.7.0
xblock[django]==1.8.0
# via
# lti-consumer-xblock
# xblock-utils
Expand Down
4 changes: 2 additions & 2 deletions requirements/ci.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ filelock==3.12.4
# via
# tox
# virtualenv
packaging==23.1
packaging==23.2
# via tox
platformdirs==3.10.0
platformdirs==3.11.0
# via virtualenv
pluggy==1.3.0
# via tox
Expand Down
Loading
Loading