[BD-32] feat: 1st batch of Open edX Events #28266

mariajgrimaldi · 2021-07-23T20:44:44Z

Description

This PR adds the first batch of Open edX Events. These events are part of the OEP-50: Hooks Extension Framework implementation plan.

Since the OEP-50 publication, there has been a lot of discussion surrounding the design of this new extension point; I'll address some here for context:

These events are defined in the library openedx-events and will be imported here to be triggered in a specific list of places.
The class OpenEdxPublicSignal -a subclass of Django signals- is used to create each event.
send_event is the method that replaces send and send_robust.
Each event sends payloads based on Data Attr classes. For more information: Open edX Events Payload Conventions
Events unit test guarantees that they are being sent in the proper context. For example, STUDENT_REGISTRATION_COMPLETED is sent after the user's registration.

For an overall understanding of the design, check out the Open edX Events ADRs and discussions:

The Open edX Events implementations used in this PR are:

Open edX Events tooling: where we implement OpenEdxPublicSignal class and methods. Implemented in the PR #7 of openedx-events
Open edX Events Data Attribute classes: where we implement the arguments based on the Python Library attr. Implemented in the PR #20 of openedx-events
Open edX Events Definitions: where we implement each event. Implemented in the PR #18 of openedx-events

Supporting information

Discuss on Hooks Extension Framework:
https://discuss.openedx.org/t/configuration-for-the-hooks-extension-framework/4527/

Testing instructions

Checkout to MJG/1st_batch_openedx_events
Install Open edX Events library:
pip install openedx-events==0.5.1
Connect your custom receiver to the signal as follows:

from openedx_events.learning.signals import STUDENT_REGISTRATION_COMPLETED

@receiver(STUDENT_REGISTRATION_COMPLETED)
def callback(**kwargs):

Deadline

None

openedx-webhooks · 2021-07-23T20:44:49Z

Thanks for the pull request, @mariajgrimaldi! I've created BLENDED-926 to keep track of it in Jira. More details are on the BD-32 project page.

When this pull request is ready, tag your edX technical lead.

mariajgrimaldi · 2021-07-26T21:21:36Z

Hello @feanil and @ormsbee! ^^ Hope you're doing good!

Here are the first Open Events on the platform! This should be a WIP as our openedx-events PRs are still under review.

feanil · 2021-08-09T17:58:13Z

Just reviewed it and is looking like I would expect. I'm excited for this to land and serve as an example of how we might build events out in the rest of the system.

mariajgrimaldi · 2021-08-18T23:48:11Z

This PR introduces Open edX Events. When openedx-events is stable enough, we can make the migration to full events removing the current signals.

mariajgrimaldi · 2021-08-19T20:20:05Z

common/djangoapps/student/models.py

