feat: upstream-downstream entity linking #36111

navinkarkera · 2025-01-15T14:48:41Z

Description

Adds tasks, signal handlers and api's for adding, updating and deleting upstream->downstream links in database.

Related openedx-learning PR: feat: model for upstream-downstream links openedx-learning#269

Supporting information

Private-ref: FAL-4004
Implement backend for tracking upstream-downstream content links modular-learning#243

Testing instructions

Checkout this PR.
Run tutor images build openedx-dev && tutor dev launch -I --skip-build in your tutor nightly setup.
Verify the new database tables by visiting django adming: http://studio.local.openedx.io:8001/admin/oel_linking/
To test new management command run
- tutor dev exec cms ./manage.py cms recreate_upstream_links --all for creating upstream links for all courses.
- tutor dev exec cms ./manage.py cms recreate_upstream_links --course <course-id> for creating upstream links for a course.
- use --force argument to recreate upstream links for already indexed courses.
Verify the links by visiting the admin page again: http://studio.local.openedx.io:8001/admin/oel_linking/
To test automatic creation of these links for new blocks, create new xblock using Library content or Problem bank component or Copy-pasting any library content in any course. The corresponding links should be created in database.
Test update of link versions by updating upstream library block and accepting or rejecting the change in course XBlock.
Test duplicating a block with upstream, copying and pasting a block with upstream.
Deleting an XBlock should delete its link.
Test course name update in links by updating course name in Advanced Settings page.
Test course import -> This does not work! Working on fixing it

Deadline

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

Other information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
If your database migration can't be rolled back easily.

openedx-webhooks · 2025-01-15T14:48:46Z

Thanks for the pull request, @navinkarkera!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

pomegranited

This is mostly working :) But when I import a course, the links don't get updated, and I'm not sure why not.

Most of my comments are nits about reducing unneeded log volume, or making logs more informative.

pomegranited · 2025-01-17T05:04:01Z

openedx/core/djangoapps/content/course_overviews/signals.py

@@ -26,6 +26,8 @@
 IMPORT_COURSE_DETAILS = Signal()
 # providing_args=["courserun_key"]
 DELETE_COURSE_DETAILS = Signal()
+# providing_args=["courserun_key", "old_name", "new_name"]
+COURSE_NAME_CHANGED = Signal()


Hmm.. I think we're supposed to put new signals in https://github.com/openedx/openedx-events so they can be used outside of the platform? But no worries if this is beyond the scope of this PR.

pomegranited · 2025-01-17T05:04:42Z

openedx/core/djangoapps/content/course_overviews/signals.py

+    # update_course_name_in_upstream_links.delay(
+    #     str(previous_course_overview.id),
+    #     updated_course_overview.display_name_with_default
+    # )


TODO: remove commented-out code

pomegranited · 2025-01-17T05:07:46Z

openedx/core/djangoapps/content_libraries/api.py

