Scrape child courses before programs and fix missing child course sections (#3580)

Author: mbertrand

learning_resources/tasks.py

@@ -45,6 +45,7 @@
     build_program_children_content_bulk,
     html_to_markdown,
     load_course_blocklist,
+    programs_needing_children_heal,
     resource_unpublished_actions,
     resource_upserted_actions,
     strip_markdown_images,
@@ -670,31 +671,55 @@ def sync_canvas_courses(canvas_course_ids, overwrite):
 @app.task(bind=True)
 def scrape_marketing_pages(self):
     """
-    Scrape marketing pages (for programs and courses)
-    and store them as content files if they dont exist
+    Scrape marketing pages for programs and courses and store them as content
+    files. Child courses are scraped before their parent programs so a program's
+    children section is built from child marketing pages that already exist.
+    Programs whose stored page is missing its children section (and whose
+    children content is now available) are re-scraped to heal them.
     """
     log.info("Running scrape_marketing_pages task")
-    resource_ids = set(
+    resource_types = dict(
         LearningResource.objects.filter(
             published=True, resource_type__in=["course", "program"]
-        ).values_list("id", flat=True)
+        ).values_list("id", "resource_type")
     )
-
     existing_page_resource_ids = set(
-        ContentFile.objects.filter(file_type="marketing_page").values_list(
+        ContentFile.objects.filter(file_type=MARKETING_PAGE_FILE_TYPE).values_list(
             "learning_resource_id", flat=True
         )
     )
-    missing_pages = list(resource_ids.difference(existing_page_resource_ids))
 
-    tasks = [
+    missing_ids = set(resource_types) - existing_page_resource_ids
+    missing_course_ids = sorted(
+        rid for rid in missing_ids if resource_types[rid] == "course"
+    )
+    program_ids_with_pages = {
+        rid
+        for rid in existing_page_resource_ids
+        if resource_types.get(rid) == "program"
+    }
+    program_ids = {rid for rid in missing_ids if resource_types[rid] == "program"}
+    program_ids |= programs_needing_children_heal(program_ids_with_pages)
+
+    course_tasks = [
         marketing_page_for_resources.si(ids)
-        for ids in chunks(
-            missing_pages,
-            chunk_size=settings.QDRANT_CHUNK_SIZE,
-        )
+        for ids in chunks(missing_course_ids, chunk_size=settings.QDRANT_CHUNK_SIZE)
     ]
-    scrape_tasks = celery.group(tasks)
+    program_tasks = [
+        marketing_page_for_resources.si(ids)
+        for ids in chunks(sorted(program_ids), chunk_size=settings.QDRANT_CHUNK_SIZE)
+    ]
+
+    if course_tasks and program_tasks:
+        scrape_tasks = celery.chain(
+            celery.group(course_tasks), celery.group(program_tasks)
+        )
+    elif course_tasks:
+        scrape_tasks = celery.group(course_tasks)
+    elif program_tasks:
+        scrape_tasks = celery.group(program_tasks)
+    else:
+        return None
     return self.replace(scrape_tasks)
 
 
@@ -723,8 +748,20 @@ def marketing_page_for_resources(resource_ids):
 
     for learning_resource in resources:
         marketing_page_url = learning_resource.url
-        scraper = scraper_for_site(marketing_page_url)
-        page_content = scraper.scrape()
+        try:
+            scraper = scraper_for_site(marketing_page_url)
+            page_content = scraper.scrape()
+        except Exception:
+            # Isolate per-resource failures so one bad page can't fail the whole
+            # chunk. When these tasks are chained (course group -> program group),
+            # a failed task poisons the chord header and the program group never
+            # runs, so keep this batch succeeding for pages that do scrape.
+            log.exception(
+                "Failed to scrape marketing page for resource %s (%s)",
+                learning_resource.id,
+                marketing_page_url,
+            )
+            continue
         if page_content:
             content_file, _ = ContentFile.objects.update_or_create(
                 learning_resource=learning_resource,

learning_resources/tasks_test.py

@@ -619,6 +619,55 @@ def test_marketing_page_for_resources_with_webdriver(mocker, settings):
     )
 
 
+@pytest.mark.django_db
+def test_marketing_page_for_resources_isolates_scrape_failures(mocker):
+    """A single resource's scrape failure must not fail the whole chunk.
+
+    When course and program tasks are chained, a chunk that raises poisons the
+    chord header and the program group never runs, so per-resource failures are
+    logged and skipped rather than propagated.
+    """
+    bad_course = models.LearningResource.objects.create(
+        title="Bad Course",
+        url="https://example.com/bad-course",
+        resource_type="course",
+        published=True,
+    )
+    good_course = models.LearningResource.objects.create(
+        title="Good Course",
+        url="https://example.com/good-course",
+        resource_type="course",
+        published=True,
+    )
+
+    good_scraper = mocker.Mock()
+    good_scraper.scrape.return_value = "<html><body><p>ok</p></body></html>"
+
+    def fake_scraper_for_site(url):
+        if url == bad_course.url:
+            msg = "scraper boom"
+            raise RuntimeError(msg)
+        return good_scraper
+
+    mocker.patch(
+        "learning_resources.tasks.scraper_for_site",
+        side_effect=fake_scraper_for_site,
+    )
+    mocker.patch("learning_resources.tasks.html_to_markdown", return_value="ok")
+    mock_generate_embeddings = mocker.patch("vector_search.tasks.generate_embeddings")
+
+    # Must not raise despite the bad course failing
+    marketing_page_for_resources([bad_course.id, good_course.id])
+
+    assert not models.ContentFile.objects.filter(learning_resource=bad_course).exists()
+    good_cf = models.ContentFile.objects.get(
+        learning_resource=good_course, file_type=MARKETING_PAGE_FILE_TYPE
+    )
+    mock_generate_embeddings.delay.assert_called_once_with(
+        [good_cf.id], "content_file", overwrite=True
+    )
+
+
 @pytest.mark.django_db
 def test_marketing_page_for_program_appends_children(mocker, settings):
     """Test that marketing_page_for_resources appends program children content"""
@@ -780,6 +829,104 @@ def test_scrape_marketing_pages(mocker, settings, mocked_celery):
     mock_group.assert_called_once()
 
 
+@pytest.mark.django_db
+def test_scrape_marketing_pages_orders_courses_before_programs(
+    mocker, settings, mocked_celery
+):
+    """Courses are scraped in a group that runs before the programs group."""
+    settings.QDRANT_CHUNK_SIZE = 10
+    course = models.LearningResource.objects.create(
+        title="Course",
+        url="https://example.com/course",
+        resource_type="course",
+        published=True,
+    )
+    program = models.LearningResource.objects.create(
+        title="Program",
+        url="https://example.com/program",
+        resource_type="program",
+        published=True,
+    )
+    si_mock = mocker.patch("learning_resources.tasks.marketing_page_for_resources.si")
+    # Make each .si(...) call's return value identify which ids it was built
+    # from, so the args passed into celery.group(...) can be told apart.
+    si_mock.side_effect = lambda ids: ("si", tuple(ids))
+
+    with pytest.raises(mocked_celery.replace_exception_class):
+        scrape_marketing_pages.delay()
+
+    # A chain enforces ordering (not a single flat group).
+    assert mocked_celery.chain.called
+    # Course task built before program task; each in its own chunk here.
+    queued_ids = [call.args[0] for call in si_mock.call_args_list]
+    assert queued_ids[0] == [course.id]
+    assert [program.id] in queued_ids
+    assert queued_ids.index([course.id]) < queued_ids.index([program.id])
+
+    # Pin the guarantee to what is actually fed into celery.chain via
+    # celery.group: Python evaluates chain's positional args left-to-right,
+    # so the first group(...) call must be the course tasks and the second
+    # must be the program tasks. This fails if the two arguments to
+    # celery.chain(celery.group(...), celery.group(...)) are ever swapped.
+    assert mocked_celery.group.call_count == 2
+    first_group_tasks, second_group_tasks = (
+        call.args[0] for call in mocked_celery.group.call_args_list
+    )
+    assert first_group_tasks == [("si", (course.id,))]
+    assert second_group_tasks == [("si", (program.id,))]
+
+
+@pytest.mark.django_db
+def test_scrape_marketing_pages_queues_healable_programs(
+    mocker, settings, mocked_celery
+):
+    """A program that already has a page but is missing its children section
+    (with a child course page available) is queued for re-scrape.
+    """
+    settings.QDRANT_CHUNK_SIZE = 10
+    course = models.LearningResource.objects.create(
+        title="Course",
+        url="https://example.com/course",
+        resource_type="course",
+        published=True,
+    )
+    ContentFile.objects.create(
+        learning_resource=course,
+        file_type=MARKETING_PAGE_FILE_TYPE,
+        file_extension=".md",
+        key=course.url,
+        content="Child copy.",
+        published=True,
+    )
+    program = models.LearningResource.objects.create(
+        title="Program",
+        url="https://example.com/program",
+        resource_type="program",
+        published=True,
+    )
+    models.LearningResourceRelationship.objects.create(
+        parent=program, child=course, relation_type="PROGRAM_COURSES"
+    )
+    # Program already has a page, but WITHOUT the children marker.
+    ContentFile.objects.create(
+        learning_resource=program,
+        file_type=MARKETING_PAGE_FILE_TYPE,
+        file_extension=".md",
+        key=program.url,
+        content="Program page, no children yet.",
+        published=True,
+    )
+    si_mock = mocker.patch("learning_resources.tasks.marketing_page_for_resources.si")
+
+    with pytest.raises(mocked_celery.replace_exception_class):
+        scrape_marketing_pages.delay()
+
+    queued_ids = [i for call in si_mock.call_args_list for i in call.args[0]]
+    # Program re-queued for healing; course already has a page so it is NOT queued.
+    assert program.id in queued_ids
+    assert course.id not in queued_ids
+
+
 @pytest.mark.parametrize("canvas_ids", [["1"], None])
 def test_sync_canvas_courses(settings, mocker, django_assert_num_queries, canvas_ids):
     """