Merge pull request #26292 from edx/tuchfarber/add_consistent_logging_credentials_tasks

Make credentials celery tasks errors consistent

openedx/core/djangoapps/credentials/tasks/v1/tasks.py

@@ -4,6 +4,7 @@ This file contains celery tasks for credentials-related functionality.
 
 
 from celery import shared_task
+from celery.exceptions import MaxRetriesExceededError
 from celery.utils.log import get_task_logger
 from django.conf import settings
 from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user
@@ -25,7 +26,7 @@ MAX_RETRIES = 11
 @set_code_owner_attribute
 def send_grade_to_credentials(self, username, course_run_key, verified, letter_grade, percent_grade):
     """ Celery task to notify the Credentials IDA of a grade change via POST. """
-    logger.info(u'Running task send_grade_to_credentials for username %s and course %s', username, course_run_key)
+    logger.info(f"Running task send_grade_to_credentials for username {username} and course {course_run_key}")
 
     countdown = 2 ** self.request.retries
     course_key = CourseKey.from_string(course_run_key)
@@ -44,8 +45,12 @@ def send_grade_to_credentials(self, username, course_run_key, verified, letter_g
             'verified': verified,
         })
 
-        logger.info(u'Sent grade for course %s to user %s', course_run_key, username)
+        logger.info(f"Sent grade for course {course_run_key} to user {username}")
 
     except Exception as exc:
-        logger.exception(u'Failed to send grade for course %s to user %s', course_run_key, username)
-        raise self.retry(exc=exc, countdown=countdown, max_retries=MAX_RETRIES)
+        error_msg = f"Failed to send grade for course {course_run_key} to user {username}."
+        logger.exception(error_msg)
+        exception = MaxRetriesExceededError(
+            f"Failed to send grade to credentials. Reason: {error_msg}"
+        )
+        raise self.retry(exc=exception, countdown=countdown, max_retries=MAX_RETRIES)

openedx/core/djangoapps/programs/tests/test_tasks.py

@@ -280,6 +280,29 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo
         self.assertFalse(mock_get_certified_programs.called)
         self.assertFalse(mock_award_program_certificate.called)
 
+    @mock.patch(TASKS_MODULE + '.get_credentials_api_client')
+    def test_failure_to_create_api_client_retries(
+        self,
+        mock_get_api_client,
+        mock_get_completed_programs,
+        mock_get_certified_programs,
+        mock_award_program_certificate
+    ):
+        """
+        Checks that we log an exception and retry if the API client isn't creating.
+        """
+        mock_get_api_client.side_effect = Exception('boom')
+        mock_get_completed_programs.return_value = {1: 1, 2: 2}
+        mock_get_certified_programs.return_value = [2]
+
+        with mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception:
+            with self.assertRaises(MaxRetriesExceededError):
+                tasks.award_program_certificates.delay(self.student.username).get()
+
+        self.assertTrue(mock_exception.called)
+        self.assertEqual(mock_get_api_client.call_count, tasks.MAX_RETRIES + 1)
+        self.assertFalse(mock_award_program_certificate.called)
+
     def _make_side_effect(self, side_effects):
         """
         DRY helper.  Returns a side effect function for use with mocks that
@@ -327,8 +350,8 @@ class AwardProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiCo
                 uuid=1,
                 username=self.student.username)
         )
-        mock_info.assert_any_call(mock.ANY, 1, self.student.username)
-        mock_info.assert_any_call(mock.ANY, 2, self.student.username)
+        mock_info.assert_any_call(f"Awarded certificate for program {1} to user {self.student.username}")
+        mock_info.assert_any_call(f"Awarded certificate for program {2} to user {self.student.username}")
 
     def test_retry_on_programs_api_errors(
         self,
@@ -781,8 +804,8 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC
                 uuid=1,
                 username=self.student.username)
         )
-        mock_info.assert_any_call(mock.ANY, 1, self.student.username)
-        mock_info.assert_any_call(mock.ANY, 2, self.student.username)
+        mock_info.assert_any_call(f"Revoked certificate for program {1} for user {self.student.username}")
+        mock_info.assert_any_call(f"Revoked certificate for program {2} for user {self.student.username}")
 
     def test_retry_on_credentials_api_errors(
         self,
@@ -865,3 +888,27 @@ class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiC
         tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
 
         self.assertEqual(mock_revoke_program_certificate.call_count, 2)
+
+    def test_get_api_client_failure_retries(
+        self,
+        mock_get_inverted_programs,
+        mock_get_certified_programs,
+        mock_revoke_program_certificate,
+    ):
+        """
+        Verify that a 404 error causes the task to fail but there is no retry.
+        """
+        mock_get_inverted_programs.return_value = self.inverted_programs
+        mock_get_certified_programs.return_value = [1, 2]
+
+        with mock.patch(
+            TASKS_MODULE + ".get_credentials_api_client"
+        ) as mock_get_api_client, mock.patch(
+            TASKS_MODULE + '.LOGGER.exception'
+        ) as mock_exception:
+            mock_get_api_client.side_effect = Exception("boom")
+            with self.assertRaises(MaxRetriesExceededError):
+                tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+        self.assertTrue(mock_exception.called)
+        self.assertEqual(mock_get_api_client.call_count, tasks.MAX_RETRIES + 1)
+        self.assertFalse(mock_revoke_program_certificate.called)