From 0cbef02b90e7cb17149d1b5b5dd0f2948468f65d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20D=C3=B6tsch?= Date: Fri, 9 Oct 2026 11:56:30 +0000 Subject: [PATCH 1/2] servershell: support multiple objects in history and changes - history view accepts multiple object_id/hostname params - add hostname column linking to inspect page - changes command/view use all selected objects - fix empty "Time Since" and recreate object_id on deleted rows --- .../serverdb/templates/serverdb/history.html | 19 ++- .../serveradmin/serverdb/tests/test_views.py | 91 +++++++++++++ .../serveradmin/serveradmin/serverdb/views.py | 120 ++++++++++++------ .../static/js/servershell/command.js | 10 +- .../servershell/modals/help_command.html | 4 +- 5 files changed, 190 insertions(+), 54 deletions(-) diff --git a/packages/serveradmin/serveradmin/serverdb/templates/serverdb/history.html b/packages/serveradmin/serveradmin/serverdb/templates/serverdb/history.html index c9633b082..29b3193e8 100644 --- a/packages/serveradmin/serveradmin/serverdb/templates/serverdb/history.html +++ b/packages/serveradmin/serveradmin/serverdb/templates/serverdb/history.html @@ -11,6 +11,9 @@

History for {{ name }}

+ {% if hostnames|length > 1 %} +

{{ hostnames|join:", " }}

+ {% endif %}
@@ -22,7 +25,7 @@

History for {{ name }}

    {% for attribute in no_history_attributes %} -
  • {{ attribute.attribute_id }}
    • +
  • {{ attribute }}
    • {% endfor %}
@@ -34,7 +37,9 @@

History for {{ name }}

- + {% for object_id in object_ids %} + + {% endfor %}
@@ -46,6 +51,7 @@

History for {{ name }}

Commit + Hostname Date Time Since App @@ -62,6 +68,7 @@

History for {{ name }}

{% if not attribute_filter or attribute_filter == key %} {{ change.commit.id }} + {{ change.hostname|default:change.object_id }} {{ change.commit.change_on|date:"r" }} {{ change.commit.change_on|timesince }} {{ change.commit.app|default:"Servershell" }} @@ -88,6 +95,7 @@

History for {{ name }}

{% if not attribute_filter or attribute_filter == key %} {{ change.commit.id }} + {{ change.hostname|default:change.object_id }} {{ change.commit.change_on|date:"r" }} {{ change.commit.change_on|timesince }} {{ change.commit.app|default:"Servershell" }} @@ -127,14 +135,15 @@

History for {{ name }}

{% elif change.change_type == 'delete' %} {{ change.commit.id }} + {{ change.hostname|default:change.object_id }} {{ change.commit.change_on|date:"r" }} - {{ change.channge_commit.change_on|timesince }} + {{ change.commit.change_on|timesince }} {{ change.commit.app|default:"Servershell" }} {{ change.commit.user }}
{% csrf_token %} - +
@@ -146,7 +155,7 @@

History for {{ name }}

