Address CR feedback

* Add `help_text` to all fields that were missing it.
* Fix "Represetation" typo
* Fix outdated `Protocol.__unicode__` method
* Refactor `Protocol.local_interface()` and `Protocol.remote_interface()`
  into `Circuit.interface_for(device)`
* Make `Protocol.site` optional, inherit from `Protocol.device.site` if
  not present
* Add more tests around feedback

For the `help_text` thing, I would add a test for that but there's
likely some violations which will have to be fixed and that's out of
scope of this PR. I'll be adding that test and will fix any current
violations in a future PR.

nsot/migrations/0036_add_protocol.py

@@ -23,7 +23,7 @@ class Migration(migrations.Migration):
                 ('circuit', models.ForeignKey(related_name='protocols', blank=True, to='nsot.Circuit', help_text='Circuit that this protocol is running over.', null=True)),
                 ('device', models.ForeignKey(related_name='protocols', to='nsot.Device', help_text='Device that this protocol is running on')),
                 ('interface', models.ForeignKey(related_name='protocols', blank=True, to='nsot.Interface', help_text='Interface this protocol is running on. Either interface or circuit must be populated.', null=True)),
-                ('site', models.ForeignKey(related_name='protocols', on_delete=django.db.models.deletion.PROTECT, to='nsot.Site', help_text='Unique ID of the Site this Protocol is under.')),
+                ('site', models.ForeignKey(related_name='protocols', on_delete=django.db.models.deletion.PROTECT, blank=True, to='nsot.Site', help_text="Unique ID of the Site this Protocol is under. If not set, this be inherited off of the device's site", null=True, verbose_name='Site')),
             ],
             options={
                 'ordering': ('device',),
@@ -33,8 +33,8 @@ class Migration(migrations.Migration):
             name='ProtocolType',
             fields=[
                 ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
-                ('name', models.CharField(help_text='Name of this type of protocol (e.g. OSPF, BGP, etc.)', max_length=16, db_index=True)),
-                ('description', models.CharField(default='', max_length=255, blank=True)),
+                ('name', models.CharField(help_text='Name of this type of protocol (e.g. OSPF, BGP, etc.)', unique=True, max_length=16, db_index=True)),
+                ('description', models.CharField(default='', help_text='A description for this ProtocolType', max_length=255, blank=True)),
             ],
         ),
         migrations.AlterField(
@@ -65,6 +65,6 @@ class Migration(migrations.Migration):
         migrations.AddField(
             model_name='protocol',
             name='type',
-            field=models.ForeignKey(related_name='protocols', to='nsot.ProtocolType'),
+            field=models.ForeignKey(related_name='protocols', to='nsot.ProtocolType', help_text='The type of this Protocol'),
         ),
     ]

nsot/models.py

@@ -1665,6 +1665,22 @@ def devices(self):
         """Return devices associated with this circuit."""
         return [i.device for i in self.interfaces]
 
+    def interface_for(self, device):
+        """
+        Given a Device object, return the interface attached to this Circuit
+        which belongs to that Device. If both ends of the Circuit are attached
+        to the Device, the A-side is returned.
+
+        If neither ends of this Circuit are attached to Device, then None is
+        returned
+        """
+        if self.endpoint_a.device == device:
+            return self.endpoint_a
+        elif self.endpoint_z and self.endpoint_z.device == device:
+            return self.endpoint_z
+        else:
+            return None
+
     def clean_site(self, value):
         """Always enforce that site is set."""
         if value is None:
