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

[navinkarkera]

Description

openedx-events: XBLOCK_DELETED, XBLOCK_DUPLICATED and XBLOCK_PUBLISHED are hooked into appropriate functions to publish xblock changes.

These signals are implemented in openedx/openedx-events#143 to handle changes to xblocks in taxonomy-connector/course-discovery.

Supporting information

• feat: adds events to handle changes in xblocks openedx-events#143
• Workflow document: docs: design workflow for xblock skill tagging taxonomy-connector#140

Testing instructions

1. Add below snippet somewhere, for example in cms/djangoapps/contentstore/signals/handlers.py

from openedx_events.content_authoring.signals import XBLOCK_PUBLISHED, XBLOCK_DELETED, XBLOCK_DUPLICATED
XBLOCK_PUBLISHED.connect(lambda **x: print(x))
XBLOCK_DELETED.connect(lambda **x: print(x))
XBLOCK_DUPLICATED.connect(lambda **x: print(x))

2. Start lms and studio and monitor studio logs using make lms-up studio-up studio-logs
3. Publish, delete and duplicate some xblocks in studio and related signals should be printed in logs.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Pre-merge checklist:

• merge feat: adds events to handle changes in xblocks openedx-events#143

[openedx-webhooks]

Thanks for the pull request, @navinkarkera! 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:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

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.

[mariajgrimaldi]

You don't have to write a step-by-step guide of how to test the events with receivers and all, but at least you could describe how to trigger them, so it's easier to test.

[navinkarkera]

@mariajgrimaldi Sorry, I am not in front of my system now but I'll soon figure out steps to test and post it.

[tecoholic]

@navinkarkera Changes look good to me at first pass. But the CI is failing. But it doesn't seem to connected to the changes in this PR, but instead some dependencies in enterprise. Can you kindly rebase this to master and see if the CI passes?

[tecoholic]

@mariajgrimaldi What would be right way to test the values these events get, assuming @navinkarkera adds the instructions on how to trigger these events? Do we have something like a openedx-events-debugger package that we can just install, to log all the events and the event data?

[mariajgrimaldi]

@tecoholic: we added a line that logs the event responses for just triggers when in a production environment, so it's not that useful for dev (I'll make a mental note on that). What a usually do is create a dummy receiver that logs the input data, that could work here.

[tecoholic]

@mariajgrimaldi Thank you for the dummy receiver suggestion. I will try to add something like that for testing.

@navinkarkera Everything looks good to me. I will test this tomorrow.

[navinkarkera]

@mariajgrimaldi @tecoholic I have added test instructions in description. While doing so I found that publish signal is only emitted for vertical and higher level blocks like sequential. I expected it to publish each xblock in vertical separately.

We want to be notified about the published vertical and video blocks. If this is not possible here, we would need to handle this in consumer. I'll investigate bit more and come back on this.

[tecoholic]

@navinkarkera

I have added test instructions in description. While doing so I found that publish signal is only emitted for vertical and higher level blocks like sequential. I expected it to publish each xblock in vertical separately.

This is understandable given the smallest content we can publish from the Studio is a Unit (VerticalBlock). Good luck with your testing.

[navinkarkera]

@tecoholic

This is understandable given the smallest content we can publish from the Studio is a Unit (VerticalBlock). Good luck with your testing.

Makes sense. We are now handling this in the xblock provider implemented in course-discovery ( ref MR: openedx/course-discovery#3692 ). It fetches all vertical and video blocks under the published item and processes it.

[tecoholic]

@navinkarkera 👍 LGTM

• I tested this: Verified that the signals are being sent when changes are made in studio
• I read through the code
• I checked for accessibility issues - NA
• Includes documentation

[navinkarkera]

@mariajgrimaldi Gentle reminder.

[mariajgrimaldi]

@navinkarkera: I'm out until Dec 20, I'll check on this when I'm back. Sorry for the delay.

Unless @felipemontoya can pick this up.

[mphilbrick211]

Friendly ping on this @mariajgrimaldi :)

[mariajgrimaldi]

Hello! I was out on vacation. I'll be checking these comments right away :)

[mariajgrimaldi]

hello @mphilbrick211: should someone from Enterprise Markhors approve this PR also? According to this comment, they're also involved in this work, and we'll probably need their support for merging.
@sameenfatima78 could you give us a hand?

[mariajgrimaldi]

@navinkarkera: exciting work! Thanks for all the info context. I'm happy with the state of the PR and all the discussion surrounding it. Do you know if there is anything else that should be addressed?

[mariajgrimaldi]

I reran the tests, but they kept failing. It seems related to the runner / system itself 🤔.

Update: it seems to be working now :)

[sameenfatima78]

@sameenfatima78 could you give us a hand?

@mariajgrimaldi Sure, I'll let my team know and will also take a look myself.

[navinkarkera]

@navinkarkera: exciting work! Thanks for all the info context. I'm happy with the state of the PR and all the discussion surrounding it. Do you know if there is anything else that should be addressed?

@mariajgrimaldi Thanks! Nothing to add from my side.

[navinkarkera]

@mariajgrimaldi Updated the MR. It should be ready for merge, let me know if something is missing.

[mariajgrimaldi]

I'll contact the team in charge, so they merge when convenient. Thank you again for your work!

[openedx-webhooks]

@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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

[dianakhuang]

Hi! Would it be possible to put these events behind a feature flag for now? edx.org isn't set up for them, and we're seeing some errors because of it.

[mariajgrimaldi]

@dianakhuang: hey there! thanks for the report. @sameenfatima78 @navinkarkera what do you say?

We could do something like:

    if settings.FEATURES["ENABLE_SEND_XBLOCK_EVENTS_OVER_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'],
        )

Let me know if you need help with this! I'd be glad to help.

[navinkarkera]

@mariajgrimaldi Thanks, created #31813. Please review and merge.

cc: @sameenfatima78 @dianakhuang