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

Jump to bottom

feat: hook xblock publish, delete and duplicate openedx-events #31350

Merged
merged 6 commits into from
Feb 14, 2023
Merged

feat: hook xblock publish, delete and duplicate openedx-events #31350

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
63de81f
feat: hook xblock publish, delete and duplicate openedx-events
navinkarkera Nov 28, 2022
d26ee63
docs: add events to docs
navinkarkera Dec 6, 2022
605db1e
feat: published xblock events to event bus
navinkarkera Jan 24, 2023
8b00576
refactor: use events test mixin to start events in isolation
navinkarkera Jan 31, 2023
ebc223f
fix: variable rename after rebase
navinkarkera Jan 31, 2023
df9f88c
refactor: send time with event explicitly
navinkarkera Feb 6, 2023
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
43 changes: 42 additions & 1 deletion cms/djangoapps/contentstore/signals/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@
from edx_toggles.toggles import SettingToggle
from opaque_keys.edx.keys import CourseKey
from openedx_events.content_authoring.data import CourseCatalogData, CourseScheduleData
from openedx_events.content_authoring.signals import COURSE_CATALOG_INFO_CHANGED
from openedx_events.content_authoring.signals import (
COURSE_CATALOG_INFO_CHANGED,
XBLOCK_DELETED,
XBLOCK_DUPLICATED,
XBLOCK_PUBLISHED,
)
from openedx_events.event_bus import get_producer
from pytz import UTC

Expand Down Expand Up @@ -166,6 +171,42 @@ def listen_for_course_catalog_info_changed(sender, signal, **kwargs):
)


@receiver(XBLOCK_PUBLISHED)
def listen_for_xblock_published(sender, signal, **kwargs):
"""
Publish XBLOCK_PUBLISHED signals onto the event bus.
"""
get_producer().send(
signal=XBLOCK_PUBLISHED, topic='xblock-published',
event_key_field='xblock_info.usage_key', event_data={'xblock_info': kwargs['xblock_info']},
event_metadata=kwargs['metadata'],
)


@receiver(XBLOCK_DELETED)
def listen_for_xblock_deleted(sender, signal, **kwargs):
"""
Publish XBLOCK_DELETED signals onto the event bus.
"""
get_producer().send(
signal=XBLOCK_DELETED, topic='xblock-deleted',
event_key_field='xblock_info.usage_key', event_data={'xblock_info': kwargs['xblock_info']},
event_metadata=kwargs['metadata'],
)


@receiver(XBLOCK_DUPLICATED)
def listen_for_xblock_duplicated(sender, signal, **kwargs):
"""
Publish XBLOCK_DUPLICATED signals onto the event bus.
"""
get_producer().send(
signal=XBLOCK_DUPLICATED, topic='xblock-duplicated',
event_key_field='xblock_info.usage_key', event_data={'xblock_info': kwargs['xblock_info']},
event_metadata=kwargs['metadata'],
)
Comment on lines +174 to +207
Copy link
Member

Choose a reason for hiding this comment

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

@robrap: hello there Robert! FYI I'm currently reviewing this PR. If you have any comments, please let us know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

  1. We hope to make it even simpler to produce to the event bus, but for now, the catalog updated example is a good guide.
  2. We have added the ability to add a timestamp when producing the signal. That should be used in cases where the timestamp is in the db. The catalog event doesn’t do this yet, but will soon. Other existing events may want to update their calls to send event as well.
  3. I imagine that these still require some infrastructure setup for the Kafka topic(s) in edx.org before this lands to not result in errors. I’m not sure what happens when the topic isn’t there.
  4. We don’t yet share event types on the same topic. I’m not sure if these are meant to, so a shared consumer is picking these up in order? If so, there is a backlog event bus ticket that needs some research.
  5. Out of curiosity, who are the intended consumers of the event bus events? Is there a design document around these new events?
  6. I wonder how we should document events using the event bus specifically?

Thank you. Excited to see new event bus events!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robrap

We don’t yet share event types on the same topic. I’m not sure if these are meant to, so a shared consumer is picking these up in order? If so, there is a backlog event bus ticket that needs some research.

That would be nice, but the current way not a deal breaker. It will just require the consumers to run a process per signal/event.

Out of curiosity, who are the intended consumers of the event bus events?

Taxonomy-connector which is installed as part of course-discovery is the intended consumer here.

We have added the ability to add a timestamp when producing the signal. That should be used in cases where the timestamp is in the db. The catalog event doesn’t do this yet, but will soon. Other existing events may want to update their calls to send event as well.

I am not sure if I follow, do we need to make some changes in the way we publish these events?

cc: @mariajgrimaldi

Copy link
Contributor

Choose a reason for hiding this comment

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

