fix: Retrieve grade and enrollment mode only once

lms/djangoapps/certificates/generation_handler.py

@@ -47,10 +47,13 @@ def generate_allowlist_certificate_task(user, course_key, generation_mode=None):
     """
     Create a task to generate an allowlist certificate for this user in this course run.
     """
-    if _can_generate_allowlist_certificate(user, course_key):
-        return _generate_certificate_task(user=user, course_key=course_key, generation_mode=generation_mode)
+    enrollment_mode = _get_enrollment_mode(user, course_key)
+    course_grade = _get_course_grade(user, course_key)
+    if _can_generate_allowlist_certificate(user, course_key, enrollment_mode):
+        return _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode,
+                                          course_grade=course_grade, generation_mode=generation_mode)
 
-    status = _set_allowlist_cert_status(user, course_key)
+    status = _set_allowlist_cert_status(user, course_key, enrollment_mode, course_grade)
     if status is not None:
         return True
 
@@ -62,25 +65,32 @@ def _generate_regular_certificate_task(user, course_key, generation_mode=None):
     Create a task to generate a regular (non-allowlist) certificate for this user in this course run, if the user is
     eligible and a certificate can be generated.
     """
-    if _can_generate_regular_certificate(user, course_key):
-        return _generate_certificate_task(user=user, course_key=course_key, generation_mode=generation_mode)
+    enrollment_mode = _get_enrollment_mode(user, course_key)
+    course_grade = _get_course_grade(user, course_key)
+    if _can_generate_regular_certificate(user, course_key, enrollment_mode, course_grade):
+        return _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode,
+                                          course_grade=course_grade, generation_mode=generation_mode)
 
-    status = _set_regular_cert_status(user, course_key)
+    status = _set_regular_cert_status(user, course_key, enrollment_mode, course_grade)
     if status is not None:
         return True
 
     return False
 
 
-def _generate_certificate_task(user, course_key, status=None, generation_mode=None):
+def _generate_certificate_task(user, course_key, enrollment_mode, course_grade, status=None, generation_mode=None):
     """
     Create a task to generate a certificate
     """
     log.info(f'About to create a regular certificate task for {user.id} : {course_key}')
 
+    course_grade_val = _get_grade_value(course_grade)
+
     kwargs = {
         'student': str(user.id),
-        'course_key': str(course_key)
+        'course_key': str(course_key),
+        'enrollment_mode': str(enrollment_mode),
+        'course_grade': str(course_grade_val)
     }
     if status is not None:
         kwargs['status'] = status
@@ -91,7 +101,7 @@ def _generate_certificate_task(user, course_key, status=None, generation_mode=No
     return True
 
 
-def _can_generate_allowlist_certificate(user, course_key):
+def _can_generate_allowlist_certificate(user, course_key, enrollment_mode):
     """
     Check if an allowlist certificate can be generated (created if it doesn't already exist, or updated if it does
     exist) for this user, in this course run.
@@ -103,7 +113,7 @@ def _can_generate_allowlist_certificate(user, course_key):
 
     log.info(f'{user.id} : {course_key} is on the certificate allowlist')
 
-    if not _can_generate_certificate_common(user, course_key):
+    if not _can_generate_certificate_common(user, course_key, enrollment_mode):
         log.info(f'One of the common checks failed. Allowlist certificate cannot be generated for {user.id} : '
                  f'{course_key}.')
         return False
@@ -112,7 +122,7 @@ def _can_generate_allowlist_certificate(user, course_key):
     return True
 
 
