From 4fc9eb8324f10163b950208fa431ad34485fc0e1 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Tue, 1 Nov 2022 09:31:27 +0000 Subject: [PATCH] fix: Change approach to processing course topics (#31200) Course topics are now created by traversing the entire course structure from top to bottom instead of starting at the sequential level and then moving up or down as needed. This also introduces a lot of debug logs to pontetially find the reason why under some circumstances new units don't get processed and end up without a discussions topic. --- .../core/djangoapps/discussions/README.rst | 2 +- .../core/djangoapps/discussions/handlers.py | 7 ++- openedx/core/djangoapps/discussions/tasks.py | 63 +++++++++++-------- 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/openedx/core/djangoapps/discussions/README.rst b/openedx/core/djangoapps/discussions/README.rst index 00bb8a2e78..5c1127d324 100644 --- a/openedx/core/djangoapps/discussions/README.rst +++ b/openedx/core/djangoapps/discussions/README.rst @@ -51,7 +51,7 @@ object attached. Finally, the handler for this discussion change signal, takes the information from the discussion change signal and compares it to the topics in the -database, and does the following; +database, and does the following: - If it sees discussions contexts (units) without topics it creates new topics link entries for the new units. This will happen if you create a new unit, diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py index 8787da82ce..c894a6a53b 100644 --- a/openedx/core/djangoapps/discussions/handlers.py +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -61,14 +61,16 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration lookup_key = topic_link.usage_key or topic_link.external_id topic_context = new_topic_map.pop(lookup_key, None) if topic_context is None: + log.info(f"[DEBUG INF-291] Unit was deleted or discussion disabled: {lookup_key}") topic_link.enabled_in_context = False try: # If the section/subsection/unit a topic is in is deleted, add that context to title. topic_link.title = "{section}|{subsection}|{unit}".format(**topic_link.context) except KeyError: - # It's possible the context is if the topic link was created before the context field was added. + # It's possible the context is empty if the link was created before the context field was added. pass else: + log.info(f"[DEBUG INF-291] Unit topic already exists, will be updated: {lookup_key}") topic_link.enabled_in_context = True topic_link.ordering = topic_context.ordering topic_link.title = topic_context.title @@ -78,6 +80,9 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration topic_link.save() log.info(f"Creating new discussion topic links for {course_key}") + log.info(f"[DEBUG INF-291] Discovered new units with keys: {[str(key) for key in new_topic_map.keys()]}") + log.info(f"[DEBUG INF-291] New unit names: {[topic.title for topic in new_topic_map.values()]}") + DiscussionTopicLink.objects.bulk_create([ DiscussionTopicLink( context_key=course_key, diff --git a/openedx/core/djangoapps/discussions/tasks.py b/openedx/core/djangoapps/discussions/tasks.py index 9cbfac57d5..0113c63d83 100644 --- a/openedx/core/djangoapps/discussions/tasks.py +++ b/openedx/core/djangoapps/discussions/tasks.py @@ -51,37 +51,48 @@ def update_discussions_settings_from_course(course_key: CourseKey) -> CourseDisc provider_type = discussions_config.provider_type def iter_discussable_units(): - subsections = store.get_items(course_key, qualifiers={"category": "sequential"}) # Start at 99 so that the initial increment starts it at 100. # This leaves the first 100 slots for the course wide topics, which is only a concern if there are more # than that many. idx = 99 - for subsection in subsections: - section = store.get_item(subsection.parent) - for unit in subsection.get_children(): - # Increment index even for skipped units so that the index is more stable and won't change - # if settings change, only if a unit is added or removed. - idx += 1 - # If unit-level visibility is enabled and the unit doesn't have discussion enabled, skip it. - if unit_level_visibility and not getattr(unit, "discussion_enabled", False): + log.info(f"[DEBUG INF-291] Unit-level visibility enabled: {unit_level_visibility}") + for section in course.get_children(): + if section.location.block_type != "chapter": + continue + for subsection in section.get_children(): + if subsection.location.block_type != "sequential": continue - # If the unit is in a graded section and graded sections aren't enabled skip it. - if subsection.graded and not enable_graded_units: - continue - # If the unit is an exam, skip it. - if subsection.is_practice_exam or subsection.is_proctored_enabled or subsection.is_time_limited: - continue - yield DiscussionTopicContext( - usage_key=unit.location, - title=unit.display_name, - group_id=None, - ordering=idx, - context={ - "section": section.display_name, - "subsection": subsection.display_name, - "unit": unit.display_name, - }, - ) + for unit in subsection.get_children(): + if unit.location.block_type != 'vertical': + continue + log.info(f"[DEBUG INF-291] Processing unit: {unit.location}") + # Increment index even for skipped units so that the index is more stable and won't change + # if settings change, only if a unit is added or removed. + idx += 1 + # If unit-level visibility is enabled and the unit doesn't have discussion enabled, skip it. + if unit_level_visibility and not getattr(unit, "discussion_enabled", False): + log.info(f"[DEBUG INF-291] Skipping unit because discussion is disbled: {unit.location}") + continue + # If the unit is in a graded section and graded sections aren't enabled skip it. + if subsection.graded and not enable_graded_units: + log.info(f"[DEBUG INF-291] Skipping unit because it's in a graded subsection: {unit.location}") + continue + # If the unit is an exam, skip it. + if subsection.is_practice_exam or subsection.is_proctored_enabled or subsection.is_time_limited: + log.info(f"[DEBUG INF-291] Skipping unit because it's in an exam: {unit.location}") + continue + log.info(f"[DEBUG INF-291] Topic will be created for unit: {unit.location}") + yield DiscussionTopicContext( + usage_key=unit.location, + title=unit.display_name, + group_id=None, + ordering=idx, + context={ + "section": section.display_name, + "subsection": subsection.display_name, + "unit": unit.display_name, + }, + ) with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): course = store.get_course(course_key)