-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: add new content event signals #32599
feat: add new content event signals #32599
Conversation
Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
5458929
to
048dc74
Compare
33f2816
to
8dafbc6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good @rpenido ! We have an open discussion about the changed library events, and this suggestion plus these changes should fix the test failures.
Could you also please update this PR to give some more context on why we're making these changes? These signals will be used by the Content Tagging app, to trigger updates on any automatically-generated system taxonomy tags.
You can reference this openedx/modular-learning#61 in the More Info section too.
3a44f7f
to
ea01a63
Compare
d3bd997
to
d758871
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido This is working well and looking good. ☀️
There's a couple of minor changes to be made and one outstanding question:
- remove the
time
param from thesend_event
calls in xmodule and content_libraries. - should we strip out the block usage_key
version
and add it as a kwarg?
Your instructions for testing block creation/updates worked exactly as described, and I was able to test the content libraries events using the steps below. Running the tests locally is also working for me. I'll approve once the changes/questions are resolved. 👍
Steps to enable and test content libraries:
-
Add
frontend-app-library-authoring
to the list of services you're running along with Studio et al -
Add the following to a new
cms/envs/private.py
file (may need to restart studio to pick up these changes, usingmake studio-restart
:from .devstack import FEATURES FEATURES['ENABLE_LIBRARY_AUTHORING_MICROFRONTEND'] = True BLOCKSTORE_API_AUTH_TOKEN = 'edxapp-insecure-devstack-key'
-
Set up the blockstore to run in devstack (ref) by running the following from the studio shell (
make studio-shell
):# Configure blockstore to run inside LMS/Studio (instead of as an external service) ./manage.py cms waffle_switch --create blockstore.use_blockstore_app_api on
./manage.py cms shell # enter python code below # Create a "Collection" that new content libraries / xblocks can be created within: from blockstore.apps.bundles.models import Collection coll, _ = Collection.objects.get_or_create(title='Devstack Content Collection', uuid='11111111-2111-4111-8111-111111111111') # Create an "Organization": from organizations.models import Organization Organization.objects.get_or_create(short_name='DeveloperInc', defaults={'name': 'DeveloperInc', 'active': True})
-
Now, you can login to Studio, and visit the Libraries tab, it should take you to Content Libraries V2 MFE: http://localhost:3001
(This is still very much WIP, and so it's not very nice to use yet.) -
Create a new library for the Org created above, and watch the "library created" event happen in the Studio logs
-
Update the library, and watch for the event
-
Add a block to the library, e.g Text block, and watch for the "library block created" event
-
Update the block, and watch for the "library block updated" event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido Nice work! Just added a suggestion regarding event handlers, everything else looks good. Please rebase and resolve the conflicts. Noting down few observations.
- Noticed a weird behaviour when updating a vertical xblock. If you change the name and press enter,
XBLOCK_UPDATED
event is fired twice (probably the block is saved twice). If you change the name and click outside, the block is saved once and a single event is fired. Not sure if it has major effect but just wanted to bring this up. XBLOCK_UPDATED
as well asXBLOCK_PUBLISHED
events are fired whenever an XBlock is published.
6bbc3da
to
0e4233e
Compare
1ad5bc5
to
d2f60df
Compare
@navinkarkera I think this is ready for review again! |
openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py
Outdated
Show resolved
Hide resolved
@rpenido Please change the status from |
57142c2
to
41ab164
Compare
c1c35b3
to
6a3cf9a
Compare
Done @navinkarkera ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- I tested this: emits events as expected
- I read through the code
-
I checked for accessibility issues - Includes documentation
-
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-secure
repository.
6a3cf9a
to
900a49a
Compare
900a49a
to
2593d5a
Compare
@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Use the following
OpenEdxPublicSignal
events triggered by content creation and editing.These events will be used in the Content Tagging app for System-defined automatic tagging to trigger updates on any automatically-generated system taxonomy tags.
This PR introduces a breaking change because it changes the following events from
django.Signal
toOpenEdxPublicSignal
:Supporting information
Testing instructions
Get latest version of
openedx-events
:openedx-events
repository to~/workspace/src/openedx-events
Setup
frontend-app-library-authoring
if not done before:cms/envs/private.py
file.make studio-shell
):Then:
lms
andstudio
usingmake dev.up.lms+studio+frontend-app-learning+frontend-app-library-authoring
cms/djangoapps/contentstore/signals/handlers.py
:http://localhost:18010/
and login with[email protected]
/edx
studio
logs monitoring:Check the log for
COURSE_CREATED
and theXBLOCK_UPDATED
event dataCheck the log for
XBLOCK_CREATED
event dataCheck the log for
XBLOCK_CREATED
event dataCheck the log for
XBLOCK_UPDATED
event dataCheck the log for
XBLOCK_CREATED
event dataCheck the log for
CONTENT_LIBRARY_CREATED
event dataCheck the log for
CONTENT_LIBRARY_UPDATED
event dataCheck the log for
LIBRARY_BLOCK_CREATED
event dataCheck the log for
LIBRARY_BLOCK_UPDATED
event dataDeadline
TBD
Other information
Depends on openedx/openedx-events#244