From dd06820cadafe4a86e9fcd35d7242e8a5f9fe48b Mon Sep 17 00:00:00 2001 From: Justin Hynes Date: Wed, 12 May 2021 11:53:19 -0400 Subject: [PATCH] refactor: consolidate duplicate utility functions in certificates app [MICROBA-1208] * consolidate `has_html_certificates_enabled` and `has_html_certificates_enabled_from_course_overview`, the latter of the two functions was created for use during our transition away from using the modulestore in the certificate app. --- lms/djangoapps/certificates/api.py | 4 +-- lms/djangoapps/certificates/generation.py | 4 +-- .../certificates/generation_handler.py | 10 +++---- .../tests/test_generation_handler.py | 2 +- .../certificates/tests/test_utils.py | 8 +++--- lms/djangoapps/certificates/utils.py | 26 +++++-------------- 6 files changed, 20 insertions(+), 34 deletions(-) diff --git a/lms/djangoapps/certificates/api.py b/lms/djangoapps/certificates/api.py index 125e5fd6bc..9ea624d466 100644 --- a/lms/djangoapps/certificates/api.py +++ b/lms/djangoapps/certificates/api.py @@ -424,8 +424,8 @@ def example_certificates_status(course_key): return ExampleCertificateSet.latest_status(course_key) -def has_html_certificates_enabled(course): - return _has_html_certificates_enabled(course) +def has_html_certificates_enabled(course_overview): + return _has_html_certificates_enabled(course_overview) def get_certificate_url(user_id=None, course_id=None, uuid=None, user_certificate=None): diff --git a/lms/djangoapps/certificates/generation.py b/lms/djangoapps/certificates/generation.py index 96cfcf5ba2..58c29c2930 100644 --- a/lms/djangoapps/certificates/generation.py +++ b/lms/djangoapps/certificates/generation.py @@ -15,7 +15,7 @@ from uuid import uuid4 from common.djangoapps.student.models import CourseEnrollment, UserProfile from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from lms.djangoapps.certificates.queue import XQueueCertInterface -from lms.djangoapps.certificates.utils import emit_certificate_event, has_html_certificates_enabled_from_course_overview +from lms.djangoapps.certificates.utils import emit_certificate_event, has_html_certificates_enabled from lms.djangoapps.grades.api import CourseGradeFactory from lms.djangoapps.instructor.access import list_with_level_from_course_key from openedx.core.djangoapps.content.course_overviews.api import get_course_overview @@ -135,7 +135,7 @@ def generate_user_certificates(student, course_key, course=None, insecure=False, xqueue.use_https = False course_overview = get_course_overview(course_key) - generate_pdf = not has_html_certificates_enabled_from_course_overview(course_overview) + generate_pdf = not has_html_certificates_enabled(course_overview) cert = xqueue.add_cert( student, diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index fad5716286..76ece3f206 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -22,7 +22,7 @@ from lms.djangoapps.certificates.queue import XQueueCertInterface from lms.djangoapps.certificates.tasks import CERTIFICATE_DELAY_SECONDS, generate_certificate from lms.djangoapps.certificates.utils import ( emit_certificate_event, - has_html_certificates_enabled_from_course_overview + has_html_certificates_enabled ) from lms.djangoapps.grades.api import CourseGradeFactory from lms.djangoapps.instructor.access import list_with_level_from_course_key @@ -216,7 +216,7 @@ def _can_generate_certificate_common(user, course_key): return False course_overview = get_course_overview(course_key) - if not has_html_certificates_enabled_from_course_overview(course_overview): + if not has_html_certificates_enabled(course_overview): log.info(f'{course_key} does not have HTML certificates enabled. Certificate cannot be generated for ' f'{user.id}.') return False @@ -329,7 +329,7 @@ def _can_set_cert_status_common(user, course_key): return False course_overview = get_course_overview(course_key) - if not has_html_certificates_enabled_from_course_overview(course_overview): + if not has_html_certificates_enabled(course_overview): return False return True @@ -456,7 +456,7 @@ def generate_user_certificates(student, course_key, course=None, insecure=False, xqueue.use_https = False course_overview = get_course_overview(course_key) - generate_pdf = not has_html_certificates_enabled_from_course_overview(course_overview) + generate_pdf = not has_html_certificates_enabled(course_overview) cert = xqueue.add_cert( student, @@ -516,7 +516,7 @@ def regenerate_user_certificates(student, course_key, course=None, xqueue.use_https = False course_overview = get_course_overview(course_key) - generate_pdf = not has_html_certificates_enabled_from_course_overview(course_overview) + generate_pdf = not has_html_certificates_enabled(course_overview) log.info(f"Started regenerating certificates for user {student.id} in course {course_key} with generate_pdf " f"status: {generate_pdf}.") diff --git a/lms/djangoapps/certificates/tests/test_generation_handler.py b/lms/djangoapps/certificates/tests/test_generation_handler.py index c3c206d64b..cb73efe86b 100644 --- a/lms/djangoapps/certificates/tests/test_generation_handler.py +++ b/lms/djangoapps/certificates/tests/test_generation_handler.py @@ -37,7 +37,7 @@ BETA_TESTER_METHOD = 'lms.djangoapps.certificates.generation_handler._is_beta_te CCX_COURSE_METHOD = 'lms.djangoapps.certificates.generation_handler._is_ccx_course' ID_VERIFIED_METHOD = 'lms.djangoapps.verify_student.services.IDVerificationService.user_is_verified' PASSING_GRADE_METHOD = 'lms.djangoapps.certificates.generation_handler._has_passing_grade' -WEB_CERTS_METHOD = 'lms.djangoapps.certificates.generation_handler.has_html_certificates_enabled_from_course_overview' +WEB_CERTS_METHOD = 'lms.djangoapps.certificates.generation_handler.has_html_certificates_enabled' @mock.patch(ID_VERIFIED_METHOD, mock.Mock(return_value=True)) diff --git a/lms/djangoapps/certificates/tests/test_utils.py b/lms/djangoapps/certificates/tests/test_utils.py index 4fad425945..1fda4e37e2 100644 --- a/lms/djangoapps/certificates/tests/test_utils.py +++ b/lms/djangoapps/certificates/tests/test_utils.py @@ -5,7 +5,7 @@ from unittest.mock import patch from django.test import TestCase -from lms.djangoapps.certificates.utils import has_html_certificates_enabled_from_course_overview +from lms.djangoapps.certificates.utils import has_html_certificates_enabled from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory @@ -22,7 +22,7 @@ class CertificateUtilityTests(TestCase): """ Test to ensure we return the correct value when the `CERTIFICATES_HTML_VIEW` setting is disabled. """ - assert not has_html_certificates_enabled_from_course_overview(self.course_overview) + assert not has_html_certificates_enabled(self.course_overview) @patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': True}) def test_has_html_certificates_enabled_from_course_overview_enabled(self): @@ -32,7 +32,7 @@ class CertificateUtilityTests(TestCase): self.course_overview.cert_html_view_enabled = True self.course_overview.save() - assert has_html_certificates_enabled_from_course_overview(self.course_overview) + assert has_html_certificates_enabled(self.course_overview) @patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': True}) def test_has_html_certificates_enabled_from_course_overview_disabled(self): @@ -42,4 +42,4 @@ class CertificateUtilityTests(TestCase): self.course_overview.cert_html_view_enabled = False self.course_overview.save() - assert not has_html_certificates_enabled_from_course_overview(self.course_overview) + assert not has_html_certificates_enabled(self.course_overview) diff --git a/lms/djangoapps/certificates/utils.py b/lms/djangoapps/certificates/utils.py index fcf76b3ac8..70102e0631 100644 --- a/lms/djangoapps/certificates/utils.py +++ b/lms/djangoapps/certificates/utils.py @@ -11,7 +11,6 @@ from opaque_keys.edx.keys import CourseKey from lms.djangoapps.certificates.models import GeneratedCertificate from openedx.core.djangoapps.content.course_overviews.api import get_course_overview -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview log = logging.getLogger(__name__) @@ -51,33 +50,20 @@ def get_certificate_url(user_id=None, course_id=None, uuid=None, user_certificat """ url = '' - course = _course_from_key(course_id) - if not course: + course_overview = _course_from_key(course_id) + if not course_overview: return url - if has_html_certificates_enabled(course): + if has_html_certificates_enabled(course_overview): url = _certificate_html_url(uuid) else: url = _certificate_download_url(user_id, course_id, user_certificate=user_certificate) return url -def has_html_certificates_enabled(course): +def has_html_certificates_enabled(course_overview): """ - Returns True if HTML certificates are enabled - """ - if not settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False): - return False - return course.cert_html_view_enabled - - -def has_html_certificates_enabled_from_course_overview(course_overview): - """ - Returns True if HTML certificates are enabled - - This is a companion function to the `has_html_certificates_enabled` function. We are in the process of refactoring - and removing the `Certificates` apps dependence on `modulestore`. These functions will be consolidated at a later - date. Progress is being tracked in MICROBA-1178. + Returns True if HTML certificates are enabled in a course run. """ if not settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False): return False @@ -120,7 +106,7 @@ def _course_from_key(course_key): """ Returns the course overview """ - return CourseOverview.get_from_id(_safe_course_key(course_key)) + return get_course_overview(_safe_course_key(course_key)) def _safe_course_key(course_key):