Merge pull request 'Form improvements and bug fixes' (#122) from 32-form-improvements into main

Reviewed-on: #122

src/servala/core/crd.py

@@ -142,11 +142,30 @@ def get_django_field(
             parent_required=is_required,
         )
     elif field_type == "array":
-        # TODO: handle items / validate items, build multi-select input
-        # if field_schema.get("items") and (choices := field_schema["items"].get("enum")):
-        #   choices = [c, c for c in choices]
-        kwargs["help_text"] = _("JSON field (array)")
-        return models.JSONField(**kwargs)
+        kwargs["help_text"] = field_schema.get("description") or _("List of values")
+        from servala.frontend.forms.widgets import DynamicArrayField
+
+        field = models.JSONField(**kwargs)
+        formfield_kwargs = {
+            "label": field.verbose_name,
+            "required": not field.blank,
+        }
+
+        array_validation = {}
+        if min_items := field_schema.get("min_items"):
+            array_validation["min_items"] = min_items
+        if max_items := field_schema.get("max_items"):
+            array_validation["max_items"] = max_items
+        if unique_items := field_schema.get("unique_items"):
+            array_validation["unique_items"] = unique_items
+        if items_schema := field_schema.get("items"):
+            array_validation["items_schema"] = items_schema
+        if array_validation:
+            formfield_kwargs["array_validation"] = array_validation
+
+        field.formfield = lambda: DynamicArrayField(**formfield_kwargs)
+
+        return field
     return models.CharField(max_length=255, **kwargs)
 
 
@@ -166,6 +185,25 @@ def unnest_data(data):
 
 
 class CrdModelFormMixin:
+    HIDDEN_FIELDS = [
+        "spec.compositeDeletePolicy",
+        "spec.compositionRef",
+        "spec.compositionRevisionRef",
+        "spec.compositionRevisionSelector",
+        "spec.compositionSelector",
+        "spec.compositionUpdatePolicy",
+        "spec.parameters.network.serviceType",
+        "spec.parameters.scheduling",
+        "spec.parameters.security",
+        "spec.parameters.size.cpu",
+        "spec.parameters.size.memory",
+        "spec.parameters.size.requests.cpu",
+        "spec.parameters.size.requests.memory",
+        "spec.publishConnectionDetailsTo",
+        "spec.resourceRef",
+        "spec.writeConnectionSecretToRef",
+    ]
+
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self.schema = self._meta.model.SCHEMA
@@ -173,6 +211,12 @@ class CrdModelFormMixin:
         for field in ("organization", "context"):
             self.fields[field].widget = forms.HiddenInput()
 
+        for name, field in self.fields.items():
+            if name in self.HIDDEN_FIELDS or any(
+                name.startswith(f) for f in self.HIDDEN_FIELDS
+            ):
+                field.widget = forms.HiddenInput()
+
         if self.instance and self.instance.pk:
             self.fields["name"].disabled = True
             self.fields["name"].help_text = _("Name cannot be changed after creation.")
@@ -180,8 +224,8 @@ class CrdModelFormMixin:
 
     def strip_title(self, field_name, label):
         field = self.fields[field_name]
-        if field and field.label.startswith(label):
-            field.label = field.label[len(label) :]
+        if field and field.label and (position := field.label.find(label)) != -1:
+            field.label = field.label[position + len(label) :]
 
     def get_fieldsets(self):
         fieldsets = []
@@ -205,57 +249,88 @@ class CrdModelFormMixin:
 
         # Process spec fields
         others = []
-        nested_fieldsets = {}
+        top_level_fieldsets = {}
+        hidden_spec_fields = []
 
         for field_name in self.fields:
             if field_name.startswith("spec."):
+                if isinstance(self.fields[field_name].widget, forms.HiddenInput):
+                    hidden_spec_fields.append(field_name)
+                    continue
+
                 parts = field_name.split(".")
-                if len(parts) == 2:  # Top-level spec field
+                if len(parts) == 2:
+                    # Top-level spec field
                     others.append(field_name)
+                elif len(parts) == 3:
+                    # Second-level field - promote to top-level fieldset
+                    fieldset_key = f"{parts[1]}.{parts[2]}"
+                    if not top_level_fieldsets.get(fieldset_key):
+                        top_level_fieldsets[fieldset_key] = {
+                            "fields": [],
+                            "fieldsets": [],
+                            "title": f"{deslugify(parts[2])}",
+                        }
+                    top_level_fieldsets[fieldset_key]["fields"].append(field_name)
                 else:
-                    parent_key = parts[1]
-                    if not nested_fieldsets.get(parent_key):
-                        nested_fieldsets[parent_key] = {
+                    # Third-level and deeper - create nested fieldsets
+                    fieldset_key = f"{parts[1]}.{parts[2]}"
+                    if not top_level_fieldsets.get(fieldset_key):
+                        top_level_fieldsets[fieldset_key] = {
                             "fields": [],
                             "fieldsets": {},
-                            "title": deslugify(parent_key),
+                            "title": f"{deslugify(parts[2])}",
                         }
-                    parent = nested_fieldsets[parent_key]
-                    if len(parts) == 3:  # Top-level within fieldset
-                        parent["fields"].append(field_name)
-                    else:
-                        sub_key = parts[2]
-                        if not parent["fieldsets"].get(sub_key):
-                            parent["fieldsets"][sub_key] = {
-                                "title": deslugify(sub_key),
-                                "fields": [],
-                            }
-                        parent["fieldsets"][sub_key]["fields"].append(field_name)
 
-        # Add nested fieldsets to fieldsets
-        for group in nested_fieldsets.values():
-            total_fields = 0
-            for fieldset in group["fieldsets"].values():
-                if (field_count := len(fieldset["fields"])) == 1:
-                    group["fields"].append(fieldset["fields"][0])
-                    fieldset["fields"] = []
+                    sub_key = parts[3]
+                    if not top_level_fieldsets[fieldset_key]["fieldsets"].get(sub_key):
+                        top_level_fieldsets[fieldset_key]["fieldsets"][sub_key] = {
+                            "title": deslugify(sub_key),
+                            "fields": [],
+                        }
+                    top_level_fieldsets[fieldset_key]["fieldsets"][sub_key][
+                        "fields"
+                    ].append(field_name)
+
+        for fieldset in top_level_fieldsets.values():
+            nested_fieldsets_list = []
+            for sub_fieldset in fieldset["fieldsets"].values():
+                if len(sub_fieldset["fields"]) == 1:
+                    # If nested fieldset has only one field, move it to parent
+                    fieldset["fields"].append(sub_fieldset["fields"][0])
                 else:
-                    title = f"{group['title']}: {fieldset['title']}: "
-                    for field in fieldset["fields"]:
+                    # Keep as nested fieldset with proper title stripping
+                    title = f"{fieldset['title']}: {sub_fieldset['title']}: "
+                    for field in sub_fieldset["fields"]:
                         self.strip_title(field, title)
-                    total_fields += field_count
-            if (total_fields + len(group["fields"])) == 1:
-                others.append(group["fields"][0])
+                    nested_fieldsets_list.append(sub_fieldset)
+
+            fieldset["fieldsets"] = nested_fieldsets_list
+            total_fields = len(fieldset["fields"]) + len(nested_fieldsets_list)
+            if total_fields == 1 and len(fieldset["fields"]) == 1:
+                others.append(fieldset["fields"][0])
             else:
-                title = f"{group['title']}: "
-                for field in group["fields"]:
+                title = f"{fieldset['title']}: "
+                for field in fieldset["fields"]:
                     self.strip_title(field, title)
-                fieldsets.append(group)
+                fieldsets.append(fieldset)
 
         # Add 'others' tab if there are any fields
         if others:
             fieldsets.append({"title": "Others", "fields": others, "fieldsets": []})
 
+        if hidden_spec_fields:
+            fieldsets.append(
+                {
+                    "title": "Advanced",
+                    "fields": hidden_spec_fields,
+                    "fieldsets": [],
+                    "hidden": True,
+                }
+            )
+
+        fieldsets.sort(key=lambda f: f.get("hidden", False))
+
         return fieldsets
 
     def get_nested_data(self):

src/servala/core/middleware.py

@@ -20,6 +20,10 @@ class OrganizationMiddleware:
         organization_slug = url.kwargs.get("organization")
         if organization_slug:
             pk = organization_slug.rsplit("-", maxsplit=1)[-1]
+            try:
+                pk = int(pk)
+            except ValueError:
+                pk = -1
             request.organization = get_object_or_404(Organization, pk=pk)
             with scope(organization=request.organization):
                 return self.get_response(request)

src/servala/core/models/service.py

@@ -587,6 +587,15 @@ class ServiceInstance(ServalaModelMixin, models.Model):
             )
 
         try:
+            spec_data = spec_data or {}
+            if "writeConnectionSecretToRef" not in spec_data:
+                spec_data["writeConnectionSecretToRef"] = {}
+
+            if not spec_data["writeConnectionSecretToRef"].get("name"):
+                service_slug = context.service_offering.service.slug
+                secret_name = f"{organization.slug}-{instance.pk}-{service_slug}"
+                spec_data["writeConnectionSecretToRef"]["name"] = secret_name
+
             create_data = {
                 "apiVersion": f"{context.group}/{context.version}",
                 "kind": context.kind,
@@ -594,7 +603,7 @@ class ServiceInstance(ServalaModelMixin, models.Model):
                     "name": name,
                     "namespace": organization.namespace,
                 },
-                "spec": spec_data or {},
+                "spec": spec_data,
             }
             if label := context.control_plane.required_label:
                 create_data["metadata"]["labels"] = {settings.DEFAULT_LABEL_KEY: label}

