fix(api): consistent error statuses on change-request endpoints

spp_api_v2_change_request/README.rst

@@ -628,6 +628,21 @@ A complete workflow from creation to application:
 Changelog
 =========
 
+19.0.2.0.3
+~~~~~~~~~~
+
+- fix(api): error statuses on change-request endpoints are now
+  consistent across the module and aligned with the platform's global
+  FastAPI error handler. ``ValidationError`` on a state transition
+  returns ``422 Unprocessable Entity`` (previously ``409 Conflict``),
+  matching what create and update already returned for the same
+  condition — a missing rejection reason or revision notes is invalid
+  input, not a conflict. ``MissingError`` returns ``404 Not Found``. An
+  authorization failure on create returns ``403 Forbidden`` (previously
+  swallowed by the generic handler as a ``500``). The ``403`` response
+  body is now a fixed generic detail instead of the raw Odoo message,
+  which named models and record rules (anti-enumeration).
+
 19.0.2.0.2
 ~~~~~~~~~~
 

spp_api_v2_change_request/__manifest__.py

@@ -1,7 +1,7 @@
 {  # pylint: disable=pointless-statement
     "name": "OpenSPP API V2 - Change Request",
     "category": "OpenSPP/Integration",
-    "version": "19.0.2.0.2",
+    "version": "19.0.2.0.3",
     "sequence": 1,
     "author": "OpenSPP.org",
     "website": "https://github.com/OpenSPP/OpenSPP2",

spp_api_v2_change_request/readme/HISTORY.md

@@ -1,3 +1,7 @@
+### 19.0.2.0.3
+
+- fix(api): error statuses on change-request endpoints are now consistent across the module and aligned with the platform's global FastAPI error handler. `ValidationError` on a state transition returns `422 Unprocessable Entity` (previously `409 Conflict`), matching what create and update already returned for the same condition — a missing rejection reason or revision notes is invalid input, not a conflict. `MissingError` returns `404 Not Found`. An authorization failure on create returns `403 Forbidden` (previously swallowed by the generic handler as a `500`). The `403` response body is now a fixed generic detail instead of the raw Odoo message, which named models and record rules (anti-enumeration).
+
 ### 19.0.2.0.2
 
 - fix(api): an authorization failure on a change-request state transition now returns `403 Forbidden` instead of `409 Conflict`. `AccessError` subclasses `UserError` in Odoo, so all six state-transition endpoints — `$submit` / `$approve` / `$reject` / `$request-revision` / `$apply` / `$reset` — which caught `UserError` and returned a conflict, reported permission failures as conflicts. A client is then told to resolve a conflict it cannot see, and one that retries on 409 loops on a permission error that will never clear. Reachable on `$apply` in particular now that applying requires the change-request manager role, where the endpoint's own scope check already returned 403, so the same endpoint reported two authorization failures with different statuses. `AccessDenied` maps to 403 the same way, matching the platform's global FastAPI error handler.

spp_api_v2_change_request/routers/change_request.py

@@ -6,7 +6,13 @@
 from urllib.parse import urlencode
 
 from odoo.api import Environment
-from odoo.exceptions import AccessDenied, AccessError, UserError, ValidationError
+from odoo.exceptions import (
+    AccessDenied,
+    AccessError,
+    MissingError,
+    UserError,
+    ValidationError,
+)
 
 from odoo.addons.fastapi.dependencies import odoo_env
 from odoo.addons.spp_api_v2.middleware.auth import get_authenticated_client
@@ -55,13 +61,36 @@ def _status_for_odoo_error(exc: Exception) -> int:
 
     ``AccessDenied`` is grouped with ``AccessError``: both report an
     authorization failure, and the platform's global handler
-    (``fastapi.error_handlers``) maps the pair to 403 the same way.
+    (``fastapi.error_handlers``) maps the pair to 403 the same way. That
+    handler is also the model for ``MissingError`` -> 404. ``ValidationError``
+    (also a ``UserError`` subclass) is invalid input and maps to 422, the
+    status the create and update endpoints use for the same condition. Only a
+    plain ``UserError`` -- a state-machine refusal such as "only pending
+    change requests can be rejected" -- is a genuine conflict.
     """
     if isinstance(exc, (AccessError, AccessDenied)):
         return status.HTTP_403_FORBIDDEN
+    if isinstance(exc, MissingError):
+        return status.HTTP_404_NOT_FOUND
+    if isinstance(exc, ValidationError):
+        return status.HTTP_422_UNPROCESSABLE_ENTITY
     return status.HTTP_409_CONFLICT
 
 
+def _detail_for_odoo_error(exc: Exception) -> str:
+    """Client-facing detail for an Odoo exception.
+
+    An ``AccessError`` message names models, records and rules -- internals a
+    client must not be able to enumerate -- so the authorization branch
+    returns a fixed generic detail. Other user-facing errors pass through:
+    their text is written for the actor ("Only pending change requests can be
+    rejected", "Rejection reason is required").
+    """
+    if isinstance(exc, (AccessError, AccessDenied)):
+        return "Not authorized to perform this action"
+    return str(exc)
+
+
 def _build_reference(p1: str, p2: str, p3: str) -> str:
     """Reconstruct CR reference from path segments (e.g., CR/2026/00001)."""
     return f"{p1}/{p2}/{p3}"
@@ -96,6 +125,14 @@ async def create_change_request(
 
     try:
         cr = service.create(cr_data, source=source)
+    except (AccessError, AccessDenied) as e:
+        # Must precede the bare except below, which reports 500 and would
+        # shadow an authorization failure the way UserError once shadowed
+        # AccessError on the transition endpoints.
+        raise HTTPException(
+            status_code=_status_for_odoo_error(e),
+            detail=_detail_for_odoo_error(e),
+        ) from e
     except ValidationError as e:
         raise HTTPException(
             status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
@@ -392,7 +429,7 @@ async def submit_change_request(
     except UserError as e:
         raise HTTPException(
             status_code=_status_for_odoo_error(e),
-            detail=str(e),
+            detail=_detail_for_odoo_error(e),
         ) from e
 
     return service.to_api_schema(cr)
@@ -434,7 +471,7 @@ async def approve_change_request(
     except UserError as e:
         raise HTTPException(
             status_code=_status_for_odoo_error(e),
-            detail=str(e),
+            detail=_detail_for_odoo_error(e),
         ) from e
 
     return service.to_api_schema(cr)
@@ -475,7 +512,7 @@ async def reject_change_request(
     except UserError as e:
         raise HTTPException(
             status_code=_status_for_odoo_error(e),
-            detail=str(e),
+            detail=_detail_for_odoo_error(e),
         ) from e
 
     return service.to_api_schema(cr)
@@ -516,7 +553,7 @@ async def request_revision_change_request(
     except UserError as e:
         raise HTTPException(
             status_code=_status_for_odoo_error(e),
-            detail=str(e),
+            detail=_detail_for_odoo_error(e),
         ) from e
 
     return service.to_api_schema(cr)
@@ -556,7 +593,7 @@ async def apply_change_request(
     except UserError as e:
         raise HTTPException(
             status_code=_status_for_odoo_error(e),
-            detail=str(e),
+            detail=_detail_for_odoo_error(e),
         ) from e
 
     return service.to_api_schema(cr)
@@ -596,7 +633,7 @@ async def reset_change_request(
     except UserError as e:
         raise HTTPException(
             status_code=_status_for_odoo_error(e),
-            detail=str(e),
+            detail=_detail_for_odoo_error(e),
         ) from e
 
     return service.to_api_schema(cr)

spp_api_v2_change_request/static/description/index.html

@@ -947,6 +947,22 @@ <h2>Changelog</h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.0.3</h1>
+<ul class="simple">
+<li>fix(api): error statuses on change-request endpoints are now
+consistent across the module and aligned with the platform’s global
+FastAPI error handler. <tt class="docutils literal">ValidationError</tt> on a state transition
+returns <tt class="docutils literal">422 Unprocessable Entity</tt> (previously <tt class="docutils literal">409 Conflict</tt>),
+matching what create and update already returned for the same
+condition — a missing rejection reason or revision notes is invalid
+input, not a conflict. <tt class="docutils literal">MissingError</tt> returns <tt class="docutils literal">404 Not Found</tt>. An
+authorization failure on create returns <tt class="docutils literal">403 Forbidden</tt> (previously
+swallowed by the generic handler as a <tt class="docutils literal">500</tt>). The <tt class="docutils literal">403</tt> response
+body is now a fixed generic detail instead of the raw Odoo message,
+which named models and record rules (anti-enumeration).</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.0.2</h1>
 <ul class="simple">
 <li>fix(api): an authorization failure on a change-request state
@@ -965,14 +981,14 @@ <h1>19.0.2.0.2</h1>
 handler.</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>fix: skip field types before getattr and isolate detail prefetch
 (#129)</li>
 </ul>
 </div>
-<div class="section" id="section-3">
+<div class="section" id="section-4">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_api_v2_change_request/tests/test_error_status_mapping.py

@@ -18,13 +18,17 @@
 import inspect
 from unittest.mock import patch
 
-from odoo.exceptions import AccessDenied, AccessError, UserError, ValidationError
+from odoo.exceptions import AccessDenied, AccessError, MissingError, UserError, ValidationError
 from odoo.tests import TransactionCase, tagged
 
 from odoo.addons.fastapi.tests.common import FastAPITransactionCase
 from odoo.addons.spp_api_v2.middleware.auth import get_authenticated_client
 
-from ..routers.change_request import _status_for_odoo_error, change_request_router
+from ..routers.change_request import (
+    _detail_for_odoo_error,
+    _status_for_odoo_error,
+    change_request_router,
+)
 from ..services.change_request_service import ChangeRequestService
 from .common import ChangeRequestTestCase
 
@@ -57,9 +61,30 @@ def test_access_denied_is_forbidden(self):
     def test_plain_user_error_is_conflict(self):
         self.assertEqual(_status_for_odoo_error(UserError("wrong state")), 409)
 
-    def test_validation_error_is_conflict(self):
-        """Documents current behaviour; arguably 422, but out of scope here."""
-        self.assertEqual(_status_for_odoo_error(ValidationError("bad")), 409)
+    def test_validation_error_is_unprocessable(self):
+        """A validation failure is invalid input, and reports 422 -- the same
+        status the create and update endpoints use for the same condition."""
+        self.assertEqual(_status_for_odoo_error(ValidationError("bad")), 422)
+
+    def test_missing_error_is_not_found(self):
+        """A record that vanished mid-transition is 404, mirroring the
+        platform's global handler (``fastapi.error_handlers``)."""
+        self.assertEqual(_status_for_odoo_error(MissingError("gone")), 404)
+
+    def test_forbidden_detail_is_generic(self):
+        """An AccessError message carries model names and rule text; the
+        client gets a generic detail instead (anti-enumeration)."""
+        detail = _detail_for_odoo_error(AccessError("secret record rule on spp.change.request"))
+        self.assertNotIn("secret", detail)
+        self.assertNotIn("spp.change.request", detail)
+        self.assertEqual(detail, _detail_for_odoo_error(AccessDenied()))
+
+    def test_non_forbidden_detail_passes_through(self):
+        self.assertEqual(_detail_for_odoo_error(UserError("wrong state")), "wrong state")
+        self.assertEqual(
+            _detail_for_odoo_error(ValidationError("Rejection reason is required")),
+            "Rejection reason is required",
+        )
 
     def test_access_error_is_not_shadowed_by_its_base_class(self):
         """The whole bug: AccessError *is* a UserError, so order matters."""
@@ -176,8 +201,33 @@ def test_reject_reports_plain_user_error_as_conflict(self):
             response = self._post("$reject", json={"reason": "duplicate request"})
         self.assertEqual(response.status_code, 409)
 
-    def test_reject_reports_validation_error_as_conflict(self):
-        """Documents current behaviour; arguably 422, but out of scope here."""
+    def test_reject_reports_validation_error_as_unprocessable(self):
         with patch.object(ChangeRequestService, "reject", side_effect=ValidationError("bad")):
             response = self._post("$reject", json={"reason": "duplicate request"})
-        self.assertEqual(response.status_code, 409)
+        self.assertEqual(response.status_code, 422)
+
+    def test_reject_forbidden_detail_is_generic(self):
+        with patch.object(
+            ChangeRequestService,
+            "reject",
+            side_effect=AccessError("record rule on spp.change.request denied user 7"),
+        ):
+            response = self._post("$reject", json={"reason": "duplicate request"})
+        self.assertEqual(response.status_code, 403)
+        self.assertNotIn("record rule", response.json()["detail"])
+
+    def test_create_reports_access_error_as_forbidden(self):
+        """create() used to swallow AccessError in its bare except and report
+        500; an authorization failure there is 403 like everywhere else."""
+        payload = {
+            "type": "ChangeRequest",
+            "requestType": {"code": "edit_individual"},
+            "registrant": {"system": "urn:openspp:vocab:id-type", "value": "TEST-123"},
+            "detail": {"given_name": "Blocked"},
+        }
+        with (
+            patch.object(ChangeRequestService, "create", side_effect=AccessError("denied")),
+            self._create_test_client() as client,
+        ):
+            response = client.post("/ChangeRequest", json=payload)
+        self.assertEqual(response.status_code, 403)