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: emit error event #189

Merged

Conversation

alangsto
Copy link
Member

@alangsto alangsto commented Oct 3, 2023

JIRA: MST-2115

Description: If an attempt's status is updated to error, emit an open edx event to be consumed by the event bus.

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Delete working branch (if not needed anymore)

@alangsto alangsto force-pushed the alangsto/exam_errored_producer branch 2 times, most recently from 07c4c84 to 4554f2c Compare October 3, 2023 13:53
@alangsto alangsto changed the title Alangsto/exam errored producer feat: emit error event Oct 3, 2023
@alangsto alangsto force-pushed the alangsto/exam_errored_producer branch from 4554f2c to 57adf15 Compare October 3, 2023 13:59
@alangsto alangsto marked this pull request as ready for review October 3, 2023 14:00
@alangsto alangsto force-pushed the alangsto/exam_errored_producer branch from 57adf15 to a9b344d Compare October 3, 2023 14:01
('EXAM_ATTEMPT_ERRORED', ExamAttemptStatus.error, False)
)
@ddt.unpack
def test_attempt_event_emitted(self, event_name, status, expect_requesting_user):
Copy link
Member Author

Choose a reason for hiding this comment

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

The diff for this is not displaying that well, but I combined all of the producer tests into one test. Each event is mocked, and then the status is updated.

"""
Emit the EXAM_ATTEMPT_ERRORED Open edX event.
"""
user_data = UserData(
Copy link
Contributor

Choose a reason for hiding this comment

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

at some point (either this PR or later) we should probably make a helper function to create this object since it's always the same

Copy link
Member Author

Choose a reason for hiding this comment

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

Created a helper function to create the UserData object

@@ -46,3 +51,17 @@ def listen_for_exam_attempt_rejected(sender, signal, **kwargs): # pylint: disab
event_data={'exam_attempt': kwargs['exam_attempt']},
event_metadata=kwargs['metadata'],
)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest just marking this entire file as pragma: no-cover since this pattern of manual sends is temporary

Copy link
Member Author

Choose a reason for hiding this comment

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

excluded this file from coverage by modifying .coveragerc

@alangsto alangsto force-pushed the alangsto/exam_errored_producer branch from a9b344d to 406283b Compare October 3, 2023 20:10
@alangsto alangsto merged commit 22baaee into MST-1789-downstream-triggers Oct 3, 2023
7 checks passed
@alangsto alangsto deleted the alangsto/exam_errored_producer branch October 3, 2023 20:21
alangsto added a commit that referenced this pull request Oct 3, 2023
ilee2u pushed a commit that referenced this pull request Oct 6, 2023
* feat: add openedx_events and edx_event_bus_kafka to INSTALLED_APPS

* feat: install confluent_kafka[avro,schema-registry] for use with edx-event-bus-kafka

* feat: add event bus and Kafka Django settings

* feat: make User.full_name field non-nullable

This commit makes the User.full_name field non-nullable to ensure that there is only one representation of the empty value - the empty string. Currently, empty values for this field can be either None or the empty string. Because we're transitioning from a nullable to non-nullable state, a default is required to handle existing rows with a null value.

This is best practice; Django discourages setting both null=True and blank=True on CharField model fields. Furthermore, this was required by our event bus work. If an empty value is represented by None, this causes issues with the event bus, because None is not JSON serializable. Instead of converting a None value to the empty string in the event producer, correcting the the model definition is a better approach.

* feat: upgrade openedx-events to 8.8.0

* feat: emit the EXAM_ATTEMPT_SUBMITTED Open edX event when an exam is submitted

* feat: emit verified event for attempt (#186)

* feat: emit rejected exam signal (#181)

* feat: add errored event producer (#189)

* chore: fix upgrades

* chore: remove confluent-kafka import

* docs: local event bus development

* fix: remove local Kafka settings

This commit removes Django settings from the local settings file, used by the local application server, related to setting up the Kafka implementation of the event bus. This is because the event bus does not work outside of a Docker container. This is because the event bus is run through the devstack networking layer, which is inaccessible by the local application server.

* feat: update topic names for events (#193)

* feat: exam reset producer (#196)

send event to the event bus when an exam attempt is reset

---------

Co-authored-by: michaelroytman <[email protected]>
Co-authored-by: Michael Roytman <[email protected]>
Co-authored-by: Zachary Hancock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants