Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: upstream-downstream entity linking #36111

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1887,6 +1887,7 @@
"openedx_learning.apps.authoring.components",
"openedx_learning.apps.authoring.contents",
"openedx_learning.apps.authoring.publishing",
"openedx_learning.apps.authoring.linking",
]


Expand Down
1 change: 1 addition & 0 deletions lms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -3380,6 +3380,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
"openedx_learning.apps.authoring.components",
"openedx_learning.apps.authoring.contents",
"openedx_learning.apps.authoring.publishing",
"openedx_learning.apps.authoring.linking",
]


Expand Down
16 changes: 16 additions & 0 deletions openedx/core/djangoapps/content/course_overviews/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure if it has any use case to be included in openedx-events. I had to create it to avoid lms-cms import issues 😅



@receiver(SignalHandler.course_published)
Expand Down Expand Up @@ -89,6 +91,7 @@ def _check_for_course_changes(previous_course_overview, updated_course_overview)
_check_for_course_start_date_changes(previous_course_overview, updated_course_overview)
_check_for_pacing_changes(previous_course_overview, updated_course_overview)
_check_for_cert_date_changes(previous_course_overview, updated_course_overview)
_check_for_display_name_change(previous_course_overview, updated_course_overview)


def _check_for_course_start_date_changes(previous_course_overview, updated_course_overview):
Expand Down Expand Up @@ -216,3 +219,16 @@ def _send_course_cert_date_change_signal():

if send_signal:
transaction.on_commit(_send_course_cert_date_change_signal)


