Skip to content

fix(spp_api_v2_change_request): fix 3 bugs and promote to Beta #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
emjay0921 wants to merge 1 commit into
base: 19.0
Choose a base branch
from
+106 −72
Open

fix(spp_api_v2_change_request): fix 3 bugs and promote to Beta #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 38 additions & 41 deletions spp_api_v2_change_request/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ OpenSPP API V2 - Change Request
!! source digest: sha256:ac0243c931848a9215f89c328fce09d312e30b92ac9c3c1f141bfe26b4fef453
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

.. |badge1| image:: https://img.shields.io/badge/maturity-Alpha-red.png
.. |badge1| image:: https://img.shields.io/badge/maturity-Beta-yellow.png
:target: https://odoo-community.org/page/development-status
:alt: Alpha
:alt: Beta
.. |badge2| image:: https://img.shields.io/badge/license-LGPL--3-blue.png
:target: http://www.gnu.org/licenses/lgpl-3.0-standalone.html
:alt: License: LGPL-3
Expand All @@ -35,17 +35,16 @@ numbers (CR/2024/00001) instead of database IDs for all operations.
Key Capabilities
~~~~~~~~~~~~~~~~

- Create change requests in draft status with registrant and detail
data
- Read individual change requests by reference or search with filters
(registrant, type, status, dates)
- Update detail data on draft change requests with optimistic locking
via If-Match headers
- Submit draft requests for approval workflow
- Approve, reject, or request revision on pending requests (requires
approval scope)
- Apply approved change requests to registrant records
- Reset rejected/revision requests to draft for resubmission
- Create change requests in draft status with registrant and detail data
- Read individual change requests by reference or search with filters
(registrant, type, status, dates)
- Update detail data on draft change requests with optimistic locking
via If-Match headers
- Submit draft requests for approval workflow
- Approve, reject, or request revision on pending requests (requires
approval scope)
- Apply approved change requests to registrant records
- Reset rejected/revision requests to draft for resubmission

Key Models
~~~~~~~~~~
Expand Down Expand Up @@ -78,26 +77,26 @@ To configure OAuth 2.0 clients with appropriate scopes:
by ``spp_api_v2``)
2. Configure OAuth 2.0 clients with appropriate scopes:

- ``change_request:read`` - Read and search change requests
- ``change_request:create`` - Create new change requests
- ``change_request:update`` - Update, submit, and reset requests
- ``change_request:approve`` - Approve, reject, or request revision
- ``change_request:apply`` - Apply approved changes to registrants
- ``change_request:read`` - Read and search change requests
- ``change_request:create`` - Create new change requests
- ``change_request:update`` - Update, submit, and reset requests
- ``change_request:approve`` - Approve, reject, or request revision
- ``change_request:apply`` - Apply approved changes to registrants

API Endpoints
~~~~~~~~~~~~~

- ``POST /ChangeRequest`` - Create new change request
- ``GET /ChangeRequest/{reference}`` - Read by reference
- ``GET /ChangeRequest`` - Search with filters
- ``PUT /ChangeRequest/{reference}`` - Update detail data
- ``POST /ChangeRequest/{reference}/$submit`` - Submit for approval
- ``POST /ChangeRequest/{reference}/$approve`` - Approve request
- ``POST /ChangeRequest/{reference}/$reject`` - Reject request
- ``POST /ChangeRequest/{reference}/$request-revision`` - Request
revision
- ``POST /ChangeRequest/{reference}/$apply`` - Apply to registrant
- ``POST /ChangeRequest/{reference}/$reset`` - Reset to draft
- ``POST /ChangeRequest`` - Create new change request
- ``GET /ChangeRequest/{reference}`` - Read by reference
- ``GET /ChangeRequest`` - Search with filters
- ``PUT /ChangeRequest/{reference}`` - Update detail data
- ``POST /ChangeRequest/{reference}/$submit`` - Submit for approval
- ``POST /ChangeRequest/{reference}/$approve`` - Approve request
- ``POST /ChangeRequest/{reference}/$reject`` - Reject request
- ``POST /ChangeRequest/{reference}/$request-revision`` - Request
revision
- ``POST /ChangeRequest/{reference}/$apply`` - Apply to registrant
- ``POST /ChangeRequest/{reference}/$reset`` - Reset to draft

