Preserve Moodle add-form state to prevent broken assignment creation

src/py_moodle/module.py

@@ -62,6 +62,98 @@ def _get_base_modedit_payload(
     }
 
 
+def _extract_modedit_form_data(form: BeautifulSoup) -> Dict[str, Any]:
+    """Extract the current values from a Moodle modedit form.
+
+    Args:
+        form: A parsed Moodle ``modedit.php`` form element.
+
+    Returns:
+        A mapping of form field names to their current values for ``input``,
+        ``textarea``, and ``select`` elements. File inputs are skipped because
+        browsers do not submit their existing values back to Moodle.
+    """
+    form_data: Dict[str, Any] = {}
+
+    for field in form.find_all(["input", "textarea", "select"]):
+        name = field.get("name")
+        if not name or field.get("type") in ["submit", "button", "reset", "file"]:
+            continue
+
+        if field.name == "textarea":
+            form_data[name] = field.text or ""
+        elif field.name == "select":
+            selected_options = [
+                option["value"]
+                for option in field.find_all("option", selected=True)
+                if option.has_attr("value")
+            ]
+            if field.has_attr("multiple"):
+                if selected_options:
+                    form_data[name] = selected_options
+            elif selected_options:
+                form_data[name] = selected_options[0]
+            else:
+                first_option = next(
+                    (
+                        option
+                        for option in field.find_all("option")
+                        if option.has_attr("value")
+                    ),
+                    None,
+                )
+                if first_option:
+                    form_data[name] = first_option["value"]
+        elif field.get("type") in ("checkbox", "radio"):
+            if field.has_attr("checked"):
+                form_data[name] = field.get("value", "1")
+        else:
+            form_data[name] = field.get("value", "")
+
+    return form_data
+
+
+def _load_modedit_form_data(
+    session: requests.Session,
+    url: str,
+    action_description: str,
+) -> tuple[Dict[str, Any], Any]:
+    """Load a Moodle modedit form page and return its current form values.
+
+    Args:
+        session: Authenticated Moodle session.
+        url: The ``modedit.php`` URL to fetch.
+        action_description: Human-readable action used in error messages.
+
+    Returns:
+        A tuple containing the extracted form data and the compatibility
+        strategy used to locate the form and parse errors.
+    """
+    compatibility = get_session_compatibility(session)
+
+    try:
+        resp = session.get(url)
+        resp.raise_for_status()
+        soup = BeautifulSoup(resp.text, "lxml")
+    except requests.RequestException as e:
+        raise MoodleModuleError(
+            f"Failed to load module form while {action_description}: {e}"
+        )
+
+    form = compatibility.find_modedit_form(soup)
+    if form is None:
+        error_message = compatibility.extract_error_message(soup)
+        if error_message:
+            raise MoodleModuleError(
+                f"Failed to load module form while {action_description}: {error_message}"
+            )
+        raise MoodleModuleError(
+            f"Could not find the module form while {action_description}."
+        )
+
+    return _extract_modedit_form_data(form), compatibility
+
+
 # --- Public Generic Functions ---
 
 
@@ -111,39 +203,45 @@ def add_generic_module(
     except MoodleCourseError as e:
         raise MoodleModuleError(f"Failed to get initial section state: {e}")
 
-    # 2. Build the full payload
+    # 2. Load the real add form so Moodle-specific defaults and hidden fields are
+    # preserved when creating the module.
+    add_form_url = (
+        f"{base_url}/course/modedit.php?"
+        f"add={urllib.parse.quote(module_name)}&type=&course={course_id}"
+        f"&section={section_number}&return=0&sr=-1"
+    )
+    form_payload, compatibility = _load_modedit_form_data(
+        session, add_form_url, f"adding {module_name}"
+    )
+
+    # 3. Build the full payload
     base_payload = _get_base_modedit_payload(
         course_id, section_number, sesskey, module_name, module_id, mode="add"
     )
-    full_payload = {**base_payload, **specific_payload}
+    full_payload = {**base_payload, **form_payload, **specific_payload}
 
-    # 3. POST to modedit.php
+    # 4. POST to modedit.php
     url = f"{base_url}/course/modedit.php"
     headers = {"Content-Type": "application/x-www-form-urlencoded"}
-    encoded_payload = urllib.parse.urlencode(full_payload)
-    compatibility = get_session_compatibility(session)
+    # ``doseq=True`` preserves repeated values for multi-select fields.
+    encoded_payload = urllib.parse.urlencode(full_payload, doseq=True)
     resp = session.post(
         url, data=encoded_payload, headers=headers, allow_redirects=False
     )
 
     # A clear success is a redirect (302 or 303) to the course page.
-    if resp.status_code not in [200, 302, 303]:
-        # A 200 OK status almost always means a silent failure.
-        # Parse the HTML to find Moodle's error message.
+    if resp.status_code not in [302, 303]:
         soup = BeautifulSoup(resp.text, "lxml")
         error_message = compatibility.extract_error_message(soup)
         if error_message:
-            # Found a specific error message.
             raise MoodleModuleError(
                 f"Form submission failed. Moodle error: {error_message}"
             )
-        else:
-            # No clear error message, but creation failed.
-            raise MoodleModuleError(
-                f"Failed to create module. Status: {resp.status_code}. Moodle returned the edit form, indicating a silent failure. Check permissions or required fields."
-            )
+        raise MoodleModuleError(
+            f"Failed to create module. Status: {resp.status_code}. Moodle returned the edit form instead of redirecting, so the activity may not have been created correctly."
+        )
 
-    # 4. Get final state and determine the new cmid
+    # 5. Get final state and determine the new cmid
     time.sleep(1)  # Give Moodle a moment to process the change
     try:
         course_data_after = get_course_with_sections_and_modules(
@@ -185,45 +283,9 @@ def update_generic_module(
     edit_url = f"{base_url}/course/modedit.php?update={cmid}"
 
     # 1. Fetch the edit page to get the current state of the form
-    try:
-        resp = session.get(edit_url)
-        resp.raise_for_status()
-        soup = BeautifulSoup(resp.text, "lxml")
-    except requests.RequestException as e:
-        raise MoodleModuleError(f"Failed to load module edit page for cmid {cmid}: {e}")
-
-    # 2. Parse the form and extract all input, textarea, and select fields
-    compatibility = get_session_compatibility(session)
-    form = compatibility.find_modedit_form(soup)
-    if not form:
-        raise MoodleModuleError("Could not find the edit form on the page.")
-
-    form_data = {}
-
-    for field in form.find_all(["input", "textarea", "select"]):
-        name = field.get("name")
-        # Ignore fields without a name or buttons
-        if not name or field.get("type") in ["submit", "button", "reset"]:
-            continue
-
-        # Logic for different tag/type combinations
-        if field.name == "textarea":
-            form_data[name] = field.text or ""
-        elif field.name == "select":
-            selected_option = field.find("option", selected=True)
-            if selected_option and selected_option.has_attr("value"):
-                form_data[name] = selected_option["value"]
-            else:
-                # If no option is selected, browsers usually submit the first one.
-                first_option = field.find("option", value=True)
-                if first_option:
-                    form_data[name] = first_option["value"]
-
-        elif field.get("type") in ("checkbox", "radio"):
-            if field.has_attr("checked"):
-                form_data[name] = field.get("value", "1")
-        else:  # Handles text, hidden, password, etc.
-            form_data[name] = field.get("value", "")
+    form_data, compatibility = _load_modedit_form_data(
+        session, edit_url, f"updating module {cmid}"
+    )
 
     # 3. Modify the form data with the user's changes
     form_data.update(specific_payload)

tests/unit/test_module_addition.py

@@ -0,0 +1,165 @@
+"""Unit tests for generic module creation helpers."""
+
+from __future__ import annotations
+
+from urllib.parse import parse_qs
+
+import pytest
+from bs4 import BeautifulSoup
+
+from py_moodle.module import (
+    MoodleModuleError,
+    _extract_modedit_form_data,
+    add_generic_module,
+)
+
+
+class _FakeResponse:
+    """Minimal response object for module unit tests."""
+
+    def __init__(self, status_code: int, text: str = ""):
+        self.status_code = status_code
+        self.text = text
+
+    def raise_for_status(self) -> None:
+        """Mimic ``requests.Response.raise_for_status``."""
+        if self.status_code >= 400:
+            raise RuntimeError(f"HTTP {self.status_code}")
+
+
+class _FakeSession:
+    """Minimal session object that records add-module requests."""
+
+    def __init__(self, post_response: _FakeResponse):
+        self.post_response = post_response
+        self.get_calls: list[str] = []
+        self.post_calls: list[dict[str, object]] = []
+
+    def get(self, url: str):
+        """Return a fixed add-form HTML page."""
+        self.get_calls.append(url)
+        return _FakeResponse(
+            200,
+            text="""
+            <form id="mform1" action="/course/modedit.php">
+                <input type="hidden" name="_qf__mod_assign_mod_form" value="1">
+                <input type="hidden" name="gradecat" value="7">
+                <input type="text" name="name" value="Old name">
+                <textarea name="activityeditor[text]">Old instructions</textarea>
+                <input
+                    type="checkbox"
+                    name="assignsubmission_file_enabled"
+                    value="1"
+                    checked
+                />
+                <select name="grade[modgrade_type]">
+                    <option value="point" selected>Point</option>
+                </select>
+            </form>
+            """,
+        )
+
+    def post(self, url: str, data=None, headers=None, allow_redirects=False):
+        """Record the outgoing POST request."""
+        self.post_calls.append(
+            {
+                "url": url,
+                "data": data,
+                "headers": headers,
+                "allow_redirects": allow_redirects,
+            }
+        )
+        return self.post_response
+
+
+def test_extract_modedit_form_data_preserves_multi_select_values():
+    """Multi-select fields should keep all selected values."""
+    soup = BeautifulSoup(
+        """
+        <form id="mform1">
+            <select name="tags[]" multiple>
+                <option value="alpha" selected>Alpha</option>
+                <option value="beta" selected>Beta</option>
+                <option value="gamma">Gamma</option>
+            </select>
+        </form>
+        """,
+        "lxml",
+    )
+
+    form_data = _extract_modedit_form_data(soup.form)
+
+    assert form_data["tags[]"] == ["alpha", "beta"]
+
+
+def test_add_generic_module_merges_fetched_form_defaults(monkeypatch):
+    """Module creation should merge the fetched add form defaults."""
+    session = _FakeSession(_FakeResponse(303))
+    course_states = iter(
+        [
+            {"sections": [{"id": 10, "section": 1, "modules": []}]},
+            {"sections": [{"id": 10, "section": 1, "modules": [{"id": 98}]}]},
+        ]
+    )
+
+    monkeypatch.setattr(
+        "py_moodle.module._get_module_id_from_name",
+        lambda *args, **kwargs: 123,
+    )
+    monkeypatch.setattr(
+        "py_moodle.module.get_course_with_sections_and_modules",
+        lambda *args, **kwargs: next(course_states),
+    )
+    monkeypatch.setattr("py_moodle.module.time.sleep", lambda *_args: None)
+
+    new_cmid = add_generic_module(
+        session=session,
+        base_url="https://moodle.example.test",
+        sesskey="sesskey123",
+        module_name="assign",
+        course_id=5,
+        section_id=10,
+        specific_payload={
+            "name": "Essay 1",
+            "activityeditor[text]": "<p>Write an essay.</p>",
+        },
+    )
+
+    assert new_cmid == 98
+    assert session.get_calls == [
+        "https://moodle.example.test/course/modedit.php?add=assign&type=&course=5&section=1&return=0&sr=-1"
+    ]
+
+    posted_data = parse_qs(session.post_calls[0]["data"])
+    assert posted_data["gradecat"] == ["7"]
+    assert posted_data["assignsubmission_file_enabled"] == ["1"]
+    assert posted_data["name"] == ["Essay 1"]
+    assert posted_data["activityeditor[text]"] == ["<p>Write an essay.</p>"]
+
+
+@pytest.mark.parametrize("status_code", [200, 400, 404, 500, 502])
+def test_add_generic_module_rejects_non_redirect_responses(monkeypatch, status_code):
+    """Any non-redirect response should be treated as a failed module creation."""
+    session = _FakeSession(
+        _FakeResponse(status_code, '<div class="alert-danger">Validation failed</div>')
+    )
+
+    monkeypatch.setattr(
+        "py_moodle.module._get_module_id_from_name",
+        lambda *args, **kwargs: 123,
+    )
+    monkeypatch.setattr(
+        "py_moodle.module.get_course_with_sections_and_modules",
+        lambda *args, **kwargs: {"sections": [{"id": 10, "section": 1, "modules": []}]},
+    )
+
+    with pytest.raises(MoodleModuleError, match="Validation failed"):
+        add_generic_module(
+            session=session,
+            base_url="https://moodle.example.test",
+            sesskey="sesskey123",
+            module_name="assign",
+            course_id=5,
+            section_id=10,
+            specific_payload={"name": "Essay 1"},
+        )