Emit the ExamAttemptSubmitted Open edX event when an exam is submitted. #178
Conversation
MichaelRoytman
changed the title
MichaelRoytman
force-pushed
the
michaelroytman/MST-2114-exam-completion-event
branch
5 times, most recently
from
4958ec4
to
0dce062
Compare
MichaelRoytman
changed the title
| @@ -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', | |||
There was a problem hiding this comment.
This is necessary because the content_id must be able to be parsed into a valid UsageKey.
MichaelRoytman
force-pushed
the
michaelroytman/MST-2114-exam-completion-event
branch
from
0dce062
to
f6be2f9
Compare
| @@ -298,6 +299,33 @@ 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') | |||
There was a problem hiding this comment.
Should this be in a separate file for testing signals specifically?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ) | ||
| ), | ||
| course_key=course_key, | ||
| usage_key=usage_key, |
There was a problem hiding this comment.
Is there also supposed to be a requesting_user passed along? Looking at https://github.com/openedx/openedx-events/blob/fec19895eed1c37e4da6305bfa9849665a832372/openedx_events/learning/data.py#L316
There was a problem hiding this comment.
Or I guess it defaults to None, and only a learner can submit their own exam
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
MichaelRoytman
force-pushed
the
michaelroytman/MST-2114-exam-completion-event
branch
4 times, most recently
from
1403167
to
6a86447
Compare
| """ | ||
| Emit the EXAM_ATTEMPT_SUBMITTED Open edX event. | ||
| """ | ||
| name = user.full_name or '' |
There was a problem hiding this comment.
This is because the full_name field of the User model can either be the empty string (blank=True) or None (null=True). In my manual testing, the value came through as None, which caused the event bus to blow up, because None is not JSON serializable.
The User model comes from the edX cookiecutter. I'm not sure what the rationale was for designing the model this way; setting both attributes to true is something Django discourages. I created a thread in Slack to try to get some information on this decision. For now, I've done this. I'd prefer to fix the User model, but this will do until I have more information. I'll discuss this during standup.
MichaelRoytman
force-pushed
the
michaelroytman/MST-2114-exam-completion-event
branch
from
6a86447
to
267dc5f
Compare
MichaelRoytman
force-pushed
the
michaelroytman/MST-2114-exam-completion-event
branch
from
267dc5f
to
980ba38
Compare
MichaelRoytman
deleted the
michaelroytman/MST-2114-exam-completion-event
branch
JIRA: MST-2114 (private)
Description: This pull request introduces a change to emit the
TODOOpen edX event when a learner submits their exam.Author concerns: None.
Dependencies:
Installation instructions: None.
Testing instructions:
Follow the instructions in How to start using the Event Bus to test out this event.
Merge checklist:
Post merge: