From 20e3fdc7828946e0119a3f373bb5fbe6adea176c Mon Sep 17 00:00:00 2001 From: Arthur Hanson Date: Wed, 28 Sep 2022 12:22:19 -0700 Subject: [PATCH] #9045 #9046 - remove legacy fields from Provider (#10377) * #9045 - remove legacy fields from Provider * Add safegaurd for legacy data to migration * 9045 remove fields from forms and tables * Update unrelated tests to use ASN model instead of Provider * Fix migrations collision Co-authored-by: jeremystretch --- netbox/circuits/api/serializers.py | 2 +- netbox/circuits/filtersets.py | 4 +- netbox/circuits/forms/bulk_edit.py | 22 +------ netbox/circuits/forms/bulk_import.py | 2 +- netbox/circuits/forms/models.py | 18 +----- .../0040_provider_remove_deprecated_fields.py | 59 +++++++++++++++++++ netbox/circuits/models/providers.py | 20 +------ netbox/circuits/tables/providers.py | 4 +- netbox/circuits/tests/test_api.py | 2 +- netbox/circuits/tests/test_filtersets.py | 14 ++--- netbox/circuits/tests/test_views.py | 18 ++---- netbox/extras/tests/test_customvalidator.py | 20 ++++--- netbox/templates/circuits/provider.html | 29 --------- netbox/utilities/tests/test_filters.py | 31 ++++------ 14 files changed, 104 insertions(+), 141 deletions(-) create mode 100644 netbox/circuits/migrations/0040_provider_remove_deprecated_fields.py diff --git a/netbox/circuits/api/serializers.py b/netbox/circuits/api/serializers.py index c1d856f39..4a8e2bd28 100644 --- a/netbox/circuits/api/serializers.py +++ b/netbox/circuits/api/serializers.py @@ -31,7 +31,7 @@ class ProviderSerializer(NetBoxModelSerializer): class Meta: model = Provider fields = [ - 'id', 'url', 'display', 'name', 'slug', 'asn', 'account', 'portal_url', 'noc_contact', 'admin_contact', + 'id', 'url', 'display', 'name', 'slug', 'account', 'comments', 'asns', 'tags', 'custom_fields', 'created', 'last_updated', 'circuit_count', ] diff --git a/netbox/circuits/filtersets.py b/netbox/circuits/filtersets.py index cee38fb18..cf250584f 100644 --- a/netbox/circuits/filtersets.py +++ b/netbox/circuits/filtersets.py @@ -65,7 +65,7 @@ class ProviderFilterSet(NetBoxModelFilterSet, ContactModelFilterSet): class Meta: model = Provider - fields = ['id', 'name', 'slug', 'asn', 'account'] + fields = ['id', 'name', 'slug', 'account'] def search(self, queryset, name, value): if not value.strip(): @@ -73,8 +73,6 @@ class ProviderFilterSet(NetBoxModelFilterSet, ContactModelFilterSet): return queryset.filter( Q(name__icontains=value) | Q(account__icontains=value) | - Q(noc_contact__icontains=value) | - Q(admin_contact__icontains=value) | Q(comments__icontains=value) ) diff --git a/netbox/circuits/forms/bulk_edit.py b/netbox/circuits/forms/bulk_edit.py index b6ba42afb..12975b5d6 100644 --- a/netbox/circuits/forms/bulk_edit.py +++ b/netbox/circuits/forms/bulk_edit.py @@ -20,10 +20,6 @@ __all__ = ( class ProviderBulkEditForm(NetBoxModelBulkEditForm): - asn = forms.IntegerField( - required=False, - label='ASN (legacy)' - ) asns = DynamicModelMultipleChoiceField( queryset=ASN.objects.all(), label=_('ASNs'), @@ -34,20 +30,6 @@ class ProviderBulkEditForm(NetBoxModelBulkEditForm): required=False, label='Account number' ) - portal_url = forms.URLField( - required=False, - label='Portal' - ) - noc_contact = forms.CharField( - required=False, - widget=SmallTextarea, - label='NOC contact' - ) - admin_contact = forms.CharField( - required=False, - widget=SmallTextarea, - label='Admin contact' - ) comments = CommentField( widget=SmallTextarea, label='Comments' @@ -55,10 +37,10 @@ class ProviderBulkEditForm(NetBoxModelBulkEditForm): model = Provider fieldsets = ( - (None, ('asn', 'asns', 'account', 'portal_url', 'noc_contact', 'admin_contact')), + (None, ('asns', 'account', )), ) nullable_fields = ( - 'asn', 'asns', 'account', 'portal_url', 'noc_contact', 'admin_contact', 'comments', + 'asns', 'account', 'comments', ) diff --git a/netbox/circuits/forms/bulk_import.py b/netbox/circuits/forms/bulk_import.py index cc2d0409a..77ebb3de9 100644 --- a/netbox/circuits/forms/bulk_import.py +++ b/netbox/circuits/forms/bulk_import.py @@ -18,7 +18,7 @@ class ProviderCSVForm(NetBoxModelCSVForm): class Meta: model = Provider fields = ( - 'name', 'slug', 'asn', 'account', 'portal_url', 'noc_contact', 'admin_contact', 'comments', + 'name', 'slug', 'account', 'comments', ) diff --git a/netbox/circuits/forms/models.py b/netbox/circuits/forms/models.py index 7bd7abbbf..17c2e7480 100644 --- a/netbox/circuits/forms/models.py +++ b/netbox/circuits/forms/models.py @@ -30,29 +30,17 @@ class ProviderForm(NetBoxModelForm): comments = CommentField() fieldsets = ( - ('Provider', ('name', 'slug', 'asn', 'asns', 'tags')), - ('Support Info', ('account', 'portal_url', 'noc_contact', 'admin_contact')), + ('Provider', ('name', 'slug', 'asns', 'tags')), + ('Support Info', ('account',)), ) class Meta: model = Provider fields = [ - 'name', 'slug', 'asn', 'account', 'portal_url', 'noc_contact', 'admin_contact', 'asns', 'comments', 'tags', + 'name', 'slug', 'account', 'asns', 'comments', 'tags', ] - widgets = { - 'noc_contact': SmallTextarea( - attrs={'rows': 5} - ), - 'admin_contact': SmallTextarea( - attrs={'rows': 5} - ), - } help_texts = { 'name': "Full name of the provider", - 'asn': "BGP autonomous system number (if applicable)", - 'portal_url': "URL of the provider's customer support portal", - 'noc_contact': "NOC email address and phone number", - 'admin_contact': "Administrative contact email address and phone number", } diff --git a/netbox/circuits/migrations/0040_provider_remove_deprecated_fields.py b/netbox/circuits/migrations/0040_provider_remove_deprecated_fields.py new file mode 100644 index 000000000..98c82204d --- /dev/null +++ b/netbox/circuits/migrations/0040_provider_remove_deprecated_fields.py @@ -0,0 +1,59 @@ +import os + +from django.db import migrations +from django.db.utils import DataError + + +def check_legacy_data(apps, schema_editor): + """ + Abort the migration if any legacy provider fields still contain data. + """ + Provider = apps.get_model('circuits', 'Provider') + + provider_count = Provider.objects.exclude(asn__isnull=True).count() + if provider_count and 'NETBOX_DELETE_LEGACY_DATA' not in os.environ: + raise DataError( + f"Unable to proceed with deleting asn field from Provider model: Found {provider_count} " + f"providers with legacy ASN data. Please ensure all legacy provider ASN data has been " + f"migrated to ASN objects before proceeding. Or, set the NETBOX_DELETE_LEGACY_DATA " + f"environment variable to bypass this safeguard and delete all legacy provider ASN data." + ) + + provider_count = Provider.objects.exclude(admin_contact='', noc_contact='', portal_url='').count() + if provider_count and 'NETBOX_DELETE_LEGACY_DATA' not in os.environ: + raise DataError( + f"Unable to proceed with deleting contact fields from Provider model: Found {provider_count} " + f"providers with legacy contact data. Please ensure all legacy provider contact data has been " + f"migrated to contact objects before proceeding. Or, set the NETBOX_DELETE_LEGACY_DATA " + f"environment variable to bypass this safeguard and delete all legacy provider contact data." + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ('circuits', '0039_unique_constraints'), + ] + + operations = [ + migrations.RunPython( + code=check_legacy_data, + reverse_code=migrations.RunPython.noop + ), + migrations.RemoveField( + model_name='provider', + name='admin_contact', + ), + migrations.RemoveField( + model_name='provider', + name='asn', + ), + migrations.RemoveField( + model_name='provider', + name='noc_contact', + ), + migrations.RemoveField( + model_name='provider', + name='portal_url', + ), + ] diff --git a/netbox/circuits/models/providers.py b/netbox/circuits/models/providers.py index 2a1e01626..bd63ff0c6 100644 --- a/netbox/circuits/models/providers.py +++ b/netbox/circuits/models/providers.py @@ -24,12 +24,6 @@ class Provider(NetBoxModel): max_length=100, unique=True ) - asn = ASNField( - blank=True, - null=True, - verbose_name='ASN', - help_text='32-bit autonomous system number' - ) asns = models.ManyToManyField( to='ipam.ASN', related_name='providers', @@ -40,18 +34,6 @@ class Provider(NetBoxModel): blank=True, verbose_name='Account number' ) - portal_url = models.URLField( - blank=True, - verbose_name='Portal URL' - ) - noc_contact = models.TextField( - blank=True, - verbose_name='NOC contact' - ) - admin_contact = models.TextField( - blank=True, - verbose_name='Admin contact' - ) comments = models.TextField( blank=True ) @@ -62,7 +44,7 @@ class Provider(NetBoxModel): ) clone_fields = ( - 'asn', 'account', 'portal_url', 'noc_contact', 'admin_contact', + 'account', ) class Meta: diff --git a/netbox/circuits/tables/providers.py b/netbox/circuits/tables/providers.py index 0ec6d439d..3e2fd1193 100644 --- a/netbox/circuits/tables/providers.py +++ b/netbox/circuits/tables/providers.py @@ -41,10 +41,10 @@ class ProviderTable(NetBoxTable): class Meta(NetBoxTable.Meta): model = Provider fields = ( - 'pk', 'id', 'name', 'asn', 'asns', 'account', 'portal_url', 'noc_contact', 'admin_contact', 'asn_count', + 'pk', 'id', 'name', 'asns', 'account', 'asn_count', 'circuit_count', 'comments', 'contacts', 'tags', 'created', 'last_updated', ) - default_columns = ('pk', 'name', 'asn', 'account', 'circuit_count') + default_columns = ('pk', 'name', 'account', 'circuit_count') class ProviderNetworkTable(NetBoxTable): diff --git a/netbox/circuits/tests/test_api.py b/netbox/circuits/tests/test_api.py index 02b489ac4..c9d2cfc40 100644 --- a/netbox/circuits/tests/test_api.py +++ b/netbox/circuits/tests/test_api.py @@ -20,7 +20,7 @@ class ProviderTest(APIViewTestCases.APIViewTestCase): model = Provider brief_fields = ['circuit_count', 'display', 'id', 'name', 'slug', 'url'] bulk_update_data = { - 'asn': 1234, + 'account': '1234', } @classmethod diff --git a/netbox/circuits/tests/test_filtersets.py b/netbox/circuits/tests/test_filtersets.py index 2646de3c2..897c87c05 100644 --- a/netbox/circuits/tests/test_filtersets.py +++ b/netbox/circuits/tests/test_filtersets.py @@ -25,11 +25,11 @@ class ProviderTestCase(TestCase, ChangeLoggedFilterSetTests): ASN.objects.bulk_create(asns) providers = ( - Provider(name='Provider 1', slug='provider-1', asn=65001, account='1234'), - Provider(name='Provider 2', slug='provider-2', asn=65002, account='2345'), - Provider(name='Provider 3', slug='provider-3', asn=65003, account='3456'), - Provider(name='Provider 4', slug='provider-4', asn=65004, account='4567'), - Provider(name='Provider 5', slug='provider-5', asn=65005, account='5678'), + Provider(name='Provider 1', slug='provider-1', account='1234'), + Provider(name='Provider 2', slug='provider-2', account='2345'), + Provider(name='Provider 3', slug='provider-3', account='3456'), + Provider(name='Provider 4', slug='provider-4', account='4567'), + Provider(name='Provider 5', slug='provider-5', account='5678'), ) Provider.objects.bulk_create(providers) providers[0].asns.set([asns[0]]) @@ -82,10 +82,6 @@ class ProviderTestCase(TestCase, ChangeLoggedFilterSetTests): params = {'slug': ['provider-1', 'provider-2']} self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2) - def test_asn(self): # Legacy field - params = {'asn': ['65001', '65002']} - self.assertEqual(self.filterset(params, self.queryset).qs.count(), 2) - def test_asn_id(self): # ASN object assignment asns = ASN.objects.all()[:2] params = {'asn_id': [asns[0].pk, asns[1].pk]} diff --git a/netbox/circuits/tests/test_views.py b/netbox/circuits/tests/test_views.py index fa6146b93..9644c0b02 100644 --- a/netbox/circuits/tests/test_views.py +++ b/netbox/circuits/tests/test_views.py @@ -23,9 +23,9 @@ class ProviderTestCase(ViewTestCases.PrimaryObjectViewTestCase): ASN.objects.bulk_create(asns) providers = ( - Provider(name='Provider 1', slug='provider-1', asn=65001), - Provider(name='Provider 2', slug='provider-2', asn=65002), - Provider(name='Provider 3', slug='provider-3', asn=65003), + Provider(name='Provider 1', slug='provider-1'), + Provider(name='Provider 2', slug='provider-2'), + Provider(name='Provider 3', slug='provider-3'), ) Provider.objects.bulk_create(providers) providers[0].asns.set([asns[0], asns[1]]) @@ -37,12 +37,8 @@ class ProviderTestCase(ViewTestCases.PrimaryObjectViewTestCase): cls.form_data = { 'name': 'Provider X', 'slug': 'provider-x', - 'asn': 65123, 'asns': [asns[6].pk, asns[7].pk], 'account': '1234', - 'portal_url': 'http://example.com/portal', - 'noc_contact': 'noc@example.com', - 'admin_contact': 'admin@example.com', 'comments': 'Another provider', 'tags': [t.pk for t in tags], } @@ -55,11 +51,7 @@ class ProviderTestCase(ViewTestCases.PrimaryObjectViewTestCase): ) cls.bulk_edit_data = { - 'asn': 65009, 'account': '5678', - 'portal_url': 'http://example.com/portal2', - 'noc_contact': 'noc2@example.com', - 'admin_contact': 'admin2@example.com', 'comments': 'New comments', } @@ -104,8 +96,8 @@ class CircuitTestCase(ViewTestCases.PrimaryObjectViewTestCase): def setUpTestData(cls): providers = ( - Provider(name='Provider 1', slug='provider-1', asn=65001), - Provider(name='Provider 2', slug='provider-2', asn=65002), + Provider(name='Provider 1', slug='provider-1'), + Provider(name='Provider 2', slug='provider-2'), ) Provider.objects.bulk_create(providers) diff --git a/netbox/extras/tests/test_customvalidator.py b/netbox/extras/tests/test_customvalidator.py index ce3b572d1..0fe507b67 100644 --- a/netbox/extras/tests/test_customvalidator.py +++ b/netbox/extras/tests/test_customvalidator.py @@ -2,7 +2,7 @@ from django.conf import settings from django.core.exceptions import ValidationError from django.test import TestCase, override_settings -from circuits.models import Provider +from ipam.models import ASN, RIR from dcim.models import Site from extras.validators import CustomValidator @@ -67,21 +67,25 @@ custom_validator = MyValidator() class CustomValidatorTest(TestCase): - @override_settings(CUSTOM_VALIDATORS={'circuits.provider': [min_validator]}) + @classmethod + def setUpTestData(cls): + RIR.objects.create(name='RIR 1', slug='rir-1') + + @override_settings(CUSTOM_VALIDATORS={'ipam.asn': [min_validator]}) def test_configuration(self): - self.assertIn('circuits.provider', settings.CUSTOM_VALIDATORS) - validator = settings.CUSTOM_VALIDATORS['circuits.provider'][0] + self.assertIn('ipam.asn', settings.CUSTOM_VALIDATORS) + validator = settings.CUSTOM_VALIDATORS['ipam.asn'][0] self.assertIsInstance(validator, CustomValidator) - @override_settings(CUSTOM_VALIDATORS={'circuits.provider': [min_validator]}) + @override_settings(CUSTOM_VALIDATORS={'ipam.asn': [min_validator]}) def test_min(self): with self.assertRaises(ValidationError): - Provider(name='Provider 1', slug='provider-1', asn=1).clean() + ASN(asn=1, rir=RIR.objects.first()).clean() - @override_settings(CUSTOM_VALIDATORS={'circuits.provider': [max_validator]}) + @override_settings(CUSTOM_VALIDATORS={'ipam.asn': [max_validator]}) def test_max(self): with self.assertRaises(ValidationError): - Provider(name='Provider 1', slug='provider-1', asn=65535).clean() + ASN(asn=65535, rir=RIR.objects.first()).clean() @override_settings(CUSTOM_VALIDATORS={'dcim.site': [min_length_validator]}) def test_min_length(self): diff --git a/netbox/templates/circuits/provider.html b/netbox/templates/circuits/provider.html index 60bf8cfbc..0fc18a368 100644 --- a/netbox/templates/circuits/provider.html +++ b/netbox/templates/circuits/provider.html @@ -19,17 +19,6 @@
Provider
- - - - - - - - - - - - - - - -
ASN - {% if object.asn %} -
- -
- {% endif %} - {{ object.asn|placeholder }} -
ASNs @@ -44,24 +33,6 @@ Account {{ object.account|placeholder }}
Customer Portal - {% if object.portal_url %} - {{ object.portal_url }} - {% else %} - {{ ''|placeholder }} - {% endif %} -
NOC Contact{{ object.noc_contact|markdown|placeholder }}
Admin Contact{{ object.admin_contact|markdown|placeholder }}
Circuits diff --git a/netbox/utilities/tests/test_filters.py b/netbox/utilities/tests/test_filters.py index 5182722d1..334f270dc 100644 --- a/netbox/utilities/tests/test_filters.py +++ b/netbox/utilities/tests/test_filters.py @@ -5,8 +5,6 @@ from django.test import TestCase from mptt.fields import TreeForeignKey from taggit.managers import TaggableManager -from circuits.filtersets import CircuitFilterSet, ProviderFilterSet -from circuits.models import Circuit, Provider from dcim.choices import * from dcim.fields import MACAddressField from dcim.filtersets import DeviceFilterSet, SiteFilterSet @@ -15,6 +13,7 @@ from dcim.models import ( ) from extras.filters import TagFilter from extras.models import TaggedItem +from ipam.filtersets import ASNFilterSet from ipam.models import RIR, ASN from netbox.filtersets import BaseFilterSet from utilities.filters import ( @@ -338,13 +337,14 @@ class DynamicFilterLookupExpressionTest(TestCase): """ @classmethod def setUpTestData(cls): + rir = RIR.objects.create(name='RIR 1', slug='rir-1') - providers = ( - Provider(name='Provider 1', slug='provider-1', asn=65001), - Provider(name='Provider 2', slug='provider-2', asn=65101), - Provider(name='Provider 3', slug='provider-3', asn=65201), + asns = ( + ASN(asn=65001, rir=rir), + ASN(asn=65101, rir=rir), + ASN(asn=65201, rir=rir), ) - Provider.objects.bulk_create(providers) + ASN.objects.bulk_create(asns) manufacturers = ( Manufacturer(name='Manufacturer 1', slug='manufacturer-1'), @@ -389,15 +389,6 @@ class DynamicFilterLookupExpressionTest(TestCase): ) Site.objects.bulk_create(sites) - rir = RIR.objects.create(name='RFC 6996', is_private=True) - - asns = [ - ASN(asn=65001, rir=rir), - ASN(asn=65101, rir=rir), - ASN(asn=65201, rir=rir) - ] - ASN.objects.bulk_create(asns) - asns[0].sites.add(sites[0]) asns[1].sites.add(sites[1]) asns[2].sites.add(sites[2]) @@ -456,19 +447,19 @@ class DynamicFilterLookupExpressionTest(TestCase): def test_provider_asn_lt(self): params = {'asn__lt': [65101]} - self.assertEqual(ProviderFilterSet(params, Provider.objects.all()).qs.count(), 1) + self.assertEqual(ASNFilterSet(params, ASN.objects.all()).qs.count(), 1) def test_provider_asn_lte(self): params = {'asn__lte': [65101]} - self.assertEqual(ProviderFilterSet(params, Provider.objects.all()).qs.count(), 2) + self.assertEqual(ASNFilterSet(params, ASN.objects.all()).qs.count(), 2) def test_provider_asn_gt(self): params = {'asn__lt': [65101]} - self.assertEqual(ProviderFilterSet(params, Provider.objects.all()).qs.count(), 1) + self.assertEqual(ASNFilterSet(params, ASN.objects.all()).qs.count(), 1) def test_provider_asn_gte(self): params = {'asn__gte': [65101]} - self.assertEqual(ProviderFilterSet(params, Provider.objects.all()).qs.count(), 2) + self.assertEqual(ASNFilterSet(params, ASN.objects.all()).qs.count(), 2) def test_site_region_negation(self): params = {'region__n': ['region-1']}