added tests for revoke program cert task

lms/djangoapps/certificates/models.py

@@ -371,12 +371,12 @@ class GeneratedCertificate(models.Model):
         self.status = CertificateStatuses.unavailable
         self.save()
         COURSE_CERT_REVOKED.send_robust(
-                sender=self.__class__,
-                user=self.user,
-                course_key=self.course_id,
-                mode=self.mode,
-                status=self.status,
-            )
+            sender=self.__class__,
+            user=self.user,
+            course_key=self.course_id,
+            mode=self.mode,
+            status=self.status,
+        )
 
     def mark_notpassing(self, grade):
         """

lms/djangoapps/certificates/tests/test_models.py

@@ -9,6 +9,7 @@ from django.core.exceptions import ValidationError
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import TestCase
 from django.test.utils import override_settings
+from mock import patch
 from opaque_keys.edx.locator import CourseKey, CourseLocator
 from path import Path as path
 
@@ -357,3 +358,16 @@ class CertificateInvalidationTest(SharedModuleStoreTestCase):
         self.assertFalse(
             CertificateInvalidation.has_certificate_invalidation(self.user, self.course_id)
         )
+
+    @patch('openedx.core.djangoapps.programs.tasks.v1.tasks.revoke_program_certificates.delay')
+    @patch(
+        'openedx.core.djangoapps.credentials.models.CredentialsApiConfig.is_learner_issuance_enabled',
+        return_value=True,
+    )
+    def test_revoke_program_certificates(self, mock_issuance, mock_revoke_task):    # pylint: disable=unused-argument
+        """ Verify that `revoke_program_certificates` is invoked upon invalidation. """
+        # Invalidate user certificate
+        self.certificate.invalidate()
+
+        self.assertEqual(mock_revoke_task.call_count, 1)
+        self.assertEqual(mock_revoke_task.call_args[0], (self.user.username, self.course_id))

openedx/core/djangoapps/programs/signals.py

@@ -134,7 +134,7 @@ def handle_course_cert_changed(sender, user, course_key, mode, status, **kwargs)
 
 
 @receiver(COURSE_CERT_REVOKED)
-def handle_course_cert_revoked(sender, user, course_key, mode, status, **kwargs):
+def handle_course_cert_revoked(sender, user, course_key, mode, status, **kwargs):  # pylint: disable=unused-argument
     """
     If programs is enabled and a learner's course certificate is revoked,
     schedule a celery task to revoke any related program certificates.
@@ -143,9 +143,9 @@ def handle_course_cert_revoked(sender, user, course_key, mode, status, **kwargs)
         sender:
             class of the object instance that sent this signal
         user:
-            django.contrib.auth.User - the user to whom a cert was awarded
+            django.contrib.auth.User - the user for which a cert was revoked
         course_key:
-            refers to the course run for which the cert was awarded
+            refers to the course run for which the cert was revoked
         mode:
             mode / certificate type, e.g. "verified"
         status:
@@ -164,7 +164,7 @@ def handle_course_cert_revoked(sender, user, course_key, mode, status, **kwargs)
         return
 
     # schedule background task to process
