j3d1/webvirtcloud
1
0
Fork 0
mirror of https://github.com/retspen/webvirtcloud synced 2026-11-09 11:46:49 +00:00
Code Issues Projects Releases Wiki Activity

fix(appsettings): a group saves what was edited, all of it or none, in one row

Browse source 
- 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".
This commit is contained in:
catborise 2026-10-25 23:20:28 +03:00
parent 5ef3437291
commit 5d8b7e3f61
4 changed files with 49 additions and 19 deletions
Download patch file Download diff file Expand all files Collapse all files

1
appsettings/templates/appsettings.html
View file

@ -61,6 +61,7 @@
<input id="setting-{{ setting.key }}" class="form-control" type="text" name="{{ setting.key }}" value="{{ setting.value }}"
maxlength="25" aria-describedby="help-{{ setting.key }}">
{% endif %}
<input type="hidden" name="was-{{ setting.key }}" value="{{ setting.value }}">
<div id="help-{{ setting.key }}" class="form-text">{% trans setting.description %}</div>
</div>
</div>

34
appsettings/tests/test_appsettings.py
View file

@ -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")

4
appsettings/tests/test_audit_rows.py
View file

@ -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, "-")])

29
appsettings/views.py
View file

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