Skip to content

Conversation

madewulf
Copy link
Member

@madewulf madewulf commented Jun 14, 2025

Add a FHIR api to retrieve org units

IA-4285 create-a-fhir-retrieval-api-for-org-units

Self proofreading checklist

  • Did I use eslint and ruff formatters?
  • Is my code clear enough and well documented?
  • Are there enough tests?
  • Documentation has been included (for new feature)

Doc

In the new readme in /api/fhir

Changes

See the readme

How to test

See the readme

Notes

We should test now the integration with an external system, that was not done yet.

Follow the Conventional Commits specification

The merge message of a pull request must follow the Conventional Commits specification.

This convention helps to automatically generate release notes.

Use lowercase for consistency.

Example:

fix: empty instance pop up

Refs: IA-3665

Note that the Jira reference is preceded by a line break.

Both the line break and the Jira reference are entered in the Add an optional extended description… field.

@kemar kemar self-requested a review June 16, 2025 08:16
Copy link
Contributor

@kemar kemar left a comment

Choose a reason for hiding this comment

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

Looks almost good to me.

I have a few suggestions that could put the iaso codebase towards more consistency though.

@@ -0,0 +1,259 @@
# FHIR Location API

This module provides a FHIR R4 compliant Location resource API that maps Iaso OrgUnit objects to FHIR Location resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This module provides a FHIR R4 compliant Location resource API that maps Iaso OrgUnit objects to FHIR Location resources.
This module provides a [FHIR R4 compliant Location resource API](https://build.fhir.org/location.html) that maps Iaso OrgUnit objects to FHIR Location resources.

@@ -0,0 +1,259 @@
# FHIR Location API
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# FHIR Location API
# FHIR (Fast Healthcare Interoperability Resources) Location API

Comment on lines +135 to +177
def _get_pagination_params(self, request):
"""Get pagination parameters from request"""
try:
page_size = int(request.query_params.get("_count", 20))
page_size = min(page_size, 100) # Max 100
except (TypeError, ValueError):
page_size = 20

try:
offset = int(request.query_params.get("_skip", 0))
except (TypeError, ValueError):
offset = 0

return page_size, offset

def _build_pagination_links(self, request, total_count: int) -> List[Dict[str, str]]:
"""Build FHIR pagination links"""
links = []

# Get pagination parameters
page_size, offset = self._get_pagination_params(request)

# Self link
self_url = request.build_absolute_uri().split("?")[0]
if request.query_params:
self_url += f"?{request.query_params.urlencode()}"
links.append({"relation": "self", "url": self_url})

# Next link
if offset + page_size < total_count:
next_params = request.query_params.copy()
next_params["_skip"] = offset + page_size
next_url = f"{request.build_absolute_uri().split('?')[0]}?{next_params.urlencode()}"
links.append({"relation": "next", "url": next_url})

# Previous link
if offset > 0:
prev_params = request.query_params.copy()
prev_params["_skip"] = max(0, offset - page_size)
prev_url = f"{request.build_absolute_uri().split('?')[0]}?{prev_params.urlencode()}"
links.append({"relation": "previous", "url": prev_url})

return links
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole pagination logic could have been implemented as a LimitOffsetPagination class:

class FHIRPaginator(pagination.LimitOffsetPagination):
    default_limit = 20
    limit_query_param = "_count"
    limit_query_description = 'Number of results to return per page.'
    offset_query_param = "_skip"
    offset_query_description = "The initial index from which to return the results."
    max_limit = 100

Avoiding mixing "views" and "pagination" logic in the same place could provide some clarity.

References:

return Response(error_response, status=status.HTTP_404_NOT_FOUND)

@action(detail=False, methods=["get"])
def metadata(self, request):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is getting metadata via a GET method a FHIR API requirement?

I'm asking because there is an existing metadata mechanism in DRF but it relies on the OPTIONS HTTP method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have put the filter and the permissions in their own files (filters.py and permissions.py) for consistency as well as renaming this one to views.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not put this file with the others into /iaso/tests/?

Is this on purpose?

path("storages/<str:storage_type>/<str:storage_customer_chosen_id>/logs", logs_per_device),
path("workflows/export/<workflow_id>/", export_workflow, name="export_workflow"),
path("workflows/import/", import_workflow, name="import_workflow"),
path("fhir/", include("iaso.api.fhir.urls")),
Copy link
Contributor

Choose a reason for hiding this comment

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

The way you implemented that makes the route unavailable in the default list. Is this done on purpose?

a

@beygorghor beygorghor changed the title Adding a FHIR API to retrieve org units IA-4285: Adding a FHIR API to retrieve org units Aug 22, 2025
@beygorghor beygorghor changed the base branch from main to develop September 9, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants