feat: adds events to handle changes in xblocks #143

navinkarkera · 2022-11-08T12:31:26Z

Description: Adds events and data classes to allow consumers to react on xblock events like published, deleted and duplicated.
The idea is to fire these events in delete_item, publish_item and _duplicate_item and update skills related to the xblock in taxonomy-connector (part of course-discovery).

Answer for a question asked in previous similar MRs:

Do consumers have to make separate requests to get more information?
Generally: Additional data if required related to the event needs to be fetch separately as it can be different for different use case.
Specifically for skill tagging: Yes, for skill tagging to work we need concatenated text representation of children xblocks for units and transcript for video xblocks which can be extracted using index_dictionary of the xblock. The idea is to add a new api to fetch block metadata and use this API in taxonomy-connector.

Related MRs:
These MRs are the foundation for storing skills related on xblocks level, specifically for unit and video xblocks.

ISSUE: Link to GitHub issue

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPI after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

openedx-webhooks · 2022-11-08T12:31:30Z

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 · 2022-11-10T15:41:43Z

Hello! Thanks for your contribution! I'll review this as soon as I can.

tecoholic

@navinkarkera Nice work covering all the necessary parts including the avro tests. I have noted one question about the data classes introduced here and some minor typos. Can you kindly address those?

openedx_events/content_authoring/data.py

openedx_events/content_authoring/signals.py

openedx_events/event_bus/avro/custom_serializers.py

tecoholic · 2022-11-11T11:28:38Z

@navinkarkera One thing that kind of stood out from the PR description was this question:

Do consumers have to make separate requests to get more information?

The answer is specific to skill tagging which is great. It can also be more generally answered, as the event is only passing the UsageKey to the consumer, all other metadata will need to be fetched by the consumer separately. Eg., User deleting an XBlock, diff of the content during publishing..etc.,

This is just a note, to help myself understand the scope of the change here and aid other reviewers, if needed. No action item here :)

navinkarkera · 2022-11-11T12:36:15Z

@tecoholic Thanks for the suggestions! Updated the MR.

The answer is specific to skill tagging which is great. It can also be more generally answered, as the event is only passing the UsageKey to the consumer, all other metadata will need to be fetched by the consumer separately. Eg., User deleting an XBlock, diff of the content during publishing..etc.,

Makes absolute sense. Thanks, i'll add this to the description.

navinkarkera · 2022-11-11T12:42:11Z

@mariajgrimaldi Sorry, when I clicked on request-a-review-again button for @tecoholic, it removed you from reviewers list and I cannot add you back.

Hello! Thanks for your contribution! I'll review this as soon as I can.

Thank you!

tecoholic

@navinkarkera 👍

I tested this: As this only introduces the data classes and the signals, there wasn't anything specific to test. The unit tests are passing, that validates the data classes.
I read through the code
I checked for accessibility issues - NA
Includes documentation - It contains inline documentation as expected.

openedx_events/content_authoring/data.py

openedx_events/event_bus/avro/tests/test_deserializer.py

mariajgrimaldi · 2022-11-21T11:24:48Z

do you have a working example for testing? if so, can you add instructions to the PR description? thanks!

navinkarkera · 2022-11-21T12:22:59Z

do you have a working example for testing? if so, can you add instructions to the PR description? thanks!

@mariajgrimaldi I don't have one yet as the work on the edx-platform side is pending. I do want to add one more signal and data class to send list of verified skill ids and xblock usage_key. The idea is that users will verify the tags/skills associated to an XBlock. We want to send this data via openedx-event signals to course-discovery and update the relevant tables. I was wondering whether I should add it in the same MR or create a separate MR once this is merged. Do let me know your thoughts here.

mariajgrimaldi · 2022-11-23T11:47:08Z

@navinkarkera: it's best to create a separate PR for those changes.

@felipemontoya: what do you think of these changes? I'd like to test a working version, for now, this looks quite good!

navinkarkera · 2022-11-28T07:14:26Z

@mariajgrimaldi Thanks! I have created a separate MR: open-craft#1 on top of this branch. The new event requires support for array types so it is also added in the MR. Please have a look.

navinkarkera · 2022-11-30T05:14:50Z

@mariajgrimaldi Any updates on this? Let me know if something needs to be changed.

navinkarkera · 2022-12-15T07:11:31Z

@mariajgrimaldi Gentle reminder.

mariajgrimaldi · 2022-12-15T07:37:14Z

@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. I dismissed my review so it's not a blocker.

I'm out of office and can't continue the review

mariajgrimaldi · 2022-12-19T10:43:31Z

can you rebase with main? thank you!

navinkarkera · 2022-12-19T13:58:58Z

can you rebase with main? thank you!

@mariajgrimaldi It is already up to date.

mariajgrimaldi · 2022-12-20T14:23:30Z

I thought rebasing would help with the coverage issue. Can you check that so we can merge? Let me know if you need help!

navinkarkera · 2022-12-21T11:44:29Z

I thought rebasing would help with the coverage issue. Can you check that so we can merge? Let me know if you need help!

@mariajgrimaldi Running make coverage locally shows that the coverage increased from 95% to 96%. Also visiting the codecov link also shows an increase of 0.13% as opposed to what is displayed here in github below.

Not sure how to proceed here. Should we implement tests for missing lines even if they are not related to any change in this MR? Please advice.

mphilbrick211 · 2022-12-27T17:17:00Z

@mariajgrimaldi Friendly ping on @navinkarkera's comment above.

mariajgrimaldi · 2022-12-30T22:46:52Z

I'm back on monday. I'll take a look at this then. Sorry for the delay!

mariajgrimaldi · 2023-01-03T13:51:53Z

hello! can you rebase with main? The codecov for the project changed, so I think it'll pass this time

navinkarkera · 2023-01-03T14:27:58Z

@mariajgrimaldi Updated! Please approve the workflow.

mariajgrimaldi

This looks great! Just one last thing, can you update the changelog entry date? thanks!

fix: documentation typo fix: remove unwanted comments and rename XBlockData docs: docstrings in tests for usage key serialization refactor: rename XBlockDuplicatedData to DuplicatedXBlockData chore: bump version and update changelog chore: bump version and update changelog fix: import order chore: bump version docs: generalize signal docs docs: update xblock_published signal docs

navinkarkera · 2023-01-03T14:49:08Z

@mariajgrimaldi Thanks! Updated.

openedx-webhooks · 2023-01-03T14:54:57Z

@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.

mariajgrimaldi · 2023-01-03T14:58:48Z

New release: https://github.com/openedx/openedx-events/releases/tag/v4.1.0

This commit combines changes implemented in following commits: - cc1889b - ec2a581 The changes were reverted as part of commit a999401 as the upstream MR in openedx-events: openedx/openedx-events#143 was not merged. It is now merged and the dependencies are udpated.

…enedx#138) This commit combines changes implemented in following commits: - cc1889b - ec2a581 The changes were reverted as part of commit a999401 as the upstream MR in openedx-events: openedx/openedx-events#143 was not merged. It is now merged and the dependencies are udpated.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 8, 2022

mariajgrimaldi self-assigned this Nov 10, 2022

mariajgrimaldi self-requested a review November 10, 2022 15:41

tecoholic suggested changes Nov 11, 2022

View reviewed changes

navinkarkera force-pushed the navin/skill-tagging-events branch from 0d1598d to 87822e2 Compare November 11, 2022 12:34

navinkarkera requested review from tecoholic and removed request for mariajgrimaldi November 11, 2022 12:40

tecoholic approved these changes Nov 11, 2022

View reviewed changes

mariajgrimaldi previously requested changes Nov 14, 2022

View reviewed changes

openedx_events/content_authoring/data.py Outdated Show resolved Hide resolved

openedx_events/content_authoring/data.py Outdated Show resolved Hide resolved

openedx_events/event_bus/avro/tests/test_deserializer.py Show resolved Hide resolved

navinkarkera force-pushed the navin/skill-tagging-events branch 2 times, most recently from 7d15816 to 7fbda3e Compare November 18, 2022 06:44

navinkarkera force-pushed the navin/skill-tagging-events branch from 7fbda3e to 9578c40 Compare November 21, 2022 12:15

navinkarkera requested a review from mariajgrimaldi November 21, 2022 12:23

navinkarkera force-pushed the navin/skill-tagging-events branch from 9578c40 to 32d9664 Compare November 23, 2022 12:45

This was referenced Nov 24, 2022

feat: adds xblock skills tagging models openedx/taxonomy-connector#119

Merged

feat: xblock skill verification event open-craft/openedx-events#1

Closed

navinkarkera mentioned this pull request Nov 28, 2022

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

Merged

1 task

navinkarkera force-pushed the navin/skill-tagging-events branch from 32d9664 to df0fc92 Compare November 30, 2022 05:13

