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

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

Browse Source 
This reverts commit 3ca006214e.
This commit is contained in:
Kyle McCormick
2020-01-23 13:52:11 -05:00
committed by GitHub
parent 9fa49606f8
commit cdb0619846
7 changed files with 72 additions and 130 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(
for course_key, course_overview in CourseOverview.get_from_ids_if_exists(
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(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)

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

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

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

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

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

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

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

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

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