refactor: move the onboarding routes into core/onboarding_routes.py (#1594)

First of the route moves from Discussion #1536, following the plan table
there: the three onboarding routes (`GET` and `POST /api/onboarding`,
`POST /api/onboarding/skip`) leave `main.py` for
`core/onboarding_routes.py`, registered with
`register_onboarding_routes(app, login_required)` the way
`release_inspect_routes` takes it. `main.py` goes down by 72 lines and
the handler bodies are the same code (I diffed them after de-indenting:
the only difference is the local name of the error tuple).

There are two commits so the move can be reviewed on its own. The first
adds 15 tests for these routes, which had none, and they pass on current
`main` before anything moves. They pin the behaviour I most wanted to
keep: a 401 without a session when auth is on, a 403 for a signed-in
non-admin (that check lives in `login_required` and goes by path, which
is why the same decorator has to be passed in), the 400 on an empty
body, the result passed through on success and failure, the 500 with the
error text on an operational error, and open access when no auth is
configured. The second commit is the move, and no test changed in it.

What I checked on the move:

- The Flask URL map (every rule, its methods and its endpoint name) is
identical before and after: 77 rules, the three onboarding ones
included.
- Full suite, ruff and basedpyright are clean.
- I ran the app from this branch and stepped through the setup wizard in
a browser. It loads its steps from the moved route and goes from Search
Mode to Metadata Provider, and `POST /api/onboarding/skip` and the
empty-body 400 answer as before.

Two small things that do change. The log lines for these routes now come
from `shelfmark.core.onboarding_routes` instead of `main`, and the error
tuple is a local copy (`_ONBOARDING_ERRORS`, the same classes as
`_IMPORT_OPERATIONAL_ERRORS`) so the module does not import `main`.
Nothing else is different.

shelfmark/core/onboarding_routes.py

@@ -0,0 +1,100 @@
+"""First-run onboarding: ``/api/onboarding`` and ``/api/onboarding/skip``."""
+
+from __future__ import annotations
+
+import sqlite3
+from importlib import import_module
+from typing import TYPE_CHECKING
+
+from flask import jsonify, request
+
+from shelfmark.core.logger import setup_logger
+
+if TYPE_CHECKING:
+    from collections.abc import Callable
+
+    from flask import Flask
+    from werkzeug.wrappers import Response
+
+logger = setup_logger(__name__)
+
+_ONBOARDING_ERRORS = (ImportError, OSError, RuntimeError, TypeError, ValueError, sqlite3.Error)
+
+
+def register_onboarding_routes(
+    app: Flask,
+    login_required: Callable[..., Callable[..., Response | tuple[Response, int]]],
+) -> None:
+    """Register the onboarding routes.
+
+    ``login_required`` is the app's decorator. It is also what limits these paths to an
+    admin session when authentication is on, so it has to be the same one the other routes use.
+    """
+
+    @app.route("/api/onboarding", methods=["GET"])
+    @login_required
+    def api_onboarding_get() -> Response | tuple[Response, int]:
+        """Get onboarding configuration including steps, fields, and current values.
+
+        Returns:
+            flask.Response: JSON with onboarding steps and values.
+
+        """
+        try:
+            # Ensure settings are registered
+            import_module("shelfmark.config.settings")
+            from shelfmark.core.onboarding import get_onboarding_config
+
+            config = get_onboarding_config()
+            return jsonify(config)
+        except _ONBOARDING_ERRORS as e:
+            logger.error_trace(f"Onboarding get error: {e}")
+            return jsonify({"error": str(e)}), 500
+
+    @app.route("/api/onboarding", methods=["POST"])
+    @login_required
+    def api_onboarding_save() -> Response | tuple[Response, int]:
+        """Save onboarding settings and mark as complete.
+
+        Request Body:
+            JSON object with all onboarding field values
+
+        Returns:
+            flask.Response: JSON with success/error status.
+
+        """
+        try:
+            # Ensure settings are registered
+            import_module("shelfmark.config.settings")
+            from shelfmark.core.onboarding import save_onboarding_settings
+
+            data = request.get_json(silent=True)
+            if not data:
+                return jsonify({"success": False, "message": "No data provided"}), 400
+
+            result = save_onboarding_settings(data)
+
+            if result["success"]:
+                return jsonify(result)
+            return jsonify(result), 400
+        except _ONBOARDING_ERRORS as e:
+            logger.error_trace(f"Onboarding save error: {e}")
+            return jsonify({"error": str(e)}), 500
+
+    @app.route("/api/onboarding/skip", methods=["POST"])
+    @login_required
+    def api_onboarding_skip() -> Response | tuple[Response, int]:
+        """Skip onboarding and mark as complete without saving any settings.
+
+        Returns:
+            flask.Response: JSON with success status.
+
+        """
+        try:
+            from shelfmark.core.onboarding import mark_onboarding_complete
+
+            mark_onboarding_complete()
+            return jsonify({"success": True, "message": "Onboarding skipped"})
+        except _ONBOARDING_ERRORS as e:
+            logger.error_trace(f"Onboarding skip error: {e}")
+            return jsonify({"error": str(e)}), 500

shelfmark/main.py

@@ -70,6 +70,7 @@ from shelfmark.core.notifications import (
     notify_admin,
     notify_user,
 )
+from shelfmark.core.onboarding_routes import register_onboarding_routes
 from shelfmark.core.prefix_middleware import PrefixMiddleware
 from shelfmark.core.release_inspect_routes import register_release_inspect_routes
 from shelfmark.core.request_helpers import (
@@ -3568,80 +3569,7 @@ def api_release_source_record(source_name: str, record_id: str) -> Response | tu
 register_settings_routes(app, login_required)
 
 
-# =============================================================================
-# Onboarding API
-# =============================================================================
-
-
-@app.route("/api/onboarding", methods=["GET"])
-@login_required
-def api_onboarding_get() -> Response | tuple[Response, int]:
-    """Get onboarding configuration including steps, fields, and current values.
-
-    Returns:
-        flask.Response: JSON with onboarding steps and values.
-
-    """
-    try:
-        # Ensure settings are registered
-        import_module("shelfmark.config.settings")
-        from shelfmark.core.onboarding import get_onboarding_config
-
-        config = get_onboarding_config()
-        return jsonify(config)
-    except _IMPORT_OPERATIONAL_ERRORS as e:
-        logger.error_trace(f"Onboarding get error: {e}")
-        return jsonify({"error": str(e)}), 500
-
-
-@app.route("/api/onboarding", methods=["POST"])
-@login_required
-def api_onboarding_save() -> Response | tuple[Response, int]:
-    """Save onboarding settings and mark as complete.
-
-    Request Body:
-        JSON object with all onboarding field values
-
-    Returns:
-        flask.Response: JSON with success/error status.
-
-    """
-    try:
-        # Ensure settings are registered
-        import_module("shelfmark.config.settings")
-        from shelfmark.core.onboarding import save_onboarding_settings
-
-        data = request.get_json(silent=True)
-        if not data:
-            return jsonify({"success": False, "message": "No data provided"}), 400
-
-        result = save_onboarding_settings(data)
-
-        if result["success"]:
-            return jsonify(result)
-        return jsonify(result), 400
-    except _IMPORT_OPERATIONAL_ERRORS as e:
-        logger.error_trace(f"Onboarding save error: {e}")
-        return jsonify({"error": str(e)}), 500
-
-
-@app.route("/api/onboarding/skip", methods=["POST"])
-@login_required
-def api_onboarding_skip() -> Response | tuple[Response, int]:
-    """Skip onboarding and mark as complete without saving any settings.
-
-    Returns:
-        flask.Response: JSON with success status.
-
-    """
-    try:
-        from shelfmark.core.onboarding import mark_onboarding_complete
-
-        mark_onboarding_complete()
-        return jsonify({"success": True, "message": "Onboarding skipped"})
-    except _IMPORT_OPERATIONAL_ERRORS as e:
-        logger.error_trace(f"Onboarding skip error: {e}")
-        return jsonify({"error": str(e)}), 500
+register_onboarding_routes(app, login_required)
 
 
 # Catch-all route for React Router (must be last)

tests/core/test_onboarding_routes.py

@@ -0,0 +1,153 @@
+"""API tests for the onboarding endpoints: auth gating, validation and error handling."""
+
+from __future__ import annotations
+
+import importlib
+from unittest.mock import patch
+
+import pytest
+
+
+@pytest.fixture(scope="module")
+def main_module():
+    """Import `shelfmark.main` with background startup disabled."""
+    with patch("shelfmark.download.orchestrator.start"):
+        import shelfmark.main as main
+
+        importlib.reload(main)
+        return main
+
+
+@pytest.fixture
+def client(main_module):
+    return main_module.app.test_client()
+
+
+def _sign_in(client, *, is_admin: bool) -> None:
+    with client.session_transaction() as sess:
+        sess["user_id"] = "someone"
+        sess["db_user_id"] = 7
+        sess["is_admin"] = is_admin
+
+
+@pytest.fixture
+def builtin_auth(main_module):
+    with patch.object(main_module, "get_auth_mode", return_value="builtin"):
+        yield
+
+
+@pytest.fixture
+def admin(client, builtin_auth):
+    _sign_in(client, is_admin=True)
+    return client
+
+
+def test_onboarding_get_returns_the_config(admin):
+    with patch(
+        "shelfmark.core.onboarding.get_onboarding_config", return_value={"steps": [], "values": {}}
+    ):
+        resp = admin.get("/api/onboarding")
+
+    assert resp.status_code == 200
+    assert resp.json == {"steps": [], "values": {}}
+
+
+def test_onboarding_get_reports_an_operational_error(admin):
+    with patch("shelfmark.core.onboarding.get_onboarding_config", side_effect=RuntimeError("boom")):
+        resp = admin.get("/api/onboarding")
+
+    assert resp.status_code == 500
+    assert resp.json == {"error": "boom"}
+
+
+def test_onboarding_save_rejects_an_empty_body(admin):
+    with patch("shelfmark.core.onboarding.save_onboarding_settings") as save:
+        resp = admin.post("/api/onboarding", json={})
+
+    assert resp.status_code == 400
+    assert resp.json == {"success": False, "message": "No data provided"}
+    save.assert_not_called()
+
+
+def test_onboarding_save_passes_the_body_on_and_returns_the_result(admin):
+    result = {"success": True, "message": "Saved"}
+    with patch("shelfmark.core.onboarding.save_onboarding_settings", return_value=result) as save:
+        resp = admin.post("/api/onboarding", json={"SEARCH_MODE": "universal"})
+
+    assert resp.status_code == 200
+    assert resp.json == result
+    save.assert_called_once_with({"SEARCH_MODE": "universal"})
+
+
+def test_onboarding_save_returns_400_with_the_result_when_it_fails(admin):
+    result = {"success": False, "message": "Invalid value", "errors": ["x"]}
+    with patch("shelfmark.core.onboarding.save_onboarding_settings", return_value=result):
+        resp = admin.post("/api/onboarding", json={"SEARCH_MODE": "nope"})
+
+    assert resp.status_code == 400
+    assert resp.json == result
+
+
+def test_onboarding_save_reports_an_operational_error(admin):
+    with patch("shelfmark.core.onboarding.save_onboarding_settings", side_effect=OSError("disk")):
+        resp = admin.post("/api/onboarding", json={"a": 1})
+
+    assert resp.status_code == 500
+    assert resp.json == {"error": "disk"}
+
+
+def test_onboarding_skip_marks_it_complete(admin):
+    with patch("shelfmark.core.onboarding.mark_onboarding_complete") as mark:
+        resp = admin.post("/api/onboarding/skip")
+
+    assert resp.status_code == 200
+    assert resp.json == {"success": True, "message": "Onboarding skipped"}
+    mark.assert_called_once_with()
+
+
+def test_onboarding_skip_reports_an_operational_error(admin):
+    with patch("shelfmark.core.onboarding.mark_onboarding_complete", side_effect=OSError("disk")):
+        resp = admin.post("/api/onboarding/skip")
+
+    assert resp.status_code == 500
+    assert resp.json == {"error": "disk"}
+
+
+@pytest.mark.parametrize(
+    ("method", "path"),
+    [("get", "/api/onboarding"), ("post", "/api/onboarding"), ("post", "/api/onboarding/skip")],
+)
+def test_onboarding_needs_a_session_when_auth_is_on(client, builtin_auth, method, path):
+    resp = getattr(client, method)(path)
+
+    assert resp.status_code == 401
+
+
+@pytest.mark.parametrize(
+    ("method", "path"),
+    [("get", "/api/onboarding"), ("post", "/api/onboarding"), ("post", "/api/onboarding/skip")],
+)
+def test_onboarding_is_admin_only_for_a_signed_in_user(client, builtin_auth, method, path):
+    _sign_in(client, is_admin=False)
+    with (
+        patch("shelfmark.core.onboarding.get_onboarding_config") as get_config,
+        patch("shelfmark.core.onboarding.save_onboarding_settings") as save,
+        patch("shelfmark.core.onboarding.mark_onboarding_complete") as mark,
+    ):
+        resp = getattr(client, method)(path, json={"a": 1})
+
+    assert resp.status_code == 403
+    assert resp.json == {"error": "Admin access required"}
+    get_config.assert_not_called()
+    save.assert_not_called()
+    mark.assert_not_called()
+
+
+def test_onboarding_is_open_when_no_auth_is_configured(main_module, client):
+    with (
+        patch.object(main_module, "get_auth_mode", return_value="none"),
+        patch("shelfmark.core.onboarding.get_onboarding_config", return_value={"steps": []}),
+    ):
+        resp = client.get("/api/onboarding")
+
+    assert resp.status_code == 200