From df3d6d160fd07faab18fdb2cf3d0b2d168bc46cc Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Wed, 10 Jul 2019 14:29:33 -0400 Subject: [PATCH] give create_api_access_request mgmt cmd an option to disconnect signals. --- .../commands/create_api_access_request.py | 67 +++++++++++++++++-- .../tests/test_create_api_access_request.py | 51 ++++++++++++-- 2 files changed, 105 insertions(+), 13 deletions(-) diff --git a/openedx/core/djangoapps/api_admin/management/commands/create_api_access_request.py b/openedx/core/djangoapps/api_admin/management/commands/create_api_access_request.py index 0073acc706..ff0ce9df4c 100644 --- a/openedx/core/djangoapps/api_admin/management/commands/create_api_access_request.py +++ b/openedx/core/djangoapps/api_admin/management/commands/create_api_access_request.py @@ -1,17 +1,48 @@ """ Management command to create an ApiAccessRequest for given users """ +from __future__ import unicode_literals + import logging -from django.core.management.base import BaseCommand, CommandError +from contextlib import contextmanager + from django.contrib.auth.models import User from django.contrib.sites.models import Site +from django.core.management.base import BaseCommand, CommandError +from django.db.models.signals import post_save, pre_save +from six import text_type from openedx.core.djangoapps.api_admin.models import ( ApiAccessConfig, ApiAccessRequest, + send_decision_email, + send_request_email ) logger = logging.getLogger(__name__) +@contextmanager +def disconnect_request_email_signals(): + """ + Context manager to be used for temporarily disconnecting the `send_request_email` + and `send_decision_email` pre/post_save signal receivers from the `ApiAccessRequest` model. + """ + post_save.disconnect( + send_request_email, sender=ApiAccessRequest, dispatch_uid="api_access_request_post_save_email" + ) + pre_save.disconnect( + send_decision_email, sender=ApiAccessRequest, dispatch_uid="api_access_request_pre_save_email" + ) + try: + yield + finally: + post_save.connect( + send_request_email, sender=ApiAccessRequest, dispatch_uid="api_access_request_post_save_email" + ) + pre_save.connect( + send_decision_email, sender=ApiAccessRequest, dispatch_uid="api_access_request_pre_save_email" + ) + + class Command(BaseCommand): """ Create an ApiAccessRequest for the given user @@ -31,6 +62,11 @@ class Command(BaseCommand): action='store_true', help='Create ApiAccessConfig if it does not exist' ) + parser.add_argument( + '--disconnect-signals', + action='store_true', + help='Disconnect the Django signal receivers that send emails when ApiAccessRequest records are saved' + ) parser.add_argument( '--status', choices=[choice[0] for choice in ApiAccessRequest.STATUS_CHOICES], @@ -49,6 +85,13 @@ class Command(BaseCommand): ) def handle(self, *args, **options): + if options.get('disconnect_signals'): + with disconnect_request_email_signals(): + self._handle(*args, **options) + else: + self._handle(*args, **options) + + def _handle(self, *args, **options): # pylint: disable=unused-argument if options.get('create_config'): self.create_api_access_config() user = self.get_user(options.get('username')) @@ -63,7 +106,7 @@ class Command(BaseCommand): try: return User.objects.get(username=username) except User.DoesNotExist: - raise CommandError(u'User {} not found'.format(username)) + raise CommandError('User {} not found'.format(username)) def create_api_access_request(self, user, status, reason, website): """ @@ -77,14 +120,23 @@ class Command(BaseCommand): reason=reason, site=Site.objects.get_current(), ) + except OSError as e: + # Ignore a specific error that occurs in the downstream `send_request_email` receiver. + # see https://openedx.atlassian.net/browse/EDUCATOR-4478 + error_msg = text_type(e) + if 'Permission denied' in error_msg and 'mako_lms' in error_msg: + logger.warning('Error sending email about access request: {}'.format(error_msg)) + else: + raise CommandError(error_msg) except Exception as e: - msg = u'Unable to create ApiAccessRequest for {}. Exception is {}: {}'.format( + msg = 'Unable to create ApiAccessRequest for {}. Exception is {}: {}'.format( user.username, type(e).__name__, e ) raise CommandError(msg) - logger.info(u'Created ApiAccessRequest for user {}'.format(user.username)) + + logger.info('Created ApiAccessRequest for user {}'.format(user.username)) def create_api_access_config(self): """ @@ -93,9 +145,10 @@ class Command(BaseCommand): try: _, created = ApiAccessConfig.objects.get_or_create(enabled=True) except Exception as e: - msg = u'Unable to create ApiAccessConfig. Exception is {}: {}'.format(type(e).__name__, e) + msg = 'Unable to create ApiAccessConfig. Exception is {}: {}'.format(type(e).__name__, e) raise CommandError(msg) + if created: - logger.info(u'Created ApiAccessConfig') + logger.info('Created ApiAccessConfig') else: - logger.info(u'ApiAccessConfig already exists') + logger.info('ApiAccessConfig already exists') diff --git a/openedx/core/djangoapps/api_admin/management/commands/tests/test_create_api_access_request.py b/openedx/core/djangoapps/api_admin/management/commands/tests/test_create_api_access_request.py index 118730974f..8d9c6e6144 100644 --- a/openedx/core/djangoapps/api_admin/management/commands/tests/test_create_api_access_request.py +++ b/openedx/core/djangoapps/api_admin/management/commands/tests/test_create_api_access_request.py @@ -1,4 +1,4 @@ -from mock import patch +import unittest import ddt from django.conf import settings @@ -6,13 +6,10 @@ from django.contrib.sites.models import Site from django.core.management import call_command from django.core.management.base import CommandError from django.test import TestCase -import unittest +from mock import patch from openedx.core.djangoapps.api_admin.management.commands import create_api_access_request -from openedx.core.djangoapps.api_admin.models import ( - ApiAccessConfig, - ApiAccessRequest, -) +from openedx.core.djangoapps.api_admin.models import ApiAccessConfig, ApiAccessRequest from student.tests.factories import UserFactory @@ -43,6 +40,20 @@ class TestCreateApiAccessRequest(TestCase): call_command(self.command, self.user.username, create_config=create_config) self.assert_models_exist(True, create_config) + @patch('openedx.core.djangoapps.api_admin.models._send_new_pending_email') + def test_create_api_access_request_signals_disconnected(self, mock_send_new_pending_email): + self.assert_models_exist(False, False) + call_command(self.command, self.user.username, create_config=True, disconnect_signals=True) + self.assert_models_exist(True, True) + self.assertFalse(mock_send_new_pending_email.called) + + @patch('openedx.core.djangoapps.api_admin.models._send_new_pending_email') + def test_create_api_access_request_signals_connected(self, mock_send_new_pending_email): + self.assert_models_exist(False, False) + call_command(self.command, self.user.username, create_config=True, disconnect_signals=False) + self.assert_models_exist(True, True) + self.assertTrue(mock_send_new_pending_email.called) + def test_config_already_exists(self): ApiAccessConfig.objects.create(enabled=True) self.assert_models_exist(False, True) @@ -64,6 +75,34 @@ class TestCreateApiAccessRequest(TestCase): self.assert_models_exist(False, False) + @patch('openedx.core.djangoapps.api_admin.models.send_request_email') + def test_api_request_permission_denied_error(self, mocked_method): + """ + When a Permission denied OSError with 'mako_lms' in the message occurs in the post_save receiver, + the models should still be created and the command should finish without raising. + """ + mocked_method.side_effect = OSError('Permission denied: something something in /tmp/mako_lms') + + self.assert_models_exist(False, False) + + call_command(self.command, self.user.username, create_config=True) + + self.assert_models_exist(True, True) + + @patch('openedx.core.djangoapps.api_admin.models.ApiAccessRequest.objects.create') + def test_api_request_other_oserrors_raise(self, mocked_method): + """ + When some other Permission denied OSError occurs, we should still raise. + """ + mocked_method.side_effect = OSError('out of disk space') + + self.assert_models_exist(False, False) + + with self.assertRaisesRegex(CommandError, 'out of disk space'): + call_command(self.command, self.user.username) + + self.assert_models_exist(False, False) + @patch('openedx.core.djangoapps.api_admin.models.ApiAccessConfig.objects.get_or_create') def test_api_config_error(self, mocked_method): mocked_method.side_effect = Exception()