+                    is_active=user.is_active,
+                ),
+                course=CourseData(
+                    course_key=course_key,


Hey @felipemontoya, I hope you're doing fine. I have a few comments I'd like to discuss with you.

The first version of COURSE_ENROLLMENT_CREATED had a different CourseData object, but I had to use this minimal version due to some test errors.

This was the CourseData:

edx/edx-platform@c9c4322#diff-772dbcb8a58ad99cf253815130ffc49571c0e088ceedd90c131653e780518204R1619

And these are the errors:
https://build.testeng.edx.org/job/edx-platform-python-pipeline-pr/33625/

More in detail:
https://build.testeng.edx.org/job/edx-platform-python-pipeline-pr/33625/testReport/junit/common.djangoapps.student.tests.test_events/TestUserEvents/Run_Tests___cms_unit___test_enrolled_after_email_change/

The errors happened because some tests admit enrollments with nonexistent courses, then, enrollment.course.id or even course.id raises errors.

My first approach was to check to do: if course then course.name else "" but if you check this line:
https://github.com/edx/edx-platform/blob/602d6a858719703aca03d640fc0273d622188781/common/djangoapps/student/models.py#L1562
If the course does not exist, then course is never defined.

What do you think I should do?

This is similar to the other case below. Do we fix the tests or do we accept the capacity of the platform to actually have course enrollments to non-existent courses only because they have a well formatted course_key?

My first thought was the same as before, we should fix the tests, but then I noticed the amount of tests failing was tremendous (the same applies for the other comment) and I'm not sure this PR should bear the burden of that gigant fix.

In this case, we could easily create a CourseData attr object deferentially in the same try catch as the CourseOverview.get_from_id or just above the event creation. In the cases where there is no info but the key, then the CourseData will have emtpy strings, but the object will still have the members, so code that looks them up will not break.

I don't have a strong opinion on this, and I'm on PTO (so I may not reply quickly), but another possibility is to not actually send these events during tests. We do this for events like course_published, where you have to explicitly tell the test "I want these signals to be emitted", because otherwise tests would take too long to run, and people would get a lot of side-effects that they weren't counting on.

It definitely has its drawbacks, but it might be better than making code overly permissive or trying to retrofit a bunch of tests. That way, if you are explicitly testing this thing, you get the full end-to-end behavior, but you don't have to worry about what the random listeners might be doing with it for the most part.

The example mixin we do this with today for some subset of our tests: https://github.com/edx/edx-platform/blob/da09bcadc571b92a42d3cb9331454f624fec0977/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py#L216-L252

It pairs together with ENABLED_CACHES in ModuleStoreIsolationMixin.

So classes opt-in to emitting Modulestore signals like: https://github.com/edx/edx-platform/blob/2a8a58ae6b566dcd5262ae08d9c1173890e1c57c/lms/djangoapps/course_api/tests/test_api.py#L121-L125

It definitely has its drawbacks, but it might be better than making code overly permissive or trying to retrofit a bunch of tests. That way, if you are explicitly testing this thing, you get the full end-to-end behavior, but you don't have to worry about what the random listeners might be doing with it for the most part.

I like this a lot. Making the code overly permissive is the complete opposite of what we are trying to achieve. Also you would not like your listeners to break the test suite because someone passed incomplete mock data to a test.

Now I'm in doubt whether this would be something that we do here at edx-platform or should it be part of the events tooling?

Hello @felipemontoya and @ormsbee! Thank you for the feedback.

It's a great idea to disable the events during testing and activate them just when we want to. That's exactly what we are going to do, create a mixin similar to:
https://github.com/edx/edx-platform/blob/da09bcadc571b92a42d3cb9331454f624fec0977/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py#L216-L252

That turns on and off the events, the latter being the default. I'm currently working on the POC; I'll be adding the PR as soon as possible. The PR is in openedx-events since I think we should equip the repo with all the necessary tools.

But we still have issues when creating the data objects like CourseData or UserData:

https://build.testeng.edx.org/job/edx-platform-python-pipeline-pr/33625/testReport/junit/common.djangoapps.student.tests.test_events/TestUserEvents/Run_Tests___cms_unit___test_enrolled_after_email_change/

To solve this, we have some proposals that I'll be soon implementing to show you, the overall idea is to solve each case, the user's profile error and the course, separately.

What do you think?

I'm all good with the mixing living in the library.

the latter being the default.

You mean only for testing, right?

About the errors with creating the attr data objects I see that turning the signal off does not solve it, since the code still calls user.profile.name and thus breaks. We'll have to see in each case if we can solve it by retrofitting the tests or making the code more permisive

That's right, only for testing. @felipemontoya

mariajgrimaldi · 2021-08-19T20:33:56Z

openedx/core/djangoapps/user_authn/views/login.py

+                pii=UserPersonalData(
+                    username=user.username,
+                    email=user.email,
+                    name=user.profile.name if hasattr(user, "profile") else "",


Hey @felipemontoya, I'm conflicted about this line. Some tests were failing when using user.profile.name, specifically these:
https://build.testeng.edx.org/job/edx-platform-python-pipeline-pr/33620/#showFailuresLink

More in detail:
https://build.testeng.edx.org/job/edx-platform-python-pipeline-pr/33620/testReport/junit/lms.djangoapps.course_goals.tests.test_goals/TestCourseGoalsAPI/Run_Tests___lms_unit___test_add_goal/

It seems that some tests use users without profiles and were crashing -obviously-. This is the best I could think of, but I didn't find any other example of accessing the user profile besides user.profile.name. Can you think of an alternative?

I also went looking for other instances of user.profile.name and their tests. They end up using something like a UserFactory (https://github.com/edx/edx-platform/blob/master/common/djangoapps/student/tests/factories.py#L77) which guarantees that the profile attr exists.

I would go with lets fix those tests as they use something like self.user = User.objects.create_user('john', '[email protected]', 'password') which is the reason no profile attr is there.

felipemontoya · 2021-08-30T19:31:03Z

common/djangoapps/student/tests/test_enrollment.py

@@ -30,7 +31,7 @@
 @ddt.ddt
 @patch.dict('django.conf.settings.FEATURES', {'ENABLE_SPECIAL_EXAMS': True})
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
-class EnrollmentTest(UrlResetMixin, SharedModuleStoreTestCase):
+class EnrollmentTest(UrlResetMixin, SharedModuleStoreTestCase, OpenEdxEventsTestMixin):


Since the default behavior for the OpenEdxEventsTestMixin is that it will not send out events unless specified in ENABLED_OPENEDX_EVENTS I suggest we make it explicit for the first tests.
By this I mean, inherit from the mixin and also put a ENABLED_OPENEDX_EVENTS = [] statement.

This way anyone looking at this as usage example will learn about the existence of the ENABLED_OPENEDX_EVENTS key setting

Great, thanks for the suggestion!

Where is the corresponding integration test for when ENABLED_OPENEDX_EVENTS is not an empty list?

Good point, we can make it explicit for the test_enrollment_created_event_emitted test below.

Also any TestCase that does not inherit OpenEdxEventsTestMixin will act as regular code not bound by test restrictions and send the events normally.

Thanks for the suggestion @nasthagiri! I changed the events tests -e.g. EnrollmentEventsTest-, so they correspond to the integration of each event to the login, registration, and enrollment process.

felipemontoya · 2021-08-30T19:58:16Z

Hey @nasthagiri, @ormsbee, @feanil. After many many cycles of review we believe this is ready to be merged.

Would you like to give us a review, or a thumbs up or something?

After this PR we expect to create one more with 5 more events as this only contains 3 from the 8 that are already defined in openedx-events.

felipemontoya · 2021-08-30T20:17:38Z

I created a rolling status and backlog spreadsheet here: https://docs.google.com/spreadsheets/d/11kK0DF4AVB02hojec9krAr36yOacIku5mFy9WYcYS8I/edit#gid=0

nasthagiri

Also, FYI on this PR from an orthogonal effort to update Course-completion and Grades events.

nasthagiri · 2021-08-31T23:25:10Z

common/djangoapps/student/models.py

+        COURSE_ENROLLMENT_CREATED.send_event(
+            enrollment=CourseEnrollmentData(
+                user=UserData(
+                    pii=UserPersonalData(


Love the abstraction layers that are being created for these data objects as part of this effort!

nasthagiri · 2021-08-31T23:26:58Z

common/djangoapps/student/tests/test_enrollment.py

@@ -30,7 +31,7 @@
 @ddt.ddt
 @patch.dict('django.conf.settings.FEATURES', {'ENABLE_SPECIAL_EXAMS': True})
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
-class EnrollmentTest(UrlResetMixin, SharedModuleStoreTestCase):
+class EnrollmentTest(UrlResetMixin, SharedModuleStoreTestCase, OpenEdxEventsTestMixin):


Where is the corresponding integration test for when ENABLED_OPENEDX_EVENTS is not an empty list?

felipemontoya · 2021-09-01T21:20:59Z

I'm very happy and excited we have reached this moment. I'll merge this PR in the morning to not trigger late deployments to prod.

@nasthagiri about the orthogonal effort you mention, could you tell me who is leading that effort to see if we can make a push for making those events be Openedx Events?

nasthagiri · 2021-09-02T02:18:30Z

@felipemontoya That orthogonal effort is the xAPI/Caliper integration effort that @ziafazal gave an update on at the last Contributors meeting. I forgot to add a link: https://github.com/edx/edx-platform/pull/28415

* Add STUDENT_REGISTRATION_COMPLETED event after the user's registration * Add SESSION_LOGIN_COMPLETED event after the user's login session * Add COURSE_ENROLLMENT_CREATED event after the user's enrollment creation

edx-pipeline-bot · 2021-09-02T18:27:28Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-09-02T18:58:57Z

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

edx-pipeline-bot · 2021-09-02T19:56:34Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

ormsbee · 2021-09-02T20:06:24Z

Congrats folks! I know it took a lot of iteration, but I'm so excited about how this work can move the platform forward!

edx-pipeline-bot · 2021-09-02T21:03:43Z

EdX Release Notice: This PR has been deployed to the production environment.

edx-status-bot · 2021-09-03T16:26:43Z

Your PR has finished running tests. There were no failures.

* Add PreEnrollmentFilter * Add PreRegisterFilter * Add PreLoginFilter For more info: openedx/edx-platform#29449 Some events that were already on the platform were also added: * Add COURSE_ENROLLMENT_CHANGED: sent after the enrollment update * Add COURSE_ENROLLMENT_CREATED event after the user's enrollment creation * Add COURSE_UNENROLLMENT_COMPLETED: sent after the user's unenrollment For more info: openedx/edx-platform#28266 openedx/edx-platform#28640

openedx-webhooks added blended PR is managed through 2U's blended developmnt program waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jul 23, 2021

This was referenced Jul 23, 2021

feat: add necessary tooling for Open edX events openedx/openedx-events#7

Merged

feat: preliminary signal definition openedx/openedx-events#18

Merged

mariajgrimaldi marked this pull request as ready for review July 23, 2021 22:07

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jul 23, 2021

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch 3 times, most recently from 47e1893 to cca6d3f Compare July 26, 2021 15:45

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch from 86c9680 to bd2d8e5 Compare July 30, 2021 14:02

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch 2 times, most recently from 87e2b33 to aabcb67 Compare August 18, 2021 23:46

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch 5 times, most recently from 18b17e6 to 7eaaed6 Compare August 19, 2021 18:51

mariajgrimaldi commented Aug 19, 2021

View reviewed changes

felipemontoya mentioned this pull request Aug 19, 2021

docs: ADR for in context discussions [BD-38] [TNL-8512] [BB-4481] #28221

Merged

This was referenced Aug 24, 2021

test: add signal isolation class and methods openedx/openedx-events#26

Merged

refactor: changed create_user with UserFactory to avoid errors #28525

Merged

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch 2 times, most recently from 9613cce to 97d814c Compare August 25, 2021 18:37

mariajgrimaldi mentioned this pull request Aug 25, 2021

refactor: replace User.objects.create_user for UserFactory p2 #28547

Merged

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch 3 times, most recently from 210d386 to e23ccc1 Compare August 30, 2021 17:12

felipemontoya reviewed Aug 30, 2021

View reviewed changes

felipemontoya approved these changes Aug 30, 2021

View reviewed changes

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch from 607fb61 to 86d73d6 Compare August 31, 2021 16:33

nasthagiri approved these changes Aug 31, 2021

View reviewed changes

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch 3 times, most recently from 694fc6a to 28692e3 Compare September 1, 2021 20:27

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch from 28692e3 to 30c9aff Compare September 2, 2021 12:30

feat: add 1st batch of Open edX events

c061859

* Add STUDENT_REGISTRATION_COMPLETED event after the user's registration * Add SESSION_LOGIN_COMPLETED event after the user's login session * Add COURSE_ENROLLMENT_CREATED event after the user's enrollment creation

mariajgrimaldi force-pushed the MJG/1st_batch_openedx_events branch from 30c9aff to c061859 Compare September 2, 2021 14:05

felipemontoya merged commit f0d319e into openedx:master Sep 2, 2021

openedx-webhooks added merged and removed needs triage labels Sep 2, 2021

mariajgrimaldi mentioned this pull request Sep 3, 2021

[BD-32] feat: add 2nd batch of Open edX Events #28640

Merged

mariajgrimaldi mentioned this pull request Nov 2, 2021

JU-6: refactor tests to avoid events failure eduNEXT/edunext-platform#566

Merged

[BD-32] feat: 1st batch of Open edX Events #28266

[BD-32] feat: 1st batch of Open edX Events #28266

Conversation

mariajgrimaldi commented Jul 23, 2021 • edited Loading

Description

Supporting information

Testing instructions

Deadline

openedx-webhooks commented Jul 23, 2021 • edited Loading

mariajgrimaldi commented Jul 26, 2021

feanil commented Aug 9, 2021

mariajgrimaldi commented Aug 18, 2021

mariajgrimaldi Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipemontoya commented Aug 30, 2021

felipemontoya commented Aug 30, 2021

nasthagiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipemontoya commented Sep 1, 2021

nasthagiri commented Sep 2, 2021

edx-pipeline-bot commented Sep 2, 2021

edx-pipeline-bot commented Sep 2, 2021

edx-pipeline-bot commented Sep 2, 2021

ormsbee commented Sep 2, 2021

edx-pipeline-bot commented Sep 2, 2021

edx-status-bot commented Sep 3, 2021

mariajgrimaldi commented Jul 23, 2021 •

edited

Loading

openedx-webhooks commented Jul 23, 2021 •

edited

Loading

mariajgrimaldi Aug 19, 2021 •

edited

Loading