From 5d8b7e3f61a7c491c4c9fee84756c1e4546f3b9a Mon Sep 17 00:00:00 2001 From: catborise Date: Sun, 25 Oct 2026 23:20:28 +0300 Subject: [PATCH] fix(appsettings): a group saves what was edited, all of it or none, in one row - A group sent all its fields, and the ones left as they were undid what another administrator had saved meanwhile. Each field now sends the value the page showed, and only a field edited since is saved. - A database error partway left the group half saved. The group is now saved in one transaction, written as one audit row: "Settings: change Console Scale to True, Console Clip Viewport to True". --- appsettings/templates/appsettings.html | 1 + appsettings/tests/test_appsettings.py | 34 +++++++++++++++++++++++--- appsettings/tests/test_audit_rows.py | 4 +-- appsettings/views.py | 29 +++++++++++----------- 4 files changed, 49 insertions(+), 19 deletions(-) diff --git a/appsettings/templates/appsettings.html b/appsettings/templates/appsettings.html index af4748f3..7ec441cb 100644 --- a/appsettings/templates/appsettings.html +++ b/appsettings/templates/appsettings.html @@ -61,6 +61,7 @@ {% endif %} +
{% trans setting.description %}
diff --git a/appsettings/tests/test_appsettings.py b/appsettings/tests/test_appsettings.py index af8ca97f..4bb41ebb 100644 --- a/appsettings/tests/test_appsettings.py +++ b/appsettings/tests/test_appsettings.py @@ -93,7 +93,7 @@ class AppSettingsTestCase(TestCase): titles = [h.text_content().strip() for h in page.xpath("//form//h2")] self.assertEqual(titles[:3], ["New VM defaults", "Disks", "Clones"]) # every setting is in one group, with a label tied to its field and its description shown - keys = page.xpath("//form//*[self::input or self::select][@name and @name!='csrfmiddlewaretoken']/@name") + keys = page.xpath("//form//*[self::input or self::select][@name and @name!='csrfmiddlewaretoken' and not(starts-with(@name, 'was-'))]/@name") names = set(AppSettings.objects.exclude(key="BOOTSTRAP_THEME").values_list("key", flat=True)) self.assertEqual(sorted(k for k in keys if k not in ("BOOTSTRAP_THEME", "language")), sorted(names)) (field,) = page.xpath("//*[@name='LOGS_PER_PAGE']") @@ -114,11 +114,39 @@ class AppSettingsTestCase(TestCase): values = dict(AppSettings.objects.filter(key__startswith="CONSOLE_").values_list("key", "value")) self.assertEqual(values, {"CONSOLE_SCALE": "True", "CONSOLE_VIEW_ONLY": "False", "CONSOLE_RESIZE_SESSION": "False", "CONSOLE_CLIP_VIEWPORT": "True"}) - self.assertEqual(sorted(Logs.objects.values_list("message", flat=True)), - ["Setting Console Clip Viewport: change to True", "Setting Console Scale: change to True"]) + # one row for the group's save + self.assertEqual(list(Logs.objects.values_list("message", flat=True)), + ["Settings: change Console Clip Viewport to True, Console Scale to True"]) self.assertEqual([str(m) for m in get_messages(response.wsgi_request)], ["Saved: Console Clip Viewport, Console Scale"]) + def test_a_group_keeps_what_another_administrator_saved_meanwhile(self): + # both opened the page with 5 and 15; the first saved 10 for the limit + AppSettings.objects.filter(key="LOGIN_FAILURE_LIMIT").update(value="10") + self.client.post(reverse("appsettings"), {"LOGIN_FAILURE_LIMIT": "5", "was-LOGIN_FAILURE_LIMIT": "5", + "LOGIN_LOCKOUT_MINUTES": "30", "was-LOGIN_LOCKOUT_MINUTES": "15"}) + values = dict(AppSettings.objects.filter(key__startswith="LOGIN_").values_list("key", "value")) + self.assertEqual(values, {"LOGIN_FAILURE_LIMIT": "10", "LOGIN_LOCKOUT_MINUTES": "30"}) + + def test_a_failed_save_changes_none_of_its_group(self): + from unittest.mock import patch + + from django.db import DatabaseError + + saves = [] + + def save(setting, *args, **kwargs): + saves.append(setting.key) + if len(saves) == 2: + raise DatabaseError("disk full") + return original(setting, *args, **kwargs) + + original = AppSettings.save + with patch.object(AppSettings, "save", save): + self.client.post(reverse("appsettings"), {"CONSOLE_SCALE": "True", "CONSOLE_CLIP_VIEWPORT": "True"}) + values = dict(AppSettings.objects.filter(key__in=["CONSOLE_SCALE", "CONSOLE_CLIP_VIEWPORT"]).values_list("key", "value")) + self.assertEqual(values, {"CONSOLE_SCALE": "False", "CONSOLE_CLIP_VIEWPORT": "False"}) + def test_one_bad_value_saves_nothing_of_its_group(self): response = self.client.post(reverse("appsettings"), {"LOGIN_FAILURE_LIMIT": "10", "LOGIN_LOCKOUT_MINUTES": "x"}) self.assertEqual(AppSettings.objects.get(key="LOGIN_FAILURE_LIMIT").value, "5") diff --git a/appsettings/tests/test_audit_rows.py b/appsettings/tests/test_audit_rows.py index 5fe6f48f..2ae96acf 100644 --- a/appsettings/tests/test_audit_rows.py +++ b/appsettings/tests/test_audit_rows.py @@ -32,7 +32,7 @@ class SettingRowTests(TestCase): self.client.post(self.url, {"BOOTSTRAP_THEME": theme}) self.client.post(self.url, {"BOOTSTRAP_THEME": theme}) # no change self.assertEqual(rows(), [ - (f"Setting {name}: change to 50", True, "-"), + (f"Settings: change {name} to 50", True, "-"), (f"Setting Theme: change to {theme}", True, "-"), ]) @@ -41,4 +41,4 @@ class SettingRowTests(TestCase): with patch.object(AppSettings, "save", side_effect=DatabaseError("disk full")): response = self.client.post(self.url, {"LOGS_PER_PAGE": "50"}, follow=True) self.assertIn("error", [m.level_tag for m in response.context["messages"]]) - self.assertEqual(rows(), [(f"Setting {name}: change to 50: DatabaseError", False, "-")]) + self.assertEqual(rows(), [(f"Settings: change {name} to 50: DatabaseError", False, "-")]) diff --git a/appsettings/views.py b/appsettings/views.py index b0f5e773..e676e2ec 100644 --- a/appsettings/views.py +++ b/appsettings/views.py @@ -3,7 +3,7 @@ from pathlib import Path from admin.decorators import superuser_only from django.conf import settings from django.contrib import messages -from django.db import DatabaseError +from django.db import DatabaseError, transaction from django.http import HttpResponseRedirect from django.shortcuts import render from django.utils.translation import gettext_noop as _ @@ -90,27 +90,28 @@ def appsettings(request): messages.success(request, msg) return HttpResponseRedirect(request.get_full_path()) + # a field counts as changed against the value the page showed (was-KEY): one left + # as it was does not undo what another administrator saved meanwhile posted = sorted(((s, request.POST[s.key]) for s in settings_by_key.values() if s.key in request.POST), key=lambda item: item[0].name) - refusals = [refused for refused in (refusal(s, value) for s, value in posted) if refused] + changed = [(s, value) for s, value in posted if value != request.POST.get(f"was-{s.key}", s.value)] + refusals = [refused for refused in (refusal(s, value) for s, value in changed) if refused] for refused in refusals: messages.error(request, refused) - if not refusals: - saved = [] + if changed and not refusals: + action = "change " + ", ".join(f"{s.name} to {value}" for s, value in changed) try: - for setting, value in posted: - if value != setting.value: - with audit(request, None, f"Setting {setting.name}", f"change to {value}"): - setting.value = value - setting.save() - saved.append(setting.name) + # one row, and all of the group or none of it + with audit(request, None, "Settings", action), transaction.atomic(): + for setting, value in changed: + setting.value = value + setting.save() except DatabaseError as err: # the audit row records it as failed messages.error(request, err) else: - if saved: - messages.success(request, _("Saved: %(settings)s") % {"settings": ", ".join(saved)}) - else: - messages.info(request, _("Nothing changed")) + messages.success(request, _("Saved: %(settings)s") % {"settings": ", ".join(s.name for s, _v in changed)}) + elif not refusals: + messages.info(request, _("Nothing changed")) return HttpResponseRedirect(request.get_full_path()) groups = []