@navinkarkera: I'm really excited that people are hopping on to the event bus. Thanks again for this work. I've got some additional thoughts and questions.

That would be nice, but the current way not a deal breaker. It will just require the consumers to run a process per signal/event.

I updated the event design section of the how-to with a note on lifecycle events.

Taxonomy-connector which is installed as part of course-discovery is the intended consumer here.

Thank you. That's helpful, and I realize I'd love to see what ADR(s) describe the overall decisions or design for the whole flow. Are there additional links you could provide?

I also added a warning to the how-to (section linked above), as well as a small note to the Observability section regarding maintainership for the event production. Since the 2U TNL team maintains cms, have they been reviewing ADRs/PRs as-needed, and are they aware that they will need to start monitoring this new event (if this is deployed to edx.org)?

Related, I added some tips to the Publishing a signal section of the how-to regarding adding a temporary(?) rollout toggle. We can iterate on this idea as needed, but I wanted to note it.

Lastly, what teams are involved and where is this work going? Is this OpenCraft work that is initially intended for edX.org, or for your own deployment? Is OpenCraft also working with Kafka?

I am not sure if I follow, do we need to make some changes in the way we publish these events?

Regarding my note about time, I tried adding a note and pointer to docs in the same Publishing a signal section. Let me know if that clears things up.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mariajgrimaldi: I added my question about documenting events as being event-bus events to this other PR comment: openedx/openedx-events#80 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robrap Thanks! Updated the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robrap Also added current time to events explicitly.

@mariajgrimaldi Let me know if we are missing something.

Copy link
Member

@sameenfatima78 sameenfatima78 Feb 6, 2023 •
edited by robrap
Loading

Choose a reason for hiding this comment

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

Lastly, what teams are involved and where is this work going? Is this OpenCraft work that is initially intended for edX.org, or for your own deployment?

@robrap This work is a blended project between OpenCraft and our team (Enterprise Markhors).

Since the 2U TNL team maintains cms, have they been reviewing ADRs/PRs as-needed, and are they aware that they will need to start monitoring this new event (if this is deployed to edx.org)?

@robrap Thank you for pointing that out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks again. I'm not sure how work is being tracked, but maybe there could be a ticket (or tickets) for the following:

  1. Checking in with TNL on event bus design and ownership for these 3 events.
  2. Related, determining whether the 3 events will be sent to a single topic, or multiple topics, which may have long term implications (unless we do one, and then supplement with the other in the future).
  3. Ensuring that the stage/production topics get created before starting to send events, as well as setting up monitoring/alerting, etc.

Other than that, this thread can be marked resolved from my end, as long as everyone else agrees. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Thank you @robrap, I'll run these by our team.



@receiver(SignalHandler.course_deleted)
def listen_for_course_delete(sender, course_key, **kwargs): # pylint: disable=unused-argument
"""
Expand Down
13 changes: 13 additions & 0 deletions cms/djangoapps/contentstore/views/block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@
from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user
from django.core.exceptions import PermissionDenied
from django.http import Http404, HttpResponse, HttpResponseBadRequest
from django.utils.timezone import timezone
from django.utils.translation import gettext as _
from django.views.decorators.http import require_http_methods
from edx_django_utils.plugins import pluggable_override
from openedx_events.content_authoring.data import DuplicatedXBlockData
from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED
from edx_proctoring.api import (
does_backend_support_onboarding,
get_exam_by_content_id,
Expand Down Expand Up @@ -959,6 +962,16 @@ def _duplicate_block(parent_usage_key, duplicate_source_usage_key, user, display
parent.children.append(dest_block.location)
store.update_item(parent, user.id)

# .. event_implemented_name: XBLOCK_DUPLICATED
XBLOCK_DUPLICATED.send_event(
time=datetime.now(timezone.utc),
xblock_info=DuplicatedXBlockData(
usage_key=dest_block.location,
block_type=dest_block.location.block_type,
source_usage_key=duplicate_source_usage_key,
)
)

return dest_block.location


Expand Down
41 changes: 40 additions & 1 deletion cms/djangoapps/contentstore/views/tests/test_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
from django.test import TestCase
from django.test.client import RequestFactory
from django.urls import reverse
from openedx_events.content_authoring.data import DuplicatedXBlockData
from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED
from openedx_events.tests.utils import OpenEdxEventsTestMixin
from edx_proctoring.exceptions import ProctoredExamNotFoundException
from opaque_keys import InvalidKeyError
from opaque_keys.edx.asides import AsideUsageKeyV2
Expand Down Expand Up @@ -550,6 +553,7 @@ def _duplicate_and_verify(self, source_usage_key, parent_usage_key, check_asides
self._check_equality(source_usage_key, usage_key, parent_usage_key, check_asides=check_asides),
"Duplicated item differs from original"
)
return usage_key

def _check_equality(self, source_usage_key, duplicate_usage_key, parent_usage_key=None, check_asides=False,
is_child=False):
Expand Down Expand Up @@ -642,11 +646,25 @@ def _duplicate_item(self, parent_usage_key, source_usage_key, display_name=None)
return self.response_usage_key(resp)


class TestDuplicateItem(ItemTest, DuplicateHelper):
class TestDuplicateItem(ItemTest, DuplicateHelper, OpenEdxEventsTestMixin):
"""
Test the duplicate method.
"""

ENABLED_OPENEDX_EVENTS = [
"org.openedx.content_authoring.xblock.duplicated.v1",
]

@classmethod
def setUpClass(cls):
"""
Set up class method for the Test class.
This method starts manually events isolation. Explanation here:
openedx/core/djangoapps/user_authn/views/tests/test_events.py#L44
"""
super().setUpClass()
cls.start_events_isolation()

def setUp(self):
""" Creates the test course structure and a few components to 'duplicate'. """
super().setUp()
Expand Down Expand Up @@ -684,6 +702,27 @@ def test_duplicate_equality(self):
self._duplicate_and_verify(self.seq_usage_key, self.chapter_usage_key)
self._duplicate_and_verify(self.chapter_usage_key, self.usage_key)

def test_duplicate_event(self):
"""
Check that XBLOCK_DUPLICATED event is sent when xblock is duplicated.
"""
event_receiver = Mock()
XBLOCK_DUPLICATED.connect(event_receiver)
usage_key = self._duplicate_and_verify(self.vert_usage_key, self.seq_usage_key)
event_receiver.assert_called()
self.assertDictContainsSubset(
{
"signal": XBLOCK_DUPLICATED,
"sender": None,
"xblock_info": DuplicatedXBlockData(
usage_key=usage_key,
block_type=usage_key.block_type,
source_usage_key=self.vert_usage_key,
),
},
event_receiver.call_args.kwargs
)

def test_ordering(self):
"""
Tests the a duplicated xblock appears immediately after its source
Expand Down
12 changes: 12 additions & 0 deletions docs/guides/hooks/events.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,3 +195,15 @@ Content Authoring Events
* - `COURSE_CATALOG_INFO_CHANGED <https://github.com/openedx/openedx-events/blob/main/openedx_events/content_authoring/signals.py#L23>`_
- org.openedx.content_authoring.course.catalog_info.changed.v1
- `2022-08-24 <https://github.com/openedx/edx-platform/blob/a8598fa1fac5e26ac212aa588e8527e727581742/cms/djangoapps/contentstore/signals/handlers.py#L111>`_

* - `XBLOCK_PUBLISHED <https://github.com/openedx/openedx-events/blob/main/openedx_events/content_authoring/signals.py#L30>`_
- org.openedx.content_authoring.xblock.published.v1
- `2022-12-06 <https://github.com/openedx/edx-platform/blob/master/xmodule/modulestore/mixed.py#L926>`_

* - `XBLOCK_DELETED <https://github.com/openedx/openedx-events/blob/main/openedx_events/content_authoring/signals.py#L42>`_
- org.openedx.content_authoring.xblock.deleted.v1
- `2022-12-06 <https://github.com/openedx/edx-platform/blob/master/xmodule/modulestore/mixed.py#L804>`_

* - `XBLOCK_DUPLICATED <https://github.com/openedx/openedx-events/blob/main/openedx_events/content_authoring/signals.py#L54>`_
- org.openedx.content_authoring.xblock.duplicated.v1
- `2022-12-06 <https://github.com/openedx/edx-platform/blob/master/cms/djangoapps/contentstore/views/item.py#L965>`_
25 changes: 23 additions & 2 deletions xmodule/modulestore/mixed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locator import LibraryLocator
from openedx_events.content_authoring.data import XBlockData
from openedx_events.content_authoring.signals import XBLOCK_DELETED, XBLOCK_PUBLISHED

from django.utils.timezone import datetime, timezone
from xmodule.assetstore import AssetMetadata

