Merge pull request #28201 from edx/crice/params

fix: Unify required parameter checks

lms/djangoapps/certificates/generation.py

@@ -10,17 +10,15 @@ These methods should be called from tasks.
 import logging
 from uuid import uuid4
 
-from common.djangoapps.student.models import CourseEnrollment, UserProfile
+from common.djangoapps.student.models import UserProfile
 from lms.djangoapps.certificates.data import CertificateStatuses
 from lms.djangoapps.certificates.models import GeneratedCertificate
 from lms.djangoapps.certificates.utils import emit_certificate_event
-from lms.djangoapps.grades.api import CourseGradeFactory
 
 log = logging.getLogger(__name__)
 
 
-def generate_course_certificate(user, course_key, status, enrollment_mode=None, course_grade=None,
-                                generation_mode=None):
+def generate_course_certificate(user, course_key, status, enrollment_mode, course_grade, generation_mode):
     """
     Generate a course certificate for this user, in this course run. If the certificate has a passing status, also emit
     a certificate event.
@@ -37,13 +35,6 @@ def generate_course_certificate(user, course_key, status, enrollment_mode=None,
         generation_mode: used when emitting an event. Options are "self" (implying the user generated the cert
             themself) and "batch" for everything else.
     """
-    if not enrollment_mode:
-        enrollment_mode = _get_enrollment_mode(user, course_key)
-    if not course_grade:
-        course_grade = _get_course_grade(user, course_key)
-    if not generation_mode:
-        generation_mode = 'batch'
-
     cert = _generate_certificate(user=user, course_key=course_key, status=status, enrollment_mode=enrollment_mode,
                                  course_grade=course_grade)
 
@@ -109,19 +100,3 @@ def _generate_certificate(user, course_key, status, enrollment_mode, course_grad
     log.info(f'Generated certificate with status {cert.status}, mode {cert.mode} and grade {cert.grade} for {user.id} '
              f': {course_key}. {created_msg}')
     return cert
-
-
-def _get_course_grade(user, course_key):
-    """
-    Get the user's course grade in this course run
-    """
-    course_grade = CourseGradeFactory().read(user, course_key=course_key)
-    return course_grade.percent
-
-
-def _get_enrollment_mode(user, course_key):
-    """
-    Get the user's enrollment mode in this course run
-    """
-    enrollment_mode, __ = CourseEnrollment.enrollment_mode_for_user(user, course_key)
-    return enrollment_mode

lms/djangoapps/certificates/tasks.py

@@ -25,20 +25,20 @@ def generate_certificate(self, **kwargs):  # pylint: disable=unused-argument
     Generates a certificate for a single user.
 
     kwargs:
-        - student: The student for whom to generate a certificate.
+        - student: The student for whom to generate a certificate. Required.
         - course_key: The course key for the course that the student is
-            receiving a certificate in.
-        - status: Certificate status (value from the CertificateStatuses model)
-        - enrollment_mode: user's enrollment mode (ex. verified)
-        - course_grade: user's course grade
+            receiving a certificate in. Required.
+        - status: Certificate status (value from the CertificateStatuses model). Defaults to 'downloadable'.
+        - enrollment_mode: User's enrollment mode (ex. verified). Required.
+        - course_grade: User's course grade. Defaults to ''.
         - generation_mode: Used when emitting an event. Options are "self" (implying the user generated the cert
-            themself) and "batch" for everything else.
+            themself) and "batch" for everything else. Defaults to 'batch'.
     """
     student = User.objects.get(id=kwargs.pop('student'))
     course_key = CourseKey.from_string(kwargs.pop('course_key'))
     status = kwargs.pop('status', CertificateStatuses.downloadable)
-    enrollment_mode = kwargs.pop('enrollment_mode', None)
-    course_grade = kwargs.pop('course_grade', None)
+    enrollment_mode = kwargs.pop('enrollment_mode')
+    course_grade = kwargs.pop('course_grade', '')
     generation_mode = kwargs.pop('generation_mode', 'batch')
 
     generate_course_certificate(user=student, course_key=course_key, status=status, enrollment_mode=enrollment_mode,

lms/djangoapps/certificates/tests/test_generation.py

@@ -2,7 +2,6 @@
 Tests for certificate generation
 """
 import logging
-from unittest import mock
 
 from common.djangoapps.course_modes.models import CourseMode
 from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory
@@ -16,9 +15,6 @@ from xmodule.modulestore.tests.factories import CourseFactory
 
 log = logging.getLogger(__name__)
 
-ENROLLMENT_METHOD = 'lms.djangoapps.certificates.generation._get_enrollment_mode'
-GRADE_METHOD = 'lms.djangoapps.certificates.generation._get_course_grade'
-
 
 class CertificateTests(EventTestMixin, ModuleStoreTestCase):
     """
@@ -163,18 +159,3 @@ class CertificateTests(EventTestMixin, ModuleStoreTestCase):
                                                      self.enrollment_mode, self.grade, self.gen_mode)
         assert generated_cert.status, CertificateStatuses.downloadable
         assert generated_cert.verify_uuid != ''
-
-    def test_generation_few_params(self):
-        """
-        Test that ensures we retrieve values as needed
-        """
-        grade = '.33'
-        enrollment_mode = CourseMode.AUDIT
-
-        with mock.patch(ENROLLMENT_METHOD, return_value=enrollment_mode):
-            with mock.patch(GRADE_METHOD, return_value=grade):
-                generated_cert = generate_course_certificate(self.u, self.key, CertificateStatuses.downloadable)
-
-                assert generated_cert.status, CertificateStatuses.downloadable
-                assert generated_cert.mode, enrollment_mode
-                assert generated_cert.grade, grade

lms/djangoapps/certificates/tests/test_tasks.py

@@ -25,10 +25,16 @@ class GenerateUserCertificateTest(TestCase):
         super().setUp()
 
         self.user = UserFactory()
+        self.course_key = 'course-v1:edX+DemoX+Demo_Course'
 
-    @ddt.data('student', 'course_key')
+    @ddt.data('student', 'course_key', 'enrollment_mode')
     def test_missing_args(self, missing_arg):
-        kwargs = {'student': 'a', 'course_key': 'b', 'otherarg': 'c'}
+        kwargs = {
+            'student': self.user.id,
+            'course_key': self.course_key,
+            'other_arg': 'shiny',
+            'enrollment_mode': CourseMode.MASTERS
+        }
         del kwargs[missing_arg]
 
         with patch('lms.djangoapps.certificates.tasks.User.objects.get'):
@@ -39,7 +45,7 @@ class GenerateUserCertificateTest(TestCase):
         """
         Verify the task handles certificate generation
         """
-        course_key = 'course-v1:edX+DemoX+Demo_Course'
+        enrollment_mode = CourseMode.VERIFIED
 
         with mock.patch(
             'lms.djangoapps.certificates.tasks.generate_course_certificate',
@@ -47,16 +53,17 @@ class GenerateUserCertificateTest(TestCase):
         ) as mock_generate_cert:
             kwargs = {
                 'student': self.user.id,
-                'course_key': course_key
+                'course_key': self.course_key,
+                'enrollment_mode': enrollment_mode
             }
 
             generate_certificate.apply_async(kwargs=kwargs)
             mock_generate_cert.assert_called_with(
                 user=self.user,
-                course_key=CourseKey.from_string(course_key),
+                course_key=CourseKey.from_string(self.course_key),
                 status=CertificateStatuses.downloadable,
-                enrollment_mode=None,
-                course_grade=None,
+                enrollment_mode=enrollment_mode,
+                course_grade='',
                 generation_mode='batch'
             )
 
@@ -64,7 +71,6 @@ class GenerateUserCertificateTest(TestCase):
         """
         Verify the task handles certificate generation custom params
         """
-        course_key = 'course-v1:edX+DemoX+Demo_Course'
         gen_mode = 'self'
         status = CertificateStatuses.notpassing
         enrollment_mode = CourseMode.AUDIT
@@ -77,7 +83,7 @@ class GenerateUserCertificateTest(TestCase):
             kwargs = {
                 'status': status,
                 'student': self.user.id,
-                'course_key': course_key,
+                'course_key': self.course_key,
                 'course_grade': course_grade,
                 'enrollment_mode': enrollment_mode,
                 'generation_mode': gen_mode,
@@ -87,7 +93,7 @@ class GenerateUserCertificateTest(TestCase):
             generate_certificate.apply_async(kwargs=kwargs)
             mock_generate_cert.assert_called_with(
                 user=self.user,
-                course_key=CourseKey.from_string(course_key),
+                course_key=CourseKey.from_string(self.course_key),
                 status=status,
                 enrollment_mode=enrollment_mode,
                 course_grade=course_grade,