-def _can_generate_regular_certificate(user, course_key):
+def _can_generate_regular_certificate(user, course_key, enrollment_mode, course_grade):
     """
     Check if a regular (non-allowlist) course certificate can be generated (created if it doesn't already exist, or
     updated if it does exist) for this user, in this course run.
@@ -125,19 +135,19 @@ def _can_generate_regular_certificate(user, course_key):
         log.info(f'{user.id} is a beta tester in {course_key}. Certificate cannot be generated.')
         return False
 
-    if not _has_passing_grade(user, course_key):
+    if not _is_passing_grade(course_grade):
         log.info(f'{user.id} does not have a passing grade in {course_key}. Certificate cannot be generated.')
         return False
 
-    if not _can_generate_certificate_common(user, course_key):
+    if not _can_generate_certificate_common(user, course_key, enrollment_mode):
         log.info(f'One of the common checks failed. Certificate cannot be generated for {user.id} : {course_key}.')
         return False
 
     log.info(f'Regular certificate can be generated for {user.id} : {course_key}')
     return True
 
 
-def _can_generate_certificate_common(user, course_key):
+def _can_generate_certificate_common(user, course_key, enrollment_mode):
     """
     Check if a course certificate can be generated (created if it doesn't already exist, or updated if it does
     exist) for this user, in this course run.
@@ -149,7 +159,6 @@ def _can_generate_certificate_common(user, course_key):
         log.info(f'{user.id} : {course_key} is on the certificate invalidation list. Certificate cannot be generated.')
         return False
 
-    enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(user, course_key)
     if enrollment_mode is None:
         log.info(f'{user.id} : {course_key} does not have an enrollment. Certificate cannot be generated.')
         return False
@@ -179,45 +188,45 @@ def _can_generate_certificate_common(user, course_key):
     return True
 
 
-def _set_allowlist_cert_status(user, course_key):
+def _set_allowlist_cert_status(user, course_key, enrollment_mode, course_grade):
     """
     Determine the allowlist certificate status for this user, in this course run and update the cert.
 
     This is used when a downloadable cert cannot be generated, but we want to provide more info about why it cannot
     be generated.
     """
-    if not _can_set_allowlist_cert_status(user, course_key):
+    if not _can_set_allowlist_cert_status(user, course_key, enrollment_mode):
         return None
 
     cert = GeneratedCertificate.certificate_for_student(user, course_key)
-    return _get_cert_status_common(user, course_key, cert)
+    return _get_cert_status_common(user, course_key, enrollment_mode, course_grade, cert)
 
 
-def _set_regular_cert_status(user, course_key):
+def _set_regular_cert_status(user, course_key, enrollment_mode, course_grade):
     """
     Determine the regular (non-allowlist) certificate status for this user, in this course run.
 
     This is used when a downloadable cert cannot be generated, but we want to provide more info about why it cannot
     be generated.
     """
-    if not _can_set_regular_cert_status(user, course_key):
+    if not _can_set_regular_cert_status(user, course_key, enrollment_mode):
         return None
 
     cert = GeneratedCertificate.certificate_for_student(user, course_key)
-    status = _get_cert_status_common(user, course_key, cert)
+    status = _get_cert_status_common(user, course_key, enrollment_mode, course_grade, cert)
     if status is not None:
         return status
 
-    if IDVerificationService.user_is_verified(user) and not _has_passing_grade(user, course_key) and cert is not None:
+    if IDVerificationService.user_is_verified(user) and not _is_passing_grade(course_grade) and cert is not None:
         if cert.status != CertificateStatuses.notpassing:
-            course_grade = _get_course_grade(user, course_key)
-            cert.mark_notpassing(course_grade.percent, source='certificate_generation')
+            course_grade_val = _get_grade_value(course_grade)
+            cert.mark_notpassing(course_grade_val, source='certificate_generation')
         return CertificateStatuses.notpassing
 
     return None
 
 
-def _get_cert_status_common(user, course_key, cert):
+def _get_cert_status_common(user, course_key, enrollment_mode, course_grade, cert):
     """
     Determine the certificate status for this user, in this course run.
 
@@ -229,28 +238,30 @@ def _get_cert_status_common(user, course_key, cert):
             cert.invalidate(source='certificate_generation')
         return CertificateStatuses.unavailable
 
-    if not IDVerificationService.user_is_verified(user) and _has_passing_grade_or_is_allowlisted(user, course_key):
+    if not IDVerificationService.user_is_verified(user) and _has_passing_grade_or_is_allowlisted(user, course_key,
+                                                                                                 course_grade):
         if cert is None:
-            _generate_certificate_task(user=user, course_key=course_key, generation_mode='batch',
-                                       status=CertificateStatuses.unverified)
+            _generate_certificate_task(user=user, course_key=course_key, enrollment_mode=enrollment_mode,
+                                       course_grade=course_grade, status=CertificateStatuses.unverified,
+                                       generation_mode='batch')
         elif cert.status != CertificateStatuses.unverified:
             cert.mark_unverified(source='certificate_generation')
         return CertificateStatuses.unverified
 
     return None
 
 
-def _can_set_allowlist_cert_status(user, course_key):
+def _can_set_allowlist_cert_status(user, course_key, enrollment_mode):
     """
     Determine whether we can set a custom (non-downloadable) cert status for an allowlist certificate
     """
     if not is_on_certificate_allowlist(user, course_key):
         return False
 
-    return _can_set_cert_status_common(user, course_key)
+    return _can_set_cert_status_common(user, course_key, enrollment_mode)
 
 
-def _can_set_regular_cert_status(user, course_key):
+def _can_set_regular_cert_status(user, course_key, enrollment_mode):
     """
     Determine whether we can set a custom (non-downloadable) cert status for a regular (non-allowlist) certificate
     """
@@ -260,17 +271,16 @@ def _can_set_regular_cert_status(user, course_key):
     if _is_beta_tester(user, course_key):
         return False
 
-    return _can_set_cert_status_common(user, course_key)
+    return _can_set_cert_status_common(user, course_key, enrollment_mode)
 
 
-def _can_set_cert_status_common(user, course_key):
+def _can_set_cert_status_common(user, course_key, enrollment_mode):
     """
     Determine whether we can set a custom (non-downloadable) cert status
     """
     if _is_cert_downloadable(user, course_key):
         return False
 
-    enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(user, course_key)
     if enrollment_mode is None:
         return False
 
@@ -329,32 +339,50 @@ def _is_ccx_course(course_key):
     return hasattr(course_key, 'ccx')
 
 
-def _has_passing_grade_or_is_allowlisted(user, course_key):
+def _has_passing_grade_or_is_allowlisted(user, course_key, course_grade):
     """
     Check if the user has a passing grade in this course run, or is on the allowlist and so is exempt from needing
     a passing grade.
     """
     if is_on_certificate_allowlist(user, course_key):
         return True
 
-    return _has_passing_grade(user, course_key)
+    return _is_passing_grade(course_grade)
 
 
-def _has_passing_grade(user, course_key):
+def _is_passing_grade(course_grade):
     """
-    Check if the user has a passing grade in this course run
+    Check if the grade is a passing grade
     """
-    course_grade = _get_course_grade(user, course_key)
-    return course_grade.passed
+    if course_grade:
+        return course_grade.passed
+    return False
+
+
+def _get_grade_value(course_grade):
+    """
+    Get the user's course grade as a percent, or an empty string if there is no grade
+    """
+    if course_grade:
+        return course_grade.percent
+    return ''
 
 
 def _get_course_grade(user, course_key):
     """
-    Get the user's course grade in this course run
+    Get the user's course grade in this course run. Note that this may be None.
     """
     return CourseGradeFactory().read(user, course_key=course_key)
 
 
+def _get_enrollment_mode(user, course_key):
+    """
+    Get the user's enrollment mode for this course run. Note that this may be None.
+    """
+    enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(user, course_key)
+    return enrollment_mode
+
+
 def _is_cert_downloadable(user, course_key):
     """
     Check if cert already exists, has a downloadable status, and has not been invalidated

lms/djangoapps/certificates/tests/test_api.py

@@ -4,6 +4,7 @@
 import uuid
 from contextlib import contextmanager
 from datetime import datetime, timedelta
