Merge branch '5.8.x'

CHANGELOG.md

@@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 This changelog format was introduced in NAV 5.4.0. Older changelogs can be
 found in the [HISTORY](HISTORY) file.
 
+## [Unreleased]
+
+### Fixed
+
+- Allow admins to configure ports with invalid or unset native VLANs in PortAdmin ([#2477](https://github.com/Uninett/nav/issues/2477), [#2786](https://github.com/Uninett/nav/pull/2786))
+- Fix bug that caused PoE config to be completely disabled for Cisco devices where at least one port did not support PoE ([#2781](https://github.com/Uninett/nav/pull/2781))
+- Fix PortAdmin save button moving around for ports without PoE support ([#2782](https://github.com/Uninett/nav/pull/2782))
+- Fix PortAdmin bug that prevented switching PoE state back and forth without reloading entire page ([#2785](https://github.com/Uninett/nav/pull/2785))
+- Fix regression that caused maintenance tasks to be un-editable ([#2783](https://github.com/Uninett/nav/issues/2783), [#2784](https://github.com/Uninett/nav/pull/2784))
+
 ## [5.8.3] - 2023-12-01
 
 ### Fixed

python/nav/portadmin/handlers.py

@@ -352,7 +352,3 @@ class POEStateNotSupportedError(ManagementError):
 
 class XMLParseError(ManagementError):
     """Raised when failing to parse XML"""
-
-
-class POEIndexNotFoundError(ManagementError):
-    """Raised when a PoE index could not be found for an interface"""

python/nav/portadmin/snmp/cisco.py

@@ -28,7 +28,6 @@
     PoeState,
     POEStateNotSupportedError,
     POENotSupportedError,
-    POEIndexNotFoundError,
 )
 from nav.models import manage
 
@@ -351,7 +350,7 @@ def _get_poe_indexes_for_interface(
         try:
             poeport = manage.POEPort.objects.get(interface=interface)
         except manage.POEPort.DoesNotExist:
-            raise POEIndexNotFoundError(
+            raise POENotSupportedError(
                 "This interface does not have PoE indexes defined"
             )
         unit_number = poeport.poegroup.index

python/nav/web/maintenance/views.py

@@ -252,9 +252,7 @@ def cancel(request, task_id):
 def edit(request, task_id=None, start_time=None, **_):
     account = get_account(request)
     quickselect = QuickSelect(service=True)
-    component_trail = None
-    component_keys = None
-    task = None
+    component_trail = component_keys_errors = component_data = task = None
 
     if task_id:
         task = get_object_or_404(MaintenanceTask, pk=task_id)

python/nav/web/portadmin/utils.py

@@ -15,7 +15,7 @@
 #
 """Util functions for the PortAdmin"""
 from __future__ import unicode_literals
-from typing import List, Sequence, Dict, Any
+from typing import List, Sequence, Dict, Any, Optional
 import re
 import logging
 from operator import attrgetter
@@ -24,6 +24,7 @@
 
 from nav.models import manage, profiles
 from nav.django.utils import is_admin
+from nav.models.profiles import Account
 from nav.portadmin.config import CONFIG
 from nav.portadmin.management import ManagementFactory
 from nav.portadmin.handlers import ManagementHandler
@@ -76,7 +77,7 @@ def find_and_populate_allowed_vlans(
 ):
     """Finds allowed vlans and indicate which interfaces can be edited"""
     allowed_vlans = find_allowed_vlans_for_user_on_netbox(account, netbox, handler)
-    set_editable_flag_on_interfaces(interfaces, allowed_vlans)
+    set_editable_flag_on_interfaces(interfaces, allowed_vlans, account)
     return allowed_vlans
 
 
@@ -126,7 +127,9 @@ def find_allowed_vlans_for_user(account):
 
 
 def set_editable_flag_on_interfaces(
-    interfaces: Sequence[manage.Interface], vlans: Sequence[FantasyVlan]
+    interfaces: Sequence[manage.Interface],
+    vlans: Sequence[FantasyVlan],
+    user: Optional[Account] = None,
 ):
     """Sets the pseudo-attribute `iseditable` on each interface in the interfaces
     list, indicating whether the PortAdmin UI should allow edits to it or not.
@@ -136,8 +139,13 @@ def set_editable_flag_on_interfaces(
     an uplink, depending on how portadmin is configured.
     """
     vlan_tags = {vlan.vlan for vlan in vlans}
+    allow_everything = not should_check_access_rights(account=user) if user else False
 
     for interface in interfaces:
+        if allow_everything:
+            interface.iseditable = True
+            continue
+
         vlan_is_acceptable = interface.vlan in vlan_tags
         is_link = bool(interface.to_netbox)
         refuse_link_edit = not CONFIG.get_link_edit() and is_link

python/nav/web/portadmin/views.py

@@ -56,7 +56,6 @@
     NoResponseError,
     ProtocolError,
     ManagementError,
-    POEIndexNotFoundError,
     XMLParseError,
     POEStateNotSupportedError,
 )
@@ -248,7 +247,6 @@ def populate_infodict(request, netbox, interfaces):
         messages.error(request, str(error))
 
     except (
-        POEIndexNotFoundError,
         XMLParseError,
         POEStateNotSupportedError,
     ) as error:

python/nav/web/static/js/src/portadmin.js

@@ -482,6 +482,9 @@ require(['libs/spin.min', 'libs/jquery-ui.min'], function (Spinner) {
         if ('ifadminstatus' in data) {
             updateAdminStatusDefault($row, data.ifadminstatus);
         }
+        if ('poe_state' in data) {
+            updatePoeDefault($row, data.poe_state);
+        }
     }
 
     function updateIfAliasDefault($row, ifalias) {
@@ -518,6 +521,15 @@ require(['libs/spin.min', 'libs/jquery-ui.min'], function (Spinner) {
         }
     }
 
+    function updatePoeDefault($row, new_value) {
+        var old_value = $row.find('option[data-orig]').val();
+        if (old_value !== new_value) {
+            console.log('Updating PoE state default from ' + old_value + ' to ' + new_value);
+            $row.find('option[data-orig]').removeAttr('data-orig');
+            $row.find('option[value=' + new_value + ']').attr('data-orig', new_value);
+        }
+    }
+
     function removeFromQueue(id) {
         if (queue_data.hasOwnProperty(id)) {
             delete queue_data[id];

python/nav/web/templates/portadmin/portlist.html

@@ -169,20 +169,19 @@
 
       {# POE STATE #}
       {% if supports_poe %}
-        {% if interface.supports_poe %}
         <div class="medium-1 small-4 column">
-          <form class="custom">
-            <select class="poelist" name="{{ interface.ifname }}">
-              {% for poe_option in poe_options %}
+          {% if interface.supports_poe %}
+            <form class="custom">
+              <select class="poelist" name="{{ interface.ifname }}">
+                {% for poe_option in poe_options %}
                   <option value="{{ poe_option.name }}" label="{{ poe_option.name }}"
                           {% if interface.poe_state.name == poe_option.name %}selected="selected"
                           data-orig="{{ poe_option.name }}"{% endif %}>
-                    {{ vlan }}
-              {% endfor %}
-            </select>
-          </form>
+                {% endfor %}
+              </select>
+            </form>
+          {% endif %}
         </div>
-        {% endif %}
       {% endif %}
 
       {# Button for saving #}

tests/integration/web/maintenance/views_test.py

@@ -13,8 +13,11 @@
 # more details.  You should have received a copy of the GNU General Public
 # License along with NAV. If not, see <http://www.gnu.org/licenses/>.
 #
+import datetime
 
+import pytest
 from django.urls import reverse
+
 from nav.compatibility import smart_str
 from nav.models.manage import Netbox
 from nav.models.msgmaint import MaintenanceTask
@@ -136,3 +139,27 @@ def test_with_end_time_before_start_time_should_fail(self, db, client, localhost
         assert not MaintenanceTask.objects.filter(
             description=data["description"]
         ).exists()
+
+
+class TestEditMaintenanceTask:
+    def test_when_existing_task_is_requested_it_should_render_with_intact_description(
+        self, db, client, localhost, empty_maintenance_task
+    ):
+        url = reverse("maintenance-edit", kwargs={"task_id": empty_maintenance_task.id})
+        response = client.get(url, follow=True)
+
+        assert response.status_code == 200
+        assert empty_maintenance_task.description in smart_str(response.content)
+
+
+@pytest.fixture
+def empty_maintenance_task(db):
+    now = datetime.datetime.now()
+    task = MaintenanceTask(
+        start_time=now,
+        end_time=now + datetime.timedelta(hours=1),
+        description="Temporary test fixture task",
+    )
+    task.save()
+    yield task
+    task.delete()

tests/unittests/portadmin/portadmin_poe_cisco_test.py

@@ -3,10 +3,7 @@
 import pytest
 
 from nav.portadmin.snmp.cisco import Cisco
-from nav.portadmin.handlers import (
-    POEIndexNotFoundError,
-    POEStateNotSupportedError,
-)
+from nav.portadmin.handlers import POEStateNotSupportedError
 from nav.models import manage
 
 
@@ -23,24 +20,24 @@ class TestGetPoeState:
     @pytest.mark.usefixtures('poeport_get_mock')
     def test_should_raise_exception_if_unknown_poe_state(self, handler_cisco):
         handler_cisco._query_netbox = Mock(return_value=76)
-        interface = Mock(interface="interface")
+        interface = Mock(ifname="interface")
         with pytest.raises(POEStateNotSupportedError):
             handler_cisco.get_poe_states([interface])
 
     @pytest.mark.usefixtures('poeport_get_mock_error')
-    def test_should_raise_exception_if_interface_is_missing_poeport(
+    def test_dict_should_give_none_if_interface_does_not_have_poeport(
         self, handler_cisco
     ):
-        interface = Mock(interface="interface")
-        with pytest.raises(POEIndexNotFoundError):
-            handler_cisco.get_poe_states([interface])
+        interface = Mock(ifname="interface")
+        states = handler_cisco.get_poe_states([interface])
+        assert states[interface.ifname] is None
 
     @pytest.mark.usefixtures('poeport_get_mock')
     def test_dict_should_give_none_if_interface_does_not_support_poe(
         self, handler_cisco
     ):
         handler_cisco._query_netbox = Mock(return_value=None)
-        interface = Mock(interface="interface")
+        interface = Mock(ifname="interface")
         states = handler_cisco.get_poe_states([interface])
         assert states[interface.ifname] is None
 

tests/unittests/web/portadmin/utils_test.py

@@ -0,0 +1,39 @@
+from unittest.mock import patch, Mock
+
+from nav.web.portadmin.utils import set_editable_flag_on_interfaces
+
+
+class TestSetEditableFlagOnInterfaces:
+    def test_when_user_is_admin_it_should_set_all_interfaces_to_editable(self):
+        with patch(
+            "nav.web.portadmin.utils.should_check_access_rights", return_value=False
+        ):
+            mock_admin = Mock()
+            mock_interfaces = [Mock(iseditable=False)] * 3
+            set_editable_flag_on_interfaces(mock_interfaces, [], mock_admin)
+
+            assert all(ifc.iseditable for ifc in mock_interfaces)
+
+    def test_when_user_is_not_admin_it_should_set_only_matching_interfaces_to_editable(
+        self,
+    ):
+        with patch(
+            "nav.web.portadmin.utils.should_check_access_rights", return_value=True
+        ):
+            mock_user = Mock()
+            mock_vlans = [Mock(vlan=42), Mock(vlan=69), Mock(vlan=666)]
+            editable_interface = Mock(vlan=666, iseditable=False)
+            mock_interfaces = [
+                Mock(vlan=99, iseditable=False),
+                editable_interface,
+                Mock(vlan=27, iseditable=False),
+            ]
+
+            set_editable_flag_on_interfaces(mock_interfaces, mock_vlans, mock_user)
+
+            assert editable_interface.iseditable
+            assert all(
+                not ifc.iseditable
+                for ifc in mock_interfaces
+                if ifc is not editable_interface
+            )