8621-featured-items-celery-task-is-not-robust (#2973)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: cp-at-mit

cms/api.py

@@ -22,7 +22,6 @@
 from cms.constants import CERTIFICATE_INDEX_SLUG, INSTRUCTOR_INDEX_SLUG
 from cms.exceptions import WagtailSpecificPageError
 from cms.models import Page
-from courses.constants import HOMEPAGE_CACHE_AGE
 from courses.models import Course, Program
 from courses.utils import (
     get_enrollable_courseruns_qs,
@@ -364,7 +363,7 @@ def create_featured_items():
     )
 
     if not valid_course_ids:
-        redis_cache.set(cache_key, [], HOMEPAGE_CACHE_AGE)
+        redis_cache.set(cache_key, [])
         return []
 
     enrollable_courses_qs = Course.objects.select_related("page").filter(
@@ -377,7 +376,7 @@ def create_featured_items():
     )
 
     if not enrollable_courseruns.exists():
-        redis_cache.set(cache_key, [], HOMEPAGE_CACHE_AGE)
+        redis_cache.set(cache_key, [])
         return []
 
     self_paced_runs = []
@@ -410,21 +409,20 @@ def create_featured_items():
     all_course_ids = self_paced_course_ids + future_course_ids + started_course_ids
 
     if not all_course_ids:
-        redis_cache.set(cache_key, [], HOMEPAGE_CACHE_AGE)
+        redis_cache.set(cache_key, [])
         return []
 
     ordering = Case(
         *[When(id=cid, then=pos) for pos, cid in enumerate(all_course_ids)],
         output_field=IntegerField(),
     )
 
-    featured_courses = list(
+    # Store only course IDs to avoid pickling issues and ensure fresh data on retrieval
+    redis_cache.set(cache_key, all_course_ids)
+
+    return list(
         Course.objects.filter(id__in=all_course_ids)
         .select_related("page")
         .prefetch_related("courseruns")
         .order_by(ordering)
     )
-
-    redis_cache.set(cache_key, featured_courses, HOMEPAGE_CACHE_AGE)
-
-    return featured_courses

cms/api_test.py

@@ -360,18 +360,18 @@ def test_create_featured_items():
 
     assert len(cache_value) == 4
     assert set(cache_value) == {
-        enrollable_future_course,
-        enrollable_other_future_course,
-        enrollable_self_paced_course,
-        in_progress_course,
+        enrollable_future_course.id,
+        enrollable_other_future_course.id,
+        enrollable_self_paced_course.id,
+        in_progress_course.id,
     }
 
-    assert cache_value[0] == enrollable_self_paced_course
-    assert cache_value[1] == enrollable_future_course
-    assert cache_value[2] == enrollable_other_future_course
-    assert cache_value[3] == in_progress_course
+    assert cache_value[0] == enrollable_self_paced_course.id
+    assert cache_value[1] == enrollable_future_course.id
+    assert cache_value[2] == enrollable_other_future_course.id
+    assert cache_value[3] == in_progress_course.id
 
-    assert unenrollable_course not in cache_value
+    assert unenrollable_course.id not in cache_value
 
 
 @pytest.mark.django_db
@@ -496,7 +496,7 @@ def test_create_featured_items_course_run_at_exact_time():
 
     assert len(result) == 1
     assert result[0] == exact_time_course
-    assert exact_time_course in cache_value
+    assert exact_time_course.id in cache_value
 
 
 @pytest.mark.django_db

cms/models.py

@@ -792,13 +792,13 @@ def get_cached_featured_products(self):
         """
         now = now_in_utc()
         redis_cache = caches["redis"]
-        cached_featured_products = redis_cache.get("CMS_homepage_featured_courses")
-        if cached_featured_products and len(cached_featured_products) > 0:
-            featured_product_ids = [course.id for course in cached_featured_products]
+        cached_featured_course_ids = redis_cache.get("CMS_homepage_featured_courses")
+        if cached_featured_course_ids and len(cached_featured_course_ids) > 0:
+            # Load fresh course data from database using cached IDs
             relevant_run_course_ids = (
                 CourseRun.objects.filter(live=True)
                 .filter(
-                    models.Q(course__id__in=featured_product_ids)
+                    models.Q(course__id__in=cached_featured_course_ids)
                     & models.Q(enrollment_start__lte=now)
                     & (
                         models.Q(enrollment_end__gte=now)
@@ -807,12 +807,28 @@ def get_cached_featured_products(self):
                 )
                 .values_list("course__id", flat=True)
             )
+
+            # Create ordering to maintain the sequence from the cache
+            ordering = models.Case(
+                *[
+                    models.When(id=cid, then=pos)
+                    for pos, cid in enumerate(cached_featured_course_ids)
+                ],
+                output_field=models.IntegerField(),
+            )
+
+            valid_course_ids = set(cached_featured_course_ids) & set(
+                relevant_run_course_ids
+            )
+
+            featured_courses = (
+                Course.objects.filter(id__in=valid_course_ids, page__live=True)
+                .select_related("page")
+                .order_by(ordering)
+            )
+
             featured_products = [
-                course.page
-                for course in cached_featured_products
-                if course.page is not None
-                and course.page.live
-                and course.id in relevant_run_course_ids
+                course.page for course in featured_courses if course.page is not None
             ]
             featured_product_pages = []
             for page in featured_products:

cms/tasks.py

@@ -2,11 +2,9 @@
 from urllib.parse import urljoin, urlparse
 
 import requests
-from django.core.cache import caches
 from mitol.common.decorators import single_task
 
 from cms.api import create_featured_items
-from cms.constants import ONE_MINUTE
 from cms.models import Page
 from main.celery import app
 from main.settings import (
@@ -109,12 +107,5 @@ def refresh_featured_homepage_items():
     """
     logger = logging.getLogger("refresh_featured_homepage_items__task")
     logger.info("Refreshing featured homepage items...")
-    # if the key is not found, the ttl will be 0 per their docs
-    # https://github.com/jazzband/django-redis?tab=readme-ov-file#get-ttl-time-to-live-from-key
-    redis_cache = caches["redis"]
-    if redis_cache.ttl("CMS_homepage_featured_courses") > (10 * ONE_MINUTE):
-        logger.info("Featured courses found in cache, moving on")
-        return
-    logger.info("No featured courses found in cache, refreshing")
     create_featured_items()
-    logger.info("New featured items created")
+    logger.info("Featured items refreshed")

courses/constants.py

@@ -30,8 +30,6 @@
     zip(ALL_ENROLL_CHANGE_STATUSES, ALL_ENROLL_CHANGE_STATUSES)
 )
 
-HOMEPAGE_CACHE_AGE = 86400  # 24 hours
-
 SYNCED_COURSE_RUN_FIELD_MSG = "This value is synced automatically with edX studio."
 
 AVAILABILITY_ANYTIME = "anytime"

main/celery_utils.py

@@ -24,7 +24,6 @@ def is_due(self, last_run_at):
         if self._apply_offset is not None and retval.is_due:
             self._apply_offset = False
             self.run_every = self._run_every
-            retval = super().is_due(last_run_at)
         return retval
 
     def __reduce__(self):

main/settings.py

@@ -1046,8 +1046,8 @@
 
 REFRESH_FEATURED_HOMEPAGE_ITEMS_FREQ = get_int(
     name="REFRESH_FEATURED_HOMEPAGE_ITEMS_FREQ",
-    default=60,
-    description="How many seconds between checking for featured items for the homepage in the local in memory cache",
+    default=86400,
+    description="How many seconds between refreshing featured items for the homepage cache",
 )
 
 REFRESH_FEATURED_HOMEPAGE_ITEMS_OFFSET = int(REFRESH_FEATURED_HOMEPAGE_ITEMS_FREQ / 2)