from . import XMODULE_FIELDS_WITH_USAGE_KEYS, ModuleStoreWriteBase
Expand Down Expand Up @@ -797,7 +800,16 @@ def delete_item(self, location, user_id, **kwargs): # lint-amnesty, pylint: dis
Delete the given item from persistence. kwargs allow modulestore specific parameters.
"""
store = self._verify_modulestore_support(location.course_key, 'delete_item')
return store.delete_item(location, user_id=user_id, **kwargs)
item = store.delete_item(location, user_id=user_id, **kwargs)
# .. event_implemented_name: XBLOCK_DELETED
XBLOCK_DELETED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=location,
block_type=location.block_type,
)
)
return item

def revert_to_published(self, location, user_id):
"""
Expand Down Expand Up @@ -911,7 +923,16 @@ def publish(self, location, user_id, **kwargs):
Returns the newly published item.
"""
store = self._verify_modulestore_support(location.course_key, 'publish')
return store.publish(location, user_id, **kwargs)
item = store.publish(location, user_id, **kwargs)
# .. event_implemented_name: XBLOCK_PUBLISHED
XBLOCK_PUBLISHED.send_event(
time=datetime.now(timezone.utc),
xblock_info=XBlockData(
usage_key=location,
block_type=location.block_type,
)
)
navinkarkera marked this conversation as resolved.
Show resolved Hide resolved
return item

@strip_key
def unpublish(self, location, user_id, **kwargs):
Expand Down
84 changes: 83 additions & 1 deletion xmodule/modulestore/tests/test_mixed_modulestore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
from unittest.mock import Mock, call, patch

import ddt
from openedx_events.content_authoring.data import XBlockData
from openedx_events.content_authoring.signals import XBLOCK_DELETED, XBLOCK_PUBLISHED
from openedx_events.tests.utils import OpenEdxEventsTestMixin
import pymongo
import pytest
# Mixed modulestore depends on django, so we'll manually configure some django settings
Expand Down Expand Up @@ -63,7 +66,7 @@
log = logging.getLogger(__name__)


class CommonMixedModuleStoreSetup(CourseComparisonTest):
class CommonMixedModuleStoreSetup(CourseComparisonTest, OpenEdxEventsTestMixin):
"""
Quasi-superclass which tests Location based apps against both split and mongo dbs (Locator and
Location-based dbs)
Expand Down Expand Up @@ -109,6 +112,20 @@ class CommonMixedModuleStoreSetup(CourseComparisonTest):
],
'xblock_mixins': modulestore_options['xblock_mixins'],
}
ENABLED_OPENEDX_EVENTS = [
"org.openedx.content_authoring.xblock.deleted.v1",
"org.openedx.content_authoring.xblock.published.v1",
]

@classmethod
def setUpClass(cls):
"""
Set up class method for the Test class.
This method starts manually events isolation. Explanation here:
openedx/core/djangoapps/user_authn/views/tests/test_events.py#L44
"""
super().setUpClass()
cls.start_events_isolation()

def setUp(self):
mariajgrimaldi marked this conversation as resolved.
Show resolved Hide resolved
"""
Expand Down Expand Up @@ -724,6 +741,71 @@ def test_publish_automatically_after_delete_unit(self, default_ms):
self.store.delete_item(vertical.location, self.user_id)
assert not self._has_changes(sequential.location)

@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_xblock_publish_event(self, default_ms):
"""
Check that XBLOCK_PUBLISHED event is sent when xblock is published.
"""
self.initdb(default_ms)
event_receiver = Mock()
XBLOCK_PUBLISHED.connect(event_receiver)

test_course = self.store.create_course('test_org', 'test_course', 'test_run', self.user_id)

# create sequential and vertical to test against
sequential = self.store.create_child(self.user_id, test_course.location, 'sequential', 'test_sequential')
self.store.create_child(self.user_id, sequential.location, 'vertical', 'test_vertical')

# publish sequential changes
self.store.publish(sequential.location, self.user_id)

event_receiver.assert_called()
self.assertDictContainsSubset(
{
"signal": XBLOCK_PUBLISHED,
"sender": None,
"xblock_info": XBlockData(
usage_key=sequential.location,
block_type=sequential.location.block_type,
),
},
event_receiver.call_args.kwargs
)

@ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
def test_xblock_delete_event(self, default_ms):
"""
Check that XBLOCK_DELETED event is sent when xblock is deleted.
"""
self.initdb(default_ms)
event_receiver = Mock()
XBLOCK_DELETED.connect(event_receiver)

test_course = self.store.create_course('test_org', 'test_course', 'test_run', self.user_id)

# create sequential and vertical to test against
sequential = self.store.create_child(self.user_id, test_course.location, 'sequential', 'test_sequential')
vertical = self.store.create_child(self.user_id, sequential.location, 'vertical', 'test_vertical')

# publish sequential changes
self.store.publish(sequential.location, self.user_id)

# delete vertical
self.store.delete_item(vertical.location, self.user_id)

event_receiver.assert_called()
self.assertDictContainsSubset(
{
"signal": XBLOCK_DELETED,
"sender": None,
"xblock_info": XBlockData(
usage_key=vertical.location,
block_type=vertical.location.block_type,
),
},
event_receiver.call_args.kwargs
)

def setup_has_changes(self, default_ms):
"""
Common set up for has_changes tests below.
Expand Down