@@ -1721,11 +1737,12 @@ def to_dict(self):
 
 class ProtocolType(models.Model):
     name = models.CharField(
-        max_length=16, db_index=True,
+        max_length=16, db_index=True, unique=True,
         help_text='Name of this type of protocol (e.g. OSPF, BGP, etc.)',
     )
     description = models.CharField(
-        max_length=255, default='', blank=True, null=False
+        max_length=255, default='', blank=True, null=False,
+        help_text='A description for this ProtocolType',
     )
     required_attributes = models.ManyToManyField(
         'Attribute', db_index=True, related_name='protocol_types',
@@ -1763,16 +1780,19 @@ def required_attributes_changed(sender, instance, action, reverse, model,
 
 class Protocol(Resource):
     """
-    Represetation of a routing protocol running over a circuit
+    Representation of a routing protocol
     """
-
     site = models.ForeignKey(
-        Site, db_index=True, related_name='protocols',
-        on_delete=models.PROTECT,
-        help_text='Unique ID of the Site this Protocol is under.',
+        Site, db_index=True, blank=True, null=True, related_name='protocols',
+        on_delete=models.PROTECT, verbose_name='Site',
+        help_text=(
+            'Unique ID of the Site this Protocol is under. If not set, this '
+            "be inherited off of the device's site"
+        )
     )
     type = models.ForeignKey(
         ProtocolType, db_index=True, related_name='protocols',
+        help_text='The type of this Protocol',
     )
     device = models.ForeignKey(
         Device, db_index=True, null=False, related_name='protocols',
@@ -1801,49 +1821,41 @@ class Protocol(Resource):
     )
 
     def __unicode__(self):
-        return u'%s over %s' % (self.get_type_display(), self.circuit)
+        description = unicode(self.type)
 
-    class Meta:
-        ordering = ('device', )
-
-    def local_interface(self):
-        """
-        Returns the local interface for this Protocol, either the set interface
-        or the local interface of circuit
-        """
-        if self.interface:
-            return self.interface
-        elif self.circuit is None:
-            return None
+        if self.circuit:
+            description += ' over %s' % self.circuit
+        elif self.interface:
+            description += ' on %s' % self.interface
+        else:
+            description += ' on %s' % self.device
 
-        for endpoint in (self.circuit.endpoint_a, self.circuit.endpoint_z):
-            if endpoint.device.id == self.device.id:
-                return endpoint
+        return description
 
-        return None
+    class Meta:
+        ordering = ('device', )
 
-    def remote_interface(self):
+    def clean_site(self, value):
         """
-        Returns the remote device's interface attached to the circuit
+        Ensure we have a site set. If one is not explicitly set, glean it from
+        the device. If the device has none, then raise a ValidationError
         """
-        if self.circuit is None:
-            return None
-
-        for endpoint in (self.circuit.endpoint_a, self.circuit.endpoint_z):
-            if endpoint.device.id != self.device.id:
-                return endpoint
-
-        return None
+        if not value:
+            value = self.device.site
 
-    def clean_site(self, value):
-        if value is None:
-            return self.device.site_id
+        if not value:
+            raise exc.ValidationError({
+                'site': (
+                    'No site was provided and the provided Device does not '
+                    'have a site defined'
+                )
+            })
 
         return value
 
     def clean_circuit(self, value):
         """ Ensure at least one endpoint on the circuit is on this device """
-        if value and self.local_interface() is None:
+        if value and value.interface_for(self.device) is None:
             raise exc.ValidationError({
                 'circuit': (
                     'At least one endpoint of the circuit must match the '
@@ -1869,6 +1881,10 @@ def clean_interface(self, value):
         return value
 
     def clean_type(self, value):
+        """
+        Ensure that all attributes are set that are required by the set
+        ProtocolType
+        """
         required = value.required_attributes.values_list(
             'name', flat=True
         )
@@ -1889,7 +1905,7 @@ def clean_type(self, value):
         return value
 
     def clean_fields(self, exclude=None):
-        self.site_id = self.clean_site(self.site_id)
+        self.site = self.clean_site(self.site)
         self.type = self.clean_type(self.type)
         self.interface = self.clean_interface(self.interface)
         self.circuit = self.clean_circuit(self.circuit)

tests/model_tests/test_circuits.py

@@ -111,3 +111,65 @@ def test_attributes(circuit):
 
     with pytest.raises(exc.ValidationError):
         circuit.set_attributes({'made_up': 'value'})
+
+
+class TestInterfaceFor(object):
+    @pytest.fixture
+    def device_z(self, site):
+        return models.Device.objects.create(site=site, hostname='foo-bar2')
+
+    @pytest.fixture
+    def interface_a(self, device):
+        return models.Interface.objects.create(device=device, name='eth0')
+
+    @pytest.fixture
+    def interface_z(self, device_z):
+        return models.Interface.objects.create(
+            device=device_z, name='eth0')
+
+    @pytest.fixture
+    def normal_circuit(self, device_z, interface_a, interface_z):
+        return models.Circuit.objects.create(
+            endpoint_a=interface_a,
+            endpoint_z=interface_z
+        )
+
+    @pytest.fixture
+    def looped_circuit(self, device, interface_a):
+        interface_z = models.Interface.objects.create(
+            device=device,
+            name='eth1'
+        )
+        return models.Circuit.objects.create(
+            endpoint_a=interface_a,
+            endpoint_z=interface_z,
+        )
+
+    def test_normal_conditions(self, device, device_z, interface_a,
+                               interface_z, normal_circuit):
+        assert normal_circuit.interface_for(device) == interface_a
+        print('interface_z via circuit id = {}'.format(normal_circuit.endpoint_z.id))
+        print('interface_z id = {}'.format(interface_z.id))
+        assert normal_circuit.interface_for(device_z) == interface_z
+
+    def test_single_sided(self, device, interface_a):
+        """
+        Make sure things don't blow up on a single-sided circuit
+        """
+        circuit = models.Circuit.objects.create(endpoint_a=interface_a)
+        assert circuit.interface_for(device) == interface_a
+
+    def test_looped_circuit(self, device, looped_circuit, interface_a):
+        """
+        Test the case when both sides of a circuit are connected to the same
+        device. The method should return endpoint_a in this case.
+        """
+        assert looped_circuit.interface_for(device) == interface_a
+
+    def test_bogus_device(self, device, device_z, looped_circuit):
+        """
+        interface_for should return None when given a device that isn't
+        connected by the circuit
+        """
+        assert looped_circuit.interface_for(device_z) is None
+        assert looped_circuit.interface_for(device) is not None

tests/model_tests/test_protocols.py

@@ -159,56 +159,77 @@ def test_mixed_attrs(self, device, circuit, bgp, area_attribute):
         assert protocol.get_attributes()['area'] == 'threeve'
 
 
-class TestInterfaces(object):
+class TestUnicode(object):
+    """
+    Tests for the __unicode__ method on Protocol
+    """
     @pytest.fixture
-    def protocol(self, circuit, bgp):
+    def interface(self, circuit):
+        return circuit.endpoint_a
+
+    @pytest.fixture
+    def base_protocol(self, bgp, circuit):
         device = circuit.endpoint_a.device
 
         return models.Protocol.objects.create(
             device=device,
-            circuit=circuit,
             type=bgp,
             attributes={
                 'asn': '1234',
             }
         )
 
-    @pytest.fixture
-    def interface(self, circuit):
-        return circuit.endpoint_a
+    def test_circuit(self, base_protocol, circuit):
+        """
+        Case when only a circuit is set on the Protocol (no interface)
+        """
+        expected = 'bgp over foo-bar1:eth0_foo-bar2:eth0'
 
-    @pytest.fixture
-    def interface_protocol(self, interface, ospf):
-        device = interface.device
+        base_protocol.circuit = circuit
+        base_protocol.save()
 
-        return models.Protocol.objects.create(
-            device=device,
-            interface=interface,
-            type=ospf,
-            attributes={'area': '1234'},
-        )
+        assert base_protocol.circuit is not None
+        assert base_protocol.interface is None
 
-    def test_local_interface(self, circuit, protocol, interface,
-                             interface_protocol):
-        assert protocol.local_interface() == circuit.endpoint_a
-        assert interface_protocol.local_interface() == interface
+        assert unicode(base_protocol) == expected
 
-    def test_remote_interface(self, circuit, protocol):
-        assert protocol.remote_interface() == circuit.endpoint_z
+    def test_interface(self, base_protocol, interface):
+        """
+        Case when only an interface is set on the Protocol (no circuit)
+        """
+        expected = 'bgp on foo-bar1:eth0'
 
-    def test_backwards_circuit(self, circuit, bgp):
+        base_protocol.interface = interface
+        base_protocol.save()
+
+        assert base_protocol.interface is not None
+        assert base_protocol.circuit is None
+
+        assert unicode(base_protocol) == expected
+
+    def test_neither_circuit_nor_interface(self, base_protocol):
         """
-        Make sure local/remote_interface returns the correct thing when the
-        device is on the Z side of the circuit instead of the A side
+        Case when neither a circuit nor interface are set
         """
-        protocol = models.Protocol.objects.create(
-            device=circuit.endpoint_z.device,
-            circuit=circuit,
-            type=bgp,
-            attributes={
-                'asn': '1234',
-            }
-        )
+        expected = 'bgp on foo-bar1'
+
+        assert base_protocol.circuit is None
+        assert base_protocol.interface is None
+
+        assert unicode(base_protocol) == expected
+
+    def test_both_circuit_and_interface(self, base_protocol, circuit,
+                                        interface):
+        """
+        Case when both a circuit and interface are set
+        """
+        expected = 'bgp over foo-bar1:eth0_foo-bar2:eth0'
+
+        base_protocol.circuit = circuit
+        base_protocol.interface = interface
+        base_protocol.save()
+
+        assert base_protocol.circuit is not None
+        assert base_protocol.interface is not None
 
-        assert protocol.local_interface() == circuit.endpoint_z
-        assert protocol.remote_interface() == circuit.endpoint_a
+        assert unicode(base_protocol) == expected