feat(instances): the XML editor shows a change before saving it

Change used to define the edited XML at once. It now opens a dialog with
the lines the edit changes in the VM's definition, and Save there defines
it. A definition libvirt refuses comes back to the dialog with the reason,
and the edit stays in the editor; before, the page reloaded with the
current XML and the edit was lost. A definition that changed since the
editor was opened (another tab, a disk added) is not overwritten: the
dialog asks to reload. An unchanged XML has nothing to save.

change_xml answers the dialog with JSON; the editor's script moves to
static/js/xml_editor.js.

instances/templates/instance.html

@@ -159,18 +159,7 @@
         //sto_input.innerHTML = pool;
     }
 </script>
-<script>
-    // The XML editor exists only for superusers
-    if (document.getElementById("editor")) {
-        var editor = ace.edit("editor");
-        editor.getSession().setMode("ace/mode/xml");
-
-        var input = $('input[name="inst_xml"]');
-        editor.getSession().on("change", function () {
-            input.val(editor.getSession().getValue());
-        });
-    }
-</script>
+<script src="{% static 'js/xml_editor.js' %}"></script>
 <script>
     function random_mac(net) {
         $.getJSON("{% url 'instances:random_mac_address' %}", function (data) {

instances/templates/instances/settings_tab.html

@@ -658,13 +658,12 @@
             {% if instance.status != 5 %}
                 <p>{% trans "If you need to edit XML please Power Off the instance" %}</p>
             {% endif %}
-            <form action="{% url 'instances:change_xml' instance.id %}" method="post" role="form" aria-label="Edit instance XML form">
+            <form action="{% url 'instances:change_xml' instance.id %}" method="post" role="form" aria-label="Edit instance XML form" id="xml-form">
                 {% csrf_token %}
                 <div class="col-sm-12 xmlheight">
                     <textarea id="editor">{{ instance.inst_xml }}</textarea>
                 </div>
                 {% if instance.status == 5 %}
-                    <input type="hidden" name="inst_xml">
                     <button type="submit" class="btn btn-lg btn-success float-end" name="change_xml">
                         {% trans "Change" %}
                     </button>
@@ -675,6 +674,28 @@
                 {% endif %}
             </form>
             <div class="clearfix"></div>
+            {% if instance.status == 5 %}
+                <div class="modal fade" id="xml-review" tabindex="-1" aria-labelledby="xml-review-title" aria-hidden="true"
+                     data-failed="{% trans "The XML could not be checked. Try again." %}">
+                    <div class="modal-dialog modal-xl modal-dialog-scrollable">
+                        <div class="modal-content">
+                            <div class="modal-header">
+                                <h5 class="modal-title" id="xml-review-title">{% trans "Review the XML change" %}</h5>
+                                <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="{% trans "Close" %}"></button>
+                            </div>
+                            <div class="modal-body">
+                                <div class="alert alert-danger d-none" role="alert" id="xml-review-error"></div>
+                                <p class="d-none" id="xml-review-same">{% trans "The XML is the same as the VM's definition; there is nothing to save." %}</p>
+                                <pre class="d-none mb-0" id="xml-review-diff"></pre>
+                            </div>
+                            <div class="modal-footer">
+                                <button type="button" class="btn btn-secondary" data-bs-dismiss="modal">{% trans "Cancel" %}</button>
+                                <button type="button" class="btn btn-success" id="xml-review-save">{% trans "Save" %}</button>
+                            </div>
+                        </div>
+                    </div>
+                </div>
+            {% endif %}
         </div>
         <div role="tabpanel" class="tab-pane tab-pane-bordered" id="users">
             <div>

instances/tests/test_change_xml.py

@@ -6,27 +6,80 @@ from django.contrib.messages import get_messages
 from django.test import TestCase
 from django.urls import reverse
 from instances.models import Instance
+from libvirt import libvirtError
+from logs.models import Logs
+
+CURRENT = "<domain>\n  <name>xml-vm</name>\n  <memory>1024</memory>\n</domain>"
+EDITED = "<domain>\n  <name>xml-vm</name>\n  <memory>2048</memory>\n</domain>"
 
 
 class ChangeXmlTestCase(TestCase):
+    """The XML editor shows what a change does before it is saved, and saves
+    only over the definition it was opened with."""
+
     def setUp(self):
         self.client.force_login(get_user_model().objects.create_superuser("xml_admin", "x@example.com", "pw"))
         compute = Compute.objects.create(name="xml-compute", hostname="127.0.0.1", login="", password="", type=4)
         self.instance = Instance.objects.create(compute=compute, name="xml-vm", uuid="11111111-2222-3333-4444-888888888888")
 
-    def post(self, status):
+    def post(self, status=5, xml=EDITED, base=CURRENT, confirm=False, define_error=None):
+        data = {"inst_xml": xml, "base": base}
+        if confirm:
+            data["confirm"] = "1"
         with patch("instances.models.wvmInstance") as wvm:
             wvm.return_value.get_status.return_value = status
-            response = self.client.post(reverse("instances:change_xml", args=[self.instance.id]), {"inst_xml": "<domain/>"})
+            wvm.return_value._XMLDesc.return_value = CURRENT
+            wvm.return_value._defineXML.side_effect = define_error
+            response = self.client.post(reverse("instances:change_xml", args=[self.instance.id]), data)
         return response, wvm.return_value._defineXML
 
-    def test_shut_off_vm_gets_the_new_definition(self):
-        _, define = self.post(status=5)
-        define.assert_called_once_with("<domain/>")
+    def test_without_confirm_the_change_is_shown_not_saved(self):
+        response, define = self.post()
+        define.assert_not_called()
+        diff = response.json()["diff"]
+        self.assertIn("-  <memory>1024</memory>", diff)
+        self.assertIn("+  <memory>2048</memory>", diff)
+
+    def test_an_unchanged_definition_shows_no_difference(self):
+        response, _ = self.post(xml=CURRENT)
+        self.assertEqual(response.json()["diff"], [])
+
+    def test_a_confirmed_change_is_saved_and_logged(self):
+        response, define = self.post(confirm=True)
+        define.assert_called_once_with(EDITED)
+        self.assertEqual(response.json(), {"saved": True})
+        self.assertIn("XML", " ".join(str(m) for m in get_messages(response.wsgi_request)))
+        row = Logs.objects.get(instance_uuid=self.instance.uuid)
+        self.assertEqual((row.message, row.success), ("VM xml-vm: change XML", True))
+
+    def test_a_definition_changed_since_the_editor_opened_is_not_overwritten(self):
+        for confirm in (False, True):
+            with self.subTest(confirm=confirm):
+                response, define = self.post(base="<domain>older</domain>", confirm=confirm)
+                define.assert_not_called()
+                self.assertEqual(response.status_code, 409)
+                self.assertIn("changed since", response.json()["error"])
+
+    def test_a_definition_libvirt_refuses_comes_back_with_the_reason(self):
+        response, define = self.post(confirm=True, define_error=libvirtError("unsupported configuration: bad memory"))
+        self.assertEqual(response.status_code, 400)
+        self.assertIn("bad memory", response.json()["error"])
+        row = Logs.objects.get(instance_uuid=self.instance.uuid)
+        self.assertEqual((row.message, row.success), ("VM xml-vm: change XML: unsupported configuration: bad memory", False))
 
     def test_running_or_paused_vm_is_refused(self):
         for status in (1, 3):
             with self.subTest(status=status):
-                response, define = self.post(status)
+                response, define = self.post(status, confirm=True)
                 define.assert_not_called()
-                self.assertIn("Power off", " ".join(str(m) for m in get_messages(response.wsgi_request)))
+                self.assertEqual(response.status_code, 409)
+                self.assertIn("Power off", response.json()["error"])
+
+    def test_the_line_breaks_a_form_posts_are_the_same_definition(self):
+        response, _ = self.post(base=CURRENT.replace("\n", "\r\n"))
+        self.assertEqual(response.status_code, 200)
+
+    def test_an_empty_definition_is_refused(self):
+        response, define = self.post(xml="  ", confirm=True)
+        define.assert_not_called()
+        self.assertEqual(response.status_code, 400)

instances/tests/test_error_handling.py

@@ -57,7 +57,7 @@ class ErrorHandlingTestCase(TestCase):
 
     def test_redirects_without_or_with_a_foreign_referer_go_to_the_vm_page(self):
         names = back_views()
-        self.assertGreaterEqual(len(names), 30)
+        self.assertGreaterEqual(len(names), 29)
         for name in names:
             for referer in (None, EXTERNAL):
                 with self.subTest(view=name, referer=referer), patch("instances.models.wvmInstance"), \

instances/tests/test_pending_edits.py

@@ -106,6 +106,13 @@ class PendingEditsTestCase(TestCase):
                 (form,) = doc.xpath("//form[@action=$a]", a=reverse(f"instances:{action}", args=[self.instance.id]))
                 self.assertTrue(all(b.get("disabled") is not None for b in form.xpath(".//button")))
 
+    def test_the_xml_editor_reviews_a_change_only_while_the_vm_is_off(self):
+        # static/js/xml_editor.js opens the review dialog on Change
+        doc = self.page(status=5)
+        self.assertEqual(len(doc.xpath("//form[@id='xml-form']//button[@type='submit']")), 1)
+        self.assertEqual(len(doc.xpath("//div[@id='xml-review']//button[@id='xml-review-save']")), 1)
+        self.assertEqual(self.page().xpath("//div[@id='xml-review']"), [])
+
     def test_the_boot_video_and_console_forms_show_the_pending_definition(self):
         cd = {"dev": "sda", "image": "pending.iso", "storage": "default", "path": "/pool/pending.iso", "bus": "sata"}
         with patch("instances.models.Instance.config_networks", new_callable=PropertyMock, return_value=[]):

instances/views.py

@@ -2,6 +2,7 @@ try:
     import crypt_r as crypt
 except ImportError:
     import crypt
+import difflib
 import functools
 import json
 import os
@@ -1454,16 +1455,35 @@ def set_bootorder(request, pk):
 @superuser_only
 @serialize_instance_mutation
 def change_xml(request, pk):
+    """
+    The XML editor's dialog: without confirm, the lines the edited XML changes
+    in the definition, to review; with confirm, the edited XML is defined.
+    Only over the definition the editor was opened with (base), so a change
+    made since is not overwritten; a form posts its line breaks as CRLF. A refusal comes back to the dialog and the
+    edited XML stays in the editor.
+    """
     instance = get_instance(request.user, pk)
     new_xml = request.POST.get("inst_xml", "")
     # The editor is filled while the VM is shut off; once it runs, the posted
     # XML would overwrite changes made since.
-    if new_xml and instance.proxy.get_status() != 5:
-        messages.error(request, _("Power off the instance to change its XML"))
-    elif new_xml:
+    if instance.proxy.get_status() != 5:
+        return JsonResponse({"error": _("Power off the instance to change its XML")}, status=409)
+    if not new_xml.strip():
+        return JsonResponse({"error": _("The XML is empty")}, status=400)
+    current = instance.inst_xml
+    if request.POST.get("base", "").replace("\r\n", "\n") != current:
+        return JsonResponse({"error": _("The definition has changed since the editor was opened. "
+                                         "Reload the page to edit the current one.")}, status=409)
+    if "confirm" not in request.POST:
+        diff = difflib.unified_diff(current.splitlines(), new_xml.splitlines(), "current", "new", lineterm="")
+        return JsonResponse({"diff": list(diff)})
+    try:
         with audit_vm(request, instance, "change XML"):
             instance.proxy._defineXML(new_xml)
-    return _back(request, pk, "xmledit")
+    except libvirtError as err:
+        return JsonResponse({"error": error_text(request, err)}, status=400)
+    messages.success(request, _("The XML definition was saved."))
+    return JsonResponse({"saved": True})
 
 
 @require_POST

static/js/xml_editor.js

@@ -0,0 +1,82 @@
+// The VM page's XML editor. Change shows the lines the edit changes in the
+// VM's definition (instances.views.change_xml), and Save in that dialog
+// defines it. A refusal shows in the dialog, and the edit stays in the editor.
+(function () {
+    var form = document.getElementById("xml-form");
+    if (!form) {
+        return; // the editor exists only for superusers
+    }
+    var editor = ace.edit("editor");
+    editor.getSession().setMode("ace/mode/xml");
+    var base = editor.getValue(); // the definition the editor opened with
+    var dialog = document.getElementById("xml-review");
+    if (!dialog) {
+        return; // a running VM's editor only shows its XML
+    }
+    var modal = bootstrap.Modal.getOrCreateInstance(dialog);
+    var error = document.getElementById("xml-review-error");
+    var same = document.getElementById("xml-review-same");
+    var diff = document.getElementById("xml-review-diff");
+    var save = document.getElementById("xml-review-save");
+    var reviewed = ""; // Save defines the XML the dialog shows, not later edits
+
+    // null after a redirect (the VM is busy or moved): the page says why
+    function post(xml, confirm) {
+        var data = new FormData(form);
+        data.set("inst_xml", xml);
+        data.set("base", base);
+        if (confirm) {
+            data.set("confirm", "1");
+        }
+        return fetch(form.action, {method: "POST", body: data, redirect: "manual"}).then(function (response) {
+            if (response.type === "opaqueredirect") {
+                location.reload();
+                return null;
+            }
+            return response.json();
+        }).catch(function () {
+            return {error: dialog.dataset.failed};
+        });
+    }
+
+    function show(answer) {
+        error.textContent = answer.error || "";
+        error.classList.toggle("d-none", !answer.error);
+        var lines = answer.diff || [];
+        same.classList.toggle("d-none", Boolean(answer.error) || lines.length > 0);
+        save.classList.toggle("d-none", lines.length === 0);
+        save.disabled = false;
+        diff.replaceChildren();
+        // skip the two file header lines; + added, - removed, @@ where
+        lines.slice(2).forEach(function (line) {
+            var row = document.createElement("div");
+            row.textContent = line;
+            row.className = {"+": "text-success", "-": "text-danger", "@": "text-body-secondary"}[line[0]] || "";
+            diff.appendChild(row);
+        });
+        diff.classList.toggle("d-none", lines.length === 0);
+    }
+
+    form.addEventListener("submit", function (event) {
+        event.preventDefault();
+        reviewed = editor.getValue();
+        post(reviewed, false).then(function (answer) {
+            if (answer) {
+                show(answer);
+                modal.show();
+            }
+        });
+    });
+
+    save.addEventListener("click", function () {
+        save.disabled = true;
+        post(reviewed, true).then(function (answer) {
+            if (answer && answer.saved) {
+                location.hash = "xmledit";
+                location.reload();
+            } else if (answer) {
+                show({error: answer.error, diff: []});
+            }
+        });
+    });
+})();