Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions lms/djangoapps/course_home_api/outline/tests/test_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -907,3 +907,37 @@ def test_vertical_icon_determined_by_icon_class(self):
response = self.client.get(reverse('course-home:course-navigation', args=[self.course.id]))
vertical_data = response.data['blocks'][str(self.vertical.location)]
assert vertical_data['icon'] == 'video'

def test_navigation_serves_fresh_data_after_publish(self):
"""
Regression test: the navigation sidebar should serve fresh data when
the modulestore has changed but the block structure cache is stale.

This simulates a production scenario where:
1. A unit is deleted and the course is auto-published
2. The block structure rebuild Celery task is queued with a 30s delay
3. A learner hits the navigation endpoint during that 30s window

Without the fix, stale block structure data gets cached for 1 hour.
"""
self.add_blocks_to_course()
CourseEnrollment.enroll(self.user, self.course.id, CourseMode.VERIFIED)

# First request — populates both block structure and navigation caches
response = self.client.get(self.url)
assert response.status_code == 200
sequential_data = response.data['blocks'][str(self.sequential.location)]
assert str(self.vertical.location) in sequential_data['children']

# Delete the vertical directly in the modulestore. Signals are disabled
# in ModuleStoreTestCase, so the block structure cache is now stale —
# mirroring the 30s window in production before the rebuild task runs.
self.store.delete_item(self.vertical.location, self.user.id)
update_outline_from_modulestore(self.course.id)

# Without the fix, this returns stale data with the deleted vertical.
# With the fix, update_collected_if_needed() detects staleness and rebuilds.
response = self.client.get(self.url)
assert response.status_code == 200
sequential_data = response.data['blocks'][str(self.sequential.location)]
assert str(self.vertical.location) not in sequential_data['children']
4 changes: 4 additions & 0 deletions lms/djangoapps/course_home_api/outline/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
from lms.djangoapps.courseware.views.views import get_cert_data
from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory
from lms.djangoapps.utils import OptimizelyClient
from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager
from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_404
from openedx.core.djangoapps.content.learning_sequences.api import get_user_course_outline
from openedx.core.djangoapps.course_groups.cohorts import get_cohort
Expand Down Expand Up @@ -483,6 +484,9 @@ def get(self, request, *args, **kwargs):
course_blocks = cache.get(cache_key)

if not course_blocks:
# Ensure the block structure cache is up-to-date before reading.
get_block_structure_manager(course_key).update_collected_if_needed()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going through the collection phase of a large course can be extremely expensive, which is why it's done asynchronously in celery tasks or management commands (it can often exceed the 30s timeout that many sites use for giving up on web worker requests). Placing it in the GET here also risks causing a stampede if it is a popular course that many concurrent users are trying to access, as parallel workers try to recompute the same collection phase data.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I appreciate this feedback. I'll look into another way of preventing the stale cache.


if getattr(enrollment, 'is_active', False) or bool(staff_access):
course_blocks = get_course_outline_block_tree(request, course_key_string, request.user)
elif allow_public_outline or allow_public or user_is_masquerading:
Expand Down
Loading