diff --git a/Makefile b/Makefile index 99569cc527..14e8d78a2d 100644 --- a/Makefile +++ b/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. diff --git a/src/backend/joanie/core/admin.py b/src/backend/joanie/core/admin.py index 66c87c2f83..820ef8b157 100644 --- a/src/backend/joanie/core/admin.py +++ b/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"" f"{certificate_definition!s}" "") + return format_html(f"{certificate_definition!s}") @admin.register(models.Course) diff --git a/src/backend/joanie/core/helpers.py b/src/backend/joanie/core/helpers.py index 8556561e76..149a68a217 100644 --- a/src/backend/joanie/core/helpers.py +++ b/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 diff --git a/src/backend/joanie/tests/test_admin_product.py b/src/backend/joanie/tests/test_admin_product.py index b49e2289ae..3397a560b7 100644 --- a/src/backend/joanie/tests/test_admin_product.py +++ b/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 diff --git a/src/backend/joanie/tests/test_commands_generate_certificates.py b/src/backend/joanie/tests/test_commands_generate_certificates.py index 4bb887da91..e9f103ea1a 100644 --- a/src/backend/joanie/tests/test_commands_generate_certificates.py +++ b/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) diff --git a/src/backend/joanie/tests/test_helpers.py b/src/backend/joanie/tests/test_helpers.py index 5fb7cf8300..5c4c184e1b 100644 --- a/src/backend/joanie/tests/test_helpers.py +++ b/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) diff --git a/src/backend/joanie/tests/test_models_enrollment.py b/src/backend/joanie/tests/test_models_enrollment.py index cdc1e389ed..c05ab5277d 100644 --- a/src/backend/joanie/tests/test_models_enrollment.py +++ b/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