{% if commit_id %} - Show complete history + Show complete history {% endif %} diff --git a/packages/serveradmin/serveradmin/serverdb/tests/test_views.py b/packages/serveradmin/serveradmin/serverdb/tests/test_views.py index 59cc652bd..3a5a6bf22 100644 --- a/packages/serveradmin/serveradmin/serverdb/tests/test_views.py +++ b/packages/serveradmin/serveradmin/serverdb/tests/test_views.py @@ -3,6 +3,7 @@ from django.contrib.auth.models import User from django.test import TransactionTestCase +from adminapi.filters import Any from serveradmin.dataset import Query from serveradmin.serverdb.models import Change @@ -66,3 +67,93 @@ def test_recreate_fails_if_hostname_exists(self): self.assertEqual(200, response.status_code) self.assertEqual('serverdb/changes.html', response.template_name) self.assertEqual(302, response.redirect_chain[0][1]) + + +class HistoryViewTest(TransactionTestCase): + fixtures = ['auth_user.json', 'test_dataset.json'] + + def setUp(self) -> None: + self.client.login(username='hannah.acker', password='hannah.acker') + + self.object_ids = [] + for i in (1, 2): + vm = Query().new_object('vm') + vm['hostname'] = f'test-serverdb-history-{i}' + vm['intern_ip'] = ip_address(f'10.0.0.{i}') + vm.commit(user=User.objects.first()) + self.object_ids.append( + Query({'hostname': f'test-serverdb-history-{i}'}).get()[ + 'object_id']) + + vms = Query({'object_id': Any(*self.object_ids)}, ['intern_ip']) + for i, vm in enumerate(vms, start=11): + vm['intern_ip'] = ip_address(f'10.0.0.{i}') + vms.commit(user=User.objects.first()) + + def _url(self, *object_ids, **params): + query = '&'.join(f'object_id={i}' for i in object_ids) + for key, value in params.items(): + query += f'&{key}={value}' + return f'/serverdb/history?{query}' + + def test_multiple_objects(self): + response = self.client.get(self._url(*self.object_ids)) + + self.assertEqual(200, response.status_code) + self.assertEqual('2 objects', response.context['name']) + changes = list(response.context['changes']) + self.assertEqual( + set(self.object_ids), {c.object_id for c in changes}) + self.assertEqual( + {'test-serverdb-history-1', 'test-serverdb-history-2'}, + {c.hostname for c in changes}) + + def test_single_object(self): + response = self.client.get(self._url(self.object_ids[0])) + + self.assertEqual(200, response.status_code) + self.assertEqual('test-serverdb-history-1', response.context['name']) + self.assertEqual( + {self.object_ids[0]}, + {c.object_id for c in response.context['changes']}) + + def test_attribute_filter(self): + response = self.client.get( + self._url(*self.object_ids, attribute_filter='intern_ip')) + + changes = list(response.context['changes']) + self.assertTrue(changes) + self.assertTrue(all('intern_ip' in c.change_json for c in changes)) + self.assertEqual( + set(self.object_ids), {c.object_id for c in changes}) + + def test_missing_object_is_skipped(self): + response = self.client.get(self._url(self.object_ids[0], 999999999)) + + self.assertEqual(200, response.status_code) + self.assertEqual( + [self.object_ids[0]], response.context['object_ids']) + self.assertIn( + '999999999', + ' '.join(str(m) for m in response.context['messages'])) + + def test_only_missing_objects_redirects(self): + response = self.client.get(self._url(999999999), follow=True) + + self.assertEqual('serverdb/changes.html', response.template_name) + self.assertEqual(302, response.redirect_chain[0][1]) + + def test_no_parameter(self): + response = self.client.get('/serverdb/history') + self.assertEqual(400, response.status_code) + + def test_changes_multiple_objects(self): + object_ids = ','.join(str(i) for i in self.object_ids) + response = self.client.get( + f'/serverdb/changes?object_id={object_ids}') + + self.assertEqual(200, response.status_code) + commit_ids = [c.id for c in response.context['commits']] + # Two creates and one shared update commit, without duplicates + self.assertEqual(3, len(commit_ids)) + self.assertEqual(len(commit_ids), len(set(commit_ids))) diff --git a/packages/serveradmin/serveradmin/serverdb/views.py b/packages/serveradmin/serveradmin/serverdb/views.py index d16b6582f..e33afdc0b 100644 --- a/packages/serveradmin/serveradmin/serverdb/views.py +++ b/packages/serveradmin/serveradmin/serverdb/views.py @@ -8,7 +8,7 @@ from django.contrib.auth.decorators import login_required from django.core.exceptions import ValidationError from django.core.paginator import Paginator -from django.db.models import Q, Subquery, OuterRef, Prefetch, CharField +from django.db.models import Q, Subquery, OuterRef, Prefetch, CharField, QuerySet from django.db.models.fields.json import KeyTextTransform from django.db.models.functions import Coalesce, Cast from django.http import HttpResponseBadRequest @@ -27,6 +27,44 @@ from serveradmin.serverdb.query_committer import CommitError, commit_query +def _parse_object_ids(values: list[str]) -> list[int]: + """Parse object ids from (possibly comma separated) request values + + Invalid entries are silently ignored. + """ + object_ids = [] + for value in values: + for object_id in value.split(','): + object_id = object_id.strip() + if object_id.isdigit() and int(object_id) not in object_ids: + object_ids.append(int(object_id)) + + return object_ids + + +def _annotate_hostname(changes: QuerySet) -> QuerySet: + """Annotate changes with the hostname of their object + + The hostname is fetched either from the server table or from the change + entry where the object was deleted. + """ + server_hostname = Server.objects.filter( + server_id=OuterRef('object_id')).values('hostname') + # Workaround: In previous versions of Serveradmin restoring objects with + # the same id was possible, so we can end up with objects being deleted + # multiple times. We only take the latest into account. + change_hostname = Change.objects.filter( + object_id=OuterRef('object_id'), + change_type=Change.Type.DELETE).values('change_json').order_by('-id')[:1] + + return changes.annotate(hostname=Coalesce( + Subquery(server_hostname), + Cast( + KeyTextTransform('hostname', Subquery(change_hostname)), + output_field=CharField()) + )) + + @login_required def changes(request): commits = ChangeCommit.objects.all().order_by('-change_on') @@ -57,9 +95,11 @@ def changes(request): 'change_json__object_id', flat=True)) commits = commits.filter(change__object_id__in=object_ids) - f_object_id = request.GET.get('object_id') - if f_object_id: - commits = commits.filter(change__object_id=f_object_id) + f_object_ids = _parse_object_ids(request.GET.getlist('object_id')) + f_object_id = ','.join(str(i) for i in f_object_ids) + if f_object_ids: + commits = commits.filter( + change__object_id__in=f_object_ids).distinct() f_user_or_app = request.GET.get('user_or_app') if f_user_or_app: @@ -75,26 +115,8 @@ def changes(request): f_attribute = None commits = commits.select_related('app', 'user') - - # This complex statement is just here to be able to prefetch the changes' - # relation including the hostname fetched either from the server table - # or from the change entry where the object was deleted. - server_hostname = Server.objects.filter( - server_id=OuterRef('object_id')).values('hostname') - # Workaround: In previous versions of Serveradmin restoring objects with - # the same id was possible, so we can end up with objects being deleted - # multiple times. We only take the latest into account. - change_hostname = Change.objects.filter( - object_id=OuterRef('object_id'), - change_type=Change.Type.DELETE).values('change_json').order_by('-id')[:1] commits = commits.prefetch_related(Prefetch( - 'change_set', - queryset=Change.objects.all().annotate(hostname=Coalesce( - Subquery(server_hostname), - Cast( - KeyTextTransform('hostname', Subquery(change_hostname)), - output_field=CharField()) - )))) + 'change_set', queryset=_annotate_hostname(Change.objects.all()))) class NoCountPaginator(Paginator): @cached_property @@ -122,25 +144,31 @@ def count(self): @login_required def history(request): - if 'object_id' in request.GET: - query = Q(server_id=request.GET['object_id']) - error_text = f'object_id {request.GET["object_id"]}' - elif 'hostname' in request.GET: - query = Q(hostname=request.GET['hostname']) - error_text = f'hostname {request.GET["hostname"]}' - else: + object_ids = _parse_object_ids(request.GET.getlist('object_id')) + hostnames = request.GET.getlist('hostname') + if not object_ids and not hostnames: return HttpResponseBadRequest( 'object_id or hostname parameter is mandatory') - try: - server = Server.objects.get(query) - except Server.DoesNotExist: + servers = list(Server.objects.filter( + Q(server_id__in=object_ids) | Q(hostname__in=hostnames) + ).order_by('hostname')) + + missing = [str(i) for i in object_ids + if i not in {s.server_id for s in servers}] + missing += [h for h in hostnames if h not in {s.hostname for s in servers}] + if not servers: messages.error( request, - f'Object with {error_text} does not or no longer exist.') + f'Object(s) {", ".join(missing)} do not or no longer exist.') return redirect(reverse('serverdb_changes')) + if missing: + messages.warning( + request, + f'Object(s) {", ".join(missing)} do not or no longer exist.') - obj_history = Change.objects.filter(object_id=server.server_id) + server_ids = [s.server_id for s in servers] + obj_history = Change.objects.filter(object_id__in=server_ids) commit_id = request.GET.get('commit_id') if commit_id: @@ -150,24 +178,32 @@ def history(request): if attribute_filter: obj_history = obj_history.filter(change_json__has_key=attribute_filter) - obj_history = obj_history.order_by('-commit_id') + obj_history = obj_history.order_by('-commit_id', '-id') obj_history = obj_history.select_related('commit__app', 'commit__user') + obj_history = _annotate_hostname(obj_history) page = request.GET.get('page', 1) pager = Paginator(obj_history, 25) page_obj = pager.get_page(page) no_history_attributes = ServertypeAttribute.objects.filter( - servertype_id=server.servertype_id - ).select_related('attribute').filter( - attribute__history=False - ).only('attribute_id') + servertype_id__in={s.servertype_id for s in servers}, + attribute__history=False, + ).order_by('attribute_id').values_list( + 'attribute_id', flat=True).distinct() + + if len(servers) == 1: + name = servers[0].hostname + else: + name = f'{len(servers)} objects' return TemplateResponse(request, 'serverdb/history.html', { 'changes': page_obj, 'commit_id': commit_id, - 'object_id': server.server_id, - 'name': server.hostname, + 'object_ids': server_ids, + 'object_ids_query': '&'.join(f'object_id={i}' for i in server_ids), + 'name': name, + 'hostnames': [s.hostname for s in servers], 'attribute_filter': attribute_filter, 'no_history_attributes': no_history_attributes, }) diff --git a/packages/serveradmin/serveradmin/servershell/static/js/servershell/command.js b/packages/serveradmin/serveradmin/servershell/static/js/servershell/command.js index 5d6339949..d0b789527 100644 --- a/packages/serveradmin/serveradmin/servershell/static/js/servershell/command.js +++ b/packages/serveradmin/serveradmin/servershell/static/js/servershell/command.js @@ -427,7 +427,7 @@ servershell.commands = { let url; let selection = servershell.get_selected(); if (selection.length > 0) { - url = servershell.urls.changes + `?object_id=${selection[0]}`; + url = servershell.urls.changes + `?object_id=${selection.join(',')}`; } else { url = servershell.urls.changes; @@ -653,12 +653,12 @@ servershell.commands = { } }, history: function(attribute_id) { - if (!validate_selected(1, 1)) { + if (!validate_selected(1, 100)) { return; } - let object_id = servershell.get_selected()[0]; - let url = servershell.urls.history + `?object_id=${object_id}`; + let selection = servershell.get_selected(); + let url = servershell.urls.history + '?' + selection.map(o => `object_id=${o}`).join('&'); if (attribute_id) { url += `&attribute_filter=${attribute_id}`; @@ -666,7 +666,7 @@ servershell.commands = { window.open(url, '_blank'); - servershell.alert(`History for ${object_id} opened in a new tab`, 'success'); + servershell.alert(`History for ${selection.length} object(s) opened in a new tab`, 'success'); }, commit: function() { spinner.enable('commit'); diff --git a/packages/serveradmin/serveradmin/servershell/templates/servershell/modals/help_command.html b/packages/serveradmin/serveradmin/servershell/templates/servershell/modals/help_command.html index 3dda99149..f6ff73868 100644 --- a/packages/serveradmin/serveradmin/servershell/templates/servershell/modals/help_command.html +++ b/packages/serveradmin/serveradmin/servershell/templates/servershell/modals/help_command.html @@ -244,13 +244,13 @@

