Skip to content

feat: handlers for taxonomy xblock openedx-events - #3710

Closed
navinkarkera wants to merge 4 commits into
openedx:masterfrom
open-craft:navin/xblock-events-handler
Closed

feat: handlers for taxonomy xblock openedx-events#3710
navinkarkera wants to merge 4 commits into
openedx:masterfrom
open-craft:navin/xblock-events-handler

Conversation

@navinkarkera

Copy link
Copy Markdown
Contributor

Description

Implements handlers to catch openedx-events related to xblocks like deletion, duplication and publish. Currently these handlers just trigger related signals provided by taxonomy-connectors which do the leg work of updating xblock skills.

Private-ref: BB-6931

Dependencies:

Merge checklist

  • Merge all dependencies.
  • Update taxonomy-connector version

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

Copy link
Copy Markdown

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.

@mphilbrick211

Copy link
Copy Markdown

@ansabgillani could you please take a look and allow tests to run?

@tecoholic tecoholic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@navinkarkera While the code is straightforward, I am wondering about the necessity of this new handler. This feels like a glue code that does the following:

                                 even handler                       event handler  
|---------------|   signal   +-------------------+  signal   +--------------------+
|openedx-events |----------->| Course Discovery  | --------->| taxonomy connector |
|---------------|            +-------------------+           +--------------------+

Is there a reason why we cannot simply add openedx-events directly as a dependency to the taxonomy connector and achieve something like this?

                                                   
                                    event handler  
|---------------|   signal   +--------------------+
|openedx-events |------------| taxonomy connector |
|---------------|            +--------------------+

Comment thread course_discovery/apps/taxonomy_support/handlers.py Outdated
Comment thread course_discovery/apps/taxonomy_support/handlers.py Outdated
@navinkarkera

Copy link
Copy Markdown
Contributor Author

@navinkarkera While the code is straightforward, I am wondering about the necessity of this new handler. This feels like a glue code that does the following:

                                 even handler                       event handler  
|---------------|   signal   +-------------------+  signal   +--------------------+
|openedx-events |----------->| Course Discovery  | --------->| taxonomy connector |
|---------------|            +-------------------+           +--------------------+

Is there a reason why we cannot simply add openedx-events directly as a dependency to the taxonomy connector and achieve something like this?

                                                   
                                    event handler  
|---------------|   signal   +--------------------+
|openedx-events |------------| taxonomy connector |
|---------------|            +--------------------+

@tecoholic Like I mentioned in openedx/taxonomy-connector#127 (comment), openedx-events is already a dependency in course-discovery so I did not include it in taxonomy-connector.

Implements handlers to catch openedx-events related to xblocks like
deletion, duplication and publish. Currently these handlers just trigger
related signals provided by taxonomy-connectors which do the leg work of
updating xblock skills.
@navinkarkera
navinkarkera force-pushed the navin/xblock-events-handler branch from e89739a to ef68e4d Compare December 5, 2022 10:17
@navinkarkera

Copy link
Copy Markdown
Contributor Author

Closing as the signals are now directly handled in openedx/taxonomy-connector#127

@openedx-webhooks

Copy link
Copy Markdown

@navinkarkera Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants