From b1d225c6dd68ec2ba3d5dc7096da381ebb7b1ab0 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Wed, 20 Nov 2024 16:34:40 +0000 Subject: [PATCH 01/18] Add a description field to maintenance components The description field will be mainly to store component descriptions for future use in the case where the original object has been deleted from the NAV database. --- python/nav/models/msgmaint.py | 1 + python/nav/models/sql/changes/sc.05.12.0010.sql | 13 +++++++++++++ 2 files changed, 14 insertions(+) create mode 100644 python/nav/models/sql/changes/sc.05.12.0010.sql diff --git a/python/nav/models/msgmaint.py b/python/nav/models/msgmaint.py index 77496fd8bd..b666cbe05a 100644 --- a/python/nav/models/msgmaint.py +++ b/python/nav/models/msgmaint.py @@ -181,6 +181,7 @@ class MaintenanceComponent(models.Model): ) key = VarcharField() value = VarcharField() + description = VarcharField(null=True, blank=True) component = LegacyGenericForeignKey('key', 'value') class Meta(object): diff --git a/python/nav/models/sql/changes/sc.05.12.0010.sql b/python/nav/models/sql/changes/sc.05.12.0010.sql new file mode 100644 index 0000000000..c081a4b1e5 --- /dev/null +++ b/python/nav/models/sql/changes/sc.05.12.0010.sql @@ -0,0 +1,13 @@ +-- Add column to maint_component table to keep descriptions of components that can no longer referenced +ALTER TABLE maint_component ADD COLUMN description VARCHAR; + +UPDATE maint_component c +SET description = n.sysname +FROM netbox n +WHERE c.key = 'netbox' AND c.value = n.netboxid::text; + +UPDATE maint_component c +SET description = s.handler || ' at ' || n.sysname +FROM service s +JOIN netbox n ON s.netboxid = n.netboxid +WHERE c.key = 'service' AND c.value = s.serviceid::text; From bcecbe9269e80221ef1324f4cdd4fca13036bc06 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 07:53:53 +0000 Subject: [PATCH 02/18] Add method to fetch component class easily This is useful to get a reference to the model class of the referenced component, even if the foreign key itself no longer resolves to an existing record --- python/nav/models/msgmaint.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/python/nav/models/msgmaint.py b/python/nav/models/msgmaint.py index b666cbe05a..1b3bcb30e6 100644 --- a/python/nav/models/msgmaint.py +++ b/python/nav/models/msgmaint.py @@ -191,6 +191,10 @@ class Meta(object): def __str__(self): return u'%s=%s' % (self.key, self.value) + def get_component_class(self) -> models.Model: + """Returns a Model class based on the database table name stored in key""" + return LegacyGenericForeignKey.get_model_class(self.key) + class MessageToMaintenanceTask(models.Model): """From NAV Wiki: The connection between messages and related maintenance From 78ee25ebc220108084c458d9a8be10466fa80ab7 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 07:59:18 +0000 Subject: [PATCH 03/18] Add new functions to build component trails They way the maintenance UI builds data structures to describe maintenance components (and their "trails") is quite obtuse, and seems to be a strange leftover from the olden days before Django was introduced to NAV. This can be done a lot cleaner, while improving on how missing components are described. These classes and functions represent an alternative way to build component trails to place in the template render context. --- python/nav/web/maintenance/utils.py | 62 ++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/python/nav/web/maintenance/utils.py b/python/nav/web/maintenance/utils.py index c512df60f8..76a9593c7d 100644 --- a/python/nav/web/maintenance/utils.py +++ b/python/nav/web/maintenance/utils.py @@ -15,15 +15,18 @@ # from calendar import HTMLCalendar +from dataclasses import dataclass from datetime import date, datetime, timedelta from time import strftime +from typing import Union, List, Iterator +from django.db import models from django.urls import reverse from django.utils.html import conditional_escape from nav.models.manage import Netbox, Room, Location, NetboxGroup from nav.models.service import Service -from nav.models.msgmaint import MaintenanceTask +from nav.models.msgmaint import MaintenanceTask, MaintenanceComponent PRIMARY_KEY_INTEGER = ('netbox', 'service') @@ -96,6 +99,63 @@ def infodict_by_state(task): } +@dataclass +class MissingComponent: + """Represents a deleted component still associated with a task""" + + model_class: models.Model + pk: Union[int, str] + description: str + + @property + def _meta(self): + """Provides a fake `_meta` attribute for compatibility with Django model + objects. + """ + return self.model_class._meta + + def __str__(self): + descr = self.description or self.pk + return f"{descr} (Component was deleted)" + + +ComponentType = Union[Location, Room, Netbox, Service, NetboxGroup, MissingComponent] + + +def component_to_trail(component: ComponentType) -> List[ComponentType]: + """Transforms a single component into a list of components that make up a trail for + display purposes. + """ + if isinstance(component, Room): + return [component.location, component] + if isinstance(component, Netbox): + return [component.room.location, component.room, component] + if isinstance(component, Service): + return [ + component.netbox.room.location, + component.netbox.room, + component.netbox, + component, + ] + return [component] + + +def get_components(task: MaintenanceTask) -> Iterator[ComponentType]: + """Yields all components associated with a task, replacing deleted components with + a MissingComponent instance. + """ + relation: MaintenanceComponent + for relation in task.maintenance_components.all(): + if relation.component is None: + yield MissingComponent( + model_class=relation.get_component_class(), + pk=relation.value, + description=relation.description, + ) + else: + yield relation.component + + def get_component_keys(post): remove = {} errors = [] From 67d4d0e1fe6712169e2fd14e53d0807b8aa60824 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 08:57:58 +0000 Subject: [PATCH 04/18] Add type annotations to `LegacyGenericForeignKey` These type hints help a lot when working in an IDE --- python/nav/models/fields.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/nav/models/fields.py b/python/nav/models/fields.py index 1f7bfa2316..cabecc557d 100644 --- a/python/nav/models/fields.py +++ b/python/nav/models/fields.py @@ -20,6 +20,7 @@ import json from datetime import datetime from decimal import Decimal +from typing import Optional from django import forms from django.db import models @@ -234,11 +235,11 @@ def __set__(self, instance, value): setattr(instance, self.cache_attr, value) @staticmethod - def get_model_name(obj): + def get_model_name(obj) -> str: return obj._meta.db_table @staticmethod - def get_model_class(table_name): + def get_model_class(table_name) -> Optional[models.Model]: """Returns a Model class based on a database table name""" classmap = {model._meta.db_table: model for model in apps.get_models()} if table_name in classmap: From cfbdd17e290783f0a888725e20a6e8c8f64279c2 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 09:09:34 +0000 Subject: [PATCH 05/18] Use new trail generators in main task view This replaces the usage of the old component trail generating functions with the new ones, and updates the templates accordingly. It also factors out `frag-component-trail.html` to a separate template fragment for clarity. --- python/nav/django/templatetags/maintenance.py | 56 +++++++++++++++++++ python/nav/web/maintenance/views.py | 27 +++------ .../web/templates/maintenance/details.html | 22 +++----- .../maintenance/frag-component-trail.html | 11 ++++ 4 files changed, 82 insertions(+), 34 deletions(-) create mode 100644 python/nav/django/templatetags/maintenance.py create mode 100644 python/nav/web/templates/maintenance/frag-component-trail.html diff --git a/python/nav/django/templatetags/maintenance.py b/python/nav/django/templatetags/maintenance.py new file mode 100644 index 0000000000..e30b5557bd --- /dev/null +++ b/python/nav/django/templatetags/maintenance.py @@ -0,0 +1,56 @@ +# +# Copyright (C) 2024 Sikt +# +# This file is part of Network Administration Visualized (NAV). +# +# NAV is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License version 3 as published by +# the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. You should have received a copy of the GNU General Public +# License along with NAV. If not, see . +# +"""Template tags for the maintenance tool""" +from django import template + +from nav.web.maintenance.utils import MissingComponent + +register = template.Library() + + +@register.filter +def model_verbose_name(model): + """Returns the capitalized verbose name of a model""" + if not model: + return + name = model._meta.verbose_name + # Keep original capitalization, if any, otherwise apply our own + # e.g. don't turn "IP device" into "Ip device", but do turn "room" into "Room" + return name if name[0].isupper() else name.capitalize() + + +@register.filter +def component_name(component): + """Returns an identifying name of a model object used as a maintenance component""" + if not component: + return "" + if hasattr(component, "sysname"): + return component.sysname + if hasattr(component, "handler"): + return component.handler + if isinstance(component, MissingComponent): + return str(component) + return component.pk + + +@register.filter +def component_db_table(component): + """Returns the database table name of a model object used as a maintenance + component. + """ + if not component: + return "" + return component._meta.db_table diff --git a/python/nav/web/maintenance/views.py b/python/nav/web/maintenance/views.py index 35d6dda95e..f13bf703c3 100644 --- a/python/nav/web/maintenance/views.py +++ b/python/nav/web/maintenance/views.py @@ -32,7 +32,11 @@ from nav.web.message import new_message, Messages from nav.web.quickselect import QuickSelect -from nav.web.maintenance.utils import components_for_keys +from nav.web.maintenance.utils import ( + components_for_keys, + get_components, + component_to_trail, +) from nav.web.maintenance.utils import task_component_trails from nav.web.maintenance.utils import get_component_keys, PRIMARY_KEY_INTEGER from nav.web.maintenance.utils import structure_component_data @@ -189,25 +193,8 @@ def historic(request): def view(request, task_id): task = get_object_or_404(MaintenanceTask, pk=task_id) - maint_components = MaintenanceComponent.objects.filter( - maintenance_task=task.id - ).values_list('key', 'value') - - component_keys = { - 'service': [], - 'netbox': [], - 'room': [], - 'location': [], - 'netboxgroup': [], - } - for key, value in maint_components: - if key in PRIMARY_KEY_INTEGER: - value = int(value) - component_keys[key].append(value) - - component_data, _ = components_for_keys(component_keys) - components = structure_component_data(component_data) - component_trail = task_component_trails(component_keys, components) + components = get_components(task) + component_trail = [component_to_trail(c) for c in components] heading = 'Task "%s"' % task.description infodict = infodict_by_state(task) diff --git a/python/nav/web/templates/maintenance/details.html b/python/nav/web/templates/maintenance/details.html index 4c9c5b2d93..49ad48356b 100644 --- a/python/nav/web/templates/maintenance/details.html +++ b/python/nav/web/templates/maintenance/details.html @@ -1,4 +1,5 @@ {% extends "maintenance/base.html" %} +{% load maintenance %} {% block content %} @@ -43,20 +44,13 @@ {% if components %}
    - {% for comp in components %} -
  • - {{ comp.title|capfirst }}: - {% for elem in comp.trail %} - {% if elem.url %} - {{ elem.name }} - {% else %} - {{ elem.name }} - {% endif %} - {% if not forloop.last %} - → - {% endif %} - {% endfor %} -
    • + {% for trail in components %} +
  • + {% with trail|last as component %} + {{ component|model_verbose_name }}: + {% include "maintenance/frag-component-trail.html" %} + {% endwith %} +
    • {% endfor %}
{% else %} diff --git a/python/nav/web/templates/maintenance/frag-component-trail.html b/python/nav/web/templates/maintenance/frag-component-trail.html new file mode 100644 index 0000000000..1b64cb2670 --- /dev/null +++ b/python/nav/web/templates/maintenance/frag-component-trail.html @@ -0,0 +1,11 @@ +{% load maintenance %} +{% for element in trail %} + {% with element|component_name as title %} + {% if element.get_absolute_url %} + {{ title }} + {% else %} + {{ title }} + {% endif %} + {% endwith %} + {% if not forloop.last %} → {% endif %} +{% endfor %} From 1bcbcae828a86ae9dd826e966df3b0b1519ea18c Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 09:10:39 +0000 Subject: [PATCH 06/18] Capitalize `Netbox` verbose name properly We would never refer to an IP device as an `ip device` in the UI, `IP` should always be upper case. --- python/nav/models/manage.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/nav/models/manage.py b/python/nav/models/manage.py index bbe29482e2..d80c849d37 100644 --- a/python/nav/models/manage.py +++ b/python/nav/models/manage.py @@ -282,8 +282,8 @@ class Netbox(models.Model): class Meta(object): db_table = 'netbox' - verbose_name = 'ip device' - verbose_name_plural = 'ip devices' + verbose_name = 'IP device' + verbose_name_plural = 'IP devices' ordering = ('sysname',) def __str__(self): From 59792fa29bfd86bf53fb3a384009249639376d58 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 09:12:44 +0000 Subject: [PATCH 07/18] Implement `get_absolute_url()` for Locations There is a definitive canonical URL for Locations, so we should reflect it in the model - and thereby make it easier for the maintenance task subsystem to make links to location components. --- python/nav/models/manage.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/nav/models/manage.py b/python/nav/models/manage.py index d80c849d37..de0d240f93 100644 --- a/python/nav/models/manage.py +++ b/python/nav/models/manage.py @@ -1155,6 +1155,9 @@ def get_all_rooms(self): locations = self.get_descendants(True) return Room.objects.filter(location__in=locations) + def get_absolute_url(self): + return reverse('location-info', kwargs={'locationid': self.pk}) + class Organization(models.Model, TreeMixin): """From NAV Wiki: The org table defines an organization which is in charge From 4be9588dcb843b08c45f178ce0f09e5a10865328 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 09:13:56 +0000 Subject: [PATCH 08/18] Clean up and sort imports The imports were getting chaotic in `views.py`. Used isort to clean it up. --- python/nav/web/maintenance/views.py | 38 +++++++++++++++++------------ 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/python/nav/web/maintenance/views.py b/python/nav/web/maintenance/views.py index f13bf703c3..00977cf5e1 100644 --- a/python/nav/web/maintenance/views.py +++ b/python/nav/web/maintenance/views.py @@ -1,5 +1,6 @@ # # Copyright (C) 2011 Uninett AS +# Copyright (C) 2024 Sikt # # This file is part of Network Administration Visualized (NAV). # @@ -15,36 +16,41 @@ # import logging - import time from datetime import datetime -from django.db import transaction, connection +from django.db import connection, transaction from django.db.models import Count, Q -from django.shortcuts import render, get_object_or_404, redirect from django.http import HttpResponseRedirect +from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse from django.utils.safestring import mark_safe +import nav.maintengine from nav.django.utils import get_account from nav.models.manage import Netbox -from nav.models.msgmaint import MaintenanceTask, MaintenanceComponent -from nav.web.message import new_message, Messages -from nav.web.quickselect import QuickSelect - +from nav.models.msgmaint import MaintenanceComponent, MaintenanceTask +from nav.web.maintenance.forms import ( + MaintenanceAddSingleNetbox, + MaintenanceCalendarForm, + MaintenanceTaskForm, +) from nav.web.maintenance.utils import ( + NAVPATH, + PRIMARY_KEY_INTEGER, + TITLE, + MaintenanceCalendar, + component_to_trail, components_for_keys, + get_component_keys, get_components, - component_to_trail, + infodict_by_state, + structure_component_data, + task_component_trails, + task_form_initial, ) -from nav.web.maintenance.utils import task_component_trails -from nav.web.maintenance.utils import get_component_keys, PRIMARY_KEY_INTEGER -from nav.web.maintenance.utils import structure_component_data -from nav.web.maintenance.utils import task_form_initial, infodict_by_state -from nav.web.maintenance.utils import MaintenanceCalendar, NAVPATH, TITLE -from nav.web.maintenance.forms import MaintenanceTaskForm, MaintenanceCalendarForm -from nav.web.maintenance.forms import MaintenanceAddSingleNetbox -import nav.maintengine +from nav.web.message import Messages, new_message +from nav.web.quickselect import QuickSelect INFINITY = datetime.max From 2863230027a6f9764fa7f79b07b36d924907d0ea Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 09:37:57 +0000 Subject: [PATCH 09/18] Refactor `get_component_keys()` This more or less removes the redundant strings used in the function, by using a list of allowed/expected component keys. --- python/nav/web/maintenance/utils.py | 31 ++++++++++------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/python/nav/web/maintenance/utils.py b/python/nav/web/maintenance/utils.py index 76a9593c7d..40d2f65bd3 100644 --- a/python/nav/web/maintenance/utils.py +++ b/python/nav/web/maintenance/utils.py @@ -28,6 +28,8 @@ from nav.models.service import Service from nav.models.msgmaint import MaintenanceTask, MaintenanceComponent +ALLOWED_COMPONENTS = ('service', 'netbox', 'room', 'location', 'netboxgroup') + PRIMARY_KEY_INTEGER = ('netbox', 'service') FIELD_KEYS = { @@ -157,31 +159,18 @@ def get_components(task: MaintenanceTask) -> Iterator[ComponentType]: def get_component_keys(post): + """Transforms GET/POST data into a dictionary of component keys. + + This would preferably be done by a Django Form class, but the original code + predates Django. + """ remove = {} errors = [] - raw_component_keys = { - 'service': post.getlist('service'), - 'netbox': post.getlist('netbox'), - 'room': post.getlist('room'), - 'location': post.getlist('location'), - 'netboxgroup': post.getlist('netboxgroup'), - } + raw_component_keys = {key: post.getlist(key) for key in ALLOWED_COMPONENTS} raw_component_keys['location'].extend(post.getlist('loc')) if 'remove' in post: - remove = { - 'service': post.getlist('remove_service'), - 'netbox': post.getlist('remove_netbox'), - 'room': post.getlist('remove_room'), - 'location': post.getlist('remove_location'), - 'netboxgroup': post.getlist('remove_netboxgroup'), - } - component_keys = { - 'service': [], - 'netbox': [], - 'room': [], - 'location': [], - 'netboxgroup': [], - } + remove = {key: post.getlist(f"remove_{key}") for key in ALLOWED_COMPONENTS} + component_keys = {key: [] for key in ALLOWED_COMPONENTS} for key in raw_component_keys: for value in raw_component_keys[key]: if not remove or value not in remove[key]: From aad22d15ad610ef55fd14c81284ae1dcbfd552fb Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 11:50:35 +0000 Subject: [PATCH 10/18] Do not join all component errors into single str Add each component error from POST data processing as its own error message. It got very unreadable when a bug caused all component keys to generate errors. --- python/nav/web/maintenance/views.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/nav/web/maintenance/views.py b/python/nav/web/maintenance/views.py index 00977cf5e1..0f6a8403f2 100644 --- a/python/nav/web/maintenance/views.py +++ b/python/nav/web/maintenance/views.py @@ -273,11 +273,11 @@ def edit(request, task_id=None, start_time=None, **_): component_data, component_data_errors = components_for_keys(component_keys) components = structure_component_data(component_data) component_trail = task_component_trails(component_keys, components) - if component_data_errors: - new_message(request, ",".join(component_data_errors), Messages.ERROR) + for error in component_data_errors: + new_message(request, error, Messages.ERROR) - if component_keys_errors: - new_message(request, ",".join(component_keys_errors), Messages.ERROR) + for error in component_keys_errors: + new_message(request, error, Messages.ERROR) if request.method == 'POST': if 'save' in request.POST: From 5180ee2a22fe35f8883833c60cebc3893c5a5c9d Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 11:52:43 +0000 Subject: [PATCH 11/18] Re-use ALLOWED_COMPONENTS for init Why hardcode the list of component keys to put in the data structure when we already have the definitive list in a constant? --- python/nav/web/maintenance/views.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/python/nav/web/maintenance/views.py b/python/nav/web/maintenance/views.py index 0f6a8403f2..7bb1b0101d 100644 --- a/python/nav/web/maintenance/views.py +++ b/python/nav/web/maintenance/views.py @@ -36,6 +36,7 @@ MaintenanceTaskForm, ) from nav.web.maintenance.utils import ( + ALLOWED_COMPONENTS, NAVPATH, PRIMARY_KEY_INTEGER, TITLE, @@ -255,13 +256,7 @@ def edit(request, task_id=None, start_time=None, **_): component_keys, component_keys_errors = get_component_keys(request.POST) elif task: - component_keys = { - 'service': [], - 'netbox': [], - 'room': [], - 'location': [], - 'netboxgroup': [], - } + component_keys = {key: [] for key in ALLOWED_COMPONENTS} for key, value in task.maintenance_components.values_list('key', 'value'): if key in PRIMARY_KEY_INTEGER: value = int(value) From ce3e0f8a2fcc702a083ff84619a89f5516e17ffd Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 11:58:47 +0000 Subject: [PATCH 12/18] Refactor maintenance edit view function This replaces `utils.components_for_keys()` with `get_components_from_keydict()`, which works more like the newer `get_components()`, but works on POST data rather than Task object data. The end result is an edit view that builds the component trails use in the maintenance in much the same way as the refactored `view()` view, which means that the HTML template for the `edit()` view can now also re-use the `frag-component-trail.html` template. --- python/nav/web/maintenance/utils.py | 86 ++++++------------- python/nav/web/maintenance/views.py | 42 ++++----- .../web/templates/maintenance/new_task.html | 34 +++----- 3 files changed, 61 insertions(+), 101 deletions(-) diff --git a/python/nav/web/maintenance/utils.py b/python/nav/web/maintenance/utils.py index 40d2f65bd3..bf2ab02053 100644 --- a/python/nav/web/maintenance/utils.py +++ b/python/nav/web/maintenance/utils.py @@ -24,6 +24,7 @@ from django.urls import reverse from django.utils.html import conditional_escape +from nav.models.fields import LegacyGenericForeignKey from nav.models.manage import Netbox, Room, Location, NetboxGroup from nav.models.service import Service from nav.models.msgmaint import MaintenanceTask, MaintenanceComponent @@ -184,68 +185,37 @@ def get_component_keys(post): return component_keys, errors -def components_for_keys(component_keys): - component_data = {} +def get_components_from_keydict( + component_keys: dict[str, List[Union[int, str]]] +) -> tuple[List[ComponentType], List[str]]: + """Fetches components from a dictionary of component keys, typically as created + from POST data. + + Returns a list of matched components and a list of errors encountered during the + process. Again, this would be better handled by a Django Form class. + """ + components = [] component_data_errors = [] - if component_keys['service']: - component_data['service'] = Service.objects.filter( - id__in=component_keys['service'] - ).values( - 'id', - 'handler', - 'netbox__id', - 'netbox__sysname', - 'netbox__ip', - 'netbox__room__id', - 'netbox__room__description', - 'netbox__room__location__id', - 'netbox__room__location__description', - ) - if not component_data['service']: - component_data_errors.append( - "service: no elements with the given identifiers found" - ) - if component_keys['netbox']: - component_data['netbox'] = Netbox.objects.filter( - id__in=component_keys['netbox'] - ).values( - 'id', - 'sysname', - 'ip', - 'room__id', - 'room__description', - 'room__location__id', - 'room__location__description', + + for key, values in component_keys.items(): + if not values: + continue + model_class = ( + LegacyGenericForeignKey.get_model_class(key) + if key in ALLOWED_COMPONENTS + else None ) - if not component_data['netbox']: - component_data_errors.append( - "netbox: no elements with the given identifiers found" - ) - if component_keys['room']: - component_data['room'] = Room.objects.filter( - id__in=component_keys['room'] - ).values('id', 'description', 'location__id', 'location__description') - if not component_data['room']: - component_data_errors.append( - "room: no elements with the given identifiers found" - ) - if component_keys['location']: - component_data['location'] = Location.objects.filter( - id__in=component_keys['location'] - ).values('id', 'description') - if not component_data['location']: - component_data_errors.append( - "location: no elements with the given identifiers found" - ) - if component_keys['netboxgroup']: - component_data['netboxgroup'] = NetboxGroup.objects.filter( - id__in=component_keys['netboxgroup'] - ).values('id', 'description') - if not component_data['netboxgroup']: + if not model_class: + component_data_errors.append(f"{key}: invalid component type") + continue + + objects = model_class.objects.filter(id__in=component_keys[key]) + components.extend(objects) + if not objects: component_data_errors.append( - "netboxgroup: no elements with the given identifiers found" + f"{key}: no elements with the given identifiers found" ) - return component_data, component_data_errors + return components, component_data_errors def structure_component_data(component_data): diff --git a/python/nav/web/maintenance/views.py b/python/nav/web/maintenance/views.py index 7bb1b0101d..34a27f0125 100644 --- a/python/nav/web/maintenance/views.py +++ b/python/nav/web/maintenance/views.py @@ -36,18 +36,15 @@ MaintenanceTaskForm, ) from nav.web.maintenance.utils import ( - ALLOWED_COMPONENTS, NAVPATH, PRIMARY_KEY_INTEGER, TITLE, MaintenanceCalendar, component_to_trail, - components_for_keys, get_component_keys, get_components, + get_components_from_keydict, infodict_by_state, - structure_component_data, - task_component_trails, task_form_initial, ) from nav.web.message import Messages, new_message @@ -246,7 +243,9 @@ def cancel(request, task_id): def edit(request, task_id=None, start_time=None, **_): account = get_account(request) quickselect = QuickSelect(service=True) - component_trail = component_keys_errors = component_data = task = None + components = task = None + component_keys_errors = [] + component_keys = {} if task_id: task = get_object_or_404(MaintenanceTask, pk=task_id) @@ -254,30 +253,25 @@ def edit(request, task_id=None, start_time=None, **_): if request.method == 'POST': component_keys, component_keys_errors = get_component_keys(request.POST) - elif task: - component_keys = {key: [] for key in ALLOWED_COMPONENTS} - for key, value in task.maintenance_components.values_list('key', 'value'): - if key in PRIMARY_KEY_INTEGER: - value = int(value) - component_keys[key].append(value) + components = get_components(task) else: component_keys, component_keys_errors = get_component_keys(request.GET) if component_keys: - component_data, component_data_errors = components_for_keys(component_keys) - components = structure_component_data(component_data) - component_trail = task_component_trails(component_keys, components) + components, component_data_errors = get_components_from_keydict(component_keys) for error in component_data_errors: new_message(request, error, Messages.ERROR) + component_trail = [component_to_trail(c) for c in components] + for error in component_keys_errors: new_message(request, error, Messages.ERROR) if request.method == 'POST': if 'save' in request.POST: task_form = MaintenanceTaskForm(request.POST) - if component_keys and not any(component_data.values()): + if component_keys and not components: new_message(request, "No components selected.", Messages.ERROR) elif not component_keys_errors and task_form.is_valid(): start_time = task_form.cleaned_data['start_time'] @@ -309,14 +303,16 @@ def edit(request, task_id=None, start_time=None, **_): sql = """DELETE FROM maint_component WHERE maint_taskid = %s""" cursor.execute(sql, (new_task.id,)) - for key in component_data: - for component in component_data[key]: - task_component = MaintenanceComponent( - maintenance_task=new_task, - key=key, - value="%s" % component['id'], - ) - task_component.save() + for component in components: + table = component._meta.db_table + descr = str(component) if table in PRIMARY_KEY_INTEGER else None + task_component = MaintenanceComponent( + maintenance_task=new_task, + key=table, + value=component.pk, + description=descr, + ) + task_component.save() new_message( request, "Saved task %s" % new_task.description, Messages.SUCCESS ) diff --git a/python/nav/web/templates/maintenance/new_task.html b/python/nav/web/templates/maintenance/new_task.html index 526f99a876..1fed9ce69c 100644 --- a/python/nav/web/templates/maintenance/new_task.html +++ b/python/nav/web/templates/maintenance/new_task.html @@ -1,4 +1,5 @@ {% extends "maintenance/base.html" %} +{% load maintenance %} {% block base_header_additional_head %} {{ block.super }} @@ -57,26 +58,19 @@

{{ heading }}

{% if components %}
    - {% for comp in components %} -
  • -
    - - - {{ comp.title|capfirst }}: -
    -
    - {% for elem in comp.trail %} - {% if elem.url %} - {{ elem.name }} - {% else %} - {{ elem.name }} - {% endif %} - {% if not forloop.last %} - → - {% endif %} - {% endfor %} -
    -
    • + {% for trail in components %} + {% with trail|last as component %} +
  • +
    + + + {{ component|model_verbose_name }}: +
    +
    + {% include "maintenance/frag-component-trail.html" %} +
    +
    • + {% endwith %} {% endfor %}
Date: Thu, 21 Nov 2024 12:01:31 +0000 Subject: [PATCH 13/18] Remove now obsolete functions and data structures These are no longer used by any code. --- python/nav/web/maintenance/utils.py | 122 ---------------------------- 1 file changed, 122 deletions(-) diff --git a/python/nav/web/maintenance/utils.py b/python/nav/web/maintenance/utils.py index bf2ab02053..59b2ed9ad8 100644 --- a/python/nav/web/maintenance/utils.py +++ b/python/nav/web/maintenance/utils.py @@ -33,26 +33,6 @@ PRIMARY_KEY_INTEGER = ('netbox', 'service') -FIELD_KEYS = { - 'service': { - 'netbox': 'netbox__', - 'room': 'netbox__room__', - 'location': 'netbox__room__location__', - }, - 'netbox': { - 'netbox': '', - 'room': 'room__', - 'location': 'room__location__', - }, - 'room': { - 'room': '', - 'location': 'location__', - }, - 'location': { - 'location': '', - }, -} - NAVPATH = [ ('Home', '/'), ('Maintenance', '/maintenance/'), @@ -218,108 +198,6 @@ def get_components_from_keydict( return components, component_data_errors -def structure_component_data(component_data): - components = {} - for key in component_data: - for component in component_data[key]: - pkey = component['id'] - if key not in components: - components[key] = {} - components[key][pkey] = component - return components - - -def task_component_trails(component_keys, components): - """Create the 'trail' of selected components - - An IP Device would have a trail consisting of a location, room and the - device itself, and a room would have a trail consisting of a location and - the room itself. - - Ex: - IP Device: -> -> - """ - - # Mapping for changing the title of the trail. - title_mapping = {'netbox': 'IP Device', 'netboxgroup': 'Device Group'} - - trails = [] - for key in component_keys: - title = title_mapping.get(key, key) - for pkey in component_keys[key]: - trail = [] - try: - comp = components[key][pkey] - except KeyError: - trail.append( - { - 'url': None, - 'title': None, - 'name': "ID %s (Component was deleted)" % pkey, - } - ) - else: - if key in ('location', 'room', 'netbox', 'service'): - location_id = comp[FIELD_KEYS[key]['location'] + "id"] - location_description = comp[ - FIELD_KEYS[key]['location'] + "description" - ] - trail.append( - { - 'url': reverse('location-info', args=[location_id]), - 'title': location_description, - 'name': location_id, - } - ) - if key in ('room', 'netbox', 'service'): - room_id = comp[FIELD_KEYS[key]['room'] + "id"] - room_description = comp[FIELD_KEYS[key]['room'] + "description"] - trail.append( - { - 'url': reverse('room-info', args=[room_id]), - 'title': room_description, - 'name': room_id, - } - ) - if key in ('netbox', 'service'): - netbox_sysname = comp[FIELD_KEYS[key]['netbox'] + "sysname"] - netbox_ip = comp[FIELD_KEYS[key]['netbox'] + "ip"] - trail.append( - { - 'url': reverse( - 'ipdevinfo-details-by-name', args=[netbox_sysname] - ), - 'title': netbox_ip, - 'name': netbox_sysname, - } - ) - if key == 'service': - trail.append( - { - 'url': None, - 'title': None, - 'name': comp['handler'], - } - ) - if key == 'netboxgroup': - trail.append( - { - 'url': reverse('netbox-group-detail', args=[comp['id']]), - 'title': '', - 'name': comp['id'], - } - ) - trails.append( - { - 'id': pkey, - 'type': key, - 'title': title, - 'trail': trail, - } - ) - return trails - - class MaintenanceCalendar(HTMLCalendar): START = 'start' END = 'end' From a84a8666bb99f6bf11d88c57e88f2c0c266260b4 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 12:18:09 +0000 Subject: [PATCH 14/18] Stop hardcoding integer primary key component list It's stupid to keep a hardcoded list of component types that only accept integer primary keys, when can be detected from the corresponding model definitions. This takes away the redundancy. --- python/nav/web/maintenance/utils.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/python/nav/web/maintenance/utils.py b/python/nav/web/maintenance/utils.py index 59b2ed9ad8..2150ec1f03 100644 --- a/python/nav/web/maintenance/utils.py +++ b/python/nav/web/maintenance/utils.py @@ -21,6 +21,7 @@ from typing import Union, List, Iterator from django.db import models +from django.db.models import IntegerField from django.urls import reverse from django.utils.html import conditional_escape @@ -31,7 +32,14 @@ ALLOWED_COMPONENTS = ('service', 'netbox', 'room', 'location', 'netboxgroup') -PRIMARY_KEY_INTEGER = ('netbox', 'service') +PRIMARY_KEY_INTEGER = [ + table + for table in ALLOWED_COMPONENTS + if isinstance( + LegacyGenericForeignKey.get_model_class(table)._meta.get_field('id'), + IntegerField, + ) +] NAVPATH = [ ('Home', '/'), From 954b8898f1bad359050d8bd0000dfca00cc73318 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Thu, 21 Nov 2024 13:32:33 +0000 Subject: [PATCH 15/18] Add news fragment --- changelog.d/3228.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3228.fixed.md diff --git a/changelog.d/3228.fixed.md b/changelog.d/3228.fixed.md new file mode 100644 index 0000000000..efc20be0a8 --- /dev/null +++ b/changelog.d/3228.fixed.md @@ -0,0 +1 @@ +Improve description of deleted maintenance components From 76b7aaf5419b6d6e883debe139d8970b76e3d8d1 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Mon, 25 Nov 2024 11:29:43 +0000 Subject: [PATCH 16/18] Rename PRIMARY_KEY_INTEGER constant As mentioned in code review, the original name doesn't convey much to new devs. --- python/nav/web/maintenance/utils.py | 4 ++-- python/nav/web/maintenance/views.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/python/nav/web/maintenance/utils.py b/python/nav/web/maintenance/utils.py index 2150ec1f03..19903234e9 100644 --- a/python/nav/web/maintenance/utils.py +++ b/python/nav/web/maintenance/utils.py @@ -32,7 +32,7 @@ ALLOWED_COMPONENTS = ('service', 'netbox', 'room', 'location', 'netboxgroup') -PRIMARY_KEY_INTEGER = [ +COMPONENTS_WITH_INTEGER_PK = [ table for table in ALLOWED_COMPONENTS if isinstance( @@ -163,7 +163,7 @@ def get_component_keys(post): for key in raw_component_keys: for value in raw_component_keys[key]: if not remove or value not in remove[key]: - if key in PRIMARY_KEY_INTEGER: + if key in COMPONENTS_WITH_INTEGER_PK: if not value.isdigit(): errors.append(key + ": argument needs to be a number") continue diff --git a/python/nav/web/maintenance/views.py b/python/nav/web/maintenance/views.py index 34a27f0125..449f97ce23 100644 --- a/python/nav/web/maintenance/views.py +++ b/python/nav/web/maintenance/views.py @@ -37,7 +37,7 @@ ) from nav.web.maintenance.utils import ( NAVPATH, - PRIMARY_KEY_INTEGER, + COMPONENTS_WITH_INTEGER_PK, TITLE, MaintenanceCalendar, component_to_trail, @@ -305,7 +305,9 @@ def edit(request, task_id=None, start_time=None, **_): cursor.execute(sql, (new_task.id,)) for component in components: table = component._meta.db_table - descr = str(component) if table in PRIMARY_KEY_INTEGER else None + descr = ( + str(component) if table in COMPONENTS_WITH_INTEGER_PK else None + ) task_component = MaintenanceComponent( maintenance_task=new_task, key=table, From f003866a7406b38521d15a030c0646200ac6a09e Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Mon, 25 Nov 2024 11:47:56 +0000 Subject: [PATCH 17/18] Display component title popups properly As noticed in code review, the refactored version of the maintenance UI did not display the same descriptive title tooltips on component trails as the original did. This solves the issue by adding a separate `component_description` template tag. --- python/nav/django/templatetags/maintenance.py | 12 ++++++++++++ .../templates/maintenance/frag-component-trail.html | 6 +++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/python/nav/django/templatetags/maintenance.py b/python/nav/django/templatetags/maintenance.py index e30b5557bd..799b1d0e5f 100644 --- a/python/nav/django/templatetags/maintenance.py +++ b/python/nav/django/templatetags/maintenance.py @@ -46,6 +46,18 @@ def component_name(component): return component.pk +@register.filter +def component_description(component): + """Returns a description of a component useful as a link title tooltip. Returns + an empty string if there is no further description than the component's name. + """ + if isinstance(component, MissingComponent): + return str(component) + if hasattr(component, "ip"): + return str(component.ip) + return getattr(component, "description", "") + + @register.filter def component_db_table(component): """Returns the database table name of a model object used as a maintenance diff --git a/python/nav/web/templates/maintenance/frag-component-trail.html b/python/nav/web/templates/maintenance/frag-component-trail.html index 1b64cb2670..fc91b656cc 100644 --- a/python/nav/web/templates/maintenance/frag-component-trail.html +++ b/python/nav/web/templates/maintenance/frag-component-trail.html @@ -1,10 +1,10 @@ {% load maintenance %} {% for element in trail %} - {% with element|component_name as title %} + {% with element|component_name as name %} {% if element.get_absolute_url %} - {{ title }} + {{ name }} {% else %} - {{ title }} + {{ name }} {% endif %} {% endwith %} {% if not forloop.last %} → {% endif %} From b2b92d3c8589b8a53d42bacab41182a3f81ddc42 Mon Sep 17 00:00:00 2001 From: Morten Brekkevold Date: Fri, 29 Nov 2024 09:46:02 +0100 Subject: [PATCH 18/18] Apply suggestions from code review Adding minor, but useful suggestions from code review Co-authored-by: Johanna England --- python/nav/django/templatetags/maintenance.py | 2 +- python/nav/models/manage.py | 4 ++-- python/nav/web/maintenance/utils.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/python/nav/django/templatetags/maintenance.py b/python/nav/django/templatetags/maintenance.py index 799b1d0e5f..e30a89e4ad 100644 --- a/python/nav/django/templatetags/maintenance.py +++ b/python/nav/django/templatetags/maintenance.py @@ -28,7 +28,7 @@ def model_verbose_name(model): return name = model._meta.verbose_name # Keep original capitalization, if any, otherwise apply our own - # e.g. don't turn "IP device" into "Ip device", but do turn "room" into "Room" + # e.g. don't turn "IP Device" into "Ip device", but do turn "room" into "Room" return name if name[0].isupper() else name.capitalize() diff --git a/python/nav/models/manage.py b/python/nav/models/manage.py index de0d240f93..4d2a4ac597 100644 --- a/python/nav/models/manage.py +++ b/python/nav/models/manage.py @@ -282,8 +282,8 @@ class Netbox(models.Model): class Meta(object): db_table = 'netbox' - verbose_name = 'IP device' - verbose_name_plural = 'IP devices' + verbose_name = 'IP Device' + verbose_name_plural = 'IP Devices' ordering = ('sysname',) def __str__(self): diff --git a/python/nav/web/maintenance/utils.py b/python/nav/web/maintenance/utils.py index 19903234e9..4f275158cc 100644 --- a/python/nav/web/maintenance/utils.py +++ b/python/nav/web/maintenance/utils.py @@ -197,7 +197,7 @@ def get_components_from_keydict( component_data_errors.append(f"{key}: invalid component type") continue - objects = model_class.objects.filter(id__in=component_keys[key]) + objects = model_class.objects.filter(id__in=values) components.extend(objects) if not objects: component_data_errors.append(