src/servala/core/rules.py

@@ -2,6 +2,10 @@ import rules
 
 
 def has_organization_role(user, org, roles):
+    from servala.core.models import Organization
+
+    if not isinstance(org, Organization):
+        return False
     memberships = org.memberships.all().filter(user=user)
     if roles:
         memberships = memberships.filter(role__in=roles)

src/servala/frontend/forms/widgets.py

@@ -0,0 +1,191 @@
+import json
+
+from django import forms
+from django.core.exceptions import ValidationError
+
+
+class DynamicArrayWidget(forms.Widget):
+    """
+    Widget for dynamic array input fields with add/remove functionality.
+    Renders as a list of input fields with + and - buttons.
+    """
+
+    template_name = "frontend/forms/dynamic_array.html"
+
+    def __init__(self, attrs=None, base_widget=None):
+        super().__init__(attrs)
+        self.base_widget = base_widget or forms.TextInput()
+
+    def format_value(self, value):
+        if value is None or value == "null":
+            return []
+        if isinstance(value, str):
+            if value.strip() == "" or value.strip().lower() == "null":
+                return []
+            try:
+                parsed = json.loads(value)
+                if parsed is None:
+                    return []
+                if isinstance(parsed, list):
+                    return [item for item in parsed if item is not None]
+                return [parsed]
+            except (json.JSONDecodeError, TypeError):
+                return [value] if value else []
+        if isinstance(value, list):
+            return [item for item in value if item is not None]
+        if value == "":
+            return []
+        return [str(value)]
+
+    def get_context(self, name, value, attrs):
+        context = super().get_context(name, value, attrs)
+        value_list = self.format_value(value)
+        if not isinstance(value_list, list):
+            value_list = []
+        context["value_list"] = value_list
+        context["value_list_json"] = json.dumps(value_list)
+        return context
+
+    def value_from_datadict(self, data, files, name):
+        value = data.get(name)
+        if value:
+            try:
+                parsed = json.loads(value)
+                if parsed:
+                    return [
+                        item
+                        for item in parsed
+                        if item is not None and str(item).strip()
+                    ]
+                return []
+            except (json.JSONDecodeError, TypeError):
+                pass
+        return []
+
+
+class DynamicArrayField(forms.JSONField):
+    """
+    Custom field that works with DynamicArrayWidget.
+    Handles the conversion between list data and JSON for k8s generated fields.
+    """
+
+    widget = DynamicArrayWidget
+
+    def __init__(self, **kwargs):
+        self.array_validation = kwargs.pop("array_validation", {})
+        kwargs.setdefault("widget", DynamicArrayWidget())
+        super().__init__(**kwargs)
+
+    def to_python(self, value):
+        """Convert the value to a Python list"""
+        if not value:
+            return []
+
+        if isinstance(value, list):
+            return [item for item in value if item is not None and str(item).strip()]
+
+        if isinstance(value, str):
+            if value.strip() == "" or value.strip().lower() == "null":
+                return []
+            try:
+                parsed = json.loads(value)
+                if not parsed:
+                    return []
+                if isinstance(parsed, list):
+                    return [item for item in parsed if item is not None]
+                return [parsed]
+            except (json.JSONDecodeError, TypeError, ValueError):
+                raise ValidationError(f"Invalid JSON: {value}")
+
+        return []
+
+    def prepare_value(self, value):
+        """Prepare value for widget rendering"""
+        if value is None:
+            return []
+        if isinstance(value, list):
+            return value
+        if isinstance(value, str):
+            try:
+                parsed = json.loads(value)
+                return parsed if isinstance(parsed, list) else [parsed]
+            except (json.JSONDecodeError, TypeError):
+                return [value] if value else []
+        return [str(value)]
+
+    def bound_data(self, data, initial):
+        """Handle bound data properly to avoid JSON parsing issues"""
+        if data is None:
+            return initial
+        # If data is already a list (from our widget), return it as-is
+        if isinstance(data, list):
+            return data
+        # If data is a string, try to parse it as JSON
+        if isinstance(data, str):
+            try:
+                parsed = json.loads(data)
+                return parsed if isinstance(parsed, list) else [parsed]
+            except (json.JSONDecodeError, TypeError):
+                return [data] if data else []
+        return data
+
+    def validate(self, value):
+        if not self.required and (not value or value == []):
+            return
+        super().validate(value)
+        if value:
+            self._validate_array_constraints(value)
+
+    def _validate_array_constraints(self, value):
+        if not isinstance(value, list):
+            return
+
+        if min_items := self.array_validation.get("min_items"):
+            if len(value) < min_items:
+                raise ValidationError(
+                    f"Array must contain at least {min_items} item(s). Currently has {len(value)}."
+                )
+
+        if max_items := self.array_validation.get("max_items"):
+            if len(value) > max_items:
+                raise ValidationError(
+                    f"Array must contain at most {max_items} item(s). Currently has {len(value)}."
+                )
+
+        if self.array_validation.get("unique_items"):
+            if len(value) != len(set(value)):
+                raise ValidationError("All array items must be unique.")
+
+        if items_schema := self.array_validation.get("items_schema"):
+            item_type = items_schema.get("type")
+
+            if item_type == "string":
+                for i, item in enumerate(value):
+                    if not isinstance(item, str):
+                        raise ValidationError(f"Item {i + 1} must be a string.")
+
+                    if min_length := items_schema.get("minLength"):
+                        if len(item) < min_length:
+                            raise ValidationError(
+                                f"Item {i + 1} must be at least {min_length} characters long."
+                            )
+
+                    if max_length := items_schema.get("maxLength"):
+                        if len(item) > max_length:
+                            raise ValidationError(
+                                f"Item {i + 1} must be at most {max_length} characters long."
+                            )
+
+                    if pattern := items_schema.get("pattern"):
+                        import re
+
+                        if not re.match(pattern, item):
+                            raise ValidationError(
+                                f"Item {i + 1} does not match the required pattern."
+                            )
+
+                    if enum_values := items_schema.get("enum"):
+                        if item not in enum_values:
+                            raise ValidationError(
+                                f"Item {i + 1} must be one of: {', '.join(enum_values)}"
+                            )

src/servala/frontend/templates/frontend/base.html

@@ -74,5 +74,6 @@
     <script src="{% static 'mazer/static/js/components/dark.js' %}"></script>
     <script src="{% static 'mazer/extensions/perfect-scrollbar/perfect-scrollbar.min.js' %}"></script>
     <script src="{% static 'mazer/compiled/js/app.js' %}"></script>
+    <script src="{% static 'js/dynamic-array.js' %}"></script>
 </body>
 </html>

src/servala/frontend/templates/frontend/forms/dynamic_array.html

@@ -0,0 +1,31 @@
+<div class="dynamic-array-widget"
+     id="{{ widget.attrs.id|default:'id_'|add:widget.name }}_container"
+     data-name="{{ widget.name }}">
+    <div class="array-items">
+        {% for item in value_list %}
+            <div class="array-item d-flex mb-2">
+                <input type="text"
+                       class="form-control array-item-input"
+                       value="{{ item|default:'' }}"
+                       data-index="{{ forloop.counter0 }}" />
+                {% if forloop.counter != 1 %}
+                    <button type="button"
+                            class="btn btn-sm btn-outline-danger ms-2 remove-array-item"
+                            title="Remove item">×</button>
+                {% endif %}
+            </div>
+        {% empty %}
+            <div class="array-item d-flex mb-2">
+                <input type="text"
+                       class="form-control array-item-input"
+                       value=""
+                       data-index="0" />
+            </div>
+        {% endfor %}
+    </div>
+    <button type="button" class="btn btn-sm btn-outline-primary add-array-item">+ Add Item</button>
+    <input type="hidden"
+           name="{{ widget.name }}"
+           value="{{ value_list_json|safe }}"
+           {% if widget.attrs.id %}id="{{ widget.attrs.id }}"{% endif %} />
+</div>

