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 diff --git a/python/nav/django/templatetags/maintenance.py b/python/nav/django/templatetags/maintenance.py new file mode 100644 index 0000000000..e30a89e4ad --- /dev/null +++ b/python/nav/django/templatetags/maintenance.py @@ -0,0 +1,68 @@ +# +# 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_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 + component. + """ + if not component: + return "" + return component._meta.db_table 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: diff --git a/python/nav/models/manage.py b/python/nav/models/manage.py index bbe29482e2..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): @@ -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 diff --git a/python/nav/models/msgmaint.py b/python/nav/models/msgmaint.py index 77496fd8bd..1b3bcb30e6 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): @@ -190,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 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; diff --git a/python/nav/web/maintenance/utils.py b/python/nav/web/maintenance/utils.py index c512df60f8..4f275158cc 100644 --- a/python/nav/web/maintenance/utils.py +++ b/python/nav/web/maintenance/utils.py @@ -15,37 +15,31 @@ # 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.db.models import IntegerField 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 - -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': '', - }, -} +from nav.models.msgmaint import MaintenanceTask, MaintenanceComponent + +ALLOWED_COMPONENTS = ('service', 'netbox', 'room', 'location', 'netboxgroup') + +COMPONENTS_WITH_INTEGER_PK = [ + table + for table in ALLOWED_COMPONENTS + if isinstance( + LegacyGenericForeignKey.get_model_class(table)._meta.get_field('id'), + IntegerField, + ) +] NAVPATH = [ ('Home', '/'), @@ -96,36 +90,80 @@ 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): + """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]: - 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 @@ -135,170 +173,37 @@ def get_component_keys(post): return component_keys, errors -def components_for_keys(component_keys): - component_data = {} - 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', - ) - 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']: - component_data_errors.append( - "netboxgroup: no elements with the given identifiers found" - ) - return component_data, component_data_errors +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. - -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: -> -> + 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 = [] - # 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, - } + 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 model_class: + component_data_errors.append(f"{key}: invalid component type") + continue + + objects = model_class.objects.filter(id__in=values) + components.extend(objects) + if not objects: + component_data_errors.append( + f"{key}: no elements with the given identifiers found" ) - return trails + return components, component_data_errors class MaintenanceCalendar(HTMLCalendar): diff --git a/python/nav/web/maintenance/views.py b/python/nav/web/maintenance/views.py index 35d6dda95e..449f97ce23 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,33 +16,40 @@ # 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.models.msgmaint import MaintenanceComponent, MaintenanceTask +from nav.web.maintenance.forms import ( + MaintenanceAddSingleNetbox, + MaintenanceCalendarForm, + MaintenanceTaskForm, +) +from nav.web.maintenance.utils import ( + NAVPATH, + COMPONENTS_WITH_INTEGER_PK, + TITLE, + MaintenanceCalendar, + component_to_trail, + get_component_keys, + get_components, + get_components_from_keydict, + infodict_by_state, + task_form_initial, +) +from nav.web.message import Messages, new_message from nav.web.quickselect import QuickSelect -from nav.web.maintenance.utils import components_for_keys -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 - INFINITY = datetime.max _logger = logging.getLogger(__name__) @@ -189,25 +197,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) @@ -252,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) @@ -260,36 +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 = { - 'service': [], - 'netbox': [], - 'room': [], - 'location': [], - 'netboxgroup': [], - } - 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) - if component_data_errors: - new_message(request, ",".join(component_data_errors), Messages.ERROR) + 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] - 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: 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'] @@ -321,14 +303,18 @@ 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 COMPONENTS_WITH_INTEGER_PK 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/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..fc91b656cc --- /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 name %} + {% if element.get_absolute_url %} + {{ name }} + {% else %} + {{ name }} + {% endif %} + {% endwith %} + {% if not forloop.last %} → {% endif %} +{% endfor %} 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 %}