Security
~~~~~~~~
Expand All @@ -110,12 +109,12 @@ enforces scope checks on each endpoint. Users must authenticate via the
Extension Points
~~~~~~~~~~~~~~~~

- Inherit ``ChangeRequestService`` to customize serialization,
validation, or business logic
- Override router endpoint functions to add custom validation or side
effects
- Extend the API schema by inheriting the Pydantic models in
``schemas/change_request.py``
- Inherit ``ChangeRequestService`` to customize serialization,
validation, or business logic
- Override router endpoint functions to add custom validation or side
effects
- Extend the API schema by inheriting the Pydantic models in
``schemas/change_request.py``

UI Location
~~~~~~~~~~~
Expand All @@ -129,11 +128,6 @@ Dependencies

``spp_api_v2``, ``spp_change_request_v2``

.. IMPORTANT::
This is an alpha version, the data model and design can change at any time without warning.
Only for development or testing purpose, do not use in production.
`More details on development status <https://odoo-community.org/page/development-status>`_

**Table of contents**

.. contents::
Expand Down Expand Up @@ -169,10 +163,13 @@ Maintainers
.. |maintainer-reichie020212| image:: https://github.com/reichie020212.png?size=40px
:target: https://github.com/reichie020212
:alt: reichie020212
.. |maintainer-emjay0921| image:: https://github.com/emjay0921.png?size=40px
:target: https://github.com/emjay0921
:alt: emjay0921

Current maintainers:

|maintainer-jeremi| |maintainer-gonzalesedwin1123| |maintainer-reichie020212|
|maintainer-jeremi| |maintainer-gonzalesedwin1123| |maintainer-reichie020212| |maintainer-emjay0921|

This module is part of the `OpenSPP/OpenSPP2 <https://github.com/OpenSPP/OpenSPP2/tree/19.0/spp_api_v2_change_request>`_ project on GitHub.

Expand Down
4 changes: 2 additions & 2 deletions spp_api_v2_change_request/__manifest__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
"author": "OpenSPP.org",
"website": "https://github.com/OpenSPP/OpenSPP2",
"license": "LGPL-3",
"development_status": "Alpha",
"maintainers": ["jeremi", "gonzalesedwin1123", "reichie020212"],
"development_status": "Beta",
"maintainers": ["jeremi", "gonzalesedwin1123", "reichie020212", "emjay0921"],
"depends": [
"spp_api_v2",
"spp_change_request_v2",
Expand Down
61 changes: 45 additions & 16 deletions spp_api_v2_change_request/routers/change_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@
change_request_router = APIRouter(tags=["ChangeRequest"], prefix="/ChangeRequest")


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}"
Comment on lines +43 to +45
Copy link

@gemini-code-assist gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This helper function is a good first step, but its repeated use in each of the 8 updated endpoints leads to significant code duplication. A more idiomatic and maintainable approach in FastAPI is to use a dependency.

You can convert this logic into a dependency that extracts the path parameters and provides the reconstructed reference string directly to the endpoint functions. This will centralize the path-parsing logic and make the endpoint signatures much cleaner.

Here's how you could refactor this:

  1. Define the dependency function (you can rename _build_reference):

    def get_cr_reference_from_path(
    p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
    p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
    p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
) -> str:
    """Dependency to reconstruct CR reference from path segments."""
    return f"{p1}/{p2}/{p3}"

  2. Use the dependency in your endpoints:

    @change_request_router.get("/{p1}/{p2}/{p3}", response_model=ChangeRequestResponse)
async def read_change_request(
    reference: Annotated[str, Depends(get_cr_reference_from_path)],
    env: Annotated[Environment, Depends(odoo_env)],
    # ... other dependencies
):
    # The `reference` variable is now injected by FastAPI.
    # No need for p1, p2, p3 parameters or the _build_reference call inside.
    service = ChangeRequestService(env)
    cr = service.find_by_reference(reference)
    # ...

Applying this pattern to all affected endpoints will greatly improve the code's readability and reduce redundancy.



@change_request_router.post(
"",
response_model=ChangeRequestResponse,
Expand Down Expand Up @@ -87,9 +92,11 @@ async def create_change_request(
return service.to_api_schema(cr)


@change_request_router.get("/{reference}", response_model=ChangeRequestResponse)
@change_request_router.get("/{p1}/{p2}/{p3}", response_model=ChangeRequestResponse)
async def read_change_request(
reference: Annotated[str, Path(description="CR reference (e.g., CR/2024/00001)")],
p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
env: Annotated[Environment, Depends(odoo_env)],
api_client: Annotated[dict, Depends(get_authenticated_client)],
response: Response,
Expand All @@ -106,6 +113,7 @@ async def read_change_request(
detail="Client does not have permission to read change requests",
)

reference = _build_reference(p1, p2, p3)
service = ChangeRequestService(env)
cr = service.find_by_reference(reference)

Expand Down Expand Up @@ -224,9 +232,11 @@ async def search_change_requests(
)


@change_request_router.put("/{reference}", response_model=ChangeRequestResponse)
@change_request_router.put("/{p1}/{p2}/{p3}", response_model=ChangeRequestResponse)
async def update_change_request(
reference: Annotated[str, Path()],
p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
update_data: ChangeRequestUpdate,
env: Annotated[Environment, Depends(odoo_env)],
api_client: Annotated[dict, Depends(get_authenticated_client)],
Expand All @@ -245,6 +255,7 @@ async def update_change_request(
detail="Client does not have permission to update change requests",
)

reference = _build_reference(p1, p2, p3)
service = ChangeRequestService(env)
cr = service.find_by_reference(reference)

Expand Down Expand Up @@ -284,9 +295,11 @@ async def update_change_request(
# === Actions ===


@change_request_router.post("/{reference}/$submit", response_model=ChangeRequestResponse)
@change_request_router.post("/{p1}/{p2}/{p3}/$submit", response_model=ChangeRequestResponse)
async def submit_change_request(
reference: Annotated[str, Path()],
p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
env: Annotated[Environment, Depends(odoo_env)],
api_client: Annotated[dict, Depends(get_authenticated_client)],
):
Expand All @@ -301,6 +314,7 @@ async def submit_change_request(
detail="Client does not have permission to submit change requests",
)

reference = _build_reference(p1, p2, p3)
service = ChangeRequestService(env)
cr = service.find_by_reference(reference)

Expand All @@ -321,9 +335,11 @@ async def submit_change_request(
return service.to_api_schema(cr)


@change_request_router.post("/{reference}/$approve", response_model=ChangeRequestResponse)
@change_request_router.post("/{p1}/{p2}/{p3}/$approve", response_model=ChangeRequestResponse)
async def approve_change_request(
reference: Annotated[str, Path()],
p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
env: Annotated[Environment, Depends(odoo_env)],
api_client: Annotated[dict, Depends(get_authenticated_client)],
action_data: ApproveActionData | None = None,
Expand All @@ -339,6 +355,7 @@ async def approve_change_request(
detail="Client does not have permission to approve change requests",
)

reference = _build_reference(p1, p2, p3)
service = ChangeRequestService(env)
cr = service.find_by_reference(reference)

Expand All @@ -360,9 +377,11 @@ async def approve_change_request(
return service.to_api_schema(cr)


@change_request_router.post("/{reference}/$reject", response_model=ChangeRequestResponse)
@change_request_router.post("/{p1}/{p2}/{p3}/$reject", response_model=ChangeRequestResponse)
async def reject_change_request(
reference: Annotated[str, Path()],
p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
action_data: RejectActionData,
env: Annotated[Environment, Depends(odoo_env)],
api_client: Annotated[dict, Depends(get_authenticated_client)],
Expand All @@ -378,6 +397,7 @@ async def reject_change_request(
detail="Client does not have permission to reject change requests",
)

reference = _build_reference(p1, p2, p3)
service = ChangeRequestService(env)
cr = service.find_by_reference(reference)

Expand All @@ -398,9 +418,11 @@ async def reject_change_request(
return service.to_api_schema(cr)


@change_request_router.post("/{reference}/$request-revision", response_model=ChangeRequestResponse)
@change_request_router.post("/{p1}/{p2}/{p3}/$request-revision", response_model=ChangeRequestResponse)
async def request_revision_change_request(
reference: Annotated[str, Path()],
p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
action_data: RequestRevisionActionData,
env: Annotated[Environment, Depends(odoo_env)],
api_client: Annotated[dict, Depends(get_authenticated_client)],
Expand All @@ -416,6 +438,7 @@ async def request_revision_change_request(
detail="Client does not have permission to request revision",
)

reference = _build_reference(p1, p2, p3)
service = ChangeRequestService(env)
cr = service.find_by_reference(reference)

Expand All @@ -436,9 +459,11 @@ async def request_revision_change_request(
return service.to_api_schema(cr)


@change_request_router.post("/{reference}/$apply", response_model=ChangeRequestResponse)
@change_request_router.post("/{p1}/{p2}/{p3}/$apply", response_model=ChangeRequestResponse)
async def apply_change_request(
reference: Annotated[str, Path()],
p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
env: Annotated[Environment, Depends(odoo_env)],
api_client: Annotated[dict, Depends(get_authenticated_client)],
):
Expand All @@ -453,6 +478,7 @@ async def apply_change_request(
detail="Client does not have permission to apply change requests",
)

reference = _build_reference(p1, p2, p3)
service = ChangeRequestService(env)
cr = service.find_by_reference(reference)

Expand All @@ -473,9 +499,11 @@ async def apply_change_request(
return service.to_api_schema(cr)


@change_request_router.post("/{reference}/$reset", response_model=ChangeRequestResponse)
@change_request_router.post("/{p1}/{p2}/{p3}/$reset", response_model=ChangeRequestResponse)
async def reset_change_request(
reference: Annotated[str, Path()],
p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")],
p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")],
p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")],
env: Annotated[Environment, Depends(odoo_env)],
api_client: Annotated[dict, Depends(get_authenticated_client)],
):
Expand All @@ -490,6 +518,7 @@ async def reset_change_request(
detail="Client does not have permission to reset change requests",
)

reference = _build_reference(p1, p2, p3)
service = ChangeRequestService(env)
cr = service.find_by_reference(reference)

Expand Down
21 changes: 18 additions & 3 deletions spp_api_v2_change_request/services/change_request_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ class ChangeRequestService:
"""Service for Change Request resource CRUD and mapping."""

def __init__(self, env: Environment):
self.env = env
ctx = dict(env.context, mail_create_nolog=True, tracking_disable=True)
self.env = env(context=ctx)

def find_by_reference(self, reference: str):
"""
Expand Down Expand Up @@ -161,7 +162,7 @@ def _serialize_detail(self, detail) -> dict[str, Any]:
# Get fields from the model
model_fields = detail._fields

# Skip internal and computed fields
# Skip internal, computed, and mail.thread fields
skip_fields = {
"id",
"create_uid",
Expand All @@ -175,6 +176,20 @@ def _serialize_detail(self, detail) -> dict[str, Any]:
"registrant_id",
"approval_state",
"is_applied",
# mail.thread fields
"message_ids",
"message_follower_ids",
"message_partner_ids",
"message_is_follower",
"has_message",
"message_needaction",
"message_needaction_counter",
"message_has_error",
"message_has_error_counter",
"message_attachment_count",
"message_has_sms_error",
"message_main_attachment_id",
"website_message_ids",
}
Comment on lines +179 to 193
Copy link

@gemini-code-assist gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Hardcoding the list of mail.thread fields to skip is effective for now, but it can be brittle. If the mail.thread mixin is updated in a future version of Odoo, this list would need to be manually updated to avoid leaking new fields.

A more robust and maintainable approach is to programmatically retrieve the fields from the mail.thread model. This ensures your exclusion list is always synchronized with the mail.thread definition.

Consider this alternative approach:

# In _serialize_detail method

# Get fields from the model
model_fields = detail._fields

# Base fields to skip
skip_fields = {
    "id",
    "create_uid",
    "create_date",
    "write_uid",
    "write_date",
    "__last_update",
    "display_name",
    "change_request_id",
    "registrant_id",
    "approval_state",
    "is_applied",
}

# Programmatically add all mail.thread fields
mail_thread_fields = self.env["mail.thread"]._fields.keys()
skip_fields.update(mail_thread_fields)

for field_name, field in model_fields.items():
    # ... rest of the method

This makes the code more resilient to changes in the underlying Odoo framework.


for field_name, field in model_fields.items():
Expand Down Expand Up @@ -451,5 +466,5 @@ def apply(self, cr):
def reset_to_draft(self, cr):
"""Reset rejected/revision CR to draft."""
if cr.approval_state not in ("rejected", "revision"):
raise UserError("Only rejected or revision-requested change requests " "can be reset to draft")
raise UserError("Only rejected or revision-requested change requests can be reset to draft")
cr.action_reset_to_draft()
Loading
Loading