From e8d82f0134184cce00330f8b3385ce9ccf1b0eb2 Mon Sep 17 00:00:00 2001 From: henrrypg Date: Fri, 3 Jul 2026 14:26:49 -0500 Subject: [PATCH 1/4] feat: add content suggestions --- .../processors/llm/llm_processor.py | 28 ++ .../processors/openedx/openedx_processor.py | 3 + .../prompts/suggest_content_improvements.txt | 42 +++ .../response_schemas/content_suggestions.json | 80 ++++++ .../content_suggestions_orchestrator.py | 241 ++++++++++++++++++ .../session_based_orchestrator.py | 36 +++ .../experimental/content_suggestions.json | 35 +++ 7 files changed, 465 insertions(+) create mode 100644 backend/openedx_ai_extensions/prompts/suggest_content_improvements.txt create mode 100644 backend/openedx_ai_extensions/response_schemas/content_suggestions.json create mode 100644 backend/openedx_ai_extensions/workflows/orchestrators/content_suggestions_orchestrator.py create mode 100644 backend/openedx_ai_extensions/workflows/profiles/experimental/content_suggestions.json diff --git a/backend/openedx_ai_extensions/processors/llm/llm_processor.py b/backend/openedx_ai_extensions/processors/llm/llm_processor.py index 35b5b109..77ec029b 100644 --- a/backend/openedx_ai_extensions/processors/llm/llm_processor.py +++ b/backend/openedx_ai_extensions/processors/llm/llm_processor.py @@ -626,6 +626,34 @@ def _extract_output_items(resp): items.append({"type": "reasoning", "role": "reasoning", "content": summary_text}) return items + def suggest_content_improvements(self): + """ + Suggest content improvements per unit based on course structure. + + Accepts an optional `extra_instructions` string in input_data — author-provided + guidelines (e.g. "Always write in third person") that the LLM checks the course + content against, in addition to its own analysis. + """ + prompt = load_prompt("suggest_content_improvements") + extra_instructions = (self.input_data or {}).get("extra_instructions") or "" + prompt = prompt.replace("{{EXTRA_INSTRUCTIONS}}", extra_instructions) + + self.input_data = None + + result = self._call_completion_wrapper(system_role=prompt) + + if "error" in result: + return result + + response = json.loads(result["response"]) + + return { + "response": response, + "usage": self.usage, + "model_used": self.extra_params.get("model", "unknown"), + "status": "success", + } + def generate_flashcards(self): """Example method showing how to generate flashcards from content.""" prompt_file_path = ( diff --git a/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py b/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py index ffa3a635..260324d5 100644 --- a/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py +++ b/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py @@ -258,6 +258,7 @@ def _serialize_block_structure_outline(self, block_structure): chapter_key, "display_name" ), "category": self.define_category(category), + "location_id": str(chapter_key), "subsections": [], } @@ -274,6 +275,7 @@ def _serialize_block_structure_outline(self, block_structure): sequential_key, "display_name" ), "category": self.define_category(seq_category), + "location_id": str(sequential_key), "units": [], } @@ -290,6 +292,7 @@ def _serialize_block_structure_outline(self, block_structure): vertical_key, "display_name" ), "category": self.define_category(vert_category), + "location_id": str(vertical_key), } sequential_info["units"].append(vertical_info) diff --git a/backend/openedx_ai_extensions/prompts/suggest_content_improvements.txt b/backend/openedx_ai_extensions/prompts/suggest_content_improvements.txt new file mode 100644 index 00000000..747b5f74 --- /dev/null +++ b/backend/openedx_ai_extensions/prompts/suggest_content_improvements.txt @@ -0,0 +1,42 @@ +- Role & Purpose + + You are an AI assistant embedded into an Open edX learning environment. Your purpose is to help course authors improve their course by reviewing the course's metadata and structure and proposing concrete content improvements. + +- Core Behaviors + + Treat the course metadata and outline provided below as your only source of truth. Do not invent units, sections, IDs, or content that are not present in it. + Each unit in the outline has a `location_id`. When you flag a unit, you MUST reference its exact `location_id` as given — never a display name, guess, or modified ID. + Only flag units that would genuinely benefit from a content change. It is fine to return zero suggestions if the course structure looks complete and well organized, or many if there are several real issues. + Base each suggestion on what is inferable from the course title, descriptions, overview, syllabus, and the position/naming of the unit within the outline (e.g. thin sections, unclear or duplicate titles, missing expected topics, inconsistent pacing, ordering issues). + +- Suggestion Writing Guidelines + + `title`: a short, action-oriented headline (e.g. "Rename this unit", "Add a worked example"), not a restatement of the unit name. + `suggestion`: the full explanation of what to change and why. Keep it to one or two sentences. + `type`: classify as exactly one of `wording`, `structure`, `pedagogy`, or `accessibility` — pick whichever best describes the nature of the issue. + `priority`: `high`, `medium`, or `low`, based on how much the issue is likely to hurt the learner experience if left unaddressed. + Do not repeat the same suggestion verbatim across multiple units. + +- The `proposed_change` field — read carefully + + `proposed_change` must be `null` unless you have one specific, final, ready-to-use replacement text to offer. + Only set it when you can fill `current` with a real value copied verbatim from the course data provided below (you have real ground truth for a unit's `display_name` from the outline — you do NOT have the actual body text of a unit, so do not fabricate a `current` value for `content_html` or `summary` unless that exact text was explicitly given to you in the context). + Never invent placeholder or generic text to fill this field just to have something there. An open-ended suggestion with no concrete replacement text (e.g. "add more examples") MUST have `proposed_change: null`. + When you do set it: `field` names which unit attribute it targets (`display_name`, `content_html`, or `summary`), `current` is the exact existing value, and `suggested` is the exact final replacement text — not a description of a change, the actual text itself. + +- Author guidelines + + The course author may have provided extra guidelines for this review below. If present, treat them as hard constraints: actively check the course content against each one and flag any unit that violates them as its own suggestion, in addition to your own analysis. If none were provided, ignore this section. + + ---------------------------- + {{EXTRA_INSTRUCTIONS}} + ---------------------------- + +- Context + + The following is the course metadata and structure to review. + +- Output rules + + The response format is enforced by a structured schema — follow it strictly. + `unit_id` must exactly match a `location_id` found in the provided outline. diff --git a/backend/openedx_ai_extensions/response_schemas/content_suggestions.json b/backend/openedx_ai_extensions/response_schemas/content_suggestions.json new file mode 100644 index 00000000..8bdc8134 --- /dev/null +++ b/backend/openedx_ai_extensions/response_schemas/content_suggestions.json @@ -0,0 +1,80 @@ +{ + "type": "json_schema", + "json_schema": { + "name": "ContentSuggestions", + "strict": true, + "schema": { + "type": "object", + "properties": { + "suggestions": { + "type": "array", + "description": "Units that would benefit from a content change, based on the provided course structure.", + "items": { + "type": "object", + "properties": { + "unit_id": { + "type": "string", + "description": "The exact 'location_id' of the unit this suggestion applies to, copied verbatim from the provided course outline." + }, + "unit_display_name": { + "type": "string", + "description": "The display name of the unit, copied from the provided course outline, for readability." + }, + "type": { + "type": "string", + "enum": ["wording", "structure", "pedagogy", "accessibility"], + "description": "The category of improvement this suggestion addresses." + }, + "priority": { + "type": "string", + "enum": ["high", "medium", "low"], + "description": "How important this suggestion is relative to others." + }, + "title": { + "type": "string", + "description": "A short, action-oriented headline for this suggestion (e.g. 'Rename this unit', 'Add a worked example')." + }, + "suggestion": { + "type": "string", + "description": "The full description of the suggestion, explaining what to change and why." + }, + "proposed_change": { + "type": ["object", "null"], + "description": "A concrete, final, ready-to-use replacement text, ONLY when one genuinely applies. Must be null when the suggestion is open-ended (no specific replacement text to give).", + "properties": { + "field": { + "type": "string", + "enum": ["display_name", "content_html", "summary"], + "description": "Which unit field this concrete change applies to." + }, + "current": { + "type": "string", + "description": "The current value of that field, copied verbatim from the provided course data." + }, + "suggested": { + "type": "string", + "description": "The proposed replacement value, ready to use as-is." + } + }, + "required": ["field", "current", "suggested"], + "additionalProperties": false + } + }, + "required": [ + "unit_id", + "unit_display_name", + "type", + "priority", + "title", + "suggestion", + "proposed_change" + ], + "additionalProperties": false + } + } + }, + "required": ["suggestions"], + "additionalProperties": false + } + } +} diff --git a/backend/openedx_ai_extensions/workflows/orchestrators/content_suggestions_orchestrator.py b/backend/openedx_ai_extensions/workflows/orchestrators/content_suggestions_orchestrator.py new file mode 100644 index 00000000..9ec60868 --- /dev/null +++ b/backend/openedx_ai_extensions/workflows/orchestrators/content_suggestions_orchestrator.py @@ -0,0 +1,241 @@ +""" +Orchestrator for course-wide content improvement suggestions. +""" +import json +import logging +import uuid +from pathlib import Path + +from openedx_ai_extensions.processors import LLMProcessor, OpenEdXProcessor +from openedx_ai_extensions.xapi.constants import EVENT_NAME_WORKFLOW_COMPLETED + +from .session_based_orchestrator import CrossSlotSessionOrchestrator + +logger = logging.getLogger(__name__) + + +class ContentSuggestionsOrchestrator(CrossSlotSessionOrchestrator): + """ + Orchestrator that reviews a whole course's structure and metadata via + OpenEdXProcessor.get_course_info, then asks an LLM to propose content + improvement suggestions per unit. + + The full suggestion list (with section/subsection/unit ancestry) is + always persisted course-wide in the session. What gets *returned* to a + given request is filtered by the caller's location: no location (or a + location outside the course tree, e.g. the course outline page) returns + everything; a specific section/subsection/unit location returns only + the suggestions under that node. + """ + + @property + def _schema_path(self): + return ( + Path(__file__).resolve().parent.parent.parent + / "response_schemas" + / "content_suggestions.json" + ) + + @staticmethod + def _build_ancestry_map(outline): + """ + Walk the course outline (list of chapters -> subsections -> units) and + return {unit_id: {section_id, section_display_name, subsection_id, + subsection_display_name}} so suggestions can carry full ancestry + without trusting the LLM to know or repeat it correctly. + """ + ancestry = {} + for chapter in outline or []: + section_id = chapter.get('location_id') + section_name = chapter.get('display_name') + for subsection in chapter.get('subsections', []) or []: + subsection_id = subsection.get('location_id') + subsection_name = subsection.get('display_name') + for unit in subsection.get('units', []) or []: + unit_id = unit.get('location_id') + if not unit_id: + continue + ancestry[unit_id] = { + 'section_id': section_id, + 'section_display_name': section_name, + 'subsection_id': subsection_id, + 'subsection_display_name': subsection_name, + } + return ancestry + + @staticmethod + def _known_location_ids(ancestry_map): + """ + Every section/subsection/unit id in the course tree, gathered from + ancestry_map. Lets the filter tell "this unit legitimately has zero + suggestions" apart from "this location isn't part of the course tree + at all" (e.g. the course's own root usage key on the outline page) — + both would otherwise produce an empty match. + """ + known = set() + for unit_id, ancestry in ancestry_map.items(): + known.add(unit_id) + if ancestry['section_id']: + known.add(ancestry['section_id']) + if ancestry['subsection_id']: + known.add(ancestry['subsection_id']) + return known + + @staticmethod + def _sort_by_course_order(suggestions, ancestry_map): + """ + Order suggestions by their unit's position in the course outline + (ancestry_map's iteration order, itself built by walking the outline + top to bottom) so cross-unit "next/previous suggestion" navigation + follows the course structure instead of arbitrary LLM output order. + """ + order = {unit_id: idx for idx, unit_id in enumerate(ancestry_map.keys())} + return sorted(suggestions, key=lambda s: order.get(s.get('unit_id'), len(order))) + + @staticmethod + def _filter_suggestions_for_location(suggestions, location_id, known_location_ids): + """ + Return only suggestions belonging under location_id (matched against + its unit, subsection, or section id). If location_id isn't part of + the course tree at all, return everything instead — that's the + course-outline case, not a "no suggestions here" case. + """ + if not location_id or location_id not in known_location_ids: + return suggestions + return [ + s for s in suggestions + if location_id in (s.get('unit_id'), s.get('section_id'), s.get('subsection_id')) + ] + + def _resolve_extra_instructions(self, input_data, metadata): + """ + Resolve which guidelines to use: an explicit non-empty value from + this request, or the course's previously stored guidelines, so every + author sees the same guidelines unless someone actively changes them. + """ + provided = (input_data or {}).get('extra_instructions') + if provided and provided.strip(): + return provided.strip() + return metadata.get('extra_instructions', '') + + def run(self, input_data): + """ + Fetch course structure and ask the LLM for content improvement + suggestions, then return the subset relevant to self.location_id. + """ + + openedx_processor = OpenEdXProcessor( + processor_config=self.profile.processor_config, + course_id=self.course_id, + user=self.user, + ) + content_result = openedx_processor.process() + + if content_result and 'error' in content_result: + return { + 'error': content_result['error'], + 'status': 'OpenEdXProcessor error' + } + + outline = json.loads(content_result.get('outline') or '[]') + ancestry_map = self._build_ancestry_map(outline) + known_location_ids = self._known_location_ids(ancestry_map) + + llm_input_content = str(content_result) + + metadata = self.session.metadata or {} + extra_instructions = self._resolve_extra_instructions(input_data, metadata) + llm_input_data = dict(input_data or {}) + llm_input_data['extra_instructions'] = extra_instructions + + with open(self._schema_path, 'r', encoding='utf-8') as f: + self.llm_processor = LLMProcessor( + config=self.profile.processor_config, + extra_params={"response_format": json.load(f)} + ) + llm_result = self.llm_processor.process(context=llm_input_content, input_data=llm_input_data) + + if llm_result and 'error' in llm_result: + return { + 'error': llm_result['error'], + 'status': 'LLMProcessor error' + } + + response_payload = llm_result.get('response', {}) or {} + suggestions = response_payload.get('suggestions', []) or [] + for suggestion in suggestions: + suggestion['id'] = str(uuid.uuid4()) + suggestion.update(ancestry_map.get(suggestion.get('unit_id'), { + 'section_id': None, + 'section_display_name': None, + 'subsection_id': None, + 'subsection_display_name': None, + })) + suggestions = self._sort_by_course_order(suggestions, ancestry_map) + + metadata['suggestions'] = suggestions + metadata['extra_instructions'] = extra_instructions + metadata['known_location_ids'] = sorted(known_location_ids) + self.session.metadata = metadata + self.session.save(update_fields=["metadata"]) + + self._emit_workflow_event(EVENT_NAME_WORKFLOW_COMPLETED) + + return { + 'status': 'completed', + 'response': { + 'suggestions': self._filter_suggestions_for_location( + suggestions, self.location_id, known_location_ids + ), + 'course_suggestions': suggestions, + 'extra_instructions': extra_instructions, + }, + } + + def get_current_session_response(self, _): + """ + Retrieve the current session state, filtered to self.location_id. + + ``status`` distinguishes "never generated / cleared" (no_suggestions) + from "generated at least once" (completed, though the filtered list + may legitimately be empty for this location). Either way, + extra_instructions is always returned so the request form can + prefill the course's stored guidelines. + """ + metadata = self.session.metadata or {} + if "suggestions" in metadata: + known_location_ids = set(metadata.get('known_location_ids', [])) + return { + 'response': { + 'suggestions': self._filter_suggestions_for_location( + metadata['suggestions'], self.location_id, known_location_ids + ), + 'course_suggestions': metadata['suggestions'], + 'extra_instructions': metadata.get('extra_instructions', ''), + }, + 'status': 'completed', + } + return { + 'response': { + 'suggestions': [], + 'course_suggestions': [], + 'extra_instructions': metadata.get('extra_instructions', ''), + }, + 'status': 'no_suggestions', + } + + def clear_session(self, _): + """ + Clear generated suggestions but keep the session row (and its + extra_instructions) alive, so the next author sees the same + guidelines prefilled instead of starting from a blank field. + """ + metadata = self.session.metadata or {} + metadata.pop('suggestions', None) + metadata.pop('known_location_ids', None) + self.session.metadata = metadata + self.session.save(update_fields=['metadata']) + return { + 'response': '', + 'status': 'session_cleared', + } diff --git a/backend/openedx_ai_extensions/workflows/orchestrators/session_based_orchestrator.py b/backend/openedx_ai_extensions/workflows/orchestrators/session_based_orchestrator.py index b6b43df0..50c284e3 100644 --- a/backend/openedx_ai_extensions/workflows/orchestrators/session_based_orchestrator.py +++ b/backend/openedx_ai_extensions/workflows/orchestrators/session_based_orchestrator.py @@ -265,3 +265,39 @@ def run_async(self, input_data): 'task_id': task.id, 'message': 'AI workflow has started' } + + +class CrossSlotSessionOrchestrator(ScopedSessionOrchestrator): # pylint: disable=abstract-method + """ + ScopedSessionOrchestrator variant whose session is shared not only across + locations, but across every UI slot (``AIWorkflowScope`` row) that points + at the same profile within a course. + + Use this instead of ``ScopedSessionOrchestrator`` when one profile is + attached to multiple scopes (e.g. a course-outline sidebar widget and an + educator-tools widget) and authors should see one shared session no + matter which widget they used. ``ScopedSessionOrchestrator`` itself keeps + keying sessions on scope, since some workflows (e.g. flashcards) are + expected to keep a separate session per widget even when they share a + profile. + + Looked up via filter().first() rather than get_or_create() because a + scope-keyed row for this same (user, profile, course_id) may already + exist from before this class was introduced — get_or_create would raise + MultipleObjectsReturned once a second scope creates its own row. + """ + + def __init__(self, workflow, user, context): # pylint: disable=super-init-not-called + BaseOrchestrator.__init__(self, workflow, user, context) # pylint: disable=non-parent-init-called + self.session = AIWorkflowSession.objects.filter( + user=self.user, + profile=self.workflow.profile, + course_id=self.course_id, + ).order_by("created_at").first() + if self.session is None: + self.session = AIWorkflowSession.objects.create( + user=self.user, + scope=self.workflow, + profile=self.workflow.profile, + course_id=self.course_id, + ) diff --git a/backend/openedx_ai_extensions/workflows/profiles/experimental/content_suggestions.json b/backend/openedx_ai_extensions/workflows/profiles/experimental/content_suggestions.json new file mode 100644 index 00000000..cfcf8dc2 --- /dev/null +++ b/backend/openedx_ai_extensions/workflows/profiles/experimental/content_suggestions.json @@ -0,0 +1,35 @@ +/* +Reviews the whole course structure and metadata, then asks the LLM to +propose content improvement suggestions per unit, each with a real +navigable link back to that unit. +*/ +{ + "orchestrator_class": "openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.ContentSuggestionsOrchestrator", + "processor_config": { + "OpenEdXProcessor": { + "function": "get_course_info", + "fields": ["title", "short_description", "overview", "outline"] + }, + "LLMProcessor": { + "function": "suggest_content_improvements", + "provider": "default" + } + }, + "actuator_config": { + "UIComponents": { + "request": { + "component": "ContentSuggestionsRequest", + "config": { + "buttonText": "Suggest improvements", + "customMessage": "Get AI suggestions to improve course content", + "preloadPreviousSession": true + } + }, + "response": { + "component": "ContentSuggestionsResponse", + "config": {} + } + } + }, + "schema_version": "1.0", +} From 8763de96c5c973d554b2007dd27cc4f9fcc9e7cd Mon Sep 17 00:00:00 2001 From: henrrypg Date: Fri, 3 Jul 2026 15:28:01 -0500 Subject: [PATCH 2/4] chore: add tests --- .../test_content_suggestions_orchestrator.py | 637 ++++++++++++++++++ 1 file changed, 637 insertions(+) create mode 100644 backend/tests/test_content_suggestions_orchestrator.py diff --git a/backend/tests/test_content_suggestions_orchestrator.py b/backend/tests/test_content_suggestions_orchestrator.py new file mode 100644 index 00000000..49775b69 --- /dev/null +++ b/backend/tests/test_content_suggestions_orchestrator.py @@ -0,0 +1,637 @@ +""" +Tests for content_suggestions_orchestrator. +""" +# pylint: disable=protected-access + +import json +from unittest.mock import Mock, patch + +import pytest +from django.contrib.auth import get_user_model +from opaque_keys.edx.keys import CourseKey + +from openedx_ai_extensions.workflows.models import AIWorkflowProfile, AIWorkflowScope, AIWorkflowSession +from openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator import ( + ContentSuggestionsOrchestrator, +) + +User = get_user_model() + + +# --------------------------------------------------------------------------- +# Shared fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture +def user(db): # pylint: disable=unused-argument + return User.objects.create_user( + username="content_suggestions_test_user", + email="content_suggestions@example.com", + password="password123", + ) + + +@pytest.fixture +def course_key(): + return CourseKey.from_string("course-v1:edX+DemoX+Demo_Course") + + +@pytest.fixture +def workflow_profile(db): # pylint: disable=unused-argument + return AIWorkflowProfile.objects.create( + slug="test-content-suggestions", + description="Content suggestions profile for tests", + base_filepath="experimental/content_suggestions.json", + content_patch="{}", + ) + + +@pytest.fixture +def workflow_scope(workflow_profile, course_key): # pylint: disable=redefined-outer-name + return AIWorkflowScope.objects.create( + location_regex=".*", + course_id=course_key, + service_variant="cms", + profile=workflow_profile, + enabled=True, + ) + + +UNIT_1 = "block-v1:edX+DemoX+Demo_Course+type@vertical+block@unit1" +UNIT_2 = "block-v1:edX+DemoX+Demo_Course+type@vertical+block@unit2" +SECTION_1 = "block-v1:edX+DemoX+Demo_Course+type@chapter+block@sec1" +SUBSECTION_1 = "block-v1:edX+DemoX+Demo_Course+type@sequential+block@sub1" + + +def build_outline(): + return [ + { + "location_id": SECTION_1, + "display_name": "Week 1", + "subsections": [ + { + "location_id": SUBSECTION_1, + "display_name": "Getting Started", + "units": [ + {"location_id": UNIT_1, "display_name": "Introduction"}, + {"location_id": UNIT_2, "display_name": "Deep Dive"}, + ], + }, + ], + }, + ] + + +def make_orchestrator(workflow_scope, user, course_key, location_id=None): # pylint: disable=redefined-outer-name + """Build a ContentSuggestionsOrchestrator for the given scope/location.""" + context = { + "course_id": str(course_key), + "location_id": location_id, + } + return ContentSuggestionsOrchestrator( + workflow=workflow_scope, + user=user, + context=context, + ) + + +@pytest.fixture +def orchestrator(workflow_scope, user, course_key): # pylint: disable=redefined-outer-name + return make_orchestrator(workflow_scope, user, course_key) + + +# =========================================================================== +# _build_ancestry_map / _known_location_ids +# =========================================================================== + + +def test_build_ancestry_map_maps_units_to_ancestry(): + ancestry = ContentSuggestionsOrchestrator._build_ancestry_map(build_outline()) + + assert ancestry[UNIT_1] == { + "section_id": SECTION_1, + "section_display_name": "Week 1", + "subsection_id": SUBSECTION_1, + "subsection_display_name": "Getting Started", + } + assert ancestry[UNIT_2]["section_id"] == SECTION_1 + + +def test_build_ancestry_map_skips_units_without_location_id(): + outline = [ + { + "location_id": SECTION_1, + "display_name": "Week 1", + "subsections": [ + { + "location_id": SUBSECTION_1, + "display_name": "Getting Started", + "units": [{"display_name": "No location"}], + }, + ], + }, + ] + ancestry = ContentSuggestionsOrchestrator._build_ancestry_map(outline) + assert not ancestry + + +def test_build_ancestry_map_empty_outline(): + assert not ContentSuggestionsOrchestrator._build_ancestry_map([]) + assert not ContentSuggestionsOrchestrator._build_ancestry_map(None) + + +def test_known_location_ids_includes_all_tree_levels(): + ancestry = ContentSuggestionsOrchestrator._build_ancestry_map(build_outline()) + known = ContentSuggestionsOrchestrator._known_location_ids(ancestry) + + assert known == {UNIT_1, UNIT_2, SECTION_1, SUBSECTION_1} + + +# =========================================================================== +# _sort_by_course_order +# =========================================================================== + + +def test_sort_by_course_order_orders_by_outline_position(): + ancestry = ContentSuggestionsOrchestrator._build_ancestry_map(build_outline()) + suggestions = [ + {"id": "s1", "unit_id": UNIT_2}, + {"id": "s2", "unit_id": UNIT_1}, + ] + + sorted_suggestions = ContentSuggestionsOrchestrator._sort_by_course_order(suggestions, ancestry) + + assert [s["id"] for s in sorted_suggestions] == ["s2", "s1"] + + +def test_sort_by_course_order_unknown_unit_goes_last(): + ancestry = ContentSuggestionsOrchestrator._build_ancestry_map(build_outline()) + suggestions = [ + {"id": "unknown", "unit_id": "not-in-outline"}, + {"id": "s2", "unit_id": UNIT_1}, + ] + + sorted_suggestions = ContentSuggestionsOrchestrator._sort_by_course_order(suggestions, ancestry) + + assert [s["id"] for s in sorted_suggestions] == ["s2", "unknown"] + + +# =========================================================================== +# _filter_suggestions_for_location +# =========================================================================== + + +def test_filter_suggestions_no_location_returns_everything(): + suggestions = [{"unit_id": UNIT_1}, {"unit_id": UNIT_2}] + result = ContentSuggestionsOrchestrator._filter_suggestions_for_location(suggestions, None, {UNIT_1, UNIT_2}) + assert result == suggestions + + +def test_filter_suggestions_location_outside_tree_returns_everything(): + suggestions = [{"unit_id": UNIT_1}, {"unit_id": UNIT_2}] + result = ContentSuggestionsOrchestrator._filter_suggestions_for_location( + suggestions, "course-v1:edX+DemoX+Demo_Course", {UNIT_1, UNIT_2} + ) + assert result == suggestions + + +def test_filter_suggestions_unit_location_matches_only_its_unit(): + suggestions = [ + {"unit_id": UNIT_1, "section_id": SECTION_1, "subsection_id": SUBSECTION_1}, + {"unit_id": UNIT_2, "section_id": SECTION_1, "subsection_id": SUBSECTION_1}, + ] + known = {UNIT_1, UNIT_2, SECTION_1, SUBSECTION_1} + + result = ContentSuggestionsOrchestrator._filter_suggestions_for_location(suggestions, UNIT_1, known) + + assert len(result) == 1 + assert result[0]["unit_id"] == UNIT_1 + + +def test_filter_suggestions_real_unit_with_zero_suggestions_returns_empty(): + """A known unit with no suggestions must return [] and NOT fall back to everything.""" + suggestions = [{"unit_id": UNIT_2, "section_id": SECTION_1, "subsection_id": SUBSECTION_1}] + known = {UNIT_1, UNIT_2, SECTION_1, SUBSECTION_1} + + result = ContentSuggestionsOrchestrator._filter_suggestions_for_location(suggestions, UNIT_1, known) + + assert result == [] + + +def test_filter_suggestions_section_location_matches_all_its_descendants(): + suggestions = [ + {"unit_id": UNIT_1, "section_id": SECTION_1, "subsection_id": SUBSECTION_1}, + {"unit_id": UNIT_2, "section_id": SECTION_1, "subsection_id": SUBSECTION_1}, + ] + known = {UNIT_1, UNIT_2, SECTION_1, SUBSECTION_1} + + result = ContentSuggestionsOrchestrator._filter_suggestions_for_location(suggestions, SECTION_1, known) + + assert len(result) == 2 + + +# =========================================================================== +# _resolve_extra_instructions +# =========================================================================== + + +def test_resolve_extra_instructions_prefers_explicit_value(orchestrator): # pylint: disable=redefined-outer-name + result = orchestrator._resolve_extra_instructions( + {"extra_instructions": "Use third person."}, {"extra_instructions": "stored"} + ) + assert result == "Use third person." + + +def test_resolve_extra_instructions_falls_back_to_stored_value(orchestrator): # pylint: disable=redefined-outer-name + result = orchestrator._resolve_extra_instructions( + {"extra_instructions": " "}, {"extra_instructions": "stored"} + ) + assert result == "stored" + + +def test_resolve_extra_instructions_no_input_data_falls_back_to_stored( + orchestrator, # pylint: disable=redefined-outer-name +): + result = orchestrator._resolve_extra_instructions(None, {"extra_instructions": "stored"}) + assert result == "stored" + + +def test_resolve_extra_instructions_defaults_to_empty_string(orchestrator): # pylint: disable=redefined-outer-name + result = orchestrator._resolve_extra_instructions({}, {}) + assert result == "" + + +# =========================================================================== +# run() — error paths +# =========================================================================== + + +@pytest.mark.django_db +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.OpenEdXProcessor") +def test_run_openedx_error(mock_openedx_class, orchestrator): # pylint: disable=redefined-outer-name + mock_openedx = Mock() + mock_openedx.process.return_value = {"error": "Course not found"} + mock_openedx_class.return_value = mock_openedx + + result = orchestrator.run({}) + + assert result == {"error": "Course not found", "status": "OpenEdXProcessor error"} + + +@pytest.mark.django_db +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.OpenEdXProcessor") +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.LLMProcessor") +def test_run_llm_error(mock_llm_class, mock_openedx_class, orchestrator): # pylint: disable=redefined-outer-name + mock_openedx = Mock() + mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx_class.return_value = mock_openedx + + mock_llm = Mock() + mock_llm.process.return_value = {"error": "AI API failed"} + mock_llm_class.return_value = mock_llm + + result = orchestrator.run({}) + + assert result == {"error": "AI API failed", "status": "LLMProcessor error"} + + +# =========================================================================== +# run() — success path +# =========================================================================== + + +def _mock_llm_success(mock_llm_class, suggestions): + mock_llm = Mock() + mock_llm.process.return_value = {"response": {"suggestions": suggestions}} + mock_llm_class.return_value = mock_llm + return mock_llm + + +@pytest.mark.django_db +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.OpenEdXProcessor") +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.LLMProcessor") +def test_run_success_enriches_and_persists_suggestions( + mock_llm_class, mock_openedx_class, orchestrator, # pylint: disable=redefined-outer-name +): + mock_openedx = Mock() + mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx_class.return_value = mock_openedx + + raw_suggestions = [ + {"unit_id": UNIT_2, "title": "Second"}, + {"unit_id": UNIT_1, "title": "First"}, + ] + _mock_llm_success(mock_llm_class, raw_suggestions) + + with patch.object(orchestrator, "_emit_workflow_event") as mock_emit: + result = orchestrator.run({"extra_instructions": "Be concise."}) + + assert result["status"] == "completed" + course_suggestions = result["response"]["course_suggestions"] + # sorted by course order: unit1 before unit2 + assert [s["title"] for s in course_suggestions] == ["First", "Second"] + for suggestion in course_suggestions: + assert "id" in suggestion + assert suggestion["section_id"] == SECTION_1 + assert suggestion["subsection_id"] == SUBSECTION_1 + assert result["response"]["extra_instructions"] == "Be concise." + mock_emit.assert_called_once() + + orchestrator.session.refresh_from_db() + assert orchestrator.session.metadata["extra_instructions"] == "Be concise." + assert set(orchestrator.session.metadata["known_location_ids"]) == {UNIT_1, UNIT_2, SECTION_1, SUBSECTION_1} + assert len(orchestrator.session.metadata["suggestions"]) == 2 + + +@pytest.mark.django_db +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.OpenEdXProcessor") +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.LLMProcessor") +def test_run_filters_response_to_current_unit_location( + mock_llm_class, mock_openedx_class, workflow_scope, user, course_key, # pylint: disable=redefined-outer-name +): + mock_openedx = Mock() + mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx_class.return_value = mock_openedx + + raw_suggestions = [ + {"unit_id": UNIT_1, "title": "First"}, + {"unit_id": UNIT_2, "title": "Second"}, + ] + _mock_llm_success(mock_llm_class, raw_suggestions) + + unit_orchestrator = make_orchestrator(workflow_scope, user, course_key, location_id=UNIT_1) + with patch.object(unit_orchestrator, "_emit_workflow_event"): + result = unit_orchestrator.run({}) + + filtered_titles = [s["title"] for s in result["response"]["suggestions"]] + assert filtered_titles == ["First"] + # course_suggestions stays unfiltered + assert len(result["response"]["course_suggestions"]) == 2 + + +@pytest.mark.django_db +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.OpenEdXProcessor") +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.LLMProcessor") +def test_run_no_suggestions_key_defaults_to_empty_list( + mock_llm_class, mock_openedx_class, orchestrator, # pylint: disable=redefined-outer-name +): + mock_openedx = Mock() + mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx_class.return_value = mock_openedx + + mock_llm = Mock() + mock_llm.process.return_value = {"response": {}} + mock_llm_class.return_value = mock_llm + + with patch.object(orchestrator, "_emit_workflow_event"): + result = orchestrator.run({}) + + assert result["status"] == "completed" + assert result["response"]["suggestions"] == [] + assert result["response"]["course_suggestions"] == [] + + +@pytest.mark.django_db +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.OpenEdXProcessor") +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.LLMProcessor") +def test_run_passes_json_schema_as_response_format( + mock_llm_class, mock_openedx_class, orchestrator, # pylint: disable=redefined-outer-name +): + mock_openedx = Mock() + mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx_class.return_value = mock_openedx + + _mock_llm_success(mock_llm_class, []) + + with patch.object(orchestrator, "_emit_workflow_event"): + orchestrator.run({}) + + call_kwargs = mock_llm_class.call_args[1] + assert "response_format" in call_kwargs["extra_params"] + + +@pytest.mark.django_db +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.OpenEdXProcessor") +@patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.LLMProcessor") +def test_run_reuses_stored_extra_instructions_when_not_provided( + mock_llm_class, mock_openedx_class, orchestrator, # pylint: disable=redefined-outer-name +): + orchestrator.session.metadata = {"extra_instructions": "Previously stored guidelines."} + orchestrator.session.save(update_fields=["metadata"]) + + mock_openedx = Mock() + mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx_class.return_value = mock_openedx + + _mock_llm_success(mock_llm_class, []) + + with patch.object(orchestrator, "_emit_workflow_event"): + result = orchestrator.run({}) + + assert result["response"]["extra_instructions"] == "Previously stored guidelines." + + +# =========================================================================== +# get_current_session_response +# =========================================================================== + + +@pytest.mark.django_db +def test_get_current_session_response_with_suggestions(orchestrator): # pylint: disable=redefined-outer-name + suggestions = [ + {"id": "s1", "unit_id": UNIT_1}, + {"id": "s2", "unit_id": UNIT_2}, + ] + orchestrator.session.metadata = { + "suggestions": suggestions, + "known_location_ids": [UNIT_1, UNIT_2], + "extra_instructions": "Be concise.", + } + orchestrator.session.save(update_fields=["metadata"]) + + result = orchestrator.get_current_session_response(None) + + assert result["status"] == "completed" + assert result["response"]["suggestions"] == suggestions + assert result["response"]["course_suggestions"] == suggestions + assert result["response"]["extra_instructions"] == "Be concise." + + +@pytest.mark.django_db +def test_get_current_session_response_filters_by_location( + workflow_scope, user, course_key, # pylint: disable=redefined-outer-name +): + unit_orchestrator = make_orchestrator(workflow_scope, user, course_key, location_id=UNIT_1) + unit_orchestrator.session.metadata = { + "suggestions": [{"id": "s1", "unit_id": UNIT_1}, {"id": "s2", "unit_id": UNIT_2}], + "known_location_ids": [UNIT_1, UNIT_2], + "extra_instructions": "", + } + unit_orchestrator.session.save(update_fields=["metadata"]) + + result = unit_orchestrator.get_current_session_response(None) + + assert [s["id"] for s in result["response"]["suggestions"]] == ["s1"] + assert result["response"]["course_suggestions"] == unit_orchestrator.session.metadata["suggestions"] + + +@pytest.mark.django_db +def test_get_current_session_response_no_suggestions_yet(orchestrator): # pylint: disable=redefined-outer-name + orchestrator.session.metadata = {"extra_instructions": "kept guidelines"} + orchestrator.session.save(update_fields=["metadata"]) + + result = orchestrator.get_current_session_response(None) + + assert result == { + "response": { + "suggestions": [], + "course_suggestions": [], + "extra_instructions": "kept guidelines", + }, + "status": "no_suggestions", + } + + +@pytest.mark.django_db +def test_get_current_session_response_empty_metadata(orchestrator): # pylint: disable=redefined-outer-name + orchestrator.session.metadata = {} + orchestrator.session.save(update_fields=["metadata"]) + + result = orchestrator.get_current_session_response(None) + + assert result["status"] == "no_suggestions" + assert result["response"]["extra_instructions"] == "" + + +# =========================================================================== +# clear_session +# =========================================================================== + + +@pytest.mark.django_db +def test_clear_session_removes_suggestions_but_keeps_extra_instructions( + orchestrator, # pylint: disable=redefined-outer-name +): + orchestrator.session.metadata = { + "suggestions": [{"id": "s1"}], + "known_location_ids": [UNIT_1], + "extra_instructions": "keep me", + } + orchestrator.session.save(update_fields=["metadata"]) + session_id = orchestrator.session.id + + result = orchestrator.clear_session(None) + + assert result == {"response": "", "status": "session_cleared"} + + orchestrator.session.refresh_from_db() + # The session row itself survives clear_session (unlike the base class, + # which deletes it) so extra_instructions stays available for prefill. + assert orchestrator.session.id == session_id + assert "suggestions" not in orchestrator.session.metadata + assert "known_location_ids" not in orchestrator.session.metadata + assert orchestrator.session.metadata["extra_instructions"] == "keep me" + + +@pytest.mark.django_db +def test_clear_session_with_empty_metadata_does_not_raise(orchestrator): # pylint: disable=redefined-outer-name + # A freshly created session already has empty metadata ({}) — the + # `metadata` column is NOT NULL, so None is never a valid stored value. + orchestrator.session.metadata = {} + orchestrator.session.save(update_fields=["metadata"]) + + result = orchestrator.clear_session(None) + + assert result == {"response": "", "status": "session_cleared"} + + +# =========================================================================== +# _schema_path +# =========================================================================== + + +def test_schema_path_points_to_content_suggestions_json(orchestrator): # pylint: disable=redefined-outer-name + schema_path = orchestrator._schema_path + assert schema_path.name == "content_suggestions.json" + assert "response_schemas" in str(schema_path) + + +# =========================================================================== +# CrossSlotSessionOrchestrator sharing behavior (via ContentSuggestionsOrchestrator) +# =========================================================================== + + +@pytest.mark.django_db +def test_session_is_shared_across_scopes_for_same_profile( + workflow_profile, course_key, user, # pylint: disable=redefined-outer-name +): + scope_a = AIWorkflowScope.objects.create( + location_regex=".*", + course_id=course_key, + service_variant="cms", + profile=workflow_profile, + ui_slot_selector_id="sidebar1", + enabled=True, + ) + scope_b = AIWorkflowScope.objects.create( + location_regex=".*", + course_id=course_key, + service_variant="cms", + profile=workflow_profile, + ui_slot_selector_id="educator-1", + enabled=True, + ) + + orchestrator_a = make_orchestrator(scope_a, user, course_key) + orchestrator_a.session.metadata = {"suggestions": [{"id": "s1", "unit_id": UNIT_1}]} + orchestrator_a.session.save(update_fields=["metadata"]) + + orchestrator_b = make_orchestrator(scope_b, user, course_key) + + assert orchestrator_b.session.id == orchestrator_a.session.id + assert orchestrator_b.session.metadata["suggestions"][0]["id"] == "s1" + + +@pytest.mark.django_db +def test_session_has_no_location_id(orchestrator): # pylint: disable=redefined-outer-name + assert orchestrator.session.location_id is None + + +@pytest.mark.django_db +def test_session_tolerates_pre_existing_duplicate_scope_rows( + workflow_profile, course_key, user, # pylint: disable=redefined-outer-name +): + """ + If two scope-keyed session rows already exist for this (user, profile, + course_id) from before CrossSlotSessionOrchestrator existed, instantiating + it must not raise MultipleObjectsReturned — it should deterministically + pick one (the earliest created). + """ + scope_a = AIWorkflowScope.objects.create( + location_regex=".*", + course_id=course_key, + service_variant="cms", + profile=workflow_profile, + ui_slot_selector_id="sidebar1", + enabled=True, + ) + scope_b = AIWorkflowScope.objects.create( + location_regex=".*", + course_id=course_key, + service_variant="cms", + profile=workflow_profile, + ui_slot_selector_id="educator-1", + enabled=True, + ) + + first = AIWorkflowSession.objects.create( + user=user, scope=scope_a, profile=workflow_profile, course_id=course_key, + ) + AIWorkflowSession.objects.create( + user=user, scope=scope_b, profile=workflow_profile, course_id=course_key, + ) + + later_orchestrator = make_orchestrator(scope_b, user, course_key) + + assert later_orchestrator.session.id == first.id From bfa66a01fe779dbc06f9bed6ebd151674530c464 Mon Sep 17 00:00:00 2001 From: henrrypg Date: Wed, 8 Jul 2026 17:08:46 -0500 Subject: [PATCH 3/4] feat: use whole course content, rename orchestrator --- .../processors/openedx/openedx_processor.py | 52 +++++++++++++++++-- .../prompts/suggest_content_improvements.txt | 16 +++--- .../content_suggestions_orchestrator.py | 28 +++++----- .../session_based_orchestrator.py | 2 +- .../experimental/content_suggestions.json | 10 ++-- .../test_content_suggestions_orchestrator.py | 17 +++--- backend/tests/test_openedx_processor.py | 48 +++++++++++++++++ 7 files changed, 132 insertions(+), 41 deletions(-) diff --git a/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py b/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py index 260324d5..ea4743d8 100644 --- a/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py +++ b/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py @@ -55,7 +55,9 @@ def no_context(self, *args, **kwargs): "Get published Open edX course content. " "This function reads the content of course unit(s) and " "converts it into a structured format suitable for LLM processing. " - "Can retrieve just the current unit, units up to the current one, or the entire sequence." + "Can retrieve just the current unit, units up to the current one, or the entire sequence. " + "When no location is available, a course ID can be given instead to retrieve the " + "content of the whole course (sections -> subsections -> units)." ), "parameters": { "type": "object", @@ -67,6 +69,13 @@ def no_context(self, *args, **kwargs): "if not provided uses the current location" ) }, + "course_id": { + "type": "string", + "description": ( + "The course ID. Only used when no location ID is available: " + "returns the whole course's content. Defaults to the current course." + ) + }, "retrieval_mode": { "type": "string", "enum": ["unit", "up_to_current_unit", "sequence"], @@ -81,8 +90,8 @@ def no_context(self, *args, **kwargs): } } }) - def get_location_content(self, location_id=None, retrieval_mode=None): - """Extract unit or sequence content from Open edX modulestore based on configuration""" + def get_location_content(self, location_id=None, course_id=None, retrieval_mode=None): + """Extract unit, sequence or whole-course content from Open edX modulestore""" try: # pylint: disable=import-error,import-outside-toplevel from xmodule.modulestore.django import modulestore @@ -90,9 +99,14 @@ def get_location_content(self, location_id=None, retrieval_mode=None): # Get char_limit from config. Useful during development char_limit = self.config.get("char_limit", None) location_id = location_id or self.location_id + store = modulestore() + + # No location at all: fall back to the whole course's content + if location_id is None: + course_key = CourseKey.from_string(course_id or self.course_id) + return self._get_course_content(store, course_key, char_limit) unit_key = UsageKey.from_string(location_id) - store = modulestore() # Get retrieval_mode from arg or config, default to 'unit' retrieval_mode = retrieval_mode or self.config.get("retrieval_mode", "unit") @@ -127,6 +141,36 @@ def get_location_content(self, location_id=None, retrieval_mode=None): except Exception as exc: # pylint: disable=broad-exception-caught return {"error": f"Error accessing content: {str(exc)}"} + def _get_course_content(self, store, course_key, char_limit=None): + """Extract content for every unit in a course, keeping the outline structure.""" + course = store.get_course(course_key) + sections = [] + for chapter in (store.get_item(key) for key in getattr(course, "children", [])): + if chapter.category != "chapter": + continue + subsections = [] + for sequential in (store.get_item(key) for key in getattr(chapter, "children", [])): + if sequential.category != "sequential": + continue + subsections.append({ + "location_id": str(sequential.location), + "display_name": sequential.display_name, + "units": [ + self._get_unit_data(store, unit_key, char_limit) + for unit_key in getattr(sequential, "children", []) + ], + }) + sections.append({ + "location_id": str(chapter.location), + "display_name": chapter.display_name, + "subsections": subsections, + }) + return { + "course_id": str(course_key), + "display_name": course.display_name, + "sections": sections, + } + def _get_unit_data(self, store, unit_key, char_limit=None): """Extract content for a single unit""" unit = store.get_item(unit_key) diff --git a/backend/openedx_ai_extensions/prompts/suggest_content_improvements.txt b/backend/openedx_ai_extensions/prompts/suggest_content_improvements.txt index 747b5f74..1ed57efb 100644 --- a/backend/openedx_ai_extensions/prompts/suggest_content_improvements.txt +++ b/backend/openedx_ai_extensions/prompts/suggest_content_improvements.txt @@ -1,13 +1,13 @@ - Role & Purpose - You are an AI assistant embedded into an Open edX learning environment. Your purpose is to help course authors improve their course by reviewing the course's metadata and structure and proposing concrete content improvements. + You are an AI assistant embedded into an Open edX learning environment. Your purpose is to help course authors improve their course by reviewing the course's actual content and structure and proposing concrete content improvements. - Core Behaviors - Treat the course metadata and outline provided below as your only source of truth. Do not invent units, sections, IDs, or content that are not present in it. - Each unit in the outline has a `location_id`. When you flag a unit, you MUST reference its exact `location_id` as given — never a display name, guess, or modified ID. - Only flag units that would genuinely benefit from a content change. It is fine to return zero suggestions if the course structure looks complete and well organized, or many if there are several real issues. - Base each suggestion on what is inferable from the course title, descriptions, overview, syllabus, and the position/naming of the unit within the outline (e.g. thin sections, unclear or duplicate titles, missing expected topics, inconsistent pacing, ordering issues). + Treat the course content provided below (sections -> subsections -> units, including each unit's blocks) as your only source of truth. Do not invent units, sections, IDs, or content that are not present in it. + Each unit has a `unit_id`. When you flag a unit, you MUST reference its exact `unit_id` as given — never a display name, guess, or modified ID. + Only flag units that would genuinely benefit from a content change. It is fine to return zero suggestions if the course looks complete and well organized, or many if there are several real issues. + Base each suggestion on the actual content of the unit's blocks (text, problems, videos) and its place in the course structure (e.g. thin or unclear content, duplicate titles, missing expected topics, inconsistent pacing, ordering issues). - Suggestion Writing Guidelines @@ -20,7 +20,7 @@ - The `proposed_change` field — read carefully `proposed_change` must be `null` unless you have one specific, final, ready-to-use replacement text to offer. - Only set it when you can fill `current` with a real value copied verbatim from the course data provided below (you have real ground truth for a unit's `display_name` from the outline — you do NOT have the actual body text of a unit, so do not fabricate a `current` value for `content_html` or `summary` unless that exact text was explicitly given to you in the context). + Only set it when you can fill `current` with a real value copied verbatim from the course data provided below (a unit's `display_name` or the actual text of one of its blocks). Never fabricate a `current` value that does not appear in the provided content. Never invent placeholder or generic text to fill this field just to have something there. An open-ended suggestion with no concrete replacement text (e.g. "add more examples") MUST have `proposed_change: null`. When you do set it: `field` names which unit attribute it targets (`display_name`, `content_html`, or `summary`), `current` is the exact existing value, and `suggested` is the exact final replacement text — not a description of a change, the actual text itself. @@ -34,9 +34,9 @@ - Context - The following is the course metadata and structure to review. + The following is the course content and structure to review. - Output rules The response format is enforced by a structured schema — follow it strictly. - `unit_id` must exactly match a `location_id` found in the provided outline. + `unit_id` must exactly match a `unit_id` found in the provided course content. diff --git a/backend/openedx_ai_extensions/workflows/orchestrators/content_suggestions_orchestrator.py b/backend/openedx_ai_extensions/workflows/orchestrators/content_suggestions_orchestrator.py index 9ec60868..7a8f511a 100644 --- a/backend/openedx_ai_extensions/workflows/orchestrators/content_suggestions_orchestrator.py +++ b/backend/openedx_ai_extensions/workflows/orchestrators/content_suggestions_orchestrator.py @@ -9,16 +9,16 @@ from openedx_ai_extensions.processors import LLMProcessor, OpenEdXProcessor from openedx_ai_extensions.xapi.constants import EVENT_NAME_WORKFLOW_COMPLETED -from .session_based_orchestrator import CrossSlotSessionOrchestrator +from .session_based_orchestrator import CourseSessionOrchestrator logger = logging.getLogger(__name__) -class ContentSuggestionsOrchestrator(CrossSlotSessionOrchestrator): +class ContentSuggestionsOrchestrator(CourseSessionOrchestrator): """ - Orchestrator that reviews a whole course's structure and metadata via - OpenEdXProcessor.get_course_info, then asks an LLM to propose content - improvement suggestions per unit. + Orchestrator that reviews a whole course's actual content via + OpenEdXProcessor.get_location_content (called with the course ID), then + asks an LLM to propose content improvement suggestions per unit. The full suggestion list (with section/subsection/unit ancestry) is always persisted course-wide in the session. What gets *returned* to a @@ -37,22 +37,22 @@ def _schema_path(self): ) @staticmethod - def _build_ancestry_map(outline): + def _build_ancestry_map(sections): """ - Walk the course outline (list of chapters -> subsections -> units) and - return {unit_id: {section_id, section_display_name, subsection_id, + Walk the course content tree (list of sections -> subsections -> units) + and return {unit_id: {section_id, section_display_name, subsection_id, subsection_display_name}} so suggestions can carry full ancestry without trusting the LLM to know or repeat it correctly. """ ancestry = {} - for chapter in outline or []: + for chapter in sections or []: section_id = chapter.get('location_id') section_name = chapter.get('display_name') for subsection in chapter.get('subsections', []) or []: subsection_id = subsection.get('location_id') subsection_name = subsection.get('display_name') for unit in subsection.get('units', []) or []: - unit_id = unit.get('location_id') + unit_id = unit.get('location_id') or unit.get('unit_id') if not unit_id: continue ancestry[unit_id] = { @@ -120,8 +120,9 @@ def _resolve_extra_instructions(self, input_data, metadata): def run(self, input_data): """ - Fetch course structure and ask the LLM for content improvement - suggestions, then return the subset relevant to self.location_id. + Fetch the whole course's content and ask the LLM for content + improvement suggestions, then return the subset relevant to + self.location_id. """ openedx_processor = OpenEdXProcessor( @@ -137,8 +138,7 @@ def run(self, input_data): 'status': 'OpenEdXProcessor error' } - outline = json.loads(content_result.get('outline') or '[]') - ancestry_map = self._build_ancestry_map(outline) + ancestry_map = self._build_ancestry_map(content_result.get('sections')) known_location_ids = self._known_location_ids(ancestry_map) llm_input_content = str(content_result) diff --git a/backend/openedx_ai_extensions/workflows/orchestrators/session_based_orchestrator.py b/backend/openedx_ai_extensions/workflows/orchestrators/session_based_orchestrator.py index 50c284e3..e398902f 100644 --- a/backend/openedx_ai_extensions/workflows/orchestrators/session_based_orchestrator.py +++ b/backend/openedx_ai_extensions/workflows/orchestrators/session_based_orchestrator.py @@ -267,7 +267,7 @@ def run_async(self, input_data): } -class CrossSlotSessionOrchestrator(ScopedSessionOrchestrator): # pylint: disable=abstract-method +class CourseSessionOrchestrator(ScopedSessionOrchestrator): # pylint: disable=abstract-method """ ScopedSessionOrchestrator variant whose session is shared not only across locations, but across every UI slot (``AIWorkflowScope`` row) that points diff --git a/backend/openedx_ai_extensions/workflows/profiles/experimental/content_suggestions.json b/backend/openedx_ai_extensions/workflows/profiles/experimental/content_suggestions.json index cfcf8dc2..073243fe 100644 --- a/backend/openedx_ai_extensions/workflows/profiles/experimental/content_suggestions.json +++ b/backend/openedx_ai_extensions/workflows/profiles/experimental/content_suggestions.json @@ -1,14 +1,14 @@ /* -Reviews the whole course structure and metadata, then asks the LLM to -propose content improvement suggestions per unit, each with a real -navigable link back to that unit. +Reviews the whole course's actual content (sections -> subsections -> units, +including each unit's blocks), then asks the LLM to propose content +improvement suggestions per unit, each with a real navigable link back to +that unit. */ { "orchestrator_class": "openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.ContentSuggestionsOrchestrator", "processor_config": { "OpenEdXProcessor": { - "function": "get_course_info", - "fields": ["title", "short_description", "overview", "outline"] + "function": "get_location_content" }, "LLMProcessor": { "function": "suggest_content_improvements", diff --git a/backend/tests/test_content_suggestions_orchestrator.py b/backend/tests/test_content_suggestions_orchestrator.py index 49775b69..d2bc8e6f 100644 --- a/backend/tests/test_content_suggestions_orchestrator.py +++ b/backend/tests/test_content_suggestions_orchestrator.py @@ -3,7 +3,6 @@ """ # pylint: disable=protected-access -import json from unittest.mock import Mock, patch import pytest @@ -284,7 +283,7 @@ def test_run_openedx_error(mock_openedx_class, orchestrator): # pylint: disable @patch("openedx_ai_extensions.workflows.orchestrators.content_suggestions_orchestrator.LLMProcessor") def test_run_llm_error(mock_llm_class, mock_openedx_class, orchestrator): # pylint: disable=redefined-outer-name mock_openedx = Mock() - mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx.process.return_value = {"sections": build_outline()} mock_openedx_class.return_value = mock_openedx mock_llm = Mock() @@ -315,7 +314,7 @@ def test_run_success_enriches_and_persists_suggestions( mock_llm_class, mock_openedx_class, orchestrator, # pylint: disable=redefined-outer-name ): mock_openedx = Mock() - mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx.process.return_value = {"sections": build_outline()} mock_openedx_class.return_value = mock_openedx raw_suggestions = [ @@ -351,7 +350,7 @@ def test_run_filters_response_to_current_unit_location( mock_llm_class, mock_openedx_class, workflow_scope, user, course_key, # pylint: disable=redefined-outer-name ): mock_openedx = Mock() - mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx.process.return_value = {"sections": build_outline()} mock_openedx_class.return_value = mock_openedx raw_suggestions = [ @@ -377,7 +376,7 @@ def test_run_no_suggestions_key_defaults_to_empty_list( mock_llm_class, mock_openedx_class, orchestrator, # pylint: disable=redefined-outer-name ): mock_openedx = Mock() - mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx.process.return_value = {"sections": build_outline()} mock_openedx_class.return_value = mock_openedx mock_llm = Mock() @@ -399,7 +398,7 @@ def test_run_passes_json_schema_as_response_format( mock_llm_class, mock_openedx_class, orchestrator, # pylint: disable=redefined-outer-name ): mock_openedx = Mock() - mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx.process.return_value = {"sections": build_outline()} mock_openedx_class.return_value = mock_openedx _mock_llm_success(mock_llm_class, []) @@ -421,7 +420,7 @@ def test_run_reuses_stored_extra_instructions_when_not_provided( orchestrator.session.save(update_fields=["metadata"]) mock_openedx = Mock() - mock_openedx.process.return_value = {"outline": json.dumps(build_outline())} + mock_openedx.process.return_value = {"sections": build_outline()} mock_openedx_class.return_value = mock_openedx _mock_llm_success(mock_llm_class, []) @@ -558,7 +557,7 @@ def test_schema_path_points_to_content_suggestions_json(orchestrator): # pylint # =========================================================================== -# CrossSlotSessionOrchestrator sharing behavior (via ContentSuggestionsOrchestrator) +# CourseSessionOrchestrator sharing behavior (via ContentSuggestionsOrchestrator) # =========================================================================== @@ -604,7 +603,7 @@ def test_session_tolerates_pre_existing_duplicate_scope_rows( ): """ If two scope-keyed session rows already exist for this (user, profile, - course_id) from before CrossSlotSessionOrchestrator existed, instantiating + course_id) from before CourseSessionOrchestrator existed, instantiating it must not raise MultipleObjectsReturned — it should deterministically pick one (the earliest created). """ diff --git a/backend/tests/test_openedx_processor.py b/backend/tests/test_openedx_processor.py index b0cd9587..c43f59e8 100644 --- a/backend/tests/test_openedx_processor.py +++ b/backend/tests/test_openedx_processor.py @@ -290,6 +290,54 @@ def test_get_location_content_truncation(mock_edx_imports, mock_keys): assert len(result["blocks"][1]["text"]) == 5 +def test_get_location_content_with_course_id_returns_whole_course(mock_edx_imports, mock_keys): + """A course ID (not a usage key) returns the whole course's content tree.""" + # pylint: disable=unused-argument + # pylint: disable=import-error, import-outside-toplevel + from xmodule.modulestore.django import modulestore + + mock_unit = MagicMock() + mock_unit.location = "unit-loc" + mock_unit.display_name = "Unit 1" + mock_unit.category = "vertical" + mock_unit.children = [] + + mock_sequential = MagicMock() + mock_sequential.location = "seq-loc" + mock_sequential.display_name = "Subsection 1" + mock_sequential.category = "sequential" + mock_sequential.children = ["unit-key"] + + mock_chapter = MagicMock() + mock_chapter.location = "chap-loc" + mock_chapter.display_name = "Section 1" + mock_chapter.category = "chapter" + mock_chapter.children = ["seq-key"] + + mock_course = MagicMock() + mock_course.display_name = "Demo Course" + mock_course.children = ["chap-key"] + + mock_store = modulestore.return_value + mock_store.get_course.return_value = mock_course + mock_store.get_item.side_effect = lambda key: { + "chap-key": mock_chapter, + "seq-key": mock_sequential, + "unit-key": mock_unit, + }[key] + + result = OpenEdXProcessor().get_location_content(course_id="course-v1:edX+DemoX+Demo_Course") + + assert result["course_id"] == "course-v1:edX+DemoX+Demo_Course" + assert result["display_name"] == "Demo Course" + section = result["sections"][0] + assert section["location_id"] == "chap-loc" + assert section["display_name"] == "Section 1" + subsection = section["subsections"][0] + assert subsection["location_id"] == "seq-loc" + assert subsection["units"][0]["unit_id"] == "unit-loc" + + def test_get_location_content_error_handling(mock_edx_imports, mock_keys): """Test that exceptions are caught and returned as errors.""" # pylint: disable=unused-argument From d14cc27d008182b0a6692cb923ad737981c07cba Mon Sep 17 00:00:00 2001 From: henrrypg Date: Wed, 8 Jul 2026 17:12:07 -0500 Subject: [PATCH 4/4] fix: remove changes to _serialize_block_structure_outline --- .../processors/openedx/openedx_processor.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py b/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py index ea4743d8..b6fdc454 100644 --- a/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py +++ b/backend/openedx_ai_extensions/processors/openedx/openedx_processor.py @@ -302,7 +302,6 @@ def _serialize_block_structure_outline(self, block_structure): chapter_key, "display_name" ), "category": self.define_category(category), - "location_id": str(chapter_key), "subsections": [], } @@ -319,7 +318,6 @@ def _serialize_block_structure_outline(self, block_structure): sequential_key, "display_name" ), "category": self.define_category(seq_category), - "location_id": str(sequential_key), "units": [], } @@ -336,7 +334,6 @@ def _serialize_block_structure_outline(self, block_structure): vertical_key, "display_name" ), "category": self.define_category(vert_category), - "location_id": str(vertical_key), } sequential_info["units"].append(vertical_info)