Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reinstate "Remove CourseOverview.get_from_id[s]_if_exists (#22918)"

Browse Source 
This reverts commit cdb0619846,
which itself reverted 3ca006214e.

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
This commit is contained in:
Kyle McCormick
2020-01-27 09:34:18 -05:00
committed by Kyle McCormick
parent e9c75e06ec
commit 28c0433352
7 changed files with 130 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
lms/djangoapps/certificates/apis/v0/views.py
View File

@@ -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):

2
lms/djangoapps/program_enrollments/rest_api/v1/views.py
View File

@@ -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)

52
openedx/core/djangoapps/content/course_overviews/models.py
View File

@@ -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='='):
"""

5
openedx/core/djangoapps/content/course_overviews/signals.py
View File

@@ -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)

73
openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py
View File

@@ -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)
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_ids_to_overviews = CourseOverview.get_from_ids_if_exists(
course.id for course in courses
)
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]
# 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)
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

66
openedx/core/lib/api/tests/test_view_utils.py Normal file
View File

@@ -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

2
openedx/core/lib/api/view_utils.py
View File

@@ -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)),