Available commands

changes none - Show changes to server database + Show changes to server database (of selected objects if any) history [attribute] - Show history of selected host. + Show history of selected objects. If attribute is given only changes for attribute are shown. From 0c81fd994a922c7f079dae96d13c1d2f777d1137 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20D=C3=B6tsch?= Date: Mon, 12 Oct 2026 13:17:57 +0000 Subject: [PATCH 2/2] first review changes, still WIP --- .../serverdb/templates/serverdb/changes.html | 6 +++- .../serverdb/templates/serverdb/history.html | 4 +-- .../serveradmin/serverdb/tests/test_views.py | 21 ++++++++--- .../serveradmin/serveradmin/serverdb/views.py | 36 ++++++------------- .../static/js/servershell/command.js | 2 +- 5 files changed, 34 insertions(+), 35 deletions(-) diff --git a/packages/serveradmin/serveradmin/serverdb/templates/serverdb/changes.html b/packages/serveradmin/serveradmin/serverdb/templates/serverdb/changes.html index 064f26e37..4952cc40e 100644 --- a/packages/serveradmin/serveradmin/serverdb/templates/serverdb/changes.html +++ b/packages/serveradmin/serveradmin/serverdb/templates/serverdb/changes.html @@ -70,7 +70,11 @@

- + {% for object_id in object_ids %} + + {% empty %} + + {% endfor %}
diff --git a/packages/serveradmin/serveradmin/serverdb/templates/serverdb/history.html b/packages/serveradmin/serveradmin/serverdb/templates/serverdb/history.html index 29b3193e8..80d9d22f1 100644 --- a/packages/serveradmin/serveradmin/serverdb/templates/serverdb/history.html +++ b/packages/serveradmin/serveradmin/serverdb/templates/serverdb/history.html @@ -1,6 +1,6 @@ {% extends "base.html" %} -{% block title %}History for {{ name }}{% endblock %} +{% block title %}History for {% if hostnames|length == 1 %}{{ hostnames.0 }}{% else %}{{ hostnames|length }} objects{% endif %}{% endblock %} {% block additional_styles %} @@ -10,7 +10,7 @@
-

History for {{ name }}

+

History for {% if hostnames|length == 1 %}{{ hostnames.0 }}{% else %}{{ hostnames|length }} objects{% endif %}

{% if hostnames|length > 1 %}

{{ hostnames|join:", " }}

{% endif %} diff --git a/packages/serveradmin/serveradmin/serverdb/tests/test_views.py b/packages/serveradmin/serveradmin/serverdb/tests/test_views.py index 3a5a6bf22..28f7f94e4 100644 --- a/packages/serveradmin/serveradmin/serverdb/tests/test_views.py +++ b/packages/serveradmin/serveradmin/serverdb/tests/test_views.py @@ -100,7 +100,9 @@ def test_multiple_objects(self): response = self.client.get(self._url(*self.object_ids)) self.assertEqual(200, response.status_code) - self.assertEqual('2 objects', response.context['name']) + self.assertEqual( + ['test-serverdb-history-1', 'test-serverdb-history-2'], + response.context['hostnames']) changes = list(response.context['changes']) self.assertEqual( set(self.object_ids), {c.object_id for c in changes}) @@ -112,7 +114,8 @@ def test_single_object(self): response = self.client.get(self._url(self.object_ids[0])) self.assertEqual(200, response.status_code) - self.assertEqual('test-serverdb-history-1', response.context['name']) + self.assertEqual( + ['test-serverdb-history-1'], response.context['hostnames']) self.assertEqual( {self.object_ids[0]}, {c.object_id for c in response.context['changes']}) @@ -147,13 +150,21 @@ def test_no_parameter(self): response = self.client.get('/serverdb/history') self.assertEqual(400, response.status_code) + def test_invalid_object_id(self): + response = self.client.get(self._url('foo')) + self.assertEqual(400, response.status_code) + def test_changes_multiple_objects(self): - object_ids = ','.join(str(i) for i in self.object_ids) - response = self.client.get( - f'/serverdb/changes?object_id={object_ids}') + object_ids = '&'.join(f'object_id={i}' for i in self.object_ids) + response = self.client.get(f'/serverdb/changes?{object_ids}') self.assertEqual(200, response.status_code) commit_ids = [c.id for c in response.context['commits']] # Two creates and one shared update commit, without duplicates self.assertEqual(3, len(commit_ids)) self.assertEqual(len(commit_ids), len(set(commit_ids))) + + def test_changes_empty_object_id(self): + # The filter form submits an empty object_id if none is given + response = self.client.get('/serverdb/changes?object_id=') + self.assertEqual(200, response.status_code) diff --git a/packages/serveradmin/serveradmin/serverdb/views.py b/packages/serveradmin/serveradmin/serverdb/views.py index e33afdc0b..c9100fdb4 100644 --- a/packages/serveradmin/serveradmin/serverdb/views.py +++ b/packages/serveradmin/serveradmin/serverdb/views.py @@ -27,21 +27,6 @@ from serveradmin.serverdb.query_committer import CommitError, commit_query -def _parse_object_ids(values: list[str]) -> list[int]: - """Parse object ids from (possibly comma separated) request values - - Invalid entries are silently ignored. - """ - object_ids = [] - for value in values: - for object_id in value.split(','): - object_id = object_id.strip() - if object_id.isdigit() and int(object_id) not in object_ids: - object_ids.append(int(object_id)) - - return object_ids - - def _annotate_hostname(changes: QuerySet) -> QuerySet: """Annotate changes with the hostname of their object @@ -95,8 +80,10 @@ def changes(request): 'change_json__object_id', flat=True)) commits = commits.filter(change__object_id__in=object_ids) - f_object_ids = _parse_object_ids(request.GET.getlist('object_id')) - f_object_id = ','.join(str(i) for i in f_object_ids) + try: + f_object_ids = [int(i) for i in request.GET.getlist('object_id') if i] + except ValueError: + return HttpResponseBadRequest('object_id must be an integer') if f_object_ids: commits = commits.filter( change__object_id__in=f_object_ids).distinct() @@ -109,7 +96,7 @@ def changes(request): f_attribute = request.GET.get('attribute') if f_attribute: # Only allow attribute filter if another filter is set (not fully index) - if f_hostname or f_object_id or f_user_or_app: + if f_hostname or f_object_ids or f_user_or_app: commits = commits.filter(change__change_json__has_key=f_attribute) else: f_attribute = None @@ -135,7 +122,7 @@ def count(self): 'from': f_from, 'until': f_until, 'hostname': f_hostname, - 'object_id': f_object_id, + 'object_ids': f_object_ids, 'commit_id': f_commit, 'user_or_app': f_user_or_app, 'attribute': f_attribute, @@ -144,7 +131,10 @@ def count(self): @login_required def history(request): - object_ids = _parse_object_ids(request.GET.getlist('object_id')) + try: + object_ids = [int(i) for i in request.GET.getlist('object_id') if i] + except ValueError: + return HttpResponseBadRequest('object_id must be an integer') hostnames = request.GET.getlist('hostname') if not object_ids and not hostnames: return HttpResponseBadRequest( @@ -192,17 +182,11 @@ def history(request): ).order_by('attribute_id').values_list( 'attribute_id', flat=True).distinct() - if len(servers) == 1: - name = servers[0].hostname - else: - name = f'{len(servers)} objects' - return TemplateResponse(request, 'serverdb/history.html', { 'changes': page_obj, 'commit_id': commit_id, 'object_ids': server_ids, 'object_ids_query': '&'.join(f'object_id={i}' for i in server_ids), - 'name': name, 'hostnames': [s.hostname for s in servers], 'attribute_filter': attribute_filter, 'no_history_attributes': no_history_attributes, diff --git a/packages/serveradmin/serveradmin/servershell/static/js/servershell/command.js b/packages/serveradmin/serveradmin/servershell/static/js/servershell/command.js index d0b789527..5e6aae74b 100644 --- a/packages/serveradmin/serveradmin/servershell/static/js/servershell/command.js +++ b/packages/serveradmin/serveradmin/servershell/static/js/servershell/command.js @@ -427,7 +427,7 @@ servershell.commands = { let url; let selection = servershell.get_selected(); if (selection.length > 0) { - url = servershell.urls.changes + `?object_id=${selection.join(',')}`; + url = servershell.urls.changes + '?' + selection.map(o => `object_id=${o}`).join('&'); } else { url = servershell.urls.changes;