From f77439f39880fb64d9a585d06fbef5f9457d158c Mon Sep 17 00:00:00 2001
From: catborise <catborise@gmail.com>
Date: Sat, 31 Oct 2026 18:05:34 +0300
Subject: [PATCH] fix(storages): a refused new volume comes back in its dialog

A New volume form the pool page refused, for a name with a space or a
character not allowed, redirected back with the reasons at the top of
the page and lost what was typed.

The form now posts to the pool page itself: a valid one makes the
volume and redirects as before, a refused one renders the page with
the dialog open on what was entered, the refused field marked with its
reason and focused. A pool stopped meanwhile has no dialog: its page
shows the reasons. The separate create_volume view and URL go.
---
 computes/urls.py                             |  7 +-
 instances/tests/test_views.py                | 24 -------
 storages/templates/create_stg_vol_block.html |  7 +-
 storages/templates/storage.html              |  7 ++
 storages/tests/test_audit_rows.py            |  3 +-
 storages/tests/test_pool_page.py             | 28 ++++++++
 storages/views.py                            | 68 +++++++-------------
 7 files changed, 63 insertions(+), 81 deletions(-)

diff --git a/computes/urls.py b/computes/urls.py
index 36b8ce80..37647d2e 100644
--- a/computes/urls.py
+++ b/computes/urls.py
@@ -6,7 +6,7 @@ from interfaces.views import interface, interfaces
 from networks.views import network, networks
 from nodedevs.views import nodedevs
 from nwfilters.views import nwfilter, nwfilters
-from storages.views import create_volume, get_volumes, storage, storages
+from storages.views import get_volumes, storage, storages
 
 from . import forms, views
 
@@ -66,11 +66,6 @@ urlpatterns = [
                 path("storages/", storages, name="storages"),
                 path("storage/<str:pool>/volumes/", get_volumes, name="volumes"),
                 path("storage/<str:pool>/", storage, name="storage"),
-                path(
-                    "storage/<str:pool>/create_volume/",
-                    create_volume,
-                    name="create_volume",
-                ),
                 path("networks/", networks, name="networks"),
                 path("network/<str:pool>/", network, name="network"),
                 path("interfaces/", interfaces, name="interfaces"),
diff --git a/instances/tests/test_views.py b/instances/tests/test_views.py
index 63a76c3d..5d8b3952 100644
--- a/instances/tests/test_views.py
+++ b/instances/tests/test_views.py
@@ -153,18 +153,6 @@ class InstancesTestCase(TestCase):
         )
         self.assertRaises(Http404)
 
-    # def test_create_volume(self):
-    #     response = self.client.post(
-    #         reverse('create_volume', args=[self.compute.id, 'default']),
-    #         {
-    #             'name': 'test',
-    #             'format': 'qcow2',
-    #             'size': '1',
-    #             'meta_prealloc': False,
-    #         },
-    #     )
-    #     self.assertRedirects(response, reverse('storage', args=[self.compute.id, 'default']))
-
     def test_create_destroy_instance(self):
         # Create
         response = self.client.get(
@@ -1126,18 +1114,6 @@ class InstancesTestCase(TestCase):
     #     del self.instance.status
     #     self.assertEqual(self.instance.status, 1)
 
-    # # create volume
-    # response = self.client.post(
-    #     reverse('create_volume', args=[compute.id, 'default']),
-    #     {
-    #         'name': 'test3',
-    #         'format': 'qcow2',
-    #         'size': '1',
-    #         'meta_prealloc': False,
-    #     },
-    # )
-    # self.assertRedirects(response, reverse('storage', args=[compute.id, 'default']))
-
     # # delete volume
     # response = self.client.post(
     #     reverse('instances:delete_vol', args=[instance.id]),
diff --git a/storages/templates/create_stg_vol_block.html b/storages/templates/create_stg_vol_block.html
index 0e37624e..af2b1295 100644
--- a/storages/templates/create_stg_vol_block.html
+++ b/storages/templates/create_stg_vol_block.html
@@ -41,7 +41,8 @@
                 <span aria-hidden="true">{% bs_icon 'plus-lg' %}</span> {% trans "New volume" %}
             </button>
 
-            <div class="modal fade" id="AddImage" tabindex="-1" role="dialog" aria-labelledby="AddImageLabel" aria-hidden="true">
+            <div class="modal fade" id="AddImage" tabindex="-1" role="dialog" aria-labelledby="AddImageLabel" aria-hidden="true"
+                 {% if form.errors %}data-open{% endif %}>
                 <div class="modal-dialog">
                     <div class="modal-content">
                         <div class="modal-header">
@@ -49,7 +50,7 @@
                             <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="{% trans "Close" %}"></button>
                         </div>
                         <div class="modal-body">
-                            <form action="{% url 'create_volume' compute_id pool %}" id="create-volume" method="post" aria-label="{% trans "Create volume to storage form" %}">
+                            <form id="create-volume" method="post" aria-label="{% trans "Create volume to storage form" %}">
                                 {% csrf_token %}
                                 <div class="row">
                                     <div class="col-12">{% bootstrap_field form.name %}</div>
@@ -61,7 +62,7 @@
                         </div>
                         <div class="modal-footer">
                             <button type="button" class="btn btn-outline-secondary" data-bs-dismiss="modal">{% trans "Cancel" %}</button>
-                            <button type="submit" form="create-volume" class="btn btn-primary">{% trans "Create" %}</button>
+                            <button type="submit" form="create-volume" class="btn btn-primary" name="create_volume">{% trans "Create" %}</button>
                         </div>
                     </div> <!-- /.modal-content -->
                 </div> <!-- /.modal-dialog -->
diff --git a/storages/templates/storage.html b/storages/templates/storage.html
index 1e268e5e..5fe784fb 100644
--- a/storages/templates/storage.html
+++ b/storages/templates/storage.html
@@ -238,6 +238,13 @@
         });
         $('#clone-convert, #clone-format').on('change', showCloneRows);
 
+        // a refused new volume: its dialog opens again on what was entered, at the refused field
+        var addImage = document.getElementById('AddImage');
+        if (addImage && addImage.hasAttribute('data-open')) {
+            addImage.addEventListener('shown.bs.modal', focusFirstError, {once: true});
+            bootstrap.Modal.getOrCreateInstance(addImage).show();
+        }
+
         $(document).ready(function() {
             var pendingUpload = null;
             var uploadInProgress = false;
diff --git a/storages/tests/test_audit_rows.py b/storages/tests/test_audit_rows.py
index a30914b7..16a4f39d 100644
--- a/storages/tests/test_audit_rows.py
+++ b/storages/tests/test_audit_rows.py
@@ -179,8 +179,7 @@ class PoolAndVolumeRowTests(TransactionTestCase):
         ])
 
     def test_volume_add_and_delete(self):
-        self.client.post(reverse("create_volume", args=[self.compute.pk, "default"]),
-                         {"name": "disk1", "format": "qcow2", "size": 1})
+        self.client.post(self.url, {"name": "disk1", "format": "qcow2", "size": 1, "create_volume": ""})
         self.client.post(self.url, {"del_volume": "1", "volname": "disk1.qcow2"})
         with patch("storages.views.libvirt_compute_lock", side_effect=claims.Busy("compute:1")):
             self.client.post(self.url, {"del_volume": "1", "volname": "disk1.qcow2"})
diff --git a/storages/tests/test_pool_page.py b/storages/tests/test_pool_page.py
index 41b46969..81ba405b 100644
--- a/storages/tests/test_pool_page.py
+++ b/storages/tests/test_pool_page.py
@@ -55,6 +55,34 @@ class PoolPageTests(TestCase):
                 self.assertEqual(doc.xpath("//div[@id=$d]//button[@data-bs-dismiss='modal'][not(@aria-label)]/text()",
                                            d=dialog), ["Cancel"])
 
+    def test_a_new_volume_is_made_from_the_pool_page(self):
+        self.conn.get_size.return_value = (10, 4)
+        self.conn.create_volume.return_value = "disk1.qcow2"
+        url = reverse("storage", args=[self.compute.pk, "default"])
+        response = self.client.post(url, {"name": "disk1", "size": "2", "format": "qcow2", "meta_prealloc": "on",
+                                          "create_volume": ""})
+        self.assertRedirects(response, url, fetch_redirect_response=False)
+        self.conn.create_volume.assert_called_once_with("disk1", 2, "qcow2", True, 0, 0)
+
+    def test_a_refused_new_volume_comes_back_with_what_was_entered(self):
+        self.conn.get_size.return_value = (10, 4)
+        self.conn.get_status.return_value = 1
+        response = self.client.post(reverse("storage", args=[self.compute.pk, "default"]),
+                                    {"name": "bad name!", "size": "3", "format": "raw", "create_volume": ""})
+        self.assertEqual(response.status_code, 200)
+        self.conn.create_volume.assert_not_called()
+        doc = html.fromstring(response.content)
+        # the dialog opens again on the values sent, the name marked with why
+        self.assertEqual(doc.xpath("//div[@id='AddImage']/@data-open"), [""])
+        self.assertEqual(doc.xpath("//div[@id='AddImage']//input[@name='name']/@value"), ["bad name!"])
+        self.assertEqual(doc.xpath("//div[@id='AddImage']//input[@name='size']/@value"), ["3"])
+        self.assertTrue(doc.xpath("//div[@id='AddImage']//input[@name='name'][contains(@class, 'is-invalid')]"))
+        # a pool stopped meanwhile shows no dialog: the page says why
+        self.conn.is_active.return_value = False
+        response = self.client.post(reverse("storage", args=[self.compute.pk, "default"]),
+                                    {"name": "bad name!", "size": "3", "format": "raw", "create_volume": ""})
+        self.assertContains(response, "The image name must not contain any special characters")
+
     def test_the_pool_is_measured_after_its_refresh(self):
         # update_volumes refreshes the pool: libvirt's space figures are as of the last refresh
         self.conn.get_size.return_value = (10, 4)
