From dca50aacc3093c29c830cccae5746247ecb6f02b Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Tue, 21 Apr 2020 16:00:21 +0500 Subject: [PATCH] added tests for revoke program cert task --- lms/djangoapps/certificates/models.py | 12 +- .../certificates/tests/test_models.py | 14 + openedx/core/djangoapps/programs/signals.py | 8 +- .../djangoapps/programs/tasks/v1/tasks.py | 89 +++--- .../programs/tasks/v1/tests/test_tasks.py | 274 ++++++++++++++++++ .../djangoapps/programs/tests/test_signals.py | 69 ++++- 6 files changed, 420 insertions(+), 46 deletions(-) diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 8ebdb94c4d..71639b1510 100644 --- a/lms/djangoapps/certificates/models.py +++ b/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): """ diff --git a/lms/djangoapps/certificates/tests/test_models.py b/lms/djangoapps/certificates/tests/test_models.py index 3d26cebd39..5a43e5b0ca 100644 --- a/lms/djangoapps/certificates/tests/test_models.py +++ b/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)) diff --git a/openedx/core/djangoapps/programs/signals.py b/openedx/core/djangoapps/programs/signals.py index 2b01d01916..0b907f650b 100644 --- a/openedx/core/djangoapps/programs/signals.py +++ b/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, diff --git a/openedx/core/djangoapps/programs/tasks/v1/tasks.py b/openedx/core/djangoapps/programs/tasks/v1/tasks.py index 6560c614cc..a280d61d32 100644 --- a/openedx/core/djangoapps/programs/tasks/v1/tasks.py +++ b/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 - ) - 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']) - except Exception as exc: - LOGGER.exception( - u'Failed to determine program certificates to be revoked for user %s with course %s', username, course_key ) + return + + # Determine which program certificates the user has already been awarded, if any. + 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 + ) 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} diff --git a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py index 230dc13862..ef3c80c6fb 100644 --- a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py +++ b/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) diff --git a/openedx/core/djangoapps/programs/tests/test_signals.py b/openedx/core/djangoapps/programs/tests/test_signals.py index 364dc089f6..dfb2817a19 100644 --- a/openedx/core/djangoapps/programs/tests/test_signals.py +++ b/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))