👌(!fixup) 3rd review

Makefile

@@ -139,7 +139,7 @@ lint-pylint: ## lint back-end python sources with pylint
 .PHONY: lint-pylint
 
 test-back: ## run back-end tests
-	bin/pytest
+	bin/pytest -k test_helpers_
 .PHONY: test-back
 
 migrate:  ## run django migrations for the joanie project.

src/backend/joanie/core/admin.py

@@ -65,7 +65,7 @@ def certificate_definition(self, obj):  # pylint: disable=no-self-use
             "admin:core_certificatedefinition_change",
             args=(certificate_definition.id,),
         )
-        return format_html(f"<a href='{url:s}'>" f"{certificate_definition!s}" "</a>")
+        return format_html(f"<a href='{url:s}'>{certificate_definition!s}</a>")
 
 
 @admin.register(models.Course)

src/backend/joanie/core/helpers.py

@@ -34,8 +34,8 @@ 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.
+    # Retrieve all enrollments in one query. Since these enrollments rely on
+    # order course runs, the count will always be pretty small.
     enrollments = order.enrollments.filter(
         course_run__in=course_runs, is_active=True
     ).select_related("user", "course_run")
@@ -44,10 +44,11 @@ def generate_certificate_for_order(order):
     # for each graded course, if not it is useless to go further
     course_enrollments = []
     for course in graded_courses:
+        course_course_runs = course.course_runs.all()
         for enrollment in enrollments:
             # Check if the enrollment relies on course by crossing
             # all course runs implied
-            if enrollment.course_run in course.course_runs.all():
+            if enrollment.course_run in course_course_runs:
                 course_enrollments.append(enrollment)
                 break
 
@@ -57,16 +58,11 @@ def generate_certificate_for_order(order):
         return 0
 
     # Otherwise, we now need to know if each enrollment has been passed
-    passed_enrollment_count = 0
     for enrollment in course_enrollments:
         if enrollment.is_passed is False:
             # If one enrollment has not been passed, no need to continue,
             # We are sure that order is not eligible for certification.
-            break
-        passed_enrollment_count += 1
-
-    if passed_enrollment_count != graded_courses_count:
-        return 0
+            return 0
 
     try:
         order.create_certificate()
@@ -96,7 +92,6 @@ def generate_certificates_for_orders(orders):
     ]
 
     for order in orders:
-        result = generate_certificate_for_order(order)
-        total += result
+        total += generate_certificate_for_order(order)
 
     return total

src/backend/joanie/tests/test_admin_product.py

@@ -292,7 +292,7 @@ def test_admin_product_should_allow_to_generate_certificate_for_related_course(
         """
         Product admin view should display a link to generate certificates for
         the couple course - product next to each related course item. This link is
-        display only for certifying products.
+        displayed only for certifying products.
         """
 
         # Create a course
@@ -347,8 +347,8 @@ def test_admin_product_generate_certificate_for_course(
         self, mock_generate_certificates
     ):
         """
-        Product Admin should contain an endpoint which trigger the `create_certificates`
-        management command with product and course as options.
+        Product Admin should contain an endpoint which triggers the
+        `create_certificates` management command with product and course as options.
         """
         user = factories.UserFactory(is_staff=True, is_superuser=True)
         self.client.login(username=user.username, password="password")
@@ -363,7 +363,7 @@ def test_admin_product_generate_certificate_for_course(
             ),
         )
 
-        # - Create certificates command should have been called
+        # - Generate certificates command should have been called
         mock_generate_certificates.assert_called_once()
 
         # Check the presence of a confirmation message

src/backend/joanie/tests/test_commands_generate_certificates.py

@@ -51,17 +51,17 @@ def test_commands_generate_certificates(self):
         )
         course = factories.CourseFactory(products=[product])
         order = factories.OrderFactory(product=product, course=course)
-        certificate = models.Certificate.objects.filter(order=order)
+        certificate_qs = models.Certificate.objects.filter(order=order)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
-        # DB queries should be minimized
+        # Calling command should generate one certificate
         call_command("generate_certificates")
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.count(), 1)
 
         # But call it again, should not create a new certificate
         call_command("generate_certificates")
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.count(), 1)
 
     def test_commands_generate_certificates_can_be_restricted_to_order(self):
         """
@@ -81,17 +81,17 @@ def test_commands_generate_certificates_can_be_restricted_to_order(self):
         )
         course = factories.CourseFactory(products=[product])
         orders = factories.OrderFactory.create_batch(2, product=product, course=course)
-        certificate = models.Certificate.objects.filter(order__in=orders)
+        certificate_qs = models.Certificate.objects.filter(order__in=orders)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
         # A certificate should be generated for the 1st order
         call_command("generate_certificates", order=orders[0].uid)
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.filter(order=orders[0]).count(), 1)
 
         # Then a certificate should be generated for the 2nd order
         call_command("generate_certificates", order=orders[1].uid)
-        self.assertEqual(certificate.count(), 2)
+        self.assertEqual(certificate_qs.filter(order=orders[1]).count(), 1)
 
     def test_commands_generate_certificates_can_be_restricted_to_course(self):
         """
@@ -117,17 +117,17 @@ def test_commands_generate_certificates_can_be_restricted_to_course(self):
             factories.OrderFactory(product=product, course=course_1),
             factories.OrderFactory(product=product, course=course_2),
         ]
-        certificate = models.Certificate.objects.filter(order__in=orders)
+        certificate_qs = models.Certificate.objects.filter(order__in=orders)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
         # A certificate should be generated for the 1st course
         call_command("generate_certificates", course=course_1.code)
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.filter(order=orders[0]).count(), 1)
 
         # Then a certificate should be generated for the 2nd course
         call_command("generate_certificates", course=course_2.code)
-        self.assertEqual(certificate.count(), 2)
+        self.assertEqual(certificate_qs.filter(order=orders[1]).count(), 1)
 
     def test_commands_generate_certificates_can_be_restricted_to_product(self):
         """
@@ -156,19 +156,19 @@ def test_commands_generate_certificates_can_be_restricted_to_product(self):
             factories.OrderFactory(course=course, product=product_1),
             factories.OrderFactory(course=course, product=product_2),
         ]
-        certificate = models.Certificate.objects.filter(order__in=orders)
+        certificate_qs = models.Certificate.objects.filter(order__in=orders)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.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)
+        self.assertEqual(certificate_qs.filter(order=orders[0]).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)
+        self.assertEqual(certificate_qs.filter(order=orders[1]).count(), 1)
 
     def test_commands_generate_certificates_can_be_restricted_to_product_course(self):
         """
@@ -203,33 +203,33 @@ def test_commands_generate_certificates_can_be_restricted_to_product_course(self
             factories.OrderFactory(course=course_2, product=product_1),
             factories.OrderFactory(course=course_2, product=product_2),
         ]
-        certificate = models.Certificate.objects.filter(order__in=orders)
+        certificate_qs = models.Certificate.objects.filter(order__in=orders)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
         # A certificate should be generated for the couple course_1 - product_1
         call_command(
             "generate_certificates", course=course_1.code, product=product_1.uid
         )
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.filter(order=orders[0]).count(), 1)
 
         # Then a certificate should be generated for the couple course_1 - product_2
         call_command(
             "generate_certificates", course=course_1.code, product=product_2.uid
         )
-        self.assertEqual(certificate.count(), 2)
+        self.assertEqual(certificate_qs.filter(order=orders[1]).count(), 1)
 
         # Then a certificate should be generated for the couple course_2 - product_1
         call_command(
             "generate_certificates", course=course_2.code, product=product_1.uid
         )
-        self.assertEqual(certificate.count(), 3)
+        self.assertEqual(certificate_qs.filter(order=orders[2]).count(), 1)
 
         # Finally, a certificate should be generated for the couple course_2 - product_2
         call_command(
             "generate_certificates", course=course_2.code, product=product_2.uid
         )
-        self.assertEqual(certificate.count(), 4)
+        self.assertEqual(certificate_qs.filter(order=orders[3]).count(), 1)
 
     def test_commands_generate_certificates_optimizes_db_queries(self):
         """
@@ -258,16 +258,16 @@ def test_commands_generate_certificates_optimizes_db_queries(self):
             factories.OrderFactory(course=course, product=product_1),
             factories.OrderFactory(course=course, product=product_2),
         ]
-        certificate = models.Certificate.objects.filter(order__in=orders)
+        certificate_qs = models.Certificate.objects.filter(order__in=orders)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.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)
+        self.assertEqual(certificate_qs.filter(order=orders[0]).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)
+        self.assertEqual(certificate_qs.filter(order=orders[1]).count(), 1)

src/backend/joanie/tests/test_helpers.py

@@ -15,7 +15,7 @@ class HelpersTestCase(TestCase):
     def test_helpers_generate_certificate_for_order_needs_graded_courses(self):
         """
         If the order relies on a certifying product which does not contain
-        graded courses any certificate should be generated.
+        graded courses, no certificate should be generated.
         """
         # Create a certifying product with one order eligible for certification
         course_run = factories.CourseRunFactory(
@@ -33,16 +33,16 @@ def test_helpers_generate_certificate_for_order_needs_graded_courses(self):
         course_run.course.product_relations.update(is_graded=False)
         course = factories.CourseFactory(products=[product])
         order = factories.OrderFactory(product=product, course=course)
-        certificate = models.Certificate.objects.filter(order=order)
-        self.assertEqual(certificate.count(), 0)
+        certificate_qs = models.Certificate.objects.filter(order=order)
+        self.assertEqual(certificate_qs.count(), 0)
 
         self.assertEqual(helpers.generate_certificate_for_order(order), 0)
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
     def test_helpers_generate_certificate_for_order_needs_gradable_course_runs(self):
         """
         If the order does not rely on gradable course runs,