+from unittest import mock
 from unittest.mock import patch
 
 import ddt
@@ -23,7 +24,6 @@
 
 from common.djangoapps.course_modes.models import CourseMode
 from common.djangoapps.course_modes.tests.factories import CourseModeFactory
-from common.djangoapps.student.models import CourseEnrollment
 from common.djangoapps.student.tests.factories import (
     CourseEnrollmentFactory,
     GlobalStaffFactory,
@@ -37,6 +37,7 @@
     create_certificate_invalidation_entry,
     create_or_update_certificate_allowlist_entry,
     example_certificates_status,
+    generate_certificate_task,
     generate_example_certificates,
     get_allowlist_entry,
     get_allowlisted_users,
@@ -66,6 +67,7 @@
     GeneratedCertificateFactory,
     CertificateInvalidationFactory
 )
+from lms.djangoapps.certificates.tests.test_generation_handler import ID_VERIFIED_METHOD, PASSING_GRADE_METHOD
 from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
 from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration
 
@@ -536,39 +538,46 @@ def test_get_certificate_with_deleted_course(self):
         assert get_certificate_for_user(self.student.username, self.nonexistent_course_id) is None
 
 
-@override_settings(CERT_QUEUE='certificates')
-class GenerateUserCertificatesTest(EventTestMixin, WebCertificateTestMixin, ModuleStoreTestCase):
+class GenerateUserCertificatesTest(ModuleStoreTestCase):
     """Tests for generating certificates for students. """
 
-    ERROR_REASON = "Kaboom!"
-    ENABLED_SIGNALS = ['course_published']
-
-    def setUp(self):  # pylint: disable=arguments-differ
-        super().setUp('lms.djangoapps.certificates.utils.tracker')
+    def setUp(self):
+        super().setUp()
 
-        self.student = UserFactory.create(
-            email='joe_user@edx.org',
-            username='joeuser',
-            password='foo'
-        )
-        self.student_no_cert = UserFactory()
-        self.course = CourseFactory.create(
-            org='edx',
-            number='verified',
-            display_name='Verified Course',
-            grade_cutoffs={'cutoff': 0.75, 'Pass': 0.5}
+        self.user = UserFactory()
+        self.course_run = CourseFactory()
+        self.course_run_key = self.course_run.id  # pylint: disable=no-member
+        self.enrollment = CourseEnrollmentFactory(
+            user=self.user,
+            course_id=self.course_run_key,
+            is_active=True,
+            mode=CourseMode.VERIFIED,
         )
-        self.enrollment = CourseEnrollment.enroll(self.student, self.course.id, mode='honor')
-        self.request_factory = RequestFactory()
 
     @patch.dict(settings.FEATURES, {'CERTIFICATES_HTML_VIEW': False})
     def test_cert_url_empty_with_invalid_certificate(self):
         """
         Test certificate url is empty if html view is not enabled and certificate is not yet generated
         """
-        url = get_certificate_url(self.student.id, self.course.id)
+        url = get_certificate_url(self.user.id, self.course_run_key)
         assert url == ''
 
+    @patch.dict(settings.FEATURES, {'CERTIFICATES_HTML_VIEW': True})
+    def test_generation(self):
+        """
+        Test that a cert is successfully generated
+        """
+        cert = get_certificate_for_user_id(self.user.id, self.course_run_key)
+        assert not cert
+
+        with mock.patch(PASSING_GRADE_METHOD, return_value=True):
+            with mock.patch(ID_VERIFIED_METHOD, return_value=True):
+                generate_certificate_task(self.user, self.course_run_key)
+
+                cert = get_certificate_for_user_id(self.user.id, self.course_run_key)
+                assert cert.status == CertificateStatuses.downloadable
+                assert cert.mode == CourseMode.VERIFIED
+
 
 @ddt.ddt
 class CertificateGenerationEnabledTest(EventTestMixin, TestCase):