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 = []