-    LOGGER.debug(
+    LOGGER.info(
         u'handling COURSE_CERT_REVOKED: username=%s, course_key=%s, mode=%s, status=%s',
         user,
         course_key,

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

@@ -52,20 +52,23 @@ def get_completed_programs(site, student):
     return meter.completed_programs_with_available_dates
 
 
-def get_inverted_programs(site, student):
+def get_inverted_programs(student):
     """
-    Given a set of completed courses, determine which programs are completed.
+    Get programs keyed by course run ID.
 
     Args:
-        site (Site): Site for which data should be retrieved.
-        student (User): Representing the student whose completed programs to check for.
+        student (User): Representing the student whose programs to check for.
 
     Returns:
-        dict of {program_UUIDs: visible_dates}
+        dict, programs keyed by course run ID
 
     """
-    meter = ProgramProgressMeter(site, student)
-    return meter.invert_programs()
+    inverted_programs = {}
+    for site in Site.objects.all():
+        meter = ProgramProgressMeter(site, student)
+        inverted_programs.update(meter.invert_programs())
+
+    return inverted_programs
 
 
 def get_certified_programs(student):
@@ -349,6 +352,30 @@ def award_course_certificate(self, username, course_run_key):
         raise self.retry(exc=exc, countdown=countdown, max_retries=MAX_RETRIES)
 
 
+def get_revokable_program_uuids(course_specific_programs, student):
+    """
+    Get program uuids for which certificate to be revoked.
+
+    Checks for existing learner certificates and filter out the program UUIDS
+    for which a certificate needs to be revoked.
+
+    Args:
+        course_specific_programs (dict[]): list of programs specific to a course
+        student (User): Representing the student whose programs to check for.
+
+    Returns:
+        list if program UUIDs for which certificates to be revoked
+
+    """
+    program_uuids_to_revoke = []
+    existing_program_uuids = get_certified_programs(student)
+    for program in course_specific_programs:
+        if program['uuid'] in existing_program_uuids:
+            program_uuids_to_revoke.append(program['uuid'])
+
+    return program_uuids_to_revoke
+
+
 def revoke_program_certificate(client, username, program_uuid):
     """
     Revoke a certificate of the given student for the given program.
@@ -403,38 +430,32 @@ def revoke_program_certificates(self, username, course_key):
         raise self.retry(countdown=countdown, max_retries=MAX_RETRIES)
 
     try:
-        try:
-            student = User.objects.get(username=username)
-        except User.DoesNotExist:
-            LOGGER.exception(u'Task revoke_program_certificates was called with invalid username %s', username)
-            # Don't retry for this case - just conclude the task.
-            return
-        inverted_programs = {}
-        for site in Site.objects.all():
-            inverted_programs.update(get_inverted_programs(site, student))
-            course_specific_programs = inverted_programs.get(str(course_key))
-            import pdb; pdb.set_trace()
+        student = User.objects.get(username=username)
+    except User.DoesNotExist:
+        LOGGER.exception(u'Task revoke_program_certificates was called with invalid username %s', username)
+        # Don't retry for this case - just conclude the task.
+        return
+
+    try:
+        inverted_programs = get_inverted_programs(student)
+        course_specific_programs = inverted_programs.get(str(course_key))
         if not course_specific_programs:
             # No reason to continue beyond this point
             LOGGER.info(
                 u'Task revoke_program_certificates was called for user %s and course %s with no engaged programs',
-                    username,
-                    course_key
-                 )
+                username,
+                course_key
+            )
             return
 
         # Determine which program certificates the user has already been awarded, if any.
-        existing_program_uuids = get_certified_programs(student)
-        program_uuids_to_revoke = []
-        for program in course_specific_programs:
-            if  program['uuid'] in existing_program_uuids:
-                program_uuids_to_revoke.append(program['uuid'])
+        program_uuids_to_revoke = get_revokable_program_uuids(course_specific_programs, student)
     except Exception as exc:
         LOGGER.exception(
             u'Failed to determine program certificates to be revoked for user %s with course %s',
-                username,
-                course_key
-            )
+            username,
+            course_key
+        )
         raise self.retry(exc=exc, countdown=countdown, max_retries=MAX_RETRIES)
 
     if program_uuids_to_revoke:
@@ -443,7 +464,7 @@ def revoke_program_certificates(self, username, course_key):
                 User.objects.get(username=settings.CREDENTIALS_SERVICE_USERNAME),
             )
         except Exception as exc:
-            LOGGER.exception('Failed to create a credentials API client to award program certificates')
+            LOGGER.exception('Failed to create a credentials API client to revoke program certificates')
             # Retry because a misconfiguration could be fixed
             raise self.retry(exc=exc, countdown=countdown, max_retries=MAX_RETRIES)
 
@@ -462,7 +483,7 @@ def revoke_program_certificates(self, username, course_key):
                 # client handles all 4XX errors the same way. In the future,
                 # we may want to fork slumber, add 429 handling, and use that
                 # in edx_rest_api_client.
-                if exc.response.status_code == 429:  # pylint: disable=no-member
+                if exc.response.status_code == 429:  # pylint: disable=no-member, no-else-raise
                     rate_limit_countdown = 60
                     LOGGER.info(
                         u"""Rate limited. Retrying task to revoke certificates for user {username} in {countdown}

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

@@ -591,3 +591,277 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase):
 
         tasks.award_course_certificate.delay(self.student.username, str(self.certificate.course_id)).get()
         self.assertFalse(mock_post_course_certificate.called)
+
+
+@skip_unless_lms
+class RevokeProgramCertificateTestCase(TestCase):
+    """
+    Test the revoke_program_certificate function
+    """
+
+    @httpretty.activate
+    def test_revoke_program_certificate(self):
+        """
+        Ensure the correct API call gets made
+        """
+        test_username = 'test-username'
+        test_client = EdxRestApiClient('http://test-server', jwt='test-token')
+
+        httpretty.register_uri(
+            httpretty.POST,
+            'http://test-server/credentials/',
+        )
+
+        tasks.revoke_program_certificate(test_client, test_username, 123)
+
+        expected_body = {
+            'username': test_username,
+            'status': 'revoked',
+            'credential': {
+                'program_uuid': 123,
+                'type': tasks.PROGRAM_CERTIFICATE,
+            }
+        }
+        last_request_body = httpretty.last_request().body.decode('utf-8')
+        self.assertEqual(json.loads(last_request_body), expected_body)
+
+
+@skip_unless_lms
+@ddt.ddt
+@mock.patch(TASKS_MODULE + '.revoke_program_certificate')
+@mock.patch(TASKS_MODULE + '.get_certified_programs')
+@mock.patch(TASKS_MODULE + '.get_inverted_programs')
+@override_settings(CREDENTIALS_SERVICE_USERNAME='test-service-username')
+class RevokeProgramCertificatesTestCase(CatalogIntegrationMixin, CredentialsApiConfigMixin, TestCase):
+    """
+    Tests for the 'revoke_program_certificates' celery task.
+    """
+
+    def setUp(self):
+        super(RevokeProgramCertificatesTestCase, self).setUp()
+
+        self.student = UserFactory.create(username='test-student')
+        self.course_key = 'course-v1:testX+test101+2T2020'
+        self.site = SiteFactory()
+        self.site_configuration = SiteConfigurationFactory(site=self.site)
+        ApplicationFactory.create(name='credentials')
+        UserFactory.create(username=settings.CREDENTIALS_SERVICE_USERNAME)
+        self.create_credentials_config()
+
+        self.inverted_programs = {self.course_key: [{'uuid': 1}, {'uuid': 2}]}
+
+    def test_inverted_programs(
+        self,
+        mock_get_inverted_programs,
+        mock_get_certified_programs,  # pylint: disable=unused-argument
+        mock_revoke_program_certificate,  # pylint: disable=unused-argument
+    ):
+        """
+        Checks that the Programs API is used correctly to determine completed
+        programs.
+        """
+        tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+        mock_get_inverted_programs.assert_any_call(self.student)
+
+    def test_revokinging_certificate(
+        self,
+        mock_get_inverted_programs,
+        mock_get_certified_programs,
+        mock_revoke_program_certificate,
+    ):
+        """
+        Checks that the Credentials API is used to revoke certificates for
+        the proper programs.
+        """
+        expected_program_uuid = 1
+        mock_get_inverted_programs.return_value = {
+            self.course_key: [{'uuid': expected_program_uuid}]
+        }
+        mock_get_certified_programs.return_value = [expected_program_uuid]
+
+        tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+
+        call_args, _ = mock_revoke_program_certificate.call_args
+        self.assertEqual(call_args[1], self.student.username)
+        self.assertEqual(call_args[2], expected_program_uuid)
+
+    @ddt.data(
+        ('credentials', 'enable_learner_issuance'),
+    )
+    @ddt.unpack
+    def test_retry_if_config_disabled(
+        self,
+        disabled_config_type,
+        disabled_config_attribute,
+        *mock_helpers
+    ):
+        """
+        Checks that the task is aborted if any relevant api configs are
+        disabled.
+        """
+        getattr(self, 'create_{}_config'.format(disabled_config_type))(**{disabled_config_attribute: False})
+        with mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_warning:
+            with self.assertRaises(MaxRetriesExceededError):
+                tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+            self.assertTrue(mock_warning.called)
+        for mock_helper in mock_helpers:
+            self.assertFalse(mock_helper.called)
+
+    def test_abort_if_invalid_username(self, *mock_helpers):
+        """
+        Checks that the task will be aborted and not retried if the username
+        passed was not found, and that an exception is logged.
+        """
+        with mock.patch(TASKS_MODULE + '.LOGGER.exception') as mock_exception:
+            tasks.revoke_program_certificates.delay('nonexistent-username', self.course_key).get()
+            self.assertTrue(mock_exception.called)
+        for mock_helper in mock_helpers:
+            self.assertFalse(mock_helper.called)
+
+    def test_abort_if_no_program(
+        self,
+        mock_get_inverted_programs,
+        mock_get_certified_programs,
+        mock_revoke_program_certificate,
+    ):
+        """
+        Checks that the task will be aborted without further action if course is
+        not part of any program.
+        """
+        mock_get_inverted_programs.return_value = {}
+        tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+        self.assertTrue(mock_get_inverted_programs.called)
+        self.assertFalse(mock_get_certified_programs.called)
+        self.assertFalse(mock_revoke_program_certificate.called)
+
+    def _make_side_effect(self, side_effects):
+        """
+        DRY helper.  Returns a side effect function for use with mocks that
+        will be called multiple times, permitting Exceptions to be raised
+        (or not) in a specified order.
+
+        See Also:
+            http://www.voidspace.org.uk/python/mock/examples.html#multiple-calls-with-different-effects
+            http://www.voidspace.org.uk/python/mock/mock.html#mock.Mock.side_effect
+
+        """
+
+        def side_effect(*_a):
+            if side_effects:
+                exc = side_effects.pop(0)
+                if exc:
+                    raise exc
+            return mock.DEFAULT
+
+        return side_effect
+
+    def test_continue_revoking_certs_if_error(
+        self,
+        mock_get_inverted_programs,
+        mock_get_certified_programs,
+        mock_revoke_program_certificate,
+    ):
+        """
+        Checks that a single failure to revoke one of several certificates
+        does not cause the entire task to fail.  Also ensures that
+        successfully revoked certs are logged as INFO and warning is logged
+        for failed requests if there are retries available.
+        """
+        mock_get_inverted_programs.return_value = self.inverted_programs
+        mock_get_certified_programs.side_effect = [[1], [1, 2]]
+        mock_revoke_program_certificate.side_effect = self._make_side_effect([Exception('boom'), None])
+
+        with mock.patch(TASKS_MODULE + '.LOGGER.info') as mock_info, \
+                mock.patch(TASKS_MODULE + '.LOGGER.warning') as mock_warning:
+            tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+
+        self.assertEqual(mock_revoke_program_certificate.call_count, 3)
+        mock_warning.assert_called_once_with(
+            u'Failed to revoke certificate for program {uuid} of user {username}.'.format(
+                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)
+
+    def test_retry_on_credentials_api_errors(
+        self,
+        mock_get_inverted_programs,
+        mock_get_certified_programs,
+        mock_revoke_program_certificate,
+    ):
+        """
+        Ensures that any otherwise-unhandled errors that arise while trying
+        to get existing program credentials (e.g. network issues or other
+        transient API errors) will cause the task to be failed and queued for
+        retry.
+        """
+        mock_get_inverted_programs.return_value = self.inverted_programs
+        mock_get_certified_programs.return_value = [1]
+        mock_get_certified_programs.side_effect = self._make_side_effect([Exception('boom'), None])
+        tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+        self.assertEqual(mock_get_certified_programs.call_count, 2)
+        self.assertEqual(mock_revoke_program_certificate.call_count, 1)
+
+    def test_retry_on_credentials_api_429_error(
+        self,
+        mock_get_inverted_programs,
+        mock_get_certified_programs,
+        mock_revoke_program_certificate,
+    ):
+        """
+        Verify that a 429 error causes the task to fail and then retry.
+        """
+        exception = exceptions.HttpClientError()
+        exception.response = mock.Mock(status_code=429)
+        mock_get_inverted_programs.return_value = self.inverted_programs
+        mock_get_certified_programs.return_value = [1, 2]
+        mock_revoke_program_certificate.side_effect = self._make_side_effect(
+            [exception, None]
+        )
+
+        tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+
+        self.assertEqual(mock_revoke_program_certificate.call_count, 3)
+
+    def test_no_retry_on_credentials_api_404_error(
+        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.
+        """
+        exception = exceptions.HttpNotFoundError()
+        exception.response = mock.Mock(status_code=404)
+        mock_get_inverted_programs.return_value = self.inverted_programs
+        mock_get_certified_programs.return_value = [1, 2]
+        mock_revoke_program_certificate.side_effect = self._make_side_effect(
+            [exception, None]
+        )
+
+        tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+
+        self.assertEqual(mock_revoke_program_certificate.call_count, 2)
+
+    def test_no_retry_on_credentials_api_4XX_error(
+        self,
+        mock_get_inverted_programs,
+        mock_get_certified_programs,
+        mock_revoke_program_certificate,
+    ):
+        """
+        Verify that other 4XX errors cause task to fail but there is no retry.
+        """
+        exception = exceptions.HttpClientError()
+        exception.response = mock.Mock(status_code=418)
+        mock_get_inverted_programs.return_value = self.inverted_programs
+        mock_get_certified_programs.return_value = [1, 2]
+        mock_revoke_program_certificate.side_effect = self._make_side_effect(
+            [exception, None]
+        )
+
+        tasks.revoke_program_certificates.delay(self.student.username, self.course_key).get()
+
+        self.assertEqual(mock_revoke_program_certificate.call_count, 2)

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

@@ -7,8 +7,12 @@ import mock
 from django.test import TestCase
 from opaque_keys.edx.keys import CourseKey
 
-from openedx.core.djangoapps.programs.signals import handle_course_cert_awarded, handle_course_cert_changed
-from openedx.core.djangoapps.signals.signals import COURSE_CERT_AWARDED, COURSE_CERT_CHANGED
+from openedx.core.djangoapps.programs.signals import (
+    handle_course_cert_awarded, handle_course_cert_changed, handle_course_cert_revoked
+)
+from openedx.core.djangoapps.signals.signals import (
+    COURSE_CERT_AWARDED, COURSE_CERT_CHANGED, COURSE_CERT_REVOKED
+)
 from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory
 from openedx.core.djangolib.testing.utils import skip_unless_lms
 from student.tests.factories import UserFactory
@@ -159,3 +163,64 @@ class CertChangedReceiverTest(TestCase):
         site_config.save()
         handle_course_cert_changed(**self.signal_kwargs)
         self.assertFalse(mock_task.called)
+
+
+# The credentials app isn't installed for the CMS.
+@skip_unless_lms
+@mock.patch('openedx.core.djangoapps.programs.tasks.v1.tasks.revoke_program_certificates.delay')
+@mock.patch(
+    'openedx.core.djangoapps.credentials.models.CredentialsApiConfig.is_learner_issuance_enabled',
+    new_callable=mock.PropertyMock,
+    return_value=False,
+)
+class CertRevokedReceiverTest(TestCase):
+    """
+    Tests for the `handle_course_cert_revoked` signal handler function.
+    """
+
+    @property
+    def signal_kwargs(self):
+        """
+        DRY helper.
+        """
+        return dict(
+            sender=self.__class__,
+            user=UserFactory.create(username=TEST_USERNAME),
+            course_key=TEST_COURSE_KEY,
+            mode='test-mode',
+            status='test-status',
+        )
+
+    def test_signal_received(self, mock_is_learner_issuance_enabled, mock_task):  # pylint: disable=unused-argument
+        """
+        Ensures the receiver function is invoked when COURSE_CERT_REVOKED is
+        sent.
+
+        Suboptimal: because we cannot mock the receiver function itself (due
+        to the way django signals work), we mock a configuration call that is
+        known to take place inside the function.
+        """
+        COURSE_CERT_REVOKED.send(**self.signal_kwargs)
+        self.assertEqual(mock_is_learner_issuance_enabled.call_count, 1)
+
+    def test_programs_disabled(self, mock_is_learner_issuance_enabled, mock_task):
+        """
+        Ensures that the receiver function does nothing when the credentials API
+        configuration is not enabled.
+        """
+        handle_course_cert_revoked(**self.signal_kwargs)
+        self.assertEqual(mock_is_learner_issuance_enabled.call_count, 1)
+        self.assertEqual(mock_task.call_count, 0)
+
+    def test_programs_enabled(self, mock_is_learner_issuance_enabled, mock_task):
+        """
+        Ensures that the receiver function invokes the expected celery task
+        when the credentials API configuration is enabled.
+        """
+        mock_is_learner_issuance_enabled.return_value = True
+
+        handle_course_cert_revoked(**self.signal_kwargs)
+
+        self.assertEqual(mock_is_learner_issuance_enabled.call_count, 1)
+        self.assertEqual(mock_task.call_count, 1)
+        self.assertEqual(mock_task.call_args[0], (TEST_USERNAME, TEST_COURSE_KEY))