From bbda7384f89344ddbe973772dcbbbae404a95e07 Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Tue, 13 Jul 2021 08:46:57 -0400 Subject: [PATCH] refactor: Remove use of CourseOverview model directly in the Certificates app [MICROBA-1289] * Remove use of the CourseOverview model directly in the Certificates app * Introduce a few new Python API functions in the `course_overview` and `catalog` django apps to replace functionality in the Certificates app. --- lms/djangoapps/certificates/api.py | 3 +- .../certificates/apis/v0/tests/test_views.py | 2 +- lms/djangoapps/certificates/apis/v0/views.py | 38 ++++++-------- lms/djangoapps/certificates/models.py | 9 +++- lms/djangoapps/certificates/views/support.py | 51 +++++++++---------- lms/djangoapps/certificates/views/webview.py | 2 +- openedx/core/djangoapps/catalog/api.py | 14 +++++ .../core/djangoapps/catalog/tests/test_api.py | 32 ++++++++++++ .../content/course_overviews/api.py | 35 ++++++++++++- .../course_overviews/tests/test_api.py | 38 +++++++++++++- 10 files changed, 166 insertions(+), 58 deletions(-) create mode 100644 openedx/core/djangoapps/catalog/tests/test_api.py diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 4b337d6911..eaddf79c01 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -44,7 +44,6 @@ from lms.djangoapps.certificates.utils import ( has_html_certificates_enabled as _has_html_certificates_enabled ) from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview log = logging.getLogger("edx.certificate") User = get_user_model() @@ -243,7 +242,7 @@ def certificate_downloadable_status(student, course_key): 'uuid': None, } - course_overview = CourseOverview.get_from_id(course_key) + course_overview = get_course_overview_or_none(course_key) if ( not certificates_viewable_for_course(course_overview) and CertificateStatuses.is_passing_status(current_status['status']) and diff --git a/lms/djangoapps/certificates/apis/v0/tests/test_views.py b/lms/djangoapps/certificates/apis/v0/tests/test_views.py index 2a88b1159d..731aec06b1 100644 --- a/lms/djangoapps/certificates/apis/v0/tests/test_views.py +++ b/lms/djangoapps/certificates/apis/v0/tests/test_views.py @@ -396,7 +396,7 @@ class CertificatesListRestApiTest(AuthAndScopesTestMixin, SharedModuleStoreTestC expected_download_url = reverse('certificates:render_cert_by_uuid', kwargs=kwargs) self.assert_success_response_for_student(response, download_url=expected_download_url) - @patch('lms.djangoapps.certificates.apis.v0.views.get_course_run_details') + @patch('openedx.core.djangoapps.content.course_overviews.api.get_course_run_details') def test_certificate_without_course(self, mock_get_course_run_details): """ Verify that certificates are returned for deleted XML courses. diff --git a/lms/djangoapps/certificates/apis/v0/views.py b/lms/djangoapps/certificates/apis/v0/views.py index 7bd98e4a9a..59beb65be2 100644 --- a/lms/djangoapps/certificates/apis/v0/views.py +++ b/lms/djangoapps/certificates/apis/v0/views.py @@ -21,8 +21,11 @@ from lms.djangoapps.certificates.api import ( get_certificates_for_user ) from lms.djangoapps.certificates.apis.v0.permissions import IsOwnerOrPublicCertificates -from openedx.core.djangoapps.catalog.utils import get_course_run_details -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.content.course_overviews.api import ( + get_course_overview_or_none, + get_course_overviews_from_ids, + get_pseudo_course_overview +) from openedx.core.djangoapps.user_api.accounts.api import visible_fields from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser @@ -97,7 +100,7 @@ class CertificatesDetailView(APIView): def get(self, request, username, course_id): """ - Gets a certificate information. + Retrieves certificate information for a user in a specified course run. Args: request (Request): Django request object. @@ -123,9 +126,10 @@ class CertificatesDetailView(APIView): data={'error_code': 'no_certificate_for_user'} ) - course_overview = CourseOverview.get_from_id(course_id) + course_overview = get_course_overview_or_none(course_id) # return 404 if it's not a PDF certificates and there is no active certificate configuration. - if not user_cert['is_pdf_certificate'] and not course_overview.has_any_active_web_certificate: + if not user_cert['is_pdf_certificate'] and (not course_overview or + not course_overview.has_any_active_web_certificate): return Response( status=404, data={'error_code': 'no_certificate_configuration_for_course'} @@ -174,7 +178,7 @@ class CertificatesListView(APIView): ) ]) def get(self, request, username): - """Get a paginated list of bookmarks for a user. + """Get a paginated list of certificates for a user. **Use Case** @@ -270,32 +274,22 @@ class CertificatesListView(APIView): passing_certificates[course_key] = course_certificate viewable_certificates = [] - for course_key, course_overview in CourseOverview.get_from_ids( - list(passing_certificates.keys()) - ).items(): + course_ids = list(passing_certificates.keys()) + course_overviews = get_course_overviews_from_ids(course_ids) + for course_key, course_overview in course_overviews.items(): if not course_overview: # For deleted XML courses in which learners have a valid certificate. # i.e. MITx/7.00x/2013_Spring - course_overview = self._get_pseudo_course_overview(course_key) + course_overview = get_pseudo_course_overview(course_key) if certificates_viewable_for_course(course_overview): course_certificate = passing_certificates[course_key] # add certificate into viewable certificate list only if it's a PDF certificate # or there is an active certificate configuration. - if course_certificate['is_pdf_certificate'] or course_overview.has_any_active_web_certificate: + if course_certificate['is_pdf_certificate'] or (course_overview and + course_overview.has_any_active_web_certificate): course_certificate['course_display_name'] = course_overview.display_name_with_default course_certificate['course_organization'] = course_overview.display_org_with_default viewable_certificates.append(course_certificate) viewable_certificates.sort(key=lambda certificate: certificate['created']) return viewable_certificates - - def _get_pseudo_course_overview(self, course_key): - """ - Returns a pseudo course overview object for deleted courses. - """ - course_run = get_course_run_details(course_key, ['title']) - return CourseOverview( - display_name=course_run.get('title'), - display_org_with_default=course_key.org, - certificates_show_before_end=True - ) diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index e18bda1748..a12bd67744 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -30,7 +30,7 @@ from lms.djangoapps.badges.events.course_complete import course_badge_check from lms.djangoapps.badges.events.course_meta import completion_check, course_group_check from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.instructor_task.models import InstructorTask -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none from openedx.core.djangoapps.signals.signals import COURSE_CERT_AWARDED, COURSE_CERT_CHANGED, COURSE_CERT_REVOKED from openedx.core.djangoapps.xmodule_django.models import NoneToEmptyManager @@ -657,7 +657,12 @@ def certificate_info_for_user(user, course_id, grade, user_is_allowlisted, user_ certificate_type = 'N/A' status = certificate_status(user_certificate) certificate_generated = status['status'] == CertificateStatuses.downloadable - can_have_certificate = CourseOverview.get_from_id(course_id).may_certify() + + can_have_certificate = False + course_overview = get_course_overview_or_none(course_id) + if course_overview: + can_have_certificate = course_overview.may_certify() + enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(user, course_id) mode_is_verified = enrollment_mode in CourseMode.VERIFIED_MODES user_is_verified = grade is not None and mode_is_verified diff --git a/lms/djangoapps/certificates/views/support.py b/lms/djangoapps/certificates/views/support.py index f09aaa0004..a2d934a1d0 100644 --- a/lms/djangoapps/certificates/views/support.py +++ b/lms/djangoapps/certificates/views/support.py @@ -25,7 +25,6 @@ from lms.djangoapps.certificates.api import generate_certificate_task, get_certi from lms.djangoapps.certificates.permissions import GENERATE_ALL_CERTIFICATES, VIEW_ALL_CERTIFICATES from lms.djangoapps.instructor_task.api import generate_certificates_for_students from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview log = logging.getLogger(__name__) @@ -113,16 +112,16 @@ def search_certificates(request): except InvalidKeyError: return HttpResponseBadRequest(_("Course id '{course_id}' is not valid").format(course_id=course_id)) else: - try: - if CourseOverview.get_from_id(course_key): - certificates = [certificate for certificate in certificates - if certificate['course_key'] == course_id] - if not certificates: - return JsonResponse([{'username': user.username, 'course_key': course_id, 'regenerate': False}]) - except CourseOverview.DoesNotExist: + course_overview = get_course_overview_or_none(course_key) + if not course_overview: msg = _("The course does not exist against the given key '{course_key}'").format(course_key=course_key) return HttpResponseBadRequest(msg) + certificates = [certificate for certificate in certificates + if certificate['course_key'] == course_id] + if not certificates: + return JsonResponse([{'username': user.username, 'course_key': course_id, 'regenerate': False}]) + return JsonResponse(certificates) @@ -245,26 +244,24 @@ def generate_certificate_for_user(request): if response is not None: return response - try: - # Check that the course exists - CourseOverview.get_from_id(params["course_key"]) - except CourseOverview.DoesNotExist: + course_overview = get_course_overview_or_none(params["course_key"]) + if not course_overview: msg = _("The course {course_key} does not exist").format(course_key=params["course_key"]) return HttpResponseBadRequest(msg) - else: - # Check that the user is enrolled in the course - if not CourseEnrollment.is_enrolled(params["user"], params["course_key"]): - msg = _("User {username} is not enrolled in the course {course_key}").format( - username=params["user"].username, - course_key=params["course_key"] - ) - return HttpResponseBadRequest(msg) - # Attempt to generate certificate - generate_certificates_for_students( - request, - params["course_key"], - student_set="specific_student", - specific_student_id=params["user"].id + # Check that the user is enrolled in the course + if not CourseEnrollment.is_enrolled(params["user"], params["course_key"]): + msg = _("User {username} is not enrolled in the course {course_key}").format( + username=params["user"].username, + course_key=params["course_key"] ) - return HttpResponse(200) + return HttpResponseBadRequest(msg) + + # Attempt to generate certificate + generate_certificates_for_students( + request, + params["course_key"], + student_set="specific_student", + specific_student_id=params["user"].id + ) + return HttpResponse(200) diff --git a/lms/djangoapps/certificates/views/webview.py b/lms/djangoapps/certificates/views/webview.py index 2fc9ee85d0..3e3861039b 100644 --- a/lms/djangoapps/certificates/views/webview.py +++ b/lms/djangoapps/certificates/views/webview.py @@ -44,7 +44,7 @@ from lms.djangoapps.certificates.models import ( ) from lms.djangoapps.certificates.permissions import PREVIEW_CERTIFICATES from lms.djangoapps.certificates.utils import emit_certificate_event, get_certificate_url -from openedx.core.djangoapps.catalog.utils import get_course_run_details +from openedx.core.djangoapps.catalog.api import get_course_run_details from openedx.core.djangoapps.certificates.api import display_date_for_certificate from openedx.core.djangoapps.lang_pref.api import get_closest_released_language from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers diff --git a/openedx/core/djangoapps/catalog/api.py b/openedx/core/djangoapps/catalog/api.py index baf3cd878c..4428eb5f32 100644 --- a/openedx/core/djangoapps/catalog/api.py +++ b/openedx/core/djangoapps/catalog/api.py @@ -5,6 +5,7 @@ Python APIs exposed by the catalog app to other in-process apps. from .utils import get_programs_by_type_slug as _get_programs_by_type_slug from .utils import get_programs as _get_programs from .utils import course_run_keys_for_program as _course_run_keys_for_program +from .utils import get_course_run_details as _get_course_run_details def get_programs_by_type(site, program_type_slug): @@ -47,3 +48,16 @@ def get_course_run_key_for_program_from_cache(program): (set): A set of Course Run Keys. """ return _course_run_keys_for_program(parent_program=program) + + +def get_course_run_details(course_key, fields_list): + """ + Retrieves course run details for a given course run key. + + Params: + course_key (CourseKey): The identifier for the course. + fields_list (List, string): A list of fields (as strings) of values we want returned. + Returns: + dict containing response from the Discovery API that includes the fields specified in `fields_list` + """ + return _get_course_run_details(course_key, fields_list) diff --git a/openedx/core/djangoapps/catalog/tests/test_api.py b/openedx/core/djangoapps/catalog/tests/test_api.py new file mode 100644 index 0000000000..ed657d992d --- /dev/null +++ b/openedx/core/djangoapps/catalog/tests/test_api.py @@ -0,0 +1,32 @@ +""" +Tests for the Catalog apps `api.py` functions. +""" +from mock import patch + +from django.test import TestCase + +from openedx.core.djangoapps.catalog.api import get_course_run_details +from openedx.core.djangoapps.catalog.tests.factories import CourseRunFactory +from openedx.core.djangolib.testing.utils import skip_unless_lms + + +@skip_unless_lms +class TestCatalogApi(TestCase): + """ + Tests for the Catalog apps `api.py` functions. + """ + + @patch("openedx.core.djangoapps.catalog.api._get_course_run_details") + def test_get_course_run_details(self, mock_get_course_run_details): + """ + Test for Python API `get_course_run_details` function. + """ + course_run = CourseRunFactory() + + mock_get_course_run_details.return_value = { + 'title': course_run['title'], + } + + results = get_course_run_details(course_run['key'], ['title']) + + assert results['title'] == course_run['title'] diff --git a/openedx/core/djangoapps/content/course_overviews/api.py b/openedx/core/djangoapps/content/course_overviews/api.py index d695b84093..17400671e8 100644 --- a/openedx/core/djangoapps/content/course_overviews/api.py +++ b/openedx/core/djangoapps/content/course_overviews/api.py @@ -3,6 +3,7 @@ CourseOverview api """ import logging +from openedx.core.djangoapps.catalog.api import get_course_run_details from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content.course_overviews.serializers import ( CourseOverviewBaseSerializer, @@ -24,9 +25,41 @@ def get_course_overview_or_none(course_id): return None +def get_pseudo_course_overview(course_id): + """ + Returns a pseudo course overview object for a deleted course. + + Params: + course_id (CourseKey): The identifier for the course. + Returns: + (Temporary) CourseOverview object representing for a deleted course. + """ + fields = ['title'] + course_run = get_course_run_details(course_id, fields) + + return CourseOverview( + display_name=course_run.get('title'), + display_org_with_default=course_id.org, + certificates_show_before_end=True + ) + + +def get_course_overviews_from_ids(course_ids): + """ + Return course overviews for the specified course ids. + + Params: + course_ids (iterable[CourseKey]) + + Returns: + dict[CourseKey, CourseOverview|None] + """ + return CourseOverview.get_from_ids(course_ids) + + def get_course_overviews(course_ids): """ - Return course_overview data for a given list of opaque_key course_ids. + Return (serialized) course_overview data for a given list of opaque_key course_ids. """ overviews = CourseOverview.objects.filter(id__in=course_ids) return CourseOverviewBaseSerializer(overviews, many=True).data diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_api.py b/openedx/core/djangoapps/content/course_overviews/tests/test_api.py index 37f972f5ca..c69fed072c 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_api.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_api.py @@ -1,16 +1,20 @@ """ course_overview api tests """ +from mock import patch from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.catalog.tests.factories import CourseRunFactory +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.content.course_overviews.api import ( get_course_overview_or_none, - get_course_overviews + get_course_overviews, + get_course_overviews_from_ids, + get_pseudo_course_overview, ) from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from ..models import CourseOverview class TestCourseOverviewsApi(ModuleStoreTestCase): @@ -39,6 +43,19 @@ class TestCourseOverviewsApi(ModuleStoreTestCase): retrieved_course_overview = get_course_overview_or_none(course_run_key) assert retrieved_course_overview is None + def test_get_course_overview_from_ids(self): + """ + Test for `get_course_overviews_from_ids` function. + """ + course_ids = [] + course_overview_data = CourseOverview.objects.all() + for course_overview in course_overview_data: + course_ids.append(course_overview.id) + + results = get_course_overviews_from_ids(course_ids) + + assert len(results) == 3 + def test_get_course_overviews(self): """ get_course_overviews should return the expected CourseOverview data @@ -61,3 +78,20 @@ class TestCourseOverviewsApi(ModuleStoreTestCase): ] for field in fields: assert field in data[0] + + @patch("openedx.core.djangoapps.content.course_overviews.api.get_course_run_details") + def test_get_pseudo_course_overview(self, mock_get_course_run_details): + """ + Test for the `get_pseudo_course_overview` function that creates a temporary course overview for courses that + have been deleted. + """ + course_run = CourseRunFactory() + mock_get_course_run_details.return_value = { + 'title': course_run['title'], + } + course_key = CourseKey.from_string(course_run['key']) + + result = get_pseudo_course_overview(course_key) + assert result.display_name == course_run['title'] + assert result.display_org_with_default == course_key.org + assert result.certificates_show_before_end