From 28c0433352d5b417dfd3653db78cb614460a6007 Mon Sep 17 00:00:00 2001
From: Kyle McCormick <kmccormick@edx.org>
Date: Mon, 27 Jan 2020 09:34:18 -0500
Subject: [PATCH] Reinstate "Remove CourseOverview.get_from_id[s]_if_exists
(#22918)"
This reverts commit cdb0619846f5dbdeaaad773bcb4a79e4e6cac1fe,
which itself reverted 3ca006214ee5fb70d061a8cd91897757ebc724a0.
The original commit (3ca006) was reverted because it was suspected
that it was causing unexpectedly-increased memcached usage and
500s in the Gradebook API. It is not clear whether that is actually
the case. We are optimistically reinstating 3ca006 and will monitor
production to see if there is an adverse effect.
MST-105
---
lms/djangoapps/certificates/apis/v0/views.py | 2 +-
.../program_enrollments/rest_api/v1/views.py | 2 +-
.../content/course_overviews/models.py | 52 +++++--------
.../content/course_overviews/signals.py | 5 +-
.../tests/test_course_overviews.py | 77 ++++++++++---------
openedx/core/lib/api/tests/test_view_utils.py | 66 ++++++++++++++++
openedx/core/lib/api/view_utils.py | 2 +-
7 files changed, 132 insertions(+), 74 deletions(-)
create mode 100644 openedx/core/lib/api/tests/test_view_utils.py
diff --git a/lms/djangoapps/certificates/apis/v0/views.py b/lms/djangoapps/certificates/apis/v0/views.py
index 4479086ac37..b8b0f47aa3d 100644
--- a/lms/djangoapps/certificates/apis/v0/views.py
+++ b/lms/djangoapps/certificates/apis/v0/views.py
@@ -260,7 +260,7 @@ class CertificatesListView(GenericAPIView):
passing_certificates[course_key] = course_certificate
viewable_certificates = []
- for course_key, course_overview in CourseOverview.get_from_ids_if_exists(
+ for course_key, course_overview in CourseOverview.get_from_ids(
list(passing_certificates.keys())
).items():
if certificates_viewable_for_course(course_overview):
diff --git a/lms/djangoapps/program_enrollments/rest_api/v1/views.py b/lms/djangoapps/program_enrollments/rest_api/v1/views.py
index 59d48f5a647..277cac8d4cb 100644
--- a/lms/djangoapps/program_enrollments/rest_api/v1/views.py
+++ b/lms/djangoapps/program_enrollments/rest_api/v1/views.py
@@ -900,7 +900,7 @@ class ProgramCourseEnrollmentOverviewView(
is_active=True,
)
- overviews = CourseOverview.get_from_ids_if_exists(course_run_keys)
+ overviews = CourseOverview.get_from_ids(course_run_keys)
course_run_resume_urls = get_resume_urls_for_enrollments(user, course_enrollments)
diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py
index e32f8068e16..fd417dbfb28 100644
--- a/openedx/core/djangoapps/content/course_overviews/models.py
+++ b/openedx/core/djangoapps/content/course_overviews/models.py
@@ -335,48 +335,34 @@ class CourseOverview(TimeStampedModel):
return course_overview or cls.load_from_module_store(course_id)
@classmethod
- def get_from_ids_if_exists(cls, course_ids):
+ def get_from_ids(cls, course_ids):
"""
- Return a dict mapping course_ids to CourseOverviews, if they exist.
+ Return a dict mapping course_ids to CourseOverviews.
- This method will *not* generate new CourseOverviews or delete outdated
- ones. It exists only as a small optimization used when CourseOverviews
- are known to exist, for common situations like the student dashboard.
+ Tries to select all CourseOverviews in one query,
+ then fetches remaining (uncached) overviews from the modulestore.
- Callers should assume that this list is incomplete and fall back to
- get_from_id if they need to guarantee CourseOverview generation.
+ Course IDs for non-existant courses will map to None.
+
+ Arguments:
+ course_ids (iterable[CourseKey])
+
+ Returns: dict[CourseKey, CourseOverview|None]
"""
- return {
+ overviews = {
overview.id: overview
- for overview
- in cls.objects.select_related('image_set').filter(
+ for overview in cls.objects.select_related('image_set').filter(
id__in=course_ids,
version__gte=cls.VERSION
)
}
-
- @classmethod
- def get_from_id_if_exists(cls, course_id):
- """
- Return a CourseOverview for the provided course_id if it exists.
- Returns None if no CourseOverview exists with the provided course_id
-
- This method will *not* generate new CourseOverviews or delete outdated
- ones. It exists only as a small optimization used when CourseOverviews
- are known to exist, for common situations like the student dashboard.
-
- Callers should assume that this list is incomplete and fall back to
- get_from_id if they need to guarantee CourseOverview generation.
- """
- try:
- course_overview = cls.objects.select_related('image_set').get(
- id=course_id,
- version__gte=cls.VERSION
- )
- except cls.DoesNotExist:
- course_overview = None
-
- return course_overview
+ for course_id in course_ids:
+ if course_id not in overviews:
+ try:
+ overviews[course_id] = cls.load_from_module_store(course_id)
+ except CourseOverview.DoesNotExist:
+ overviews[course_id] = None
+ return overviews
def clean_id(self, padding_char='='):
"""
diff --git a/openedx/core/djangoapps/content/course_overviews/signals.py b/openedx/core/djangoapps/content/course_overviews/signals.py
index a683bf7ca3d..1ce66e3b0d2 100644
--- a/openedx/core/djangoapps/content/course_overviews/signals.py
+++ b/openedx/core/djangoapps/content/course_overviews/signals.py
@@ -25,7 +25,10 @@ def _listen_for_course_publish(sender, course_key, **kwargs): # pylint: disable
Catches the signal that a course has been published in Studio and
updates the corresponding CourseOverview cache entry.
"""
- previous_course_overview = CourseOverview.get_from_ids_if_exists([course_key]).get(course_key)
+ try:
+ previous_course_overview = CourseOverview.objects.get(id=course_key)
+ except CourseOverview.DoesNotExist:
+ previous_course_overview = None
updated_course_overview = CourseOverview.load_from_module_store(course_key)
_check_for_course_changes(previous_course_overview, updated_course_overview)
diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py
index a7cd1fa65cd..3b11a36be7f 100644
--- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py
+++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py
@@ -16,6 +16,7 @@ from django.conf import settings
from django.db.utils import IntegrityError
from django.test.utils import override_settings
from django.utils import timezone
+from opaque_keys.edx.keys import CourseKey
from PIL import Image
from six.moves import range # pylint: disable=ungrouped-imports
@@ -550,46 +551,48 @@ class CourseOverviewTestCase(CatalogIntegrationMixin, ModuleStoreTestCase, Cache
.format(filter_),
)
- def test_get_from_ids_if_exists(self):
+ def test_get_from_ids(self):
+ """
+ Assert that CourseOverviews.get_from_ids works as expected.
+
+ We expect that if we have four courses, of which:
+ * two have cached course overviews,
+ * one does *not* have a cache course overview, and
+ * one has an *out-of-date* course overview, that
+ all four course overviews will appear in teh resulting dictionary,
+ with the former two coming from the CourseOverviews SQL cache
+ and the latter two coming from the modulestore.
+ """
course_with_overview_1 = CourseFactory.create(emit_signals=True)
course_with_overview_2 = CourseFactory.create(emit_signals=True)
course_without_overview = CourseFactory.create(emit_signals=False)
-
- courses = [course_with_overview_1, course_with_overview_2, course_without_overview]
- course_ids_to_overviews = CourseOverview.get_from_ids_if_exists(
- course.id for course in courses
- )
-
- # We should see the ones that have published CourseOverviews
- # (when the signals were emitted), but not the one that didn't issue
- # a publish signal.
- self.assertEqual(len(course_ids_to_overviews), 2)
- self.assertIn(course_with_overview_1.id, course_ids_to_overviews)
- self.assertIn(course_with_overview_2.id, course_ids_to_overviews)
- self.assertNotIn(course_without_overview.id, course_ids_to_overviews)
-
- # But if we set a CourseOverview to be an old version, it shouldn't be
- # returned by get_from_ids_if_exists()
- overview_2 = course_ids_to_overviews[course_with_overview_2.id]
- overview_2.version = CourseOverview.VERSION - 1
- overview_2.save()
- course_ids_to_overviews = CourseOverview.get_from_ids_if_exists(
- course.id for course in courses
- )
- self.assertEqual(len(course_ids_to_overviews), 1)
- self.assertIn(course_with_overview_1.id, course_ids_to_overviews)
-
- def test_get_from_id_if_exists(self):
- course_with_overview = CourseFactory.create(emit_signals=True)
- course_id_to_overview = CourseOverview.get_from_id_if_exists(course_with_overview.id)
- self.assertEqual(course_with_overview.id, course_id_to_overview.id)
-
- overview_prev_version = CourseOverview.get_from_id_if_exists(course_with_overview.id)
- overview_prev_version.version = CourseOverview.VERSION - 1
- overview_prev_version.save()
-
- course_id_to_overview = CourseOverview.get_from_id_if_exists(course_with_overview.id)
- self.assertEqual(course_id_to_overview, None)
+ course_with_old_overview = CourseFactory.create(emit_signals=True)
+ old_overview = CourseOverview.objects.get(id=course_with_old_overview.id)
+ old_overview.version = CourseOverview.VERSION - 1
+ old_overview.save()
+
+ courses = [
+ course_with_overview_1,
+ course_with_overview_2,
+ course_without_overview,
+ course_with_old_overview,
+ ]
+ non_existent_course_key = CourseKey.from_string('course-v1:This+Course+IsFake')
+ course_ids = [course.id for course in courses] + [non_existent_course_key]
+
+ with mock.patch.object(
+ CourseOverview,
+ 'load_from_module_store',
+ wraps=CourseOverview.load_from_module_store
+ ) as mock_load_from_modulestore:
+ overviews_by_id = CourseOverview.get_from_ids(course_ids)
+ assert len(overviews_by_id) == 5
+ assert overviews_by_id[course_with_overview_1.id].id == course_with_overview_1.id
+ assert overviews_by_id[course_with_overview_2.id].id == course_with_overview_2.id
+ assert overviews_by_id[course_with_old_overview.id].id == course_with_old_overview.id
+ assert overviews_by_id[old_overview.id].id == old_overview.id
+ assert overviews_by_id[non_existent_course_key] is None
+ assert mock_load_from_modulestore.call_count == 3
@ddt.ddt
diff --git a/openedx/core/lib/api/tests/test_view_utils.py b/openedx/core/lib/api/tests/test_view_utils.py
new file mode 100644
index 00000000000..ac3c5779375
--- /dev/null
+++ b/openedx/core/lib/api/tests/test_view_utils.py
@@ -0,0 +1,66 @@
+"""
+Tests for (some of) the view utilities.
+"""
+from django.conf.urls import url
+from django.test.utils import override_settings
+from rest_framework.response import Response
+from rest_framework.test import APITestCase
+from rest_framework.views import APIView
+
+from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
+from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
+
+from ..view_utils import DeveloperErrorViewMixin, verify_course_exists
+
+
+class MockAPIView(DeveloperErrorViewMixin, APIView):
+ """
+ Mock API view for testing.
+ """
+
+ @verify_course_exists
+ def get(self, request, course_id):
+ """
+ Mock GET handler for testing.
+ """
+ return Response("Success {}".format(course_id))
+
+urlpatterns = [
+ url(r'^mock/(?P<course_id>.*)/$', MockAPIView.as_view()), # Only works with new-style course keys
+]
+
+
+@override_settings(ROOT_URLCONF=__name__)
+class VerifyCourseExistsTestCase(SharedModuleStoreTestCase, APITestCase):
+ """
+ Tests for behavior of @verify_course_exists.
+ """
+ def test_bad_key_404(self):
+ response = self.client.get('/mock/this-is-a-bad-key/')
+ assert response.status_code == 404
+
+ def test_nonexistent_course_404(self):
+ CourseFactory()
+ response = self.client.get('/mock/course-v1:Non+Existent+Course/')
+ assert response.status_code == 404
+
+ def test_course_exists_200(self):
+ course = CourseFactory(
+ org="This", course="IsA", run="Course",
+ default_store=ModuleStoreEnum.Type.split,
+ )
+ response = self.client.get('/mock/{}/'.format(course.id))
+ assert response.status_code == 200
+
+ def test_course_with_outdated_overview_200(self):
+ course = CourseFactory(
+ org="This", course="IsAnother", run="Course",
+ default_store=ModuleStoreEnum.Type.split,
+ )
+ course_overview = CourseOverview.get_from_id(course.id)
+ course_overview.version = CourseOverview.VERSION - 1
+ course_overview.save()
+ response = self.client.get('/mock/{}/'.format(course.id))
+ assert response.status_code == 200
diff --git a/openedx/core/lib/api/view_utils.py b/openedx/core/lib/api/view_utils.py
index 47cc8e3a7a5..b25c072fc18 100644
--- a/openedx/core/lib/api/view_utils.py
+++ b/openedx/core/lib/api/view_utils.py
@@ -417,7 +417,7 @@ def verify_course_exists(view_func):
error_code='invalid_course_key'
)
- if not CourseOverview.get_from_id_if_exists(course_key):
+ if not CourseOverview.course_exists(course_key):
raise self.api_error(
status_code=status.HTTP_404_NOT_FOUND,
developer_message=u"Requested grade for unknown course {course}".format(course=text_type(course_key)),
--
GitLab