From 869a04479535d852c163fe186e388a083b8159af Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Mon, 11 Dec 2023 11:18:18 +0100 Subject: [PATCH 01/12] Add regression test for #2783 --- .../integration/web/maintenance/views_test.py | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/integration/web/maintenance/views_test.py b/tests/integration/web/maintenance/views_test.py index 8109fcaae9..1987c62c2d 100644 --- a/tests/integration/web/maintenance/views_test.py +++ b/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 . # +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() From 1de2f9ab5c16f9e260817dd3dcf64ef4b1ba3215 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Mon, 11 Dec 2023 11:04:29 +0100 Subject: [PATCH 02/12] Initialize all variables that could be unassigned Due to the convoluted logic of the edit() function, multiple variables may be referenced before proper assignment unless they are given a value of `None` first. Unfortunately, at least two such variables were potentially referenced without being initialized first (among other things, causing the crash mentioned in #2783). This ensures all variables that may remain "unassigned" are initialized to `None` at the start of the function. --- python/nav/web/maintenance/views.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/python/nav/web/maintenance/views.py b/python/nav/web/maintenance/views.py index 7d4d653e9f..9eab84a44b 100644 --- a/python/nav/web/maintenance/views.py +++ b/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) From d727a4826cc4c359f5ea074b0223b06748f47504 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Tue, 12 Dec 2023 11:49:21 +0100 Subject: [PATCH 03/12] Allow admins to change VLANs on all ports It doesn't matter that a port is set to an invalid native VLAN, if the user has admin-level access, they should still be allowed to edit the port and fix the problem. --- python/nav/web/portadmin/utils.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/python/nav/web/portadmin/utils.py b/python/nav/web/portadmin/utils.py index 221b06e170..072054b6bd 100644 --- a/python/nav/web/portadmin/utils.py +++ b/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 From 2e73fd76dae2a66734b77144e0e1f4714acc9e9c Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Wed, 13 Dec 2023 13:51:53 +0100 Subject: [PATCH 04/12] Test set_editable_flag_on_interfaces() --- tests/unittests/web/portadmin/__init__.py | 0 tests/unittests/web/portadmin/utils_test.py | 39 +++++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 tests/unittests/web/portadmin/__init__.py create mode 100644 tests/unittests/web/portadmin/utils_test.py diff --git a/tests/unittests/web/portadmin/__init__.py b/tests/unittests/web/portadmin/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/unittests/web/portadmin/utils_test.py b/tests/unittests/web/portadmin/utils_test.py new file mode 100644 index 0000000000..84ccff0824 --- /dev/null +++ b/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 + ) From 072a164ec11a593e0204e0e13bf665a2f5410dbd Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Tue, 5 Dec 2023 12:12:42 +0100 Subject: [PATCH 05/12] Fix handling of interfaces without PoE support POEIndexNotFoundError will be raised if POEPorts do not exist for an interface, and this is likely to be the case for an interface that does not support PoE. get_poe_states should set state to None for these interfaces, like it does if PoeNotSupportedError is raised. --- python/nav/portadmin/snmp/cisco.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/portadmin/snmp/cisco.py b/python/nav/portadmin/snmp/cisco.py index bbe1a9d18e..7a3c255886 100644 --- a/python/nav/portadmin/snmp/cisco.py +++ b/python/nav/portadmin/snmp/cisco.py @@ -351,7 +351,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 From e4985c1b7d249256d714c080ed372a4f280fa791 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Tue, 5 Dec 2023 12:25:34 +0100 Subject: [PATCH 06/12] Fix logic in tests interface not having poeport should be considered as the interface not supporting poe --- tests/unittests/portadmin/portadmin_poe_cisco_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unittests/portadmin/portadmin_poe_cisco_test.py b/tests/unittests/portadmin/portadmin_poe_cisco_test.py index 2c48c638d0..2a279bb00e 100644 --- a/tests/unittests/portadmin/portadmin_poe_cisco_test.py +++ b/tests/unittests/portadmin/portadmin_poe_cisco_test.py @@ -28,12 +28,12 @@ def test_should_raise_exception_if_unknown_poe_state(self, handler_cisco): 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]) + 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( From 878c20160121148ddd4affaf9e6d244d958a1851 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Tue, 5 Dec 2023 12:26:51 +0100 Subject: [PATCH 07/12] Fix mock object properties --- tests/unittests/portadmin/portadmin_poe_cisco_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unittests/portadmin/portadmin_poe_cisco_test.py b/tests/unittests/portadmin/portadmin_poe_cisco_test.py index 2a279bb00e..c2be889fc6 100644 --- a/tests/unittests/portadmin/portadmin_poe_cisco_test.py +++ b/tests/unittests/portadmin/portadmin_poe_cisco_test.py @@ -23,7 +23,7 @@ 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]) @@ -31,7 +31,7 @@ def test_should_raise_exception_if_unknown_poe_state(self, handler_cisco): def test_dict_should_give_none_if_interface_does_not_have_poeport( self, handler_cisco ): - interface = Mock(interface="interface") + interface = Mock(ifname="interface") states = handler_cisco.get_poe_states([interface]) assert states[interface.ifname] is None @@ -40,7 +40,7 @@ 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 From 33381e1a4b82f4e708326e62d9eefc4e9bd7f279 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Tue, 5 Dec 2023 13:13:35 +0100 Subject: [PATCH 08/12] Remove unused exception --- python/nav/portadmin/handlers.py | 4 ---- python/nav/portadmin/snmp/cisco.py | 1 - python/nav/web/portadmin/views.py | 2 -- tests/unittests/portadmin/portadmin_poe_cisco_test.py | 5 +---- 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/python/nav/portadmin/handlers.py b/python/nav/portadmin/handlers.py index 5a2e8816a1..7c74e559df 100644 --- a/python/nav/portadmin/handlers.py +++ b/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""" diff --git a/python/nav/portadmin/snmp/cisco.py b/python/nav/portadmin/snmp/cisco.py index 7a3c255886..d2f4fa5b0c 100644 --- a/python/nav/portadmin/snmp/cisco.py +++ b/python/nav/portadmin/snmp/cisco.py @@ -28,7 +28,6 @@ PoeState, POEStateNotSupportedError, POENotSupportedError, - POEIndexNotFoundError, ) from nav.models import manage diff --git a/python/nav/web/portadmin/views.py b/python/nav/web/portadmin/views.py index 211910965a..e021807750 100644 --- a/python/nav/web/portadmin/views.py +++ b/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: diff --git a/tests/unittests/portadmin/portadmin_poe_cisco_test.py b/tests/unittests/portadmin/portadmin_poe_cisco_test.py index c2be889fc6..717d0252d2 100644 --- a/tests/unittests/portadmin/portadmin_poe_cisco_test.py +++ b/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 From 50a90fbc5bbd641055832a71fd468fc7275d8f0b Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Tue, 12 Dec 2023 11:36:08 +0100 Subject: [PATCH 09/12] Update origvalue on save for poe state --- python/nav/web/static/js/src/portadmin.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/python/nav/web/static/js/src/portadmin.js b/python/nav/web/static/js/src/portadmin.js index 4b1224d1d7..621b0c8864 100644 --- a/python/nav/web/static/js/src/portadmin.js +++ b/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]; From a9c9c2ea35d2ba229f0f31ef8e41cc5656f171b8 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Tue, 5 Dec 2023 13:29:08 +0100 Subject: [PATCH 10/12] Always occupy space if PoE column is active If poe column is active, which means some interfaces will have a poe state dropdown option, the lines that do not have a poe state dropdown still needs to occupy the space, else the save button will shift to the left and not be lined up with all the other save buttons. --- python/nav/web/templates/portadmin/portlist.html | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/python/nav/web/templates/portadmin/portlist.html b/python/nav/web/templates/portadmin/portlist.html index e6f40e6a98..06ef3c7d9c 100644 --- a/python/nav/web/templates/portadmin/portlist.html +++ b/python/nav/web/templates/portadmin/portlist.html @@ -169,20 +169,20 @@ {# POE STATE #} {% if supports_poe %} - {% if interface.supports_poe %}
-
- + {% for poe_option in poe_options %}
+ {% endfor %} + + + {% endif %}
- {% endif %} {% endif %} {# Button for saving #} From a9aa56c077e1f1f3c51fc7e8eeea4ea6a5f7f12d Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Tue, 5 Dec 2023 13:34:25 +0100 Subject: [PATCH 11/12] Remove random vlan line This was probably left over from basing the Poe dropdown on the vlan dropdown --- python/nav/web/templates/portadmin/portlist.html | 1 - 1 file changed, 1 deletion(-) diff --git a/python/nav/web/templates/portadmin/portlist.html b/python/nav/web/templates/portadmin/portlist.html index 06ef3c7d9c..342e52033f 100644 --- a/python/nav/web/templates/portadmin/portlist.html +++ b/python/nav/web/templates/portadmin/portlist.html @@ -177,7 +177,6 @@