fix broken images in subscription emails

learning_resources/tasks.py

@@ -38,11 +38,16 @@
     get_bucket_by_name,
     get_s3_prefix_for_source,
 )
-from learning_resources.models import ContentFile, LearningResource
+from learning_resources.models import (
+    ContentFile,
+    LearningResource,
+    LearningResourceImage,
+)
 from learning_resources.site_scrapers.utils import scraper_for_site
 from learning_resources.utils import (
     build_program_children_content_bulk,
     html_to_markdown,
+    image_url_is_reachable,
     load_course_blocklist,
     programs_needing_children_heal,
     resource_unpublished_actions,
@@ -847,3 +852,35 @@ def cleanup_deleted_content_files():
         error = "cleanup_deleted_content_files threw an error"
         log.exception(error)
         return error
+
+
+@app.task(acks_late=True, reject_on_worker_lost=True)
+def prune_unreachable_resource_images():
+    """
+    Delete LearningResourceImage records whose URLs no longer resolve, so that
+    resources fall back to the default image everywhere (API, search, emails)
+    instead of rendering a broken image.
+    """
+    image_ids = (
+        LearningResource.objects.filter(published=True, image__isnull=False)
+        .values_list("image_id", flat=True)
+        .distinct()
+    )
+    removed = 0
+    for image in LearningResourceImage.objects.filter(id__in=image_ids).iterator():
+        if image_url_is_reachable(image.url):
+            continue
+        resource_ids = list(image.learningresource_set.values_list("id", flat=True))
+        log.info(
+            "Pruning unreachable image %s from resources %s", image.url, resource_ids
+        )
+        # FK is SET_NULL, so deleting the record clears it on referencing resources
+        image.delete()
+        removed += 1
+        for resource in LearningResource.objects.filter(
+            id__in=resource_ids, published=True
+        ):
+            resource_upserted_actions(
+                resource, percolate=False, generate_embeddings=False
+            )
+    return removed

learning_resources/tasks_test.py

@@ -26,6 +26,7 @@
     get_youtube_data,
     get_youtube_transcripts,
     marketing_page_for_resources,
+    prune_unreachable_resource_images,
     scrape_marketing_pages,
     sync_canvas_courses,
     update_next_start_date_and_prices,
@@ -1187,3 +1188,58 @@ def test_cleanup_deleted_content_files_returns_error_on_unexpected_exception(moc
     result = cleanup_deleted_content_files()
 
     assert result == "cleanup_deleted_content_files threw an error"
+
+
+def test_prune_unreachable_resource_images(mocker):
+    """
+    prune_unreachable_resource_images should delete image records with dead
+    URLs and reindex the affected published resources
+    """
+    good_resource = LearningResourceFactory.create(is_course=True)
+    bad_resource = LearningResourceFactory.create(is_course=True)
+    unpublished_resource = LearningResourceFactory.create(
+        is_course=True, published=False
+    )
+    reachable_mock = mocker.patch(
+        "learning_resources.tasks.image_url_is_reachable",
+        side_effect=lambda url: url == good_resource.image.url,
+    )
+    upserted_mock = mocker.patch("learning_resources.tasks.resource_upserted_actions")
+
+    removed = prune_unreachable_resource_images()
+
+    assert removed == 1
+    good_resource.refresh_from_db()
+    bad_resource.refresh_from_db()
+    unpublished_resource.refresh_from_db()
+    assert good_resource.image is not None
+    assert bad_resource.image is None
+    # unpublished resources should not be checked at all
+    assert unpublished_resource.image is not None
+    checked_urls = [call.args[0] for call in reachable_mock.call_args_list]
+    assert unpublished_resource.image.url not in checked_urls
+    upserted_mock.assert_called_once_with(
+        bad_resource, percolate=False, generate_embeddings=False
+    )
+
+
+def test_prune_unreachable_resource_images_shared_image(mocker):
+    """
+    Images are deduplicated by URL, so a single dead image record can be shared
+    by many resources. All published referencing resources should be reindexed.
+    """
+    shared_image = factories.LearningResourceImageFactory.create(
+        url="http://example.com/dead.jpg"
+    )
+    shared = LearningResourceFactory.create_batch(2, is_course=True, image=shared_image)
+    mocker.patch("learning_resources.tasks.image_url_is_reachable", return_value=False)
+    upserted_mock = mocker.patch("learning_resources.tasks.resource_upserted_actions")
+
+    # a shared image is only checked (and deleted) once
+    assert prune_unreachable_resource_images() == 1
+    assert not models.LearningResourceImage.objects.filter(id=shared_image.id).exists()
+    for resource in shared:
+        resource.refresh_from_db()
+        assert resource.image is None
+    reindexed = {call.args[0].id for call in upserted_mock.call_args_list}
+    assert reindexed == {resource.id for resource in shared}

learning_resources/utils.py

@@ -4,6 +4,8 @@
 import re
 from collections import defaultdict
 from functools import cache
+from hashlib import md5
+from http import HTTPStatus
 from shutil import which
 from typing import TYPE_CHECKING
 
@@ -45,6 +47,7 @@
 log = logging.getLogger()
 
 BLOCKLIST_CACHE_TIMEOUT = 60 * 60 * 24
+IMAGE_URL_REACHABLE_CACHE_TIMEOUT = 60 * 60 * 24
 
 
 # edX block types that never have content
@@ -227,6 +230,40 @@
     return blocklist
 
 
+def image_url_is_reachable(url: str) -> bool:
+    """
+    Check whether an image URL responds successfully, caching the result.
+
+    Args:
+        url (str): the image URL to check
+
+    Returns:
+        bool: True if the URL responds successfully
+    """
+    cache_key = f"image_url_reachable:{md5(url.encode('utf-8')).hexdigest()}"  # noqa: S324
+    redis_cache = caches["redis"]
+    reachable = redis_cache.get(cache_key)
+    if reachable is None:
+        try:
+            response = requests.head(
+                url, timeout=settings.REQUESTS_TIMEOUT, allow_redirects=True
+            )
+            if response.status_code in (
+                HTTPStatus.METHOD_NOT_ALLOWED,
+                HTTPStatus.NOT_IMPLEMENTED,
+            ):
+                # Some servers reject HEAD requests; retry without the body
+                response = requests.get(
+                    url, timeout=settings.REQUESTS_TIMEOUT, stream=True
+                )
+                response.close()
+            reachable = response.ok
+        except requests.RequestException:
+            reachable = False
+        redis_cache.set(cache_key, reachable, timeout=IMAGE_URL_REACHABLE_CACHE_TIMEOUT)
+    return reachable
+
+
 def load_course_duplicates(etl_source: str) -> list:
     """
     Get a list of blocklisted course ids for an ETL pipeline source

learning_resources/utils_test.py

@@ -8,6 +8,8 @@
 
 import markdown
 import pytest
+import requests
+import responses
 import yaml
 from faker import Faker
 
@@ -45,6 +47,7 @@
     build_program_children_content,
     build_program_children_content_bulk,
     filter_valid_edx_module_ids,
+    image_url_is_reachable,
     is_loggable_missing_content_id,
     is_valid_edx_module_id,
     log_missing_content_file,
@@ -1304,3 +1307,45 @@ def test_sanitize_llm_text(text, expected):
     assert result == expected
     # The result must always be storable: strict UTF-8 encoding cannot raise
     result.encode("utf-8")
+
+
+@pytest.mark.parametrize(
+    ("status", "expected"),
+    [(200, True), (301, True), (404, False), (403, False), (500, False)],
+)
+def test_image_url_is_reachable(mocked_responses, status, expected):
+    """image_url_is_reachable should be True only for successful responses"""
+    url = "http://example.com/image.jpg"
+    mocked_responses.add(responses.HEAD, url, status=status)
+    assert image_url_is_reachable(url) is expected
+
+
+def test_image_url_is_reachable_head_not_allowed(mocked_responses):
+    """image_url_is_reachable should fall back to GET if HEAD is rejected"""
+    url = "http://example.com/image.jpg"
+    mocked_responses.add(responses.HEAD, url, status=405)
+    mocked_responses.add(responses.GET, url, status=200)
+    assert image_url_is_reachable(url) is True
+
+
+def test_image_url_is_reachable_connection_error(mocked_responses):
+    """image_url_is_reachable should be False if the request errors out"""
+    url = "http://example.com/image.jpg"
+    mocked_responses.add(
+        responses.HEAD, url, body=requests.exceptions.ConnectionError()
+    )
+    assert image_url_is_reachable(url) is False
+
+
+def test_image_url_is_reachable_caches_result(mocked_responses, settings):
+    """image_url_is_reachable should cache results per URL"""
+    settings.CACHES = {
+        **settings.CACHES,
+        "redis": {"BACKEND": "django.core.cache.backends.locmem.LocMemCache"},
+    }
+    url = "http://example.com/image.jpg"
+    mocked_responses.add(responses.HEAD, url, status=404)
+    assert image_url_is_reachable(url) is False
+    # second call hits the cache; RequestsMock would fail on a second request
+    assert image_url_is_reachable(url) is False
+    assert len(mocked_responses.calls) == 1

learning_resources_search/tasks.py

@@ -27,7 +27,7 @@
     LearningResourceDepartment,
     LearningResourceOfferor,
 )
-from learning_resources.utils import load_course_blocklist
+from learning_resources.utils import image_url_is_reachable, load_course_blocklist
 from learning_resources.views import FeaturedViewSet
 from learning_resources_search import indexing_api as api
 from learning_resources_search.api import (
@@ -220,6 +220,21 @@ def key_func(x):
     return grouped_data
 
 
+def _validated_resource_image_url(resource):
+    """
+    Return the resource's image URL if it is reachable, otherwise the default
+    resource image. Email clients can't fall back on their own, so a dead URL
+    would render as a broken image icon.
+    """
+    if (
+        resource.image
+        and resource.image.url
+        and image_url_is_reachable(resource.image.url)
+    ):
+        return resource.image.url
+    return frontend_absolute_url("/images/default_resource.jpg")
+
+
 def _get_percolated_rows(resources, subscription_type):
     """
     Get percolated rows for a list of learning resources and subscription type
@@ -232,6 +247,7 @@ def _get_percolated_rows(resources, subscription_type):
             source_type=subscription_type
         )
         if percolated.count() > 0:
+            resource_image_url = _validated_resource_image_url(resource)
             percolated_users = set(percolated.values_list("users", flat=True))
             all_users.update(percolated_users)
             for user in percolated_users:
@@ -251,9 +267,7 @@ def _get_percolated_rows(resources, subscription_type):
                     {
                         "resource_url": resource_url,
                         "resource_title": resource.title,
-                        "resource_image_url": resource.image.url
-                        if resource.image
-                        else frontend_absolute_url("/images/default_resource.jpg"),
+                        "resource_image_url": resource_image_url,
                         "resource_type": LearningResourceType[
                             resource.resource_type
                         ].value,

learning_resources_search/tasks_test.py

@@ -46,6 +46,7 @@
     _group_percolated_rows,
     _infer_percolate_group,
     _maybe_finish_reindex_job,
+    _validated_resource_image_url,
     bulk_deindex_learning_resources,
     deindex_document,
     deindex_run_content_files,
@@ -64,6 +65,7 @@
 from main.factories import TaskBatchFactory, TaskJobFactory, UserFactory
 from main.models import TaskBatch, TaskJob
 from main.test_utils import assert_not_raises
+from main.utils import frontend_absolute_url
 
 pytestmark = pytest.mark.django_db
 User = get_user_model()
@@ -86,6 +88,14 @@ def mocked_api(mocker):
     return mocker.patch("learning_resources_search.tasks.api")
 
 
+@pytest.fixture(autouse=True)
+def mock_image_url_is_reachable(mocker):
+    """Mock the image URL reachability check to avoid network requests"""
+    return mocker.patch(
+        "learning_resources_search.tasks.image_url_is_reachable", return_value=True
+    )
+
+
 def test_upsert_learning_resource(mocked_api):
     """Test that upsert_learning_resourc will serialize the learning resource data and upsert it to the OS index"""
     resource = LearningResourceFactory.create()
@@ -1724,6 +1734,31 @@ def get_percolator(res):
         assert topic in template_data
 
 
+@pytest.mark.parametrize("reachable", [True, False])
+def test_validated_resource_image_url(mock_image_url_is_reachable, reachable):
+    """
+    The digest email should use the resource image only if its URL is
+    reachable, and the default image otherwise
+    """
+    mock_image_url_is_reachable.return_value = reachable
+    resource = LearningResourceFactory.create(is_course=True)
+    validated_url = _validated_resource_image_url(resource)
+    if reachable:
+        assert validated_url == resource.image.url
+    else:
+        assert validated_url == frontend_absolute_url("/images/default_resource.jpg")
+    mock_image_url_is_reachable.assert_called_once_with(resource.image.url)
+
+
+def test_validated_resource_image_url_no_image(mock_image_url_is_reachable):
+    """The digest email should use the default image if the resource has none"""
+    resource = LearningResourceFactory.create(is_course=True, no_image=True)
+    assert _validated_resource_image_url(resource) == frontend_absolute_url(
+        "/images/default_resource.jpg"
+    )
+    mock_image_url_is_reachable.assert_not_called()
+
+
 def test_subscription_digest_subject():
     """
     Test that email generates a dynamic subject based

main/settings_celery.py

@@ -201,6 +201,12 @@
                 minute=0, hour=6, day_of_week=6
             ),  # 2:00am EST on Friday
         },
+        "weekly-prune-unreachable-resource-images": {
+            "task": "learning_resources.tasks.prune_unreachable_resource_images",
+            "schedule": crontab(
+                minute=0, hour=10, day_of_week=0
+            ),  # 6:00am EST on Sunday
+        },
         "cleanup-deleted-content-files-every-1-days": {
             "task": "learning_resources.tasks.cleanup_deleted_content_files",
             "schedule": crontab(