👌(!fixup) 2nd review

src/backend/joanie/core/admin.py

@@ -25,7 +25,7 @@
 
 def summarize_certification_to_user(request, count):
     """
-    Display a message after create_certificates command has been launched
+    Display a message after generate_certificates command has been launched
     """
     if count == 0:
         messages.warning(
@@ -61,13 +61,11 @@ def certificate_definition(self, obj):  # pylint: disable=no-self-use
         """Retrieve the certification definition from the related order."""
         certificate_definition = obj.order.product.certificate_definition
 
-        return format_html(
-            (
-                f"<a href='{reverse('admin:core_certificatedefinition_change', args=(certificate_definition.id,), )}'>"
-                f"{str(certificate_definition)}"
-                "</a>"
-            )
+        url = reverse(
+            "admin:core_certificatedefinition_change",
+            args=(certificate_definition.id,),
         )
+        return format_html(f"<a href='{url:s}'>" f"{certificate_definition!s}" "</a>")
 
 
 @admin.register(models.Course)
@@ -94,8 +92,8 @@ class CourseAdmin(DjangoObjectActions, TranslatableAdmin):
     @takes_instance_or_queryset
     def generate_certificates(self, request, queryset):  # pylint: disable=no-self-use
         """
-        Custom action to launch create_certificates management command
-        over the selected courses
+        Custom action to generate certificates for a collection of courses
+        passed as a queryset
         """
         certificate_generated_count = helpers.generate_certificates_for_orders(
             models.Order.objects.filter(course__in=queryset)
@@ -192,8 +190,8 @@ def get_urls(self):
     @takes_instance_or_queryset
     def generate_certificates(self, request, queryset):  # pylint: disable=no-self-use
         """
-        Custom action to launch create_certificates management command
-        over the selected products
+        Custom action to generate certificates for a collection of products
+        passed as a queryset
         """
         certificate_generated_count = helpers.generate_certificates_for_orders(
             models.Order.objects.filter(product__in=queryset)
@@ -248,7 +246,7 @@ def get_related_courses_as_html(obj):  # pylint: disable=no-self-use
                 )
 
                 if is_certifying:
-                    # Add a button go generate certificate
+                    # Add a button to generate certificate
                     generate_certificates_url = reverse(
                         f"admin:{ACTION_NAME_GENERATE_CERTIFICATES}",
                         kwargs={"product_id": obj.id, "course_code": course.code},
@@ -286,7 +284,7 @@ def cancel(self, request, queryset):  # pylint: disable=no-self-use
     @takes_instance_or_queryset
     def generate_certificates(self, request, queryset):  # pylint: disable=no-self-use
         """
-        Custom action to launch create_certificates management commands
+        Custom action to launch generate_certificates management commands
         over the order selected
         """
         certificate_generated_count = helpers.generate_certificates_for_orders(queryset)

src/backend/joanie/core/helpers.py

@@ -9,18 +9,20 @@
 
 def generate_certificate_for_order(order):
     """
-    Check if order is eligible for certification then generate certificate is it is.
+    Check if order is eligible for certification then generate certificate if it is.
 
     Eligibility means that order contains
     one passed enrollment per graded courses.
 
     Return:
-        0 : if the order is not eligible for certification
+        0: if the order is not eligible for certification
         1: if a certificate has been generated for the current order
     """
-    graded_courses = order.target_courses.filter(
-        order_relations__is_graded=True
-    ).order_by("order_relations__position")
+    graded_courses = (
+        order.target_courses.filter(order_relations__is_graded=True)
+        .order_by("order_relations__position")
+        .prefetch_related("course_runs")
+    )
     graded_courses_count = len(graded_courses)
 
     if graded_courses_count == 0:
@@ -32,20 +34,20 @@ def generate_certificate_for_order(order):
         start__lte=timezone.now(),
     )
 
+    # Retrieve all enrollments in one queries, as these enrollments which relies on
+    # order course runs, the count will be always pretty small.
     enrollments = order.enrollments.filter(
         course_run__in=course_runs, is_active=True
     ).select_related("user", "course_run")
 
     # Cross graded courses and enrollments to check there is an active enrollment
-    # for each graded courses, if not it is useless to go further
+    # for each graded course, if not it is useless to go further
     course_enrollments = []
     for course in graded_courses:
-        for enrollment in enrollments.iterator():
+        for enrollment in enrollments:
             # Check if the enrollment relies on course by crossing
             # all course runs implied
-            if course.course_runs.filter(
-                resource_link=enrollment.course_run.resource_link
-            ).exists():
+            if enrollment.course_run in course.course_runs.all():
                 course_enrollments.append(enrollment)
                 break
 

src/backend/joanie/core/management/commands/create_certificates.py → src/backend/joanie/core/management/commands/generate_certificates.py

@@ -1,4 +1,4 @@
-"""Management command to create all pending certificates."""
+"""Management command to generate all pending certificates."""
 import logging
 
 from django.core.management import BaseCommand
@@ -7,7 +7,7 @@
 from joanie.core import models
 from joanie.core.helpers import generate_certificates_for_orders
 
-logger = logging.getLogger("joanie.core.create_certificates")
+logger = logging.getLogger("joanie.core.generate_certificates")
 
 
 class Command(BaseCommand):

src/backend/joanie/tests/test_commands_create_certificates.py → src/backend/joanie/tests/test_commands_generate_certificates.py

@@ -1,4 +1,4 @@
-"""Test suite for the management command 'create_certificates'"""
+"""Test suite for the management command 'generate_certificates'"""
 import uuid
 from datetime import timedelta
 
@@ -10,9 +10,9 @@
 
 
 class CreateCertificatesTestCase(TestCase):
-    """Test case for the management command 'create_certificates'"""
+    """Test case for the management command 'generate_certificates'"""
 
-    def test_commands_create_certificates_has_options(
+    def test_commands_generate_certificates_has_options(
         self,
     ):  # pylint: disable=no-self-use
         """
@@ -28,9 +28,9 @@ def test_commands_create_certificates_has_options(
         }
 
         # TypeError: Unknown option(s) should not be raised
-        call_command("create_certificates", **options)
+        call_command("generate_certificates", **options)
 
-    def test_commands_create_certificates(self):
+    def test_commands_generate_certificates(self):
         """
         If a certifying product contains graded courses with gradable course runs
         and a user purchased this product and passed all gradable course runs,
@@ -56,16 +56,14 @@ def test_commands_create_certificates(self):
         self.assertEqual(certificate.count(), 0)
 
         # DB queries should be minimized
-        with self.assertNumQueries(9):
-            call_command("create_certificates")
+        call_command("generate_certificates")
         self.assertEqual(certificate.count(), 1)
 
         # But call it again, should not create a new certificate
-        with self.assertNumQueries(1):
-            call_command("create_certificates")
+        call_command("generate_certificates")
         self.assertEqual(certificate.count(), 1)
 
-    def test_commands_create_certificates_can_be_restricted_to_order(self):
+    def test_commands_generate_certificates_can_be_restricted_to_order(self):
         """
         If `order` option is used, the review is restricted to it.
         """
@@ -88,16 +86,14 @@ def test_commands_create_certificates_can_be_restricted_to_order(self):
         self.assertEqual(certificate.count(), 0)
 
         # A certificate should be generated for the 1st order
-        with self.assertNumQueries(9):
-            call_command("create_certificates", order=orders[0].uid)
+        call_command("generate_certificates", order=orders[0].uid)
         self.assertEqual(certificate.count(), 1)
 
         # Then a certificate should be generated for the 2nd order
-        with self.assertNumQueries(7):
-            call_command("create_certificates", order=orders[1].uid)
+        call_command("generate_certificates", order=orders[1].uid)
         self.assertEqual(certificate.count(), 2)
 
-    def test_commands_create_certificates_can_be_restricted_to_course(self):
+    def test_commands_generate_certificates_can_be_restricted_to_course(self):
         """
         If `course` option is used, the review is restricted to it.
         """
@@ -126,16 +122,14 @@ def test_commands_create_certificates_can_be_restricted_to_course(self):
         self.assertEqual(certificate.count(), 0)
 
         # A certificate should be generated for the 1st course
-        with self.assertNumQueries(9):
-            call_command("create_certificates", course=course_1.code)
+        call_command("generate_certificates", course=course_1.code)
         self.assertEqual(certificate.count(), 1)
 
         # Then a certificate should be generated for the 2nd course
-        with self.assertNumQueries(8):
-            call_command("create_certificates", course=course_2.code)
+        call_command("generate_certificates", course=course_2.code)
         self.assertEqual(certificate.count(), 2)
 
-    def test_commands_create_certificates_can_be_restricted_to_product(self):
+    def test_commands_generate_certificates_can_be_restricted_to_product(self):
         """
         If `product` option is used, the review is restricted to it.
         """
@@ -168,15 +162,15 @@ def test_commands_create_certificates_can_be_restricted_to_product(self):
 
         # A certificate should be generated for the 1st product
         with self.assertNumQueries(9):
-            call_command("create_certificates", product=product_1.uid)
+            call_command("generate_certificates", product=product_1.uid)
         self.assertEqual(certificate.count(), 1)
 
         # Then a certificate should be generated for the 2nd product
         with self.assertNumQueries(8):
-            call_command("create_certificates", product=product_2.uid)
+            call_command("generate_certificates", product=product_2.uid)
         self.assertEqual(certificate.count(), 2)
 
-    def test_commands_create_certificates_can_be_restricted_to_product_course(self):
+    def test_commands_generate_certificates_can_be_restricted_to_product_course(self):
         """
         `product` and `course` options can be used together to restrict review to them.
         """
@@ -214,29 +208,66 @@ def test_commands_create_certificates_can_be_restricted_to_product_course(self):
         self.assertEqual(certificate.count(), 0)
 
         # A certificate should be generated for the couple course_1 - product_1
-        with self.assertNumQueries(9):
-            call_command(
-                "create_certificates", course=course_1.code, product=product_1.uid
-            )
+        call_command(
+            "generate_certificates", course=course_1.code, product=product_1.uid
+        )
         self.assertEqual(certificate.count(), 1)
 
         # Then a certificate should be generated for the couple course_1 - product_2
-        with self.assertNumQueries(8):
-            call_command(
-                "create_certificates", course=course_1.code, product=product_2.uid
-            )
+        call_command(
+            "generate_certificates", course=course_1.code, product=product_2.uid
+        )
         self.assertEqual(certificate.count(), 2)
 
         # Then a certificate should be generated for the couple course_2 - product_1
-        with self.assertNumQueries(8):
-            call_command(
-                "create_certificates", course=course_2.code, product=product_1.uid
-            )
+        call_command(
+            "generate_certificates", course=course_2.code, product=product_1.uid
+        )
         self.assertEqual(certificate.count(), 3)
 
         # Finally, a certificate should be generated for the couple course_2 - product_2
-        with self.assertNumQueries(7):
-            call_command(
-                "create_certificates", course=course_2.code, product=product_2.uid
-            )
+        call_command(
+            "generate_certificates", course=course_2.code, product=product_2.uid
+        )
         self.assertEqual(certificate.count(), 4)
+
+    def test_commands_generate_certificates_optimizes_db_queries(self):
+        """
+        The management command should optimize db access
+        """
+        # Create two certifying products with order eligible for certification.
+        [cr1, cr2, cr3, cr4, cr5, cr6] = factories.CourseRunFactory.create_batch(
+            6,
+            enrollment_end=timezone.now() + timedelta(hours=1),
+            enrollment_start=timezone.now() - timedelta(hours=1),
+            is_gradable=True,
+            start=timezone.now() - timedelta(hours=1),
+        )
+        product_1 = factories.ProductFactory(
+            price="0.00",
+            type=enums.PRODUCT_TYPE_CREDENTIAL,
+            target_courses=[cr1.course, cr2.course, cr3.course],
+        )
+        product_2 = factories.ProductFactory(
+            price="0.00",
+            type=enums.PRODUCT_TYPE_CREDENTIAL,
+            target_courses=[cr4.course, cr5.course, cr6.course],
+        )
+        course = factories.CourseFactory(products=[product_1, product_2])
+        orders = [
+            factories.OrderFactory(course=course, product=product_1),
+            factories.OrderFactory(course=course, product=product_2),
+        ]
+        certificate = models.Certificate.objects.filter(order__in=orders)
+
+        self.assertEqual(certificate.count(), 0)
+
+        # A certificate should be generated for the 1st product
+        with self.assertNumQueries(9):
+            call_command("generate_certificates", product=product_1.uid)
+        self.assertEqual(certificate.count(), 1)
+
+        # Then a certificate should be generated for the 2nd product
+        with self.assertNumQueries(8):
+            call_command("generate_certificates", product=product_2.uid)
+        self.assertEqual(certificate.count(), 2)