Simplify CORS in Dockerized madrona-portal by pollardld · Pull Request #33 · Ecotrust/mp-map-groups

pollardld · 2026-04-10T23:12:29Z

DRF API views replacing the rpc4django /rpc endpoint for the mapgroups app.

Replaces these former XML-RPC methods:
get_sharing_groups, update_map_group

Adds API endpoints

Copilot

Pull request overview

This PR replaces the legacy rpc4django-based RPC interface for mapgroups with Django REST Framework (DRF) API views, intended to expose equivalent functionality via /api/... endpoints.

Changes:

Removed the rpc4django requirement (commented out in requirements.txt).
Added new DRF API views to list sharing groups and patch-update a map group.
Added api_urlpatterns in mapgroups/urls.py to route the new API endpoints.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.

File	Description
requirements.txt	Removes `rpc4django` from declared requirements (now only a comment).
mapgroups/urls.py	Adds `api_urlpatterns` and imports the new DRF views for API routing.
mapgroups/api.py	Introduces DRF `APIView` endpoints to replace prior RPC methods.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T23:16:02Z

+from mapgroups.api import SharingGroupListView, MapGroupUpdateView  # noqa: E402
+
+api_urlpatterns = [
+    re_path(r'^sharing-groups/$', SharingGroupListView.as_view()),
+    re_path(r'^map-groups/(?P<pk>\d+)/$', MapGroupUpdateView.as_view()),
+]


Importing DRF API views at module import time makes mapgroups.urls unimportable unless djangorestframework is installed, even for projects that only use the HTML views. Consider moving these imports into a separate mapgroups/api_urls.py module or guarding them with a try/except so the non-API URLconf doesn’t hard-require DRF.

Suggested change

from mapgroups.api import SharingGroupListView, MapGroupUpdateView # noqa: E402

api_urlpatterns = [

re_path(r'^sharing-groups/$', SharingGroupListView.as_view()),

re_path(r'^map-groups/(?P<pk>\d+)/$', MapGroupUpdateView.as_view()),

]

try:

from mapgroups.api import SharingGroupListView, MapGroupUpdateView # noqa: E402

except ImportError:

api_urlpatterns = []

else:

api_urlpatterns = [

re_path(r'^sharing-groups/$', SharingGroupListView.as_view()),

re_path(r'^map-groups/(?P<pk>\d+)/$', MapGroupUpdateView.as_view()),

]

We are explicitly installing and importing DRF, so I recommend ignoring this suggestion as being overwrought.

Copilot · 2026-04-10T23:16:02Z

+api_urlpatterns = [
+    re_path(r'^sharing-groups/$', SharingGroupListView.as_view()),
+    re_path(r'^map-groups/(?P<pk>\d+)/$', MapGroupUpdateView.as_view()),
+]


api_urlpatterns is defined here but nothing in this repository includes it (e.g., tests/urls.py only includes mapgroups.urls.urls()). If these endpoints are intended to be reachable, wire them into the package’s exported urls() (or provide a documented include helper) and/or update the test URLconf accordingly.

Suggested change

]

]

def api_urls(namespace='mapgroups-api'):

"""Returns a 3-tuple for use with include() for API endpoints.

This exposes the DRF routes defined in ``api_urlpatterns`` so projects

can mount them explicitly, e.g. under an ``/api/`` prefix.

"""

return (api_urlpatterns, 'mapgroups', namespace)

This is INTERESTING! Does this mean this could support writing API calls in different modules (like mp-map-groups) and having them hosted at 'URL/api/' without the core module (madrona-portal) needing to define these?

That would be WAY better than madrona-portal assuming mp-map-groups is installed and hard-coding its inclusion in its own urls.py!

Copilot · 2026-04-10T23:16:02Z

+        for membership in request.user.mapgroupmember_set.all():
+            group = membership.map_group
+            members = sorted(
+                member.user_name_for_group()
+                for member in group.mapgroupmember_set.all()
+            )
+            data.append({
+                'group_name': group.name,
+                'group_slug': group.permission_group.name,
+                'members': members,
+                'is_mapgroup': True,
+            })


SharingGroupListView.get() performs per-group member queries (group.mapgroupmember_set.all() inside a loop), which will become N+1 DB queries for users in many groups. Consider using select_related/prefetch_related to fetch memberships, groups, and member users in bulk, and build the response from the prefetched relations.

Copilot · 2026-04-10T23:16:02Z