src/servala/frontend/templates/includes/tabbed_fieldset_form.html

@@ -31,9 +31,9 @@
                 {% for field in fieldset.fields %}
                     {% with field=form|get_field:field %}{{ field.as_field_group }}{% endwith %}
                 {% endfor %}
-                {% for subfieldset in fieldset.fieldsets.values %}
+                {% for subfieldset in fieldset.fieldsets %}
                     {% if subfieldset.fields %}
-                        <h4>{{ subfieldset.title }}</h4>
+                        <h4 class="mt-3">{{ subfieldset.title }}</h4>
                         {% for field in subfieldset.fields %}
                             {% with field=form|get_field:field %}{{ field.as_field_group }}{% endwith %}
                         {% endfor %}

src/servala/frontend/views/service.py

@@ -346,6 +346,10 @@ class ServiceInstanceUpdateView(
         kwargs["instance"] = self.object.spec_object
         return kwargs
 
+    def form_invalid(self, form):
+        messages.error(self.request, form.errors)
+        return super().form_invalid(form)
+
     def form_valid(self, form):
         try:
             spec_data = form.get_nested_data().get("spec")

src/servala/static/js/dynamic-array.js

@@ -0,0 +1,126 @@
+/**
+ * Dynamic Array Widget JavaScript
+ * Provides functionality for adding and removing array items dynamically
+ */
+
+const initDynamicArrayWidget = () => {
+    const containers = document.querySelectorAll('.dynamic-array-widget')
+
+    containers.forEach(container => {
+        const itemsContainer = container.querySelector('.array-items')
+        const addButton = container.querySelector('.add-array-item')
+        const hiddenInput = container.querySelector('input[type="hidden"]')
+        const name = container.dataset.name
+
+        addButton?.addEventListener('click', () => {
+            addArrayItem(itemsContainer, name)
+            updateHiddenInput(container)
+        })
+        addRemoveEventListeners(container)
+        addInputEventListeners(container)
+        updateRemoveButtonVisibility(container)
+    })
+}
+
+const addArrayItem = (itemsContainer, name) => {
+    const itemCount = itemsContainer.querySelectorAll('.array-item').length
+    const itemHtml = `
+        <div class="array-item d-flex mb-2">
+            <input type="text" class="form-control array-item-input" value="" data-index="${itemCount}" />
+            <button type="button" class="btn btn-sm btn-outline-danger ms-2 remove-array-item" title="Remove item">×</button>
+        </div>
+    `
+    itemsContainer.insertAdjacentHTML('beforeend', itemHtml)
+
+    // Add event listeners to the new item
+    const container = itemsContainer.closest('.dynamic-array-widget')
+    addRemoveEventListeners(container)
+    addInputEventListeners(container)
+    updateRemoveButtonVisibility(container)
+
+    // Focus on the new input
+    const newItem = itemsContainer.lastElementChild
+    const newInput = newItem.querySelector('.array-item-input')
+    newInput?.focus()
+}
+
+const addRemoveEventListeners = (container) => {
+    const removeButtons = container.querySelectorAll('.remove-array-item')
+    removeButtons.forEach(button => {
+        button.removeEventListener('click', handleRemoveItem)
+        button.addEventListener('click', handleRemoveItem)
+    })
+}
+
+const addInputEventListeners = (container) => {
+    const inputs = container.querySelectorAll('.array-item-input')
+    inputs.forEach(input => {
+        // removal should not be necessary, but better safe than sorry
+        input.removeEventListener('input', handleInputChange)
+        input.removeEventListener('blur', handleInputChange)
+        input.addEventListener('input', handleInputChange)
+        input.addEventListener('blur', handleInputChange)
+    })
+}
+
+const handleRemoveItem = (ev) => {
+    const button = ev.target
+    const arrayItem = button.closest('.array-item')
+    const container = button.closest('.dynamic-array-widget')
+    const itemsContainer = container.querySelector('.array-items')
+
+    const itemCount = itemsContainer.querySelectorAll('.array-item').length
+    if (itemCount <= 1) {
+        const input = arrayItem.querySelector('.array-item-input')
+        if (input) { input.value = '' }
+    } else {
+        arrayItem.remove()
+    }
+
+    updateHiddenInput(container)
+    reindexItems(container)
+    updateRemoveButtonVisibility(container)
+}
+
+const handleInputChange = (ev) => {
+    const container = ev.target.closest('.dynamic-array-widget')
+    updateHiddenInput(container)
+}
+
+const updateHiddenInput = (container) => {
+    const inputs = container.querySelectorAll('.array-item-input')
+    const hiddenInput = container.querySelector('input[type="hidden"]')
+
+    if (!hiddenInput) return
+
+    const values = Array.from(inputs)
+        .map(input => input.value.trim())
+        .filter(value => value !== '')
+
+    hiddenInput.value = JSON.stringify(values)
+}
+
+const reindexItems = (container) => {
+    const items = container.querySelectorAll('.array-item-input')
+    items.forEach((input, index) => {
+        input.dataset.index = index
+    })
+}
+
+const updateRemoveButtonVisibility = (container) => {
+    const items = container.querySelectorAll('.array-item')
+    const removeButtons = container.querySelectorAll('.remove-array-item')
+
+    // Show/hide remove buttons based on item count
+    removeButtons.forEach(button => {
+        if (items.length <= 1) {
+            button.style.display = 'none'
+        } else {
+            button.style.display = 'inline-block'
+        }
+    })
+}
+
+document.addEventListener('DOMContentLoaded', initDynamicArrayWidget)
+document.addEventListener('htmx:afterSwap', initDynamicArrayWidget)
+document.addEventListener('htmx:afterSettle', initDynamicArrayWidget)