diff --git a/storages/views.py b/storages/views.py
index c9749b2a..0a7aed90 100644
--- a/storages/views.py
+++ b/storages/views.py
@@ -117,7 +117,6 @@ def storage(request, compute_id, pool):
     :return:
     """
     compute = get_object_or_404(Compute, pk=compute_id)
-    meta_prealloc = False
     form = CreateVolumeForm()
 
     conn = wvmStorage(
@@ -178,6 +177,27 @@ def storage(request, compute_id, pool):
                 )
             return redirect(reverse("storage", args=[compute.id, pool]))
             # return HttpResponseRedirect(request.get_full_path())
+        if "create_volume" in request.POST:
+            form = CreateVolumeForm(request.POST)
+            if form.is_valid():
+                data = form.cleaned_data
+                with audit(request, compute, subject, f"add volume {data['name']}") as entry:
+                    name = conn.create_volume(
+                        data["name"],
+                        data["size"],
+                        data["format"],
+                        data["meta_prealloc"] and data["format"] == "qcow2",
+                        int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_UID),
+                        int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_GID),
+                    )
+                    entry.action = f"add volume {name}"
+                messages.success(request, _("Image file %(name)s is created successfully") % {"name": name})
+                return redirect(reverse("storage", args=[compute.id, pool]))
+            # refused: the page shows the dialog again, with what was entered and why;
+            # a pool stopped meanwhile has no dialog, so the reasons go to the page
+            if not state:
+                for msg_err in form.errors.values():
+                    messages.error(request, msg_err.as_text())
         if "cln_volume" in request.POST:
             form = CloneImage(request.POST)
             if form.is_valid():
@@ -199,7 +219,7 @@ def storage(request, compute_id, pool):
                     messages.error(request, msg_err.as_text())
             return redirect(reverse("storage", args=[compute.id, pool]))
 
-    # read only for the page: every action above redirects
+    # read only for the page: every action above redirects, but a refused new volume
     # first: it refreshes the pool, and libvirt measures a pool's space only then
     volumes = conn.update_volumes() if state else None
     for volume in volumes or []:
@@ -224,50 +244,6 @@ def storage(request, compute_id, pool):
     return render(request, "storage.html", locals())
 
 
-@superuser_only
-def create_volume(request, compute_id, pool):
-    """
-    :param request:
-    :param compute_id: compute id
-    :param pool: pool name
-    :return:
-    """
-    compute = get_object_or_404(Compute, pk=compute_id)
-    meta_prealloc = False
-
-    conn = wvmStorage(
-        compute.hostname, compute.login, compute.password, compute.type, pool
-    )
-
-    form = CreateVolumeForm(request.POST or None)
-    if form.is_valid():
-        data = form.cleaned_data
-        if data["meta_prealloc"] and data["format"] == "qcow2":
-            meta_prealloc = True
-
-        disk_owner_uid = int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_UID)
-        disk_owner_gid = int(app_settings.INSTANCE_VOLUME_DEFAULT_OWNER_GID)
-
-        with audit(request, compute, f"Storage pool {pool}", f"add volume {data['name']}") as entry:
-            name = conn.create_volume(
-                data["name"],
-                data["size"],
-                data["format"],
-                meta_prealloc,
-                disk_owner_uid,
-                disk_owner_gid,
-            )
-            entry.action = f"add volume {name}"
-        messages.success(
-            request, _("Image file %(name)s is created successfully") % {"name": name}
-        )
-    else:
-        for msg_err in form.errors.values():
-            messages.error(request, msg_err.as_text())
-
-    return redirect(reverse("storage", args=[compute.id, pool]))
-
-
 def get_volumes(request, compute_id, pool):
     """
     :param request: