Avoid errors caused by None tags (#2680)

Author: mbertrand

learning_resources/etl/loaders.py

@@ -741,6 +741,8 @@ def calculate_completeness(
         content_keys = CONTENT_TAG_CATEGORIES.values()
         content_tags_dict = dict(zip(content_keys, [0] * len(content_keys)))
         for content_file_tags in content_tags:
+            if content_file_tags is None:
+                continue
             for content_tag in content_file_tags:
                 category = CONTENT_TAG_CATEGORIES.get(content_tag)
                 if category:
@@ -799,7 +801,7 @@ def load_content_files(
         content_files_ids = []
         content_tags = []
         for content_file in content_files_data:
-            content_tags.append(content_file.get("content_tags", []))
+            content_tags.append(content_file.get("content_tags") or [])
             content_files_ids.append(load_content_file(course_run, content_file))
         for file in (
             ContentFile.objects.filter(run=course_run)

learning_resources/etl/loaders_test.py

@@ -1881,6 +1881,72 @@ def test_calculate_completeness(mocker, is_scholar_course, tag_counts, expected_
     assert mock_index.call_count == (1 if resource.completeness != 1.0 else 0)
 
 
+def test_calculate_completeness_with_none_content_tags(mocker):
+    """Test that calculate_completeness handles None values in content_tags list"""
+    mock_index = mocker.patch("learning_resources.etl.loaders.update_index")
+    resource = LearningResourceFactory.create(
+        is_course=True,
+        etl_source=ETLSource.ocw.name,
+        platform=LearningResourcePlatformFactory.create(code=PlatformType.ocw.name),
+        offered_by=LearningResourceOfferorFactory.create(is_ocw=True),
+        completeness=0.0,
+    )
+    run = resource.runs.first()
+
+    # Create some content files with tags
+    content_tag = LearningResourceContentTagFactory.create(name="Lecture Videos")
+    ContentFileFactory.create_batch(12, run=run, content_tags=[content_tag])
+
+    # Test with content_tags list containing None values
+    content_tags_with_none = [["Lecture Videos"]] * 12 + [None, None]
+
+    # Should not raise an error and should calculate score based on non-None values
+    score = calculate_completeness(run, content_tags=content_tags_with_none)
+    assert score == 0.2  # 12 lecture videos / 24 = 0.5 * 0.4 = 0.2
+    assert mock_index.call_count == 1
+
+
+def test_load_content_files_with_none_content_tags(mocker):
+    """Test that load_content_files handles None content_tags in source data"""
+    course = LearningResourceFactory.create(is_course=True, create_runs=False)
+    course_run = LearningResourceRunFactory.create(
+        published=True, learning_resource=course
+    )
+
+    # Content data with some files having None content_tags
+    content_data = [
+        {"uid": "file1", "content_tags": ["Lecture Videos"]},
+        {"uid": "file2", "content_tags": None},  # None value
+        {"uid": "file3", "content_tags": ["Lecture Notes"]},
+        {"uid": "file4"},  # Missing content_tags key
+    ]
+
+    mock_load_content_file = mocker.patch(
+        "learning_resources.etl.loaders.load_content_file",
+        side_effect=lambda run, _data: ContentFileFactory.create(run=run).id,
+        autospec=True,
+    )
+    mocker.patch(
+        "learning_resources_search.plugins.tasks.index_run_content_files.si",
+    )
+    mock_calc_score = mocker.patch(
+        "learning_resources.etl.loaders.calculate_completeness"
+    )
+
+    # Should not raise an error
+    result = load_content_files(course_run, content_data, calc_completeness=True)
+
+    assert mock_load_content_file.call_count == len(content_data)
+    assert mock_calc_score.call_count == 1
+    assert len(result) == len(content_data)
+
+    # Verify content_tags passed to calculate_completeness doesn't contain None
+    call_args = mock_calc_score.call_args
+    content_tags_arg = call_args.kwargs.get("content_tags")
+    # All None values should have been converted to empty lists
+    assert all(tags == [] or isinstance(tags, list) for tags in content_tags_arg)
+
+
 def test_course_with_unpublished_force_ingest_is_test_mode():
     """
     Test that a course with force_ingest set to True

learning_resources/etl/ocw.py

@@ -308,27 +308,33 @@ def transform_run(course_data: dict) -> dict:
     return {
         "run_id": course_data["run_id"],
         "published": True,
-        "instructors": parse_instructors(course_data.get("instructors", [])),
-        "description": clean_data(course_data.get("course_description_html")),
+        "instructors": parse_instructors(course_data.get("instructors") or []),
+        "description": clean_data(
+            course_data.get("course_description_html")
+            or course_data.get("course_description ")
+            or ""
+        ),
         "year": year,
         "semester": semester,
         "status": RunStatus.current.value,
         "image": {
             "url": urljoin(settings.OCW_BASE_URL, image_src) if image_src else None,
             "description": course_data.get("course_image_metadata", {}).get(
                 "description"
-            ),
+            )
+            or "",
             "alt": (
                 course_data.get("course_image_metadata", {})
                 .get("image_metadata", {})
                 .get("image-alt")
-            ),
+            )
+            or "",
         },
-        "level": transform_levels(course_data.get("level", [])),
+        "level": transform_levels(course_data.get("level") or []),
         "last_modified": course_data.get("last_modified"),
-        "title": course_data.get("course_title"),
-        "slug": course_data.get("slug"),
-        "url": course_data["url"],
+        "title": course_data.get("course_title") or "",
+        "slug": course_data.get("slug") or "",
+        "url": course_data["url"] or "",
         "availability": Availability.anytime.name,
         "delivery": parse_delivery(course_data),
         "format": [Format.asynchronous.name],

learning_resources/etl/ocw_test.py

@@ -15,13 +15,15 @@
     Format,
     LearningResourceDelivery,
     Pace,
+    RunStatus,
 )
 from learning_resources.etl.constants import CourseNumberType, ETLSource
 from learning_resources.etl.ocw import (
     parse_learn_topics,
     transform_content_files,
     transform_contentfile,
     transform_course,
+    transform_run,
 )
 from learning_resources.factories import (
     ContentFileFactory,
@@ -366,3 +368,93 @@ def test_parse_topics(mocker, has_learn_topics):
             {"name": "Political Science"},
             {"name": "Political Science"},
         ]
+
+
+@pytest.mark.parametrize(
+    ("course_description_html", "course_description", "expected_description"),
+    [
+        (
+            "<p>This is a course description</p>",
+            None,
+            "<p>This is a course description</p>",
+        ),
+        (None, "Fallback description text", "Fallback description text"),
+        (None, None, ""),
+        (
+            "<p>Primary description</p>",
+            "Fallback description",
+            "<p>Primary description</p>",
+        ),
+        ("", "Fallback description text", "Fallback description text"),
+        ("", "", ""),
+        ("<p>Valid HTML</p><script>alert('xss')</script>", None, "<p>Valid HTML</p>"),
+    ],
+)
+def test_transform_run_description_handling(
+    settings, course_description_html, course_description, expected_description
+):
+    """Test that transform_run correctly handles various description field scenarios"""
+    settings.OCW_BASE_URL = "http://test.edu/"
+
+    # Build minimal course_data with required fields
+    course_data = {
+        "run_id": "test-run-id",
+        "url": "http://test.edu/test-course",
+        "instructors": [],
+        "term": "Fall",
+        "year": 2024,
+        "level": ["Undergraduate"],  # Use enum value, not name
+        "image_src": None,
+        "course_image_metadata": {},
+        "course_title": "Test Course",
+        "slug": "test-course",
+        "last_modified": "2024-01-01T00:00:00Z",
+    }
+
+    # Add description fields based on test parameters
+    if course_description_html is not None:
+        course_data["course_description_html"] = course_description_html
+    if course_description is not None:
+        # Note: the key has a trailing space, which seems like a typo in the original code
+        course_data["course_description "] = course_description
+
+    result = transform_run(course_data)
+
+    # Verify the description was processed correctly
+    assert result["description"] == clean_data(expected_description)
+
+    # Verify other required fields are present
+    assert result["run_id"] == "test-run-id"
+    assert result["published"] is True
+    assert result["status"] == RunStatus.current.value
+    assert result["year"] == 2024
+    assert result["semester"] == "Fall"
+    assert result["level"] == ["undergraduate"]  # Output is lowercase enum name
+    assert result["availability"] == Availability.anytime.name
+    assert result["delivery"] is not None
+    assert result["format"] == [Format.asynchronous.name]
+    assert result["pace"] == [Pace.self_paced.name]
+
+
+def test_transform_run_missing_description_fields(settings):
+    """Test transform_run when description fields are missing entirely"""
+    settings.OCW_BASE_URL = "http://test.edu/"
+
+    course_data = {
+        "run_id": "test-run-id",
+        "url": "http://test.edu/test-course",
+        "instructors": [],
+        "term": None,
+        "year": None,
+        "level": [],
+        "image_src": None,
+        "course_image_metadata": {},
+        "course_title": "Test Course",
+        "slug": "test-course",
+        "last_modified": "2024Fcontent_ta-01-01T00:00:00Z",
+    }
+
+    result = transform_run(course_data)
+
+    # Should return empty string when description fields are missing or None
+    assert result["description"] == ""