Add another test for the clone form errors and fix the message generation

Author: matthiask

content_editor/admin.py

@@ -266,7 +266,7 @@ def save_related(self, request, form, formsets, change):
             else:
                 self.message_user(
                     request,
-                    gettext("Cloning plugins failed: {}").format(form.errors),
+                    gettext("Cloning plugins failed: {}").format(clone_form.errors),
                     level=messages.ERROR,
                 )
 

tests/README.md

@@ -6,6 +6,7 @@
 - `test_clone_plugins_functionality`: End-to-end UI + database testing
 - `test_clone_backend_logic`: Backend-only form processing
 - `test_clone_insert_between_existing_content`: Insert positioning tests
+- `test_clone_backend_validation_error`: Error message validation when cloning form is invalid
 
 ### Critical Insights
 **Clone Ordering**: `_clone_ordering` = ordering value to insert *before*. After save, existing content gets renormalized but cloned content keeps its target ordering value. This is correct behavior.
@@ -14,6 +15,59 @@
 
 **Region Targeting**: Use `[data-region="sidebar"] .order-machine-insert-target` for region-specific insert targets.
 
+## Test Writing Guidelines
+
+### Be Strict - No Fallbacks
+- **Always test the exact expected behavior** - don't use fallback logic that would mask failing code
+- **Verify specific error messages** - check for exact text, field names, and validation details
+- **Assert on precise conditions** - avoid "if this doesn't work, try that" patterns
+- **Let tests fail when code is broken** - fallbacks hide bugs and prevent proper debugging
+- **Test database state directly** - verify exact counts, field values, and relationships
+- **Use specific selectors** - don't fall back to generic ones that might match wrong elements
+- **Validate full user experience** - error messages must be visible to actual users
+
+### Common Anti-Patterns to Avoid
+- `assert True` after conditional checks (masks real failures)
+- `try/except` blocks that suppress assertion errors
+- Generic error message checks instead of specific validation
+- Counting "at least N" instead of "exactly N" when exact is expected
+- Checking "something worked" instead of "the right thing worked correctly"
+
+Example of **BAD** test pattern:
+```python
+# DON'T DO THIS - fallbacks hide real issues
+if "expected error" in messages:
+    assert True  # Found expected error
+else:
+    # Fallback that masks the real problem
+    if any_error_messages:
+        assert True  # "At least some error occurred"
+
+# DON'T DO THIS - vague assertions
+assert len(items) >= 2  # Could be wrong number
+try:
+    some_assertion()
+except AssertionError:
+    pass  # Ignoring failures
+```
+
+Example of **GOOD** test pattern:
+```python
+# DO THIS - strict assertions reveal real issues
+assert "Cloning plugins failed" in error_message, f"Expected error not found: {error_message}"
+assert "This field is required" in error_message, f"Field validation missing: {error_message}"
+
+# DO THIS - precise validation
+assert len(sidebar_items) == 3, f"Expected exactly 3 items, got {len(sidebar_items)}"
+assert item.ordering == 200, f"Expected ordering 200, got {item.ordering}"
+```
+
+### When Adding New Tests
+1. **First make the test fail** - verify it catches the bug/missing feature
+2. **Make it pass with minimal code** - implement just enough to pass
+3. **Verify error messages reach users** - check admin UI displays them correctly
+4. **Test edge cases strictly** - empty inputs, invalid data, boundary conditions
+
 ### Running Tests
 ```bash
 python -m pytest tests/testapp/test_playwright.py -v

tests/testapp/test_playwright.py

@@ -1548,3 +1548,158 @@ def test_clone_backend_logic():
 
     print("✓ Backend cloning test completed successfully")
     print(f"✓ Cloned content with orderings: {sidebar_orderings}")
+
+
+@pytest.mark.django_db
+def test_clone_backend_validation_error():
+    """Test that validation errors in the CloneForm are properly displayed to the user."""
+    # Create test user and login
+    User.objects.create_superuser("admin", "[email protected]", "password")
+    client = Client()
+    client.login(username="admin", password="password")
+
+    # Create article with content in main region
+    article = Article.objects.create(title="Clone Validation Test Article")
+
+    # Add some content to clone
+    richtext1 = article.testapp_richtext_set.create(
+        text="<p>Test content to clone</p>", region="main", ordering=10
+    )
+
+    # Get the admin change URL
+    admin_url = reverse("admin:testapp_article_change", args=[article.pk])
+
+    # First, GET the form to get proper formset data
+    response = client.get(admin_url)
+    assert response.status_code == 200
+
+    # Extract formset management data from the form
+    soup = BeautifulSoup(response.content, "html.parser")
+
+    # Find all the formset management form fields
+    management_fields = {}
+    for input_field in soup.find_all("input"):
+        name = input_field.get("name", "")
+        value = input_field.get("value", "")
+        if name and (
+            "TOTAL_FORMS" in name
+            or "INITIAL_FORMS" in name
+            or "MIN_NUM_FORMS" in name
+            or "MAX_NUM_FORMS" in name
+        ):
+            management_fields[name] = value
+        elif name == "csrfmiddlewaretoken":
+            csrf_token = value
+
+    # Add existing inline data (to preserve existing content)
+    inline_data = {}
+    for input_field in soup.find_all("input"):
+        name = input_field.get("name", "")
+        value = input_field.get("value", "")
+        if name and (
+            name.startswith("testapp_")
+            and (
+                "-id" in name
+                or "-DELETE" in name
+                or any(
+                    field in name for field in ["text", "file", "ordering", "region"]
+                )
+            )
+        ):
+            inline_data[name] = value
+
+    # Create invalid clone form data - missing required _clone_region field
+    # This should trigger a validation error
+    invalid_form_data = QueryDict(mutable=True)
+    invalid_form_data["title"] = article.title
+    invalid_form_data["csrfmiddlewaretoken"] = csrf_token
+    invalid_form_data["_continue"] = "Save and continue editing"
+
+    # Add management form data
+    for key, value in management_fields.items():
+        invalid_form_data[key] = value
+
+    # Add existing inline data
+    for key, value in inline_data.items():
+        invalid_form_data[key] = value
+
+    # Add clone data but with missing _clone_region (required field)
+    # This should cause CloneForm.is_valid() to return False
+    invalid_form_data.appendlist("_clone", f"testapp.richtext:{richtext1.pk}")
+    # Intentionally omit _clone_region to trigger validation error
+
+    print("Submitting invalid clone form data (missing _clone_region)...")
+
+    # Submit the form with invalid clone data
+    response = client.post(admin_url, invalid_form_data)
+
+    print(f"POST response status: {response.status_code}")
+
+    # Check if we got a redirect (success) or stayed on the same page (error)
+    if response.status_code == 302:
+        print(f"Redirected to: {response['Location']}")
+        # Follow the redirect to check for error messages
+        response = client.get(response["Location"])
+
+    # Parse the response content to look for error messages
+    soup = BeautifulSoup(response.content, "html.parser")
+
+    # Look for Django messages (both error and success messages)
+    messages_divs = soup.find_all("div", class_="messagelist")
+    error_messages = []
+    success_messages = []
+
+    for messages_div in messages_divs:
+        # Look for error messages specifically
+        error_li = messages_div.find_all("li", class_="error")
+        success_li = messages_div.find_all("li", class_="success")
+
+        for li in error_li:
+            error_messages.append(li.get_text().strip())
+        for li in success_li:
+            success_messages.append(li.get_text().strip())
+
+    # Also look for general message containers that might contain errors
+    if not error_messages:
+        # Try alternative selectors for error messages
+        alt_errors = soup.find_all("li", class_="error")
+        for li in alt_errors:
+            error_messages.append(li.get_text().strip())
+
+    print(f"Error messages found: {error_messages}")
+    print(f"Success messages found: {success_messages}")
+
+    # Verify that we got a cloning validation error message with specific field error details
+    # The error should mention "Cloning plugins failed" and include the missing field error
+    cloning_error_found = False
+    field_error_found = False
+
+    for msg in error_messages:
+        if "Cloning plugins failed" in msg:
+            cloning_error_found = True
+            print(f"✓ Found cloning error message: {msg}")
+
+            # Check if the error message contains details about the missing field
+            if "_clone_region" in msg or "This field is required" in msg:
+                field_error_found = True
+                print(f"✓ Error message includes field validation details: {msg}")
+            break
+
+    # Both conditions must be met for the test to pass
+    assert cloning_error_found, (
+        f"Expected 'Cloning plugins failed' message not found in: {error_messages}"
+    )
+    assert field_error_found, (
+        "Expected field error details (_clone_region or 'This field is required') not found in error message"
+    )
+
+    # Verify that cloning actually failed (no content should be cloned to sidebar)
+    article.refresh_from_db()
+    sidebar_count = article.testapp_richtext_set.filter(region="sidebar").count()
+    assert sidebar_count == 0, (
+        f"Cloning should have failed, but found {sidebar_count} items in sidebar"
+    )
+
+    print("✓ Validation correctly prevented cloning and showed detailed error message")
+
+    print("✓ Clone form validation error test completed")