This was referenced Nov 30, 2022

feat: xblock deleted and duplicated event handlers openedx/taxonomy-connector#127

Merged

feat: handlers for taxonomy xblock openedx-events openedx/course-discovery#3710

Closed

navinkarkera mentioned this pull request Dec 20, 2022

feat: refresh_xblock_skills management command openedx/taxonomy-connector#129

Merged

11 tasks

navinkarkera mentioned this pull request Dec 30, 2022

refactor: revert openedx-events related changes openedx/taxonomy-connector#136

Merged

navinkarkera force-pushed the navin/skill-tagging-events branch from 1934a83 to 6d86019 Compare January 3, 2023 14:27

mariajgrimaldi approved these changes Jan 3, 2023

View reviewed changes

navinkarkera force-pushed the navin/skill-tagging-events branch from 6d86019 to faad90a Compare January 3, 2023 14:48

mariajgrimaldi merged commit 75216de into openedx:main Jan 3, 2023

navinkarkera mentioned this pull request Jan 3, 2023

feat: refresh_xblock_skills and event handlers for openedx-events openedx/taxonomy-connector#138

Merged

7 tasks

navinkarkera mentioned this pull request Jan 4, 2023

feat: xblock skill verification event #165

Merged

9 tasks

pomegranited mentioned this pull request Jul 4, 2023

feat: add new content authoring event signals #244

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds events to handle changes in xblocks #143

feat: adds events to handle changes in xblocks #143

navinkarkera commented Nov 8, 2022 •

edited by mariajgrimaldi

Loading

openedx-webhooks commented Nov 8, 2022

mariajgrimaldi commented Nov 10, 2022

tecoholic left a comment

tecoholic commented Nov 11, 2022

navinkarkera commented Nov 11, 2022

navinkarkera commented Nov 11, 2022

tecoholic left a comment

mariajgrimaldi commented Nov 21, 2022

navinkarkera commented Nov 21, 2022

mariajgrimaldi commented Nov 23, 2022

navinkarkera commented Nov 28, 2022

navinkarkera commented Nov 30, 2022

navinkarkera commented Dec 15, 2022

mariajgrimaldi commented Dec 15, 2022 •

edited

Loading

mariajgrimaldi commented Dec 19, 2022

navinkarkera commented Dec 19, 2022

mariajgrimaldi commented Dec 20, 2022

navinkarkera commented Dec 21, 2022

mphilbrick211 commented Dec 27, 2022

mariajgrimaldi commented Dec 30, 2022

mariajgrimaldi commented Jan 3, 2023

navinkarkera commented Jan 3, 2023

mariajgrimaldi left a comment

navinkarkera commented Jan 3, 2023

openedx-webhooks commented Jan 3, 2023

mariajgrimaldi commented Jan 3, 2023

feat: adds events to handle changes in xblocks #143

feat: adds events to handle changes in xblocks #143

Conversation

navinkarkera commented Nov 8, 2022 • edited by mariajgrimaldi Loading

openedx-webhooks commented Nov 8, 2022

mariajgrimaldi commented Nov 10, 2022

tecoholic left a comment

Choose a reason for hiding this comment

tecoholic commented Nov 11, 2022

navinkarkera commented Nov 11, 2022

navinkarkera commented Nov 11, 2022

tecoholic left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Nov 21, 2022

navinkarkera commented Nov 21, 2022

mariajgrimaldi commented Nov 23, 2022

navinkarkera commented Nov 28, 2022

navinkarkera commented Nov 30, 2022

navinkarkera commented Dec 15, 2022

mariajgrimaldi commented Dec 15, 2022 • edited Loading

mariajgrimaldi commented Dec 19, 2022

navinkarkera commented Dec 19, 2022

mariajgrimaldi commented Dec 20, 2022

navinkarkera commented Dec 21, 2022

mphilbrick211 commented Dec 27, 2022

mariajgrimaldi commented Dec 30, 2022

mariajgrimaldi commented Jan 3, 2023

navinkarkera commented Jan 3, 2023

mariajgrimaldi left a comment

Choose a reason for hiding this comment

navinkarkera commented Jan 3, 2023

openedx-webhooks commented Jan 3, 2023

mariajgrimaldi commented Jan 3, 2023

navinkarkera commented Nov 8, 2022 •

edited by mariajgrimaldi

Loading

mariajgrimaldi commented Dec 15, 2022 •

edited

Loading