+    Returns all map groups the authenticated user belongs to, plus any
+    public sharing groups defined in settings.SHARING_TO_PUBLIC_GROUPS.
+    """


Accessing settings.SHARING_TO_PUBLIC_GROUPS directly will raise AttributeError in environments that don’t define it, turning this endpoint into a 500. Prefer getattr(settings, 'SHARING_TO_PUBLIC_GROUPS', []) (or an explicit default) so the API degrades safely when the setting is absent.

Copilot · 2026-04-10T23:16:03Z

+    def patch(self, request: Request, pk: int) -> Response:
+        mg = get_object_or_404(MapGroup, id=pk, owner=request.user)
+        options: dict[str, Any] = request.data
+        changed = False
+
+        if options.get('update_name'):
+            mg.name = options['name']
+            changed = True
+
+        if options.get('update_blurb'):
+            mg.blurb = options['blurb']
+            changed = True
+
+        if options.get('update_is_open'):
+            mg.is_open = options['is_open']
+            changed = True


MapGroupUpdateView.patch() can raise KeyError (and return a 500) when update_name/update_blurb/update_is_open is truthy but the corresponding field key is missing from the body. Use a DRF serializer (or explicit validation) to validate presence/types and return a 400 with a clear error message instead of relying on direct indexing into request.data.

Copilot · 2026-04-10T23:16:03Z

+class SharingGroupListView(APIView):
+    """GET /api/sharing-groups/


New API endpoints are introduced here but there are no tests covering authentication, response shape, and update behavior (success + validation failures). Since the repo already has a Django test suite, add API tests to prevent regressions when replacing the RPC functionality.

Copilot · 2026-04-10T23:16:03Z

+"""DRF API views replacing the rpc4django /rpc endpoint for the mapgroups app.
+
+Replaces these former XML-RPC methods:
+    get_sharing_groups, update_map_group
+"""


The PR title mentions simplifying CORS in a Dockerized portal, but the changes in this PR primarily remove rpc4django and add DRF API views/endpoints for mapgroups. Please update the PR title/description to accurately describe the scope, or split the work if CORS changes are intended but missing.

rhodges

Please address Copilot's concerns:

Rename the PR to clarify that you are replacing RPC API with DRF
Handle DNE exceptions in api.py
Rewrite API inclusion to see if we can remove explicit definitions for mp-map-groups (and other modules) in madrona-portal's urls.py file
See if Copilot in VS code can't offer a scalability fix for those looped DB queries

rhodges · 2026-04-14T23:27:39Z

+api_urlpatterns = [
+    re_path(r'^sharing-groups/$', SharingGroupListView.as_view()),
+    re_path(r'^map-groups/(?P<pk>\d+)/$', MapGroupUpdateView.as_view()),
+]


This is INTERESTING! Does this mean this could support writing API calls in different modules (like mp-map-groups) and having them hosted at 'URL/api/' without the core module (madrona-portal) needing to define these?

That would be WAY better than madrona-portal assuming mp-map-groups is installed and hard-coding its inclusion in its own urls.py!

rhodges · 2026-04-14T23:28:50Z

+from mapgroups.api import SharingGroupListView, MapGroupUpdateView  # noqa: E402
+
+api_urlpatterns = [
+    re_path(r'^sharing-groups/$', SharingGroupListView.as_view()),
+    re_path(r'^map-groups/(?P<pk>\d+)/$', MapGroupUpdateView.as_view()),
+]


We are explicitly installing and importing DRF, so I recommend ignoring this suggestion as being overwrought.

Implement DRF API views for map groups and update URL configuration

f8e48c3

pollardld requested review from paigewilliams and rhodges April 10, 2026 23:12

pollardld self-assigned this Apr 10, 2026

Copilot AI review requested due to automatic review settings April 10, 2026 23:12

Copilot started reviewing on behalf of pollardld April 10, 2026 23:13 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

rhodges requested changes Apr 14, 2026

View reviewed changes

-]
+]
+def api_urls(namespace='mapgroups-api'):
+    """Returns a 3-tuple for use with include() for API endpoints.
+    This exposes the DRF routes defined in ``api_urlpatterns`` so projects
+    can mount them explicitly, e.g. under an ``/api/`` prefix.
+    """
+    return (api_urlpatterns, 'mapgroups', namespace)

		class SharingGroupListView(APIView):
		"""GET /api/sharing-groups/

Conversation

pollardld commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

rhodges Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

rhodges Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

rhodges left a comment

Choose a reason for hiding this comment

Uh oh!

rhodges Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

rhodges Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants