From cdb0619846f5dbdeaaad773bcb4a79e4e6cac1fe Mon Sep 17 00:00:00 2001
From: Kyle McCormick <kmccormick@edx.org>
Date: Thu, 23 Jan 2020 13:52:11 -0500
Subject: [PATCH] Revert "Remove CourseOverview.get_from_id[s]_if_exists
(#22918)" (#22926)
This reverts commit 3ca006214ee5fb70d061a8cd91897757ebc724a0.
---
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, 74 insertions(+), 132 deletions(-)
delete 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 b8b0f47aa3d..4479086ac37 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(
+ for course_key, course_overview in CourseOverview.get_from_ids_if_exists(
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 277cac8d4cb..59d48f5a647 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(course_run_keys)
+ overviews = CourseOverview.get_from_ids_if_exists(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 fd417dbfb28..e32f8068e16 100644
--- a/openedx/core/djangoapps/content/course_overviews/models.py
+++ b/openedx/core/djangoapps/content/course_overviews/models.py
@@ -335,34 +335,48 @@ class CourseOverview(TimeStampedModel):
return course_overview or cls.load_from_module_store(course_id)
@classmethod
- def get_from_ids(cls, course_ids):
+ def get_from_ids_if_exists(cls, course_ids):
"""
- Return a dict mapping course_ids to CourseOverviews.
+ Return a dict mapping course_ids to CourseOverviews, if they exist.
- Tries to select all CourseOverviews in one query,
- then fetches remaining (uncached) overviews from the modulestore.
+ 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.
- Course IDs for non-existant courses will map to None.
-
- Arguments:
- course_ids (iterable[CourseKey])
-
- Returns: dict[CourseKey, CourseOverview|None]
+ Callers should assume that this list is incomplete and fall back to
+ get_from_id if they need to guarantee CourseOverview generation.
"""
- overviews = {
+ return {
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
)
}
- 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
+
+ @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
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 1ce66e3b0d2..a683bf7ca3d 100644
--- a/openedx/core/djangoapps/content/course_overviews/signals.py
+++ b/openedx/core/djangoapps/content/course_overviews/signals.py
@@ -25,10 +25,7 @@ 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.
"""
- try:
- previous_course_overview = CourseOverview.objects.get(id=course_key)
- except CourseOverview.DoesNotExist:
- previous_course_overview = None
+ previous_course_overview = CourseOverview.get_from_ids_if_exists([course_key]).get(course_key)
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 3b11a36be7f..a7cd1fa65cd 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,7 +16,6 @@ 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
@@ -551,48 +550,46 @@ class CourseOverviewTestCase(CatalogIntegrationMixin, ModuleStoreTestCase, Cache
.format(filter_),
)
- 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.
- """
+ def test_get_from_ids_if_exists(self):
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)
- 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
+
+ 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)
@ddt.ddt
diff --git a/openedx/core/lib/api/tests/test_view_utils.py b/openedx/core/lib/api/tests/test_view_utils.py
deleted file mode 100644
index ac3c5779375..00000000000
--- a/openedx/core/lib/api/tests/test_view_utils.py
+++ /dev/null
@@ -1,66 +0,0 @@
-"""
-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 b25c072fc18..47cc8e3a7a5 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.course_exists(course_key):
+ if not CourseOverview.get_from_id_if_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