def _check_for_display_name_change(previous_course_overview, updated_course_overview):
"""
Checks for change in display name of course and sends COURSE_NAME_CHANGED signal.
"""
if previous_course_overview.display_name_with_default != updated_course_overview.display_name_with_default:
COURSE_NAME_CHANGED.send(
sender=None,
courserun_key=str(previous_course_overview.id),
old_name=previous_course_overview.display_name_with_default,
new_name=updated_course_overview.display_name_with_default,
)
25 changes: 25 additions & 0 deletions openedx/core/djangoapps/content_libraries/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -1952,3 +1952,28 @@ def import_blocks_create_task(library_key, course_key, use_course_key_as_block_i
log.info(f"Import block task created: import_task={import_task} "
f"celery_task={result.id}")
return import_task


def create_or_update_xblock_upstream_link(xblock, course_key: str, course_name: str, created: datetime | None = None):
"""
Create or update upstream->downstream link in database for given xblock.
"""
if not xblock.upstream:
return None
upstream_usage_key = UsageKeyV2.from_string(xblock.upstream)
try:
lib_component = get_component_from_usage_key(upstream_usage_key)
except ObjectDoesNotExist:
log.error(f"Library component not found for {upstream_usage_key}")
lib_component = None
authoring_api.update_or_create_entity_link(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible that a course is imported which has an xblock that an upstream which does not exist in this instance. We want to still save them and link them when the library block is imported (this is not implemented yet, I think I'll do it as part of a future task).

lib_component,
upstream_usage_key=xblock.upstream,
upstream_context_key=str(upstream_usage_key.context_key),
downstream_context_key=course_key,
downstream_context_title=course_name,
downstream_usage_key=str(xblock.usage_key),
version_synced=xblock.upstream_version,
version_declined=xblock.upstream_version_declined,
created=created,
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
"""
Management command to recreate upstream-dowstream links in PublishableEntityLink for course(s).

This command can be run for all the courses or for given list of courses.
"""

from __future__ import annotations

import logging
from datetime import datetime, timezone

from django.core.management.base import BaseCommand, CommandError
from django.utils.translation import gettext as _

from openedx.core.djangoapps.content.course_overviews.models import CourseOverview

from ...tasks import create_or_update_upstream_links

log = logging.getLogger(__name__)


class Command(BaseCommand):
"""
Recreate links for course(s) in PublishableEntityLink table.

Examples:
# Recreate upstream links for two courses.
$ ./manage.py cms recreate_upstream_links --course course-v1:edX+DemoX.1+2014 \
--course course-v1:edX+DemoX.2+2015
# Recreate upstream links for all courses.
$ ./manage.py cms recreate_upstream_links --all
# Force recreate links for all courses including completely processed ones.
$ ./manage.py cms recreate_upstream_links --all
"""

def add_arguments(self, parser):
parser.add_argument(
'--course',
metavar=_('COURSE_KEY'),
action='append',
help=_('Recreate links for xblocks under given course keys. For eg. course-v1:edX+DemoX.1+2014'),
default=[],
)
parser.add_argument(
'--all',
action='store_true',
help=_(
'Recreate links for xblocks under all courses. NOTE: this can take long time depending'
' on number of course and xblocks'
),
)
parser.add_argument(
'--force',
action='store_true',
help=_('Recreate links even for completely processed courses.'),
)

def handle(self, *args, **options):
"""
Handle command
"""
courses = options['course']
should_process_all = options['all']
force = options['force']
time_now = datetime.now(tz=timezone.utc)
if not courses and not should_process_all:
raise CommandError('Either --course or --all argument should be provided.')

if should_process_all and courses:
raise CommandError('Only one of --course or --all argument should be provided.')

if should_process_all:
courses = CourseOverview.get_all_course_keys()
for course in courses:
log.info(f"Start processing upstream->dowstream links in course: {course}")
create_or_update_upstream_links.delay(str(course), force, created=time_now)
50 changes: 47 additions & 3 deletions openedx/core/djangoapps/content_libraries/signal_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,34 @@
import logging

from django.conf import settings
from django.db.models.signals import post_save, post_delete, m2m_changed
from django.db.models.signals import m2m_changed, post_delete, post_save
from django.dispatch import receiver

from opaque_keys import InvalidKeyError
from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2
from openedx_events.content_authoring.data import (
ContentObjectChangedData,
LibraryCollectionData,
XBlockData,
)
from openedx_events.content_authoring.signals import (
CONTENT_OBJECT_ASSOCIATIONS_CHANGED,
LIBRARY_COLLECTION_CREATED,
LIBRARY_COLLECTION_DELETED,
LIBRARY_COLLECTION_UPDATED,
XBLOCK_CREATED,
XBLOCK_DELETED,
XBLOCK_UPDATED,
)
from openedx_learning.api.authoring import get_component, get_components
from openedx_learning.api.authoring import delete_entity_link, get_component, get_components
from openedx_learning.api.authoring_models import Collection, CollectionPublishableEntity, Component, PublishableEntity

from lms.djangoapps.grades.api import signals as grades_signals
from openedx.core.djangoapps.content.course_overviews.signals import COURSE_NAME_CHANGED

from .api import library_component_usage_key
from .models import ContentLibrary, LtiGradedResource

from .tasks import create_or_update_xblock_upstream_link, update_course_name_in_upstream_links

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -203,3 +208,42 @@ def library_collection_entities_changed(sender, instance, action, pk_set, **kwar

for component in components.all():
_library_collection_component_changed(component, library.library_key)


@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))
Comment on lines +213 to +224
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be fixed now.



@receiver(XBLOCK_DELETED)
def delete_upstream_downstream_link_handler(**kwargs):
"""
Delete upstream->downstream link from database on xblock delete.
"""
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

delete_entity_link(str(xblock_info.usage_key))


@receiver(COURSE_NAME_CHANGED, dispatch_uid="update_course_name_in_upstream_links_handler")
def update_course_name_in_upstream_links_handler(courserun_key, old_name, new_name, **kwargs):
"""
Handler to update course names in upstream->downstream links on change.
"""
log.info(f"Updating course name in upstream->downstream links from '{old_name}' to '{new_name}'")
update_course_name_in_upstream_links.delay(
courserun_key,
new_name,
)
68 changes: 68 additions & 0 deletions openedx/core/djangoapps/content_libraries/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,23 @@
from __future__ import annotations

import logging
from datetime import datetime, timezone

from celery import shared_task
from celery_utils.logged_task import LoggedTask
from celery.utils.log import get_task_logger
from edx_django_utils.monitoring import set_code_owner_attribute, set_code_owner_attribute_from_module
from opaque_keys.edx.keys import UsageKey

from user_tasks.tasks import UserTask, UserTaskStatus
from xblock.fields import Scope

from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.locator import BlockUsageLocator
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
from openedx.core.lib import ensure_cms
from openedx_learning.api.authoring import get_entity_links, get_or_create_learning_context_link_status
from openedx_learning.api.authoring_models import LearningContextLinksStatusChoices
from xmodule.capa_block import ProblemBlock
from xmodule.library_content_block import ANY_CAPA_TYPE_VALUE, LegacyLibraryContentBlock
from xmodule.modulestore import ModuleStoreEnum
Expand Down Expand Up @@ -169,6 +174,69 @@ def duplicate_children(
self.status.fail({'raw_error_msg': str(exception)})


@shared_task(base=LoggedTask)
@set_code_owner_attribute
def create_or_update_xblock_upstream_link(usage_key):
"""
Create or update upstream link for a single xblock.
"""
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:
return
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}')
Copy link
Contributor

Choose a reason for hiding this comment

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

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}')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be possible hence logging the whole exception.

return
api.create_or_update_xblock_upstream_link(xblock, str(xblock.course_id), course_name)


@shared_task(base=LoggedTask)
@set_code_owner_attribute
def create_or_update_upstream_links(course_key_str: str, force: bool = False, created: datetime | None = None):
"""
A Celery task to create or update upstream downstream links in database from course xblock content.
"""
ensure_cms("create_or_update_upstream_links may only be executed in a CMS context")

if not created:
created = datetime.now(timezone.utc)
course_status = get_or_create_learning_context_link_status(course_key_str, created)
if course_status.status in [
LearningContextLinksStatusChoices.COMPLETED,
LearningContextLinksStatusChoices.PROCESSING
] and not force:
return
store = modulestore()
course_key = CourseKey.from_string(course_key_str)
course_status.status = LearningContextLinksStatusChoices.PROCESSING
course_status.save()
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}')
Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here.

return
xblocks = store.get_items(course_key, settings={"upstream": lambda x: x is not None})
for xblock in xblocks:
api.create_or_update_xblock_upstream_link(xblock, course_key_str, course_name, created)
course_status.status = LearningContextLinksStatusChoices.COMPLETED
course_status.save()


@shared_task(base=LoggedTask)
@set_code_owner_attribute
def update_course_name_in_upstream_links(course_key_str: str, new_course_name: str):
"""
Celery task to update course name in upstream->downstream entity links.
"""
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

)


def _sync_children(
task: LibrarySyncChildrenTask,
store: MixedModuleStore,
Expand Down
Loading
Loading