+    Create or update upstream->downstream link in database for given xblock.
+    """
+    if not xblock.upstream:
+        log.info(f"No upstream found for xblock: {xblock.usage_key}")


nit: this will generate a lot of log messages when people change course names, because most course blocks aren't upstream-linked.

Suggested change

log.info(f"No upstream found for xblock: {xblock.usage_key}")

pomegranited · 2025-01-17T05:08:27Z

openedx/core/djangoapps/content_libraries/api.py

+    except ObjectDoesNotExist:
+        log.exception("Library block not found!")
+        lib_component = None
+    authoring_api.update_or_create_entity_link(


If we're returning None when we do nothing, shouldn't this return something truthy?

pomegranited · 2025-01-17T05:11:33Z

openedx/core/djangoapps/content_libraries/signal_handlers.py

+@receiver(XBLOCK_CREATED)
+@receiver(XBLOCK_UPDATED)
+def create_or_update_upstream_downstream_link_handler(**kwargs):
+    """
+    Automatically create or update upstream->downstream link in database.
+    """
+    xblock_info = kwargs.get("xblock_info", None)
+    if not xblock_info or not isinstance(xblock_info, XBlockData):
+        log.error("Received null or incorrect data for event")
+        return
+
+    create_or_update_xblock_upstream_link.delay(str(xblock_info.usage_key))


I expected that this handler would take care of creating links when importing a course, but it didn't! I only saw a log message for the top-level course block:

No upstream or upstream_version found for xblock: block-v1:OpenCraft+DemoX+1+type@course+block@course

I had to run the management command to set my imported course links.

pomegranited · 2025-01-17T05:26:47Z

openedx/core/djangoapps/content_libraries/tasks.py

+    ensure_cms("create_or_update_xblock_upstream_link may only be executed in a CMS context")
+    xblock = modulestore().get_item(UsageKey.from_string(usage_key))
+    if not xblock.upstream or not xblock.upstream_version:
+        TASK_LOGGER.info(f"No upstream or upstream_version found for xblock: {xblock.usage_key}")


ditto nit: this will generate a lot of log messages when people update their blocks -- I think we should remove this line.

Suggested change

TASK_LOGGER.info(f"No upstream or upstream_version found for xblock: {xblock.usage_key}")

pomegranited · 2025-01-17T05:47:59Z

openedx/core/djangoapps/content_libraries/api.py

+    try:
+        lib_component = get_component_from_usage_key(upstream_usage_key)
+    except ObjectDoesNotExist:
+        log.exception("Library block not found!")


I don't think we need a log.exception here, because this could happen as part of normal operation, e.g if the lib component gets deleted. But it would be good to know a bit more about what it means:

Suggested change

log.exception("Library block not found!")

log.error(f"Library component not found for {upstream_usage_key} -- removing from upstream link")

pomegranited · 2025-01-17T05:50:53Z

openedx/core/djangoapps/content_libraries/tasks.py

+    try:
+        course_name = CourseOverview.get_from_id(xblock.course_id).display_name_with_default
+    except CourseOverview.DoesNotExist:
+        TASK_LOGGER.exception(f'Could not find course: {xblock.course_id}')


Do we need to log the whole exception? I think an error is enough:

Suggested change

TASK_LOGGER.exception(f'Could not find course: {xblock.course_id}')

TASK_LOGGER.error(f'Could not find course: {xblock.course_id}')

pomegranited · 2025-01-17T05:51:32Z

openedx/core/djangoapps/content_libraries/tasks.py

+    try:
+        course_name = CourseOverview.get_from_id(course_key).display_name_with_default
+    except CourseOverview.DoesNotExist:
+        TASK_LOGGER.exception(f'Could not find course: {course_key_str}')


ditto nit: do we need to log the whole exception? I don't think it will tell us much more than the error message does.
And shouldn't we mark the task as failed?

Suggested change

TASK_LOGGER.exception(f'Could not find course: {course_key_str}')

TASK_LOGGER.error(f'Could not find course: {course_key_str}')

course_status.status = CourseLinksStatusChoices.FAILED

course_status.save()

pomegranited · 2025-01-17T05:55:49Z

openedx/core/djangoapps/content_libraries/tasks.py

+    updated_time = datetime.now(timezone.utc)
+    get_entity_links({"downstream_context_key": course_key_str}).update(
+        downstream_context_title=new_course_name,
+        updated=updated_time


nit: if updated becomes an auto-now field, we don't have to specify it here.

navinkarkera · 2025-01-17T06:16:39Z

@pomegranited Thanks for the review! About the course imports, I am working on it (I did mention it in the description but it was probably after you started reviewing it).

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 15, 2025

navinkarkera force-pushed the navin/fal-4004/link-table branch 6 times, most recently from e3e8965 to 54565b7 Compare January 16, 2025 13:48

navinkarkera mentioned this pull request Jan 16, 2025

feat: model for upstream-downstream links openedx/openedx-learning#269

Draft

pomegranited requested changes Jan 17, 2025

View reviewed changes

navinkarkera added 14 commits January 17, 2025 11:57

feat: entity linking command

2383580

fixup! feat: entity linking command

e11779b

fixup! feat: entity linking command

5092a93

fixup! feat: entity linking command

3084c5d

fixup! feat: entity linking command

70b8f6c

fixup! feat: entity linking command

f7f0df6

test: recreate_upstream_links management command

2740ca1

test: upstream - downstream link tasks

50e7329

chore: fix lint issues

77f0d0a

refactor: fix import issues

60a596f

temp: point openedx-learning to dev branch

4c8a538

refactor: check for upstream_version

14c566a

test: fix failing tests

02a8a82

feat: create upstream links after course import

60b4f43

navinkarkera force-pushed the navin/fal-4004/link-table branch from 54565b7 to 60b4f43 Compare January 17, 2025 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: upstream-downstream entity linking #36111

feat: upstream-downstream entity linking #36111

navinkarkera commented Jan 15, 2025 •

edited

Loading

openedx-webhooks commented Jan 15, 2025

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Update the status of your PR

Where can I find more information?

When can I expect my changes to be merged?

pomegranited left a comment

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

pomegranited Jan 17, 2025

navinkarkera commented Jan 17, 2025

	log.exception("Library block not found!")
	log.error(f"Library component not found for {upstream_usage_key} -- removing from upstream link")

	TASK_LOGGER.exception(f'Could not find course: {xblock.course_id}')
	TASK_LOGGER.error(f'Could not find course: {xblock.course_id}')

-        TASK_LOGGER.exception(f'Could not find course: {course_key_str}')
+        TASK_LOGGER.error(f'Could not find course: {course_key_str}')
+        course_status.status = CourseLinksStatusChoices.FAILED
+        course_status.save()

feat: upstream-downstream entity linking #36111

Are you sure you want to change the base?

feat: upstream-downstream entity linking #36111

Conversation

navinkarkera commented Jan 15, 2025 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

openedx-webhooks commented Jan 15, 2025

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Update the status of your PR

Where can I find more information?

When can I expect my changes to be merged?

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navinkarkera commented Jan 17, 2025

navinkarkera commented Jan 15, 2025 •

edited

Loading