-        any certificate should be generated.
+        no certificate should be generated.
         """
         # Create a certifying product with one order eligible for certification
         course_run = factories.CourseRunFactory(
@@ -58,18 +58,18 @@ def test_helpers_generate_certificate_for_order_needs_gradable_course_runs(self)
         )
         course = factories.CourseFactory(products=[product])
         order = factories.OrderFactory(product=product, course=course)
-        certificate = models.Certificate.objects.filter(order=order)
-        self.assertEqual(certificate.count(), 0)
+        certificate_qs = models.Certificate.objects.filter(order=order)
+        self.assertEqual(certificate_qs.count(), 0)
 
         self.assertEqual(helpers.generate_certificate_for_order(order), 0)
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
         # - Now flag the course run as gradable
         course_run.is_gradable = True
         course_run.save()
 
         self.assertEqual(helpers.generate_certificate_for_order(order), 1)
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.count(), 1)
 
     def test_helpers_generate_certificate_for_order_needs_enrollments_has_been_passed(
         self,
@@ -91,22 +91,22 @@ def test_helpers_generate_certificate_for_order_needs_enrollments_has_been_passe
         )
         course = factories.CourseFactory(products=[product])
         order = factories.OrderFactory(product=product, course=course)
-        certificate = models.Certificate.objects.filter(order=order)
-        self.assertEqual(certificate.count(), 0)
+        certificate_qs = models.Certificate.objects.filter(order=order)
+        self.assertEqual(certificate_qs.count(), 0)
 
         # Simulate that all enrollments are not passed
         with mock.patch.object(DummyLMSBackend, "get_grades") as mock_get_grades:
             mock_get_grades.return_value = {"passed": False}
             self.assertEqual(helpers.generate_certificate_for_order(order), 0)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
         # Simulate that all enrollments are passed
         with mock.patch.object(DummyLMSBackend, "get_grades") as mock_get_grades:
             mock_get_grades.return_value = {"passed": True}
             self.assertEqual(helpers.generate_certificate_for_order(order), 1)
 
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.count(), 1)
 
     def test_helpers_generate_certificate_for_order(self):
         """
@@ -129,19 +129,19 @@ def test_helpers_generate_certificate_for_order(self):
         )
         course = factories.CourseFactory(products=[product])
         order = factories.OrderFactory(product=product, course=course)
-        certificate = models.Certificate.objects.filter(order=order)
+        certificate_qs = models.Certificate.objects.filter(order=order)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
         # DB queries should be minimized
         with self.assertNumQueries(7):
             self.assertEqual(helpers.generate_certificate_for_order(order), 1)
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.count(), 1)
 
-        # But call it again, should not create a new certificate
+        # But calling it again, should not create a new certificate
         with self.assertNumQueries(4):
             self.assertEqual(helpers.generate_certificate_for_order(order), 0)
-        self.assertEqual(certificate.count(), 1)
+        self.assertEqual(certificate_qs.count(), 1)
 
     def test_helpers_generate_certificates_for_orders(self):
         """
@@ -180,17 +180,17 @@ def test_helpers_generate_certificates_for_orders(self):
             factories.OrderFactory(product=product_1, course=course, is_canceled=True),
         ]
 
-        certificate = models.Certificate.objects.filter(order__in=orders)
+        certificate_qs = models.Certificate.objects.filter(order__in=orders)
 
-        self.assertEqual(certificate.count(), 0)
+        self.assertEqual(certificate_qs.count(), 0)
 
         self.assertEqual(
             helpers.generate_certificates_for_orders(models.Order.objects.all()), 10
         )
-        self.assertEqual(certificate.count(), 10)
+        self.assertEqual(certificate_qs.count(), 10)
 
         # But call it again, should not create a new certificate
         self.assertEqual(
             helpers.generate_certificates_for_orders(models.Order.objects.all()), 0
         )
-        self.assertEqual(certificate.count(), 10)
+        self.assertEqual(certificate_qs.count(), 10)

src/backend/joanie/tests/test_models_enrollment.py

@@ -130,7 +130,7 @@ def test_models_enrollment_is_passed(self, mock_get_grades, _):
         # - Call it again should return the same result
         mock_get_grades.reset_mock()
         self.assertIs(enrollment.is_passed, True)
-        # - But `get_grades` should have not been called again
+        # - But `get_grades` should not have been called again
         mock_get_grades.assert_not_called()
 
     @override_settings(JOANIE_ENROLLMENT_GRADE_CACHE_TTL=600)
@@ -162,7 +162,7 @@ def test_models_enrollment_is_passed_not_cached_on_failure(
             username=enrollment.user.username, resource_link=course_run.resource_link
         )
 
-        # - Call it again should trigger the `get_grades` method
+        # - Calling it again should trigger the `get_grades` method
         mock_get_grades.reset_mock()
         mock_get_grades.return_value = {"passed": True}
         mock_get_grades.side_effect = None