Skip to content

E
ecommerce
Commit 5fafea0c by Clinton Blackburn
Options

Removed configuration validation on startup 

The false-positives raised by this validation for tests has lead us (me, at least) to ignore this validation. When developing locally, bad data prevents the application from starting, which prevents me from easily fixing that data via Django admin. This check costs more than it's worth. It's time for it to go.

ECOM-7108
parent 2841dfde
Showing with 5 additions and 180 deletions
ecommerce/core/config.py
import logging
from django.apps import AppConfig
from django.db import DatabaseError
log = logging.getLogger(__name__)
class CoreAppConfig(AppConfig):
name = 'ecommerce.core'
verbose_name = 'Core'
OPERATIONAL_ERROR_MESSAGE = 'DB error when validating configuration - most likely DB was not created yet - skipping'
def ready(self):
super(CoreAppConfig, self).ready()
# Ensures that the initialized Celery app is loaded when Django starts.
# Allows Celery tasks to bind themselves to an initialized instance of the Celery library.
# noinspection PyUnresolvedReferences
from ecommerce import celery_app # pylint: disable=unused-variable
from ecommerce.core.models import validate_configuration
# Operational error means database did not contain SiteConfiguration table - ok to skip since it means there
# are no SiteConfiguration models to validate. Also, this exception was only observed in tests and test run
# just fine with this suppression - most likely test code hit this line before the DB is populated
try:
# Here we're are consciously violating Django's warning about not interacting with DB in AppConfig.ready
# We know what we're doing, have considered a couple of other approaches and discussed it in great length:
# https://github.com/edx/ecommerce/pull/630#discussion-diff-58026881
validate_configuration()
except DatabaseError:
log.exception(self.OPERATIONAL_ERROR_MESSAGE)
ecommerce/core/models.py
......@@ -254,14 +254,6 @@ class SiteConfiguration(models.Model):
"""
return self.from_email or settings.OSCAR_FROM_EMAIL
def clean_fields(self, exclude=None):
""" Validates model fields """
if not exclude or 'payment_processors' not in exclude:
self._clean_payment_processors()
if not exclude or 'client_side_payment_processor' not in exclude:
self._clean_client_side_payment_processor()
@cached_property
def segment_client(self):
return SegmentClient(self.segment_key, debug=settings.DEBUG)
......@@ -587,9 +579,3 @@ class BusinessClient(models.Model):
'Failed to create BusinessClient. BusinessClient name may not be empty.'
)
super(BusinessClient, self).save(*args, **kwargs)
def validate_configuration():
""" Validates all existing SiteConfiguration models """
for config in SiteConfiguration.objects.all():
config.clean_fields()
ecommerce/core/tests/test_config.py deleted 100644 → 0
""" Tests for CoreAppConfig """
import mock
from django.db import OperationalError
from django.test import SimpleTestCase
from ecommerce import core
from ecommerce.core.config import CoreAppConfig
class TestAppConfig(SimpleTestCase):
""" Test suite for CoreAppConfig class """
def test_ready_validates_configuration(self):
""" Tests that method `ready` invokes `models.validate_configuration` method"""
config = CoreAppConfig('core', core)
with mock.patch('ecommerce.core.models.validate_configuration') as patched_validate:
config.ready()
self.assertTrue(patched_validate.called)
def test_ready_validate_suppresses_operational_error(self):
""" Tests that django.db.OperationalError is suppressed and logged in `ready` method """
config = CoreAppConfig('core', core)
with mock.patch('ecommerce.core.models.validate_configuration') as patched_validate:
with mock.patch('ecommerce.core.config.log') as patched_log:
patched_validate.side_effect = OperationalError
config.ready()
self.assertTrue(patched_validate.called)
patched_log.exception.assert_called_once_with(config.OPERATIONAL_ERROR_MESSAGE)
ecommerce/core/tests/test_models.py
......@@ -11,7 +11,7 @@ from edx_rest_api_client.auth import SuppliedJwtAuth
from requests.exceptions import ConnectionError
from ecommerce.core.exceptions import VerificationStatusError
from ecommerce.core.models import BusinessClient, User, SiteConfiguration, validate_configuration
from ecommerce.core.models import BusinessClient, User, SiteConfiguration
from ecommerce.core.tests import toggle_switch
from ecommerce.extensions.catalogue.tests.mixins import CourseCatalogTestMixin
from ecommerce.extensions.payment.tests.processors import DummyProcessor, AnotherDummyProcessor
......@@ -187,8 +187,10 @@ class UserTests(CourseCatalogTestMixin, LmsApiMockMixin, TestCase):
def test_deactivation_exception_handling(self):
"""Verify an error is logged if an exception happens."""
def callback(*args): # pylint: disable=unused-argument
raise ConnectionError
user = self.create_user()
self.mock_deactivation_api(self.request, user.username, response=callback)
......@@ -228,76 +230,6 @@ class SiteConfigurationTests(TestCase):
site_config = _make_site_config(payment_processors_str)
self.assertEqual(site_config.payment_processors_set, expected_result)
@ddt.data("paypal", "paypal, cybersource", "paypal , cybersource")
def test_clean_fields_valid_values_pass_validation(self, payment_processors_str):
"""
Tests that valid payment_processors value passes validation
:param str payment_processors_str: comma-separated string of processor names (potentially with spaces)
"""
site_config = _make_site_config(payment_processors_str)
with mock.patch("ecommerce.extensions.payment.helpers.get_processor_class_by_name") as patched_proc_by_name:
patched_proc_by_name.return_value = DummyProcessor
try:
site_config.clean_fields()
except ValidationError as exc:
self.fail(exc.message)
@ddt.data(" ", " \t ", "\t\n\r")
def test_clean_fields_whitespace_payment_processor_fail_validation(self, payment_processors_str):
"""
Tests that whitespace-only payment_processor values fail validation
:param str payment_processors_str: comma-separated string of processor names (potentially with spaces)
"""
site_config = _make_site_config(payment_processors_str)
with self.assertRaises(ValidationError) as err:
site_config.clean_fields()
self.assertEqual(
err.message, "Invalid payment processors field: must not only contain whitespace characters"
)
def test_clean_fields_unknown_payment_processor_fail_validation(self):
"""
Tests that validation fails if payment_processors field contains unknown payment processor names
"""
site_config = _make_site_config("unknown_payment_processor")
with self.assertRaises(ValidationError):
site_config.clean_fields()
def test_clean_fields_payment_processor_excluded_always_pass(self):
"""
Tests that `clean_fields` pass if "payment_processors" are excluded, regardless of validity
"""
site_config = _make_site_config("")
site_config.clean_fields(exclude={"payment_processors"})
site_config.payment_processors = "irrelevant-get_processor_by_name-is-patched"
site_config.clean_fields(exclude={"payment_processors"})
@ddt.data(None, '', ' ')
def test_clean_client_side_payment_processor_with_empty_value(self, value):
""" Verify validation succeeds if no value is set for the client_side_payment_processor field. """
site_config = _make_site_config('paypal')
site_config.client_side_payment_processor = value
site_config.clean_fields()
def test_clean_client_side_payment_processor_with_invalid_processor(self):
""" Verify an error is raised if the value client_side_payment_processor is not in the list
of available payment processors. """
site_config = _make_site_config('paypal')
site_config.client_side_payment_processor = 'bad-value'
with self.assertRaises(ValidationError):
site_config.clean_fields()
def test_clean_client_side_payment_processor(self):
""" Verify no error is raised if the value of client_side_payment_processor is in the
list of available payment processors. """
processor = 'paypal'
site_config = _make_site_config(processor)
site_config.client_side_payment_processor = processor
site_config.clean_fields()
@staticmethod
def _enable_processor_switches(processors):
for processor in processors:
......@@ -408,52 +340,9 @@ class SiteConfigurationTests(TestCase):
"""
token = self.mock_access_token_response()
client = self.site.siteconfiguration.enterprise_api_client
client_store = client._store # pylint: disable=protected-access
client_store = client._store # pylint: disable=protected-access
client_auth = client_store['session'].auth
self.assertEqual(client_store['base_url'], ENTERPRISE_API_URL)
self.assertIsInstance(client_auth, SuppliedJwtAuth)
self.assertEqual(client_auth.token, token)
class HelperMethodTests(TestCase):
""" Tests helper methods in models.py """
def setUp(self):
""" setUp test """
self.site_config_objects = mock.Mock()
patcher = mock.patch('ecommerce.core.models.SiteConfiguration.objects', self.site_config_objects)
patcher.start()
self.addCleanup(patcher.stop)
@override_settings(PAYMENT_PROCESSORS=[
'ecommerce.extensions.payment.tests.processors.DummyProcessor',
'ecommerce.extensions.payment.tests.processors.AnotherDummyProcessor',
])
def test_validate_configuration_passes(self):
"""
Test that site configurations with available payment processor(s) pass validation
"""
config1 = _make_site_config(DummyProcessor.NAME)
config2 = _make_site_config(DummyProcessor.NAME + ',' + AnotherDummyProcessor.NAME)
self.site_config_objects.all.return_value = [config1, config2]
validate_configuration() # checks that no exception is thrown
@override_settings(PAYMENT_PROCESSORS=[
'ecommerce.extensions.payment.tests.processors.DummyProcessor',
])
def test_validate_configuration_fails(self):
"""
Test that site configurations with unknown payment processor(s) fail validation
"""
config1 = _make_site_config(DummyProcessor.NAME)
config2 = _make_site_config(DummyProcessor.NAME + ',' + AnotherDummyProcessor.NAME)
self.site_config_objects.all.return_value = [config1, config2]
with self.assertRaises(ValidationError):
validate_configuration()
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment