feat: xblock deleted and duplicated event handlers #127

navinkarkera · 2022-11-30T11:01:00Z

Description

Implement signals and handlers for xblock delete and duplicated events.

Related MRs:

Merge checklist:

Any new requirements are in the right place (do not manually modify the requirements/*.txt files)
- make upgrade && make requirements have been run to regenerate requirements
./manage.py makemigrations has been run
- Checkout the Database Migration Confluence page for helpful tips on creating migrations.
- Note: This must be run if you modified any models.
  - It may or may not make a migration depending on exactly what you modified, but it should still be run.
Version bumped
Changelog record added

Post merge:

Tag pushed and a new version released
- Note: Assets will be added automatically. You just need to provide a tag (should match your version number) and title and description.
After versioned build finishes in GitHub Actions, verify version has been pushed to PyPI
- Each step in the release build has a condition flag that checks if the rest of the steps are done and if so will deploy to PyPi.
  (so basically once your build finishes, after maybe a minute you should see the new version in PyPi automatically (on refresh))
PR created in course-discovery to upgrade dependencies (including taxonomy-connector)
- This must be done after the version is visible in PyPi as make upgrade in course-discovery will look for the latest version in PyPi.

openedx-webhooks · 2022-11-30T11:01:06Z

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.

taxonomy/signals/handlers.py

tecoholic

@navinkarkera I have some design questions and suggested some changes. Kindly see the inline comments.

tecoholic · 2022-12-02T05:01:26Z

taxonomy/signals/signals.py

@@ -8,3 +8,5 @@
 UPDATE_COURSE_SKILLS = Signal()
 UPDATE_PROGRAM_SKILLS = Signal()
 UPDATE_XBLOCK_SKILLS = Signal()
+XBLOCK_DELETED = Signal()
+XBLOCK_DUPLICATED = Signal()


I am unsure about these signals for the reason I mentioned in the course discovery PR and also because, these signals cannot be actually emitted by the taxonomy connector for other apps to listen.

What the taxonomy connector could possibly emit are DELETE_XBLOCK_SKILLS, DUPLICATE_XBLOCK_SKILLS at the start of the processing or XBLOCK_SKILLS_DELETED, XBLOCK_SKILLS_DUPLICATED at the end of the processing depending on the kind of signal that's emitted.

Again, it seems the only reason for these signals to be defined here, is to allow for them to be handled in handlers.py. I would suggest we think if this is actually necessary. Wouldn't it be better to not define them at all and do the following in handlers.py?

from .signals import ( UPDATE_COURSE_SKILLS, UPDATE_PROGRAM_SKILLS, UPDATE_XBLOCK_SKILLS, - XBLOCK_DELETED, - XBLOCK_DUPLICATED, ) + from openedx_events.content_authoring.signals import XBLOCK_DELETED, XBLOCK_DUPLICATED, XBLOCK_PUBLISHED

I guess the answer again is in the question, why can't we add openedx-events as a dependency to the taxonomy-connector.

@tecoholic I don't have a strong reason to not include openedx-events as a dependency to taxonomy-connector. openedx-events is already added as a dependency in course-discovery through event-bus-kafka so I did not include it here. This also keeps taxonomy-connector independent of openedx-events if that matters at all. I think we can ask @sameenfatima78 for her advice here.

Discussed this with @saleem-latif and while we both agree with using standardized signals as suggested by @tecoholic, we are still unsure about how taxonomy-connector would listen these events if these signals are not being fired from course-discovery? But other than that, installing this new dependency in taxonomy-connector won't be an issue.

@sameenfatima78

we are still unsure about how taxonomy-connector would listen these events if these signals are not being fired from course-discovery

In the current design, they are fired from course-discovery as implemented in openedx/course-discovery#3710. But if we directly include openedx-events as dependency in taxonomy, it is not required. The events fired in openedx/edx-platform#31350 can be directly handled here.

@tecoholic @sameenfatima78 Updated to include openedx-events directly as dependency and handle xblock signals. Please review.

Please re-run the failed CI job, the test has nothing to do with this MR (it is using random values for primary keys which fails sometimes due to not being unique).

taxonomy/utils.py

taxonomy/tasks.py

taxonomy/utils.py

taxonomy/signals/handlers.py

tecoholic

@navinkarkera 👍 I found one place where the docstring needs a correction. Other than that this looks great.

I tested this: relying on the unit tests here as there aren't any testing instructions
I read through the code
I checked for accessibility issues - NA
Includes documentation

PS: You have mentioned that the CI Error, I ran the unit tests locally and everything seems to pass. My guess is the Faker.rand_int might have generated the same ID in the failing test.

sameenfatima78

Left some feedback, also the coverage seems to be failing.

taxonomy/utils.py

navinkarkera · 2022-12-12T12:38:12Z

Left some feedback, also the coverage seems to be failing.

~~The changes in this MR has 100% coverage as seen in coverage:patch section of CI.~~
Sorry, got confused with a different MR. Will fix it.

Implements handlers for openedx-events XBLOCK_PUBLISHED, XBLOCK_DELETED, and XBLOCK_DUPLICATED. chore: bump version and update changelog Update taxonomy/signals/handlers.py Co-authored-by: Arunmozhi <tecoholic@users.noreply.github.com>

navinkarkera · 2022-12-12T13:41:23Z

@sameenfatima78 Updated MR. Also implemented tests for missing lines but the check is still failing here.
If we open the link for pull request in codecov, it shows that 100% coverage. So not sure what ti do here.

sameenfatima78 · 2022-12-14T17:41:55Z

@sameenfatima78 Updated MR. Also implemented tests for missing lines but the check is still failing here. If we open the link for pull request in codecov, it shows that 100% coverage. So not sure what ti do here.

@navinkarkera Hmm, I remember there was a known issue where coverage only gets calculated for the first commit, the rest of the commits are ignored by CI. This seems to be the case here I guess. You can merge the PR for now and I'll file a ticket in our backlog to look into this issue.

navinkarkera · 2022-12-15T07:07:07Z

You can merge the PR for now and I'll file a ticket in our backlog to look into this issue.

@sameenfatima78 I don't have appropriate rights to merge. 🚫

sameenfatima78 · 2022-12-15T07:58:43Z

You can merge the PR for now and I'll file a ticket in our backlog to look into this issue.

@sameenfatima78 I don't have appropriate rights to merge. 🚫

I'll merge it for you. Just wanted to confirm if all changes are finalized.

navinkarkera · 2022-12-15T09:25:37Z

I'll merge it for you. Just wanted to confirm if all changes are finalized.

@sameenfatima78 Confirmed. 👍

openedx-webhooks · 2022-12-15T10:03:40Z

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

HammadAhmadWaqas · 2022-12-27T13:47:22Z

@navinkarkera
Version change is missing in init.py from this PR. So, It was never deployed in course-discovery before.
Today I created a version bump PR in course-discovery which includes your code and found out there is a problem with code merged in this PR. Could you please have a look?

Traceback (most recent call last):
  File "manage.py", line 15, in <module>
    execute_from_command_line(sys.argv)
  File "/edx/app/discovery/discovery/.tox/py38-django32/lib/python3.8/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line
    utility.execute()
  File "/edx/app/discovery/discovery/.tox/py38-django32/lib/python3.8/site-packages/django/core/management/__init__.py", line 395, in execute
    django.setup()
  File "/edx/app/discovery/discovery/.tox/py38-django32/lib/python3.8/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/edx/app/discovery/discovery/.tox/py38-django32/lib/python3.8/site-packages/django/apps/registry.py", line 122, in populate
    app_config.ready()
  File "/edx/app/discovery/discovery/.tox/py38-django32/lib/python3.8/site-packages/taxonomy/apps.py", line 25, in ready
    from .signals import handlers  # pylint: disable=unused-import,import-outside-toplevel
  File "/edx/app/discovery/discovery/.tox/py38-django32/lib/python3.8/site-packages/taxonomy/signals/handlers.py", line 8, in <module>
    from openedx_events.content_authoring.data import DuplicatedXBlockData, XBlockData
ImportError: cannot import name 'DuplicatedXBlockData' from 'openedx_events.content_authoring.data' (/edx/app/discovery/discovery/.tox/py38-django32/lib/python3.8/site-packages/openedx_events/content_authoring/data.py)

navinkarkera · 2022-12-28T07:07:43Z

@HammadAhmadWaqas Apologies for the confusion. I missed to update the dependency in base.in, instead updated dev.txt and test.txt directly to point to the version of openedx-events. This issue should be fixed by #129 as it updates init.py as well as the requirements properly. cc: @sameenfatima78

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

navinkarkera mentioned this pull request Nov 30, 2022

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

Closed

2 tasks

tecoholic reviewed Dec 2, 2022

View reviewed changes

taxonomy/signals/handlers.py Outdated Show resolved Hide resolved

tecoholic suggested changes Dec 2, 2022

View reviewed changes

navinkarkera force-pushed the navin/events-handler branch from a65e300 to a4a07f0 Compare December 5, 2022 10:00

tecoholic reviewed Dec 5, 2022

View reviewed changes

taxonomy/tasks.py Outdated Show resolved Hide resolved

tecoholic reviewed Dec 5, 2022

View reviewed changes

taxonomy/tasks.py Outdated Show resolved Hide resolved

tecoholic reviewed Dec 5, 2022

View reviewed changes

taxonomy/utils.py Show resolved Hide resolved

navinkarkera force-pushed the navin/events-handler branch from 63824f3 to 67b4d64 Compare December 6, 2022 13:08

tecoholic reviewed Dec 9, 2022

View reviewed changes

taxonomy/signals/handlers.py Outdated Show resolved Hide resolved

tecoholic approved these changes Dec 9, 2022

View reviewed changes

navinkarkera force-pushed the navin/events-handler branch from 596e3b2 to ef1223c Compare December 9, 2022 11:48

sameenfatima78 reviewed Dec 12, 2022

View reviewed changes

taxonomy/utils.py Outdated Show resolved Hide resolved

taxonomy/utils.py Show resolved Hide resolved

taxonomy/utils.py Show resolved Hide resolved

navinkarkera force-pushed the navin/events-handler branch from ef1223c to 72ba366 Compare December 12, 2022 12:37

navinkarkera force-pushed the navin/events-handler branch from 72ba366 to cc1889b Compare December 12, 2022 13:31

sameenfatima78 closed this Dec 14, 2022

sameenfatima78 reopened this Dec 14, 2022

sameenfatima78 approved these changes Dec 14, 2022

View reviewed changes

sameenfatima78 merged commit c7e8dc9 into openedx:master Dec 15, 2022

navinkarkera deleted the navin/events-handler branch December 15, 2022 10:06

navinkarkera mentioned this pull request Dec 27, 2022

feat: refresh_xblock_skills management command #129

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: xblock deleted and duplicated event handlers #127

feat: xblock deleted and duplicated event handlers #127

navinkarkera commented Nov 30, 2022 •

edited

Loading

openedx-webhooks commented Nov 30, 2022

tecoholic left a comment

tecoholic Dec 2, 2022

navinkarkera Dec 5, 2022

sameenfatima78 Dec 6, 2022

navinkarkera Dec 6, 2022

navinkarkera Dec 6, 2022 •

edited

Loading

tecoholic left a comment

sameenfatima78 left a comment

navinkarkera commented Dec 12, 2022 •

edited

Loading

navinkarkera commented Dec 12, 2022

sameenfatima78 commented Dec 14, 2022

navinkarkera commented Dec 15, 2022

sameenfatima78 commented Dec 15, 2022

navinkarkera commented Dec 15, 2022

openedx-webhooks commented Dec 15, 2022

HammadAhmadWaqas commented Dec 27, 2022

navinkarkera commented Dec 28, 2022

feat: xblock deleted and duplicated event handlers #127

feat: xblock deleted and duplicated event handlers #127

Conversation

navinkarkera commented Nov 30, 2022 • edited Loading

Description

openedx-webhooks commented Nov 30, 2022

tecoholic left a comment

Choose a reason for hiding this comment

tecoholic Dec 2, 2022

Choose a reason for hiding this comment

navinkarkera Dec 5, 2022

Choose a reason for hiding this comment

sameenfatima78 Dec 6, 2022

Choose a reason for hiding this comment

navinkarkera Dec 6, 2022

Choose a reason for hiding this comment

navinkarkera Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

tecoholic left a comment

Choose a reason for hiding this comment

sameenfatima78 left a comment

Choose a reason for hiding this comment

navinkarkera commented Dec 12, 2022 • edited Loading

navinkarkera commented Dec 12, 2022

sameenfatima78 commented Dec 14, 2022

navinkarkera commented Dec 15, 2022

sameenfatima78 commented Dec 15, 2022

navinkarkera commented Dec 15, 2022

openedx-webhooks commented Dec 15, 2022

HammadAhmadWaqas commented Dec 27, 2022

navinkarkera commented Dec 28, 2022

navinkarkera commented Nov 30, 2022 •

edited

Loading

navinkarkera Dec 6, 2022 •

edited

Loading

navinkarkera commented Dec 12, 2022 •

edited

Loading