Merged
Conversation
* Initial plan * Add sorting, timestamps and taxa_count to TaxaList API Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: mihow <158175+mihow@users.noreply.github.com> * Optimize taxa_count with query annotation Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: mihow <158175+mihow@users.noreply.github.com> * Format lists with trailing commas per black style Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: mihow <158175+mihow@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mihow <158175+mihow@users.noreply.github.com> Co-authored-by: Anna Viklund <annamariaviklund@gmail.com>
✅ Deploy Preview for antenna-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for antenna-ssec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end taxa-list management: backend serializers, viewsets, permissions, nested routes, and tests; frontend routes, pages, hooks, components, models, and translations to list, add, and remove taxa from taxa lists. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (UI)
participant Auth as Auth Service
participant API as Django API
participant DB as Database
UI->>Auth: include auth token
UI->>API: POST /taxa/lists/{taxaListId}/taxa/?project_id={pid} { taxon_id }
API->>Auth: validate token
Auth-->>API: token OK
API->>DB: fetch TaxaList scoped to project
DB-->>API: TaxaList record
API->>DB: fetch Taxon by id
DB-->>API: Taxon record / not found
alt taxon exists & not duplicate
API->>DB: create M2M association (taxalist.taxa.add)
DB-->>API: association persisted
API-->>UI: 201 Created + taxon payload
else duplicate or invalid
API-->>UI: 400 Bad Request / 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
|
Exciting! I will look into the missing BE parts. But would love to merge some version of this sooner and continue to improve |
Member
Author
Yes, let's try get something out! I'm fine leaving everything under "Notes" for later. I have updated the PR description with current status. We can now add and remove taxa lists members from UI! 🎉 I think these two backend issues would be nice to fix before we try get this out, at least the first one (listed under "Missing BE stuff")... 
|
Member
Author
|
Some observations when using the new endpoints to add and remove taxa from lists:
Just some thoughts, I'm fine leaving this as is for now! Thank you so much for the help @mohamedelabbas1996, it worked well to hook up with FE 🙏 |
Member
Author
|
Question: does it make sense to check "can update" permission for the taxa list, before rendering controls for "Add taxon" and "Remove taxon"? This is what I do know! :) |
… model)
Refactors taxa list endpoints to use nested routes and proper HTTP methods
while keeping the simple ManyToManyField (no through model).
**Backend changes:**
- Created TaxaListTaxonViewSet with nested routes under /taxa/lists/{id}/taxa/
- Simple serializers for input validation and output
- Removed deprecated add_taxon/remove_taxon actions from TaxaListViewSet
- Uses Django M2M .add() and .remove() methods directly
**Frontend changes:**
- Updated useAddTaxaListTaxon to POST to /taxa/ endpoint
- Updated useRemoveTaxaListTaxon to use DELETE method
**API changes:**
- POST /taxa/lists/{id}/taxa/ - Add taxon (returns 201, 400 for duplicates)
- DELETE /taxa/lists/{id}/taxa/by-taxon/{taxon_id}/ - Remove taxon (returns 204, 404 for non-existent)
- GET /taxa/lists/{id}/taxa/ - List taxa in list
- Removed POST /taxa/lists/{id}/add_taxon/ (deprecated)
- Removed POST /taxa/lists/{id}/remove_taxon/ (deprecated)
**Benefits:**
- Same API as project membership (consistency)
- No migration needed (keeps existing simple M2M)
- Proper HTTP semantics (POST=201, DELETE=204)
- RESTful nested resource design
**Tests:**
- Added comprehensive test suite (13 tests, all passing)
- Tests for CRUD operations, validation, and error cases
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Collaborator
|
@annavik @mohamedelabbas1996 I updated the endpoints to use the same pattern as the project-user membership API, but no through-model to keep it a little simpler. I have in a separate PR here until I test it. Feel free to take a look before then. #1104 |
* fix: entity createdAt guard, utils project field, taxa-list query type and cache key, table loading state - entity.ts: createdAt getter was guarding on updated_at instead of created_at - utils.ts: revert project_id back to project; existing serializers (Device, Site, StorageSource, SourceImageCollection) declare the field as project - useTaxaListDetails: generic was TaxaList (client shape) instead of ServerTaxaList (raw API shape); added projectId to queryKey to avoid cross-project cache hits - taxa-list-details: isLoading on Table was gated on !id which is always falsy (id is a route param), so the loading state never showed Co-Authored-By: Claude <noreply@anthropic.com> * fix: restrict TaxaListTaxonViewSet to IsActiveStaffOrReadOnly; update tests TaxaListTaxonViewSet had permission_classes = [] (open to everyone). Set to IsActiveStaffOrReadOnly to match the parent TaxaListViewSet pattern. Test setUp now creates users with is_staff=True so POST/DELETE pass the permission check. Co-Authored-By: Claude <noreply@anthropic.com> * fix: backend cleanup in taxa-list views - TaxonTaxaListFilter docstring: include_descendants default was documented as false but the code defaults to True; corrected docs to match - get_taxa_list: added "from None" to suppress chained DoesNotExist traceback in the NotFound exception - create: replaced Taxon.objects.get with get_object_or_404 to close the race between serializer validation and the lookup - added get_object_or_404 import Co-Authored-By: Claude <noreply@anthropic.com> * feat: UX — permission gating and double-submit guards on taxa-list pages - taxa-list-details: wrap AddTaxaListTaxonPopover in canUpdate check so users without write permission do not see the add button - add-taxa-list-taxon: disable the Add button while the mutation is in flight to prevent duplicate submissions - remove-taxa-list-taxon-dialog: disable the Confirm button while the remove mutation is in flight - taxa-lists: use the paginated total from the API instead of the length of the current page for the results count Co-Authored-By: Claude <noreply@anthropic.com> * chore: naming fixes, dead-code removal, missing useEffect dep - language.ts: rename MESSAGE_MESSAGE_REMOVE_TAXA_LIST_TAXON_CONFIRM to MESSAGE_REMOVE_TAXA_LIST_TAXON_CONFIRM (duplicated prefix); update enum key, translation map entry, and the one usage site - new-entity-dialog.tsx: remove unused API_ROUTES import - useSidebarSections.tsx: remove expression that computes a value but never assigns or returns it - breadcrumbs.tsx: add setMainBreadcrumb to useEffect dependency array Co-Authored-By: Claude <noreply@anthropic.com> * fix: transfer focus to dialog after preventDefault in onOpenAutoFocus When e.preventDefault() stops Radix's default focus behaviour the dialog has no focused element, which breaks keyboard navigation and screen readers. Explicitly focus the dialog content after preventing the default in all three sites: taxa-list-details, species, and occurrences. Co-Authored-By: Claude <noreply@anthropic.com> * fix: gate TaxaListTaxonViewSet on project membership, not is_staff IsActiveStaffOrReadOnly blocks any non-staff user from POST/DELETE, but the intent of taxa lists is that any project member can manage them. Added IsProjectMemberOrReadOnly to ami/base/permissions.py: safe methods are open; unsafe methods check project.members via the active project resolved by ProjectMixin.get_active_project(). Reverted test users back to plain create_user — Project.objects.create with owner= auto-adds the owner as a member via ensure_owner_membership, which is all the permission now requires. Co-Authored-By: Claude <noreply@anthropic.com> * fix: scope taxa list lookup to active project, add missing useEffect dep - get_taxa_list() now validates that the taxa list belongs to the active project, preventing cross-project manipulation via URL params - Added setDetailBreadcrumb to useEffect dependency array Co-Authored-By: Claude <noreply@anthropic.com> * fix: cleanup UI fixes * fix: allow superusers to add taxa --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Anna Viklund <annamariaviklund@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
ami/main/api/views.py (2)
1687-1691:listaction bypasses pagination.The
listmethod returns all taxa in the list without pagination. While taxa lists are likely small, this is inconsistent with the rest of the API and could cause issues if a list grows large. Consider using the paginator fromself.paginate_querysetor documenting that this endpoint is intentionally unpaginated.Proposed fix using DRF pagination
def list(self, request, taxalist_pk=None): """List all taxa in the taxa list.""" queryset = self.get_queryset() - serializer = self.get_serializer(queryset, many=True) - return Response({"count": queryset.count(), "results": serializer.data}) + page = self.paginate_queryset(queryset) + if page is not None: + serializer = self.get_serializer(page, many=True) + return self.get_paginated_response(serializer.data) + serializer = self.get_serializer(queryset, many=True) + return Response({"count": queryset.count(), "results": serializer.data})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/api/views.py` around lines 1687 - 1691, The list method currently returns the full queryset bypassing DRF pagination; change it to call self.paginate_queryset(queryset) and if that returns a page, serialize that page with self.get_serializer(page, many=True) and return self.get_paginated_response(serializer.data), otherwise fall back to the existing Response for the full queryset; update the method that defines list in the view (the list method on this view class) to use self.paginate_queryset, self.get_serializer, and self.get_paginated_response accordingly.
1648-1661: Consider removing the silent fallback whenprojectisNone.Since
require_project = Trueis set,get_active_project()will raiseHttp404before reaching this code if no project is provided. Theif project:guard on line 1659 is therefore dead code, and silently skipping project association could mask bugs ifrequire_projectis ever changed toFalse.Suggested tightening
def perform_create(self, serializer): - """ - Create a TaxaList and automatically assign it to the active project. - - Users cannot manually assign taxa lists to projects for security reasons. - A taxa list is always created in the context of the active project. - - `@TODO` Do we need to check permissions here? Is this user allowed to add taxa lists to this project? - """ instance = serializer.save() project = self.get_active_project() - if project: - instance.projects.add(project) + assert project, "require_project=True should guarantee a project" + instance.projects.add(project)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/api/views.py` around lines 1648 - 1661, The current perform_create method silently skips associating the new TaxaList when project is None even though require_project = True makes get_active_project() raise Http404; remove the dead if guard and directly call project = self.get_active_project() followed by instance.projects.add(project) (or add an explicit assertion after get_active_project to make the requirement explicit) so that missing project conditions are not silently ignored; update perform_create, referencing perform_create, serializer.save, get_active_project, and instance.projects.add.ui/src/pages/taxa-lists/taxa-lists.tsx (1)
15-16: Consider clarifying the intent of destructuringidfrom route params.On the taxa-lists list page,
idappears to serve only as a guard inisLoading={!id && isLoading}(line 54) — presumably to suppress the table loading state when a detail-view is active in a master-detail layout. If this page is never rendered alongside a detail route that provides:id, the guard is a no-op. A brief comment would help future readers understand the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/pages/taxa-lists/taxa-lists.tsx` around lines 15 - 16, Destructure `id` from `useParams()` is used as a master/detail guard (see `TaxaLists` and the `isLoading={!id && isLoading}` expression) to suppress the list table loading state when a detail route with `:id` is active; add a short in-line comment next to the `const { projectId, id } = useParams()` and/or the `isLoading` calculation explaining that `id` may be present only in a split master-detail layout and is used intentionally to avoid showing the list loading spinner when a detail view is rendered alongside the list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/main/api/views.py`:
- Around line 1627-1661: TaxaListViewSet currently inherits the default
IsActiveStaffOrReadOnly via DefaultViewSet, which prevents non-staff project
members from creating/updating taxa lists; update TaxaListViewSet to set
permission_classes = [ObjectPermission] (or [IsProjectMemberOrReadOnly] if your
project uses that) to match other project-scoped viewsets (e.g., ProjectViewSet,
DeviceViewSet), and ensure the chosen permission class is imported at the top of
the file; keep the rest of TaxaListViewSet (get_queryset, perform_create,
get_active_project usage) unchanged.
In `@ui/src/pages/taxa-list-details/add-taxa-list-taxon/add-taxa-list-taxon.tsx`:
- Around line 34-36: Replace the hardcoded label 'Select a taxon' passed to the
TaxonSelect component's triggerLabel prop with a translated string using the app
i18n key (e.g., STRING.SELECT_A_TAXON) via the translation function (t).
Import/use the translation helper used in this file (or call useTranslation to
get t) and import the STRING constants if needed; if STRING.SELECT_A_TAXON does
not exist, add that key to the translation constants and translations. Update
the JSX so triggerLabel={taxon ? taxon.name : t(STRING.SELECT_A_TAXON)}.
In
`@ui/src/pages/taxa-list-details/remove-taxa-list-taxon/remove-taxa-list-taxon-dialog.tsx`:
- Around line 17-22: The dialog can briefly show stale success state if reopened
before the hook's onSuccess timeout clears it; update
remove-taxa-list-taxon-dialog.tsx to reset the mutation when the dialog opens by
calling the mutation's reset method (from useRemoveTaxaListTaxon) as soon as
isOpen becomes true (e.g., in an effect or the open handler) so
isSuccess/isLoading/error are cleared on open; if reset isn't exposed by the
hook, add and export a reset function from useRemoveTaxaListTaxon and use that
here.
In `@ui/src/pages/taxa-list-details/taxa-list-details.tsx`:
- Around line 40-48: The breadcrumb is using a hardcoded 'Loading...' string
inside the useEffect; update the useEffect that calls setDetailBreadcrumb (which
depends on taxaList and setDetailBreadcrumb) to use the existing translation
helper instead (call translate(STRING.LOADING_DATA) and append '...' like other
usages) when taxaList is falsy so the breadcrumb title is consistent with other
UI text.
---
Duplicate comments:
In `@ui/src/data-services/hooks/taxa-lists/useTaxaListDetails.ts`:
- Around line 17-20: useAuthorizedQuery should be typed with ServerTaxaList and
must include projectId in the cache key to avoid cross-project collisions;
ensure the call to useAuthorizedQuery<ServerTaxaList> uses queryKey:
[API_ROUTES.TAXA_LISTS, projectId, id] and the url still includes
?project_id=${projectId}, then remove the stray duplicate review/comment marker
left in this file so only the corrected query call remains (check
useAuthorizedQuery, API_ROUTES.TAXA_LISTS, projectId, id, ServerTaxaList).
In `@ui/src/pages/taxa-list-details/add-taxa-list-taxon/add-taxa-list-taxon.tsx`:
- Around line 44-45: The duplicate review comment can be removed—no code change
required because the double-submit guard is already implemented via the Button's
disabled prop (disabled={!taxon || isLoading}); keep that logic in
add-taxa-list-taxon.tsx and delete the redundant review comment or mark it
resolved so only one confirmation remains referencing the Button and the
taxon/isLoading guard.
In
`@ui/src/pages/taxa-list-details/remove-taxa-list-taxon/remove-taxa-list-taxon-dialog.tsx`:
- Around line 56-57: The disabled prop on the Button in
RemoveTaxaListTaxonDialog is correctly using disabled={isLoading || isSuccess}
to prevent double submissions; keep this logic in the Button component (the JSX
around Button in remove-taxa-list-taxon-dialog.tsx) and do not remove or alter
the isLoading or isSuccess checks so the mutation cannot be retriggered while
loading or after success.
In `@ui/src/pages/taxa-list-details/taxa-list-details.tsx`:
- Around line 69-71: The review is a duplicate noting the Add action is now
gated by taxaList?.canUpdate; remove the redundant/duplicate review comment and
keep the conditional render for AddTaxaListTaxonPopover (component
AddTaxaListTaxonPopover and the taxaList?.canUpdate check) as-is so only one
reviewers' note remains and no code changes are needed.
- Around line 79-81: Remove the stray "[duplicate_comment]" marker left in the
review/content and ensure the component props remain as: error={error},
isLoading={isLoading}, items={species}; specifically verify the isLoading prop
on the taxa list table (isLoading) isn’t inverted and delete the
duplicate_comment token from the PR comment to avoid confusion.
---
Nitpick comments:
In `@ami/main/api/views.py`:
- Around line 1687-1691: The list method currently returns the full queryset
bypassing DRF pagination; change it to call self.paginate_queryset(queryset) and
if that returns a page, serialize that page with self.get_serializer(page,
many=True) and return self.get_paginated_response(serializer.data), otherwise
fall back to the existing Response for the full queryset; update the method that
defines list in the view (the list method on this view class) to use
self.paginate_queryset, self.get_serializer, and self.get_paginated_response
accordingly.
- Around line 1648-1661: The current perform_create method silently skips
associating the new TaxaList when project is None even though require_project =
True makes get_active_project() raise Http404; remove the dead if guard and
directly call project = self.get_active_project() followed by
instance.projects.add(project) (or add an explicit assertion after
get_active_project to make the requirement explicit) so that missing project
conditions are not silently ignored; update perform_create, referencing
perform_create, serializer.save, get_active_project, and instance.projects.add.
In `@ui/src/pages/taxa-lists/taxa-lists.tsx`:
- Around line 15-16: Destructure `id` from `useParams()` is used as a
master/detail guard (see `TaxaLists` and the `isLoading={!id && isLoading}`
expression) to suppress the list table loading state when a detail route with
`:id` is active; add a short in-line comment next to the `const { projectId, id
} = useParams()` and/or the `isLoading` calculation explaining that `id` may be
present only in a split master-detail layout and is used intentionally to avoid
showing the list loading spinner when a detail view is rendered alongside the
list.
ui/src/pages/taxa-list-details/add-taxa-list-taxon/add-taxa-list-taxon.tsx
Show resolved
Hide resolved
ui/src/pages/taxa-list-details/remove-taxa-list-taxon/remove-taxa-list-taxon-dialog.tsx
Show resolved
Hide resolved
# Conflicts: # ami/main/tests.py # ami/ml/serializers.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ami/ml/serializers.py (1)
161-168:createoverride is now dead code and can be removed.Since
"project"is no longer a declared serializer field, DRF will never include it invalidated_data. Thepopalways returnsNone, theif project:branch is unreachable, and the project association is now handled byperform_createin the view. The entire override reduces to an unnecessary call tosuper().create(validated_data).♻️ Proposed cleanup
- def create(self, validated_data): - project = validated_data.pop("project", None) - instance = super().create(validated_data) - - if project: - instance.projects.add(project) - - return instance🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/ml/serializers.py` around lines 161 - 168, The create method override in the serializer (def create(self, validated_data)) is dead code because "project" is no longer a serializer field and project association is handled in the view's perform_create; remove the entire create(self, validated_data) override (including the pop, conditional add, and return) so the serializer uses the base class implementation directly and avoid redundant code in the class.ami/main/tests.py (1)
3448-3450: Suggest adding permission-enforcement tests.All new tests authenticate as the project owner. Consider adding tests for:
- Unauthenticated users (
self.client.logout()/ noforce_authenticate) — expecting HTTP 401/403 on POST/DELETE.- Authenticated non-members — expecting HTTP 403 on write operations per
IsProjectMemberOrReadOnly.- Cross-project access — taxa list belonging to a different project returning HTTP 404 (scoped by
get_taxa_list()).This ensures the permission layer added in
TaxaListTaxonViewSetis actually exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ami/main/tests.py` around lines 3448 - 3450, Add permission-enforcement tests to TaxaListTaxonAPITestCase to exercise TaxaListTaxonViewSet and IsProjectMemberOrReadOnly: add (1) an unauthenticated test that calls POST/DELETE after self.client.logout() (or clearing force_authenticate) and asserts HTTP 401/403, (2) an authenticated non-member test that creates a user not on the project, authenticates them and asserts POST/DELETE return HTTP 403, and (3) a cross-project test that targets a taxa list belonging to a different project (use a different project/taxa_list fixture and call get_taxa_list()) and asserts the view returns HTTP 404; reuse existing helper methods/setup in TaxaListTaxonAPITestCase to create users/projects and endpoints when possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ami/main/tests.py`:
- Line 3453: The hardcoded password in the test fixture (the call to
User.objects.create_user(...) in the setUp method) triggers Ruff S106; fix by
either replacing the literal with a shared test constant (e.g., TEST_PASSWORD)
used across tests or by adding a local suppression comment on the create_user
line (append "# noqa: S106"); apply the same change to the create_user call
inside TaxaListTaxonValidationTestCase.setUp so both occurrences are handled
consistently.
- Around line 3457-3458: Change the test fixture creation to use the uppercase
rank convention: replace the lowercase literal "species" used when creating
self.taxon1 and self.taxon2 via Taxon.objects.create(...) with the canonical
TaxonRank.SPECIES.name (i.e., "SPECIES") so ranks match the rest of the codebase
and satisfy test_rank_formatting assertions.
---
Nitpick comments:
In `@ami/main/tests.py`:
- Around line 3448-3450: Add permission-enforcement tests to
TaxaListTaxonAPITestCase to exercise TaxaListTaxonViewSet and
IsProjectMemberOrReadOnly: add (1) an unauthenticated test that calls
POST/DELETE after self.client.logout() (or clearing force_authenticate) and
asserts HTTP 401/403, (2) an authenticated non-member test that creates a user
not on the project, authenticates them and asserts POST/DELETE return HTTP 403,
and (3) a cross-project test that targets a taxa list belonging to a different
project (use a different project/taxa_list fixture and call get_taxa_list()) and
asserts the view returns HTTP 404; reuse existing helper methods/setup in
TaxaListTaxonAPITestCase to create users/projects and endpoints when possible.
In `@ami/ml/serializers.py`:
- Around line 161-168: The create method override in the serializer (def
create(self, validated_data)) is dead code because "project" is no longer a
serializer field and project association is handled in the view's
perform_create; remove the entire create(self, validated_data) override
(including the pop, conditional add, and return) so the serializer uses the base
class implementation directly and avoid redundant code in the class.
Use "SPECIES" instead of "species" to match TaxonRank convention used throughout the codebase. Co-Authored-By: Claude <noreply@anthropic.com>
Match the permission pattern used by other project-scoped viewsets (ProjectViewSet, DeploymentViewSet, SiteViewSet, DeviceViewSet). Without this, non-staff project members cannot manage taxa lists. Co-Authored-By: Claude <noreply@anthropic.com>
The project write field was removed from the serializer in favour of server-side assignment in ProcessingServiceViewSet.perform_create(). Co-Authored-By: Claude <noreply@anthropic.com>
Tests were sending `project` as a POST field, but the write-only field was removed from ProcessingServiceSerializer in favor of server-side assignment via `project_id` query parameter. Also removes dead code in the serializer's create() override and fixes Prettier formatting. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Merge plan & interaction with #1110 and #1133Permission fix (latest commit)
Recommended merge order
Inline review commentsI've added inline comments on the two lines that will need updates when #1110 and #1133 merge. |
mihow
reviewed
Feb 18, 2026
| "created_at", | ||
| "updated_at", | ||
| ] | ||
| permission_classes = [IsProjectMemberOrReadOnly] |
Collaborator
There was a problem hiding this comment.
After #1110 merges: Switch back to ObjectPermission. The new check_permission() routes M2M models (where get_project_accessor() returns "projects") through check_model_level_permission(), which uses global Django permissions instead of guardian object-level perms.
This will work correctly only if create_taxalist, update_taxalist, delete_taxalist are added to the AuthorizedUser role definition in #1110. Without those, taxa list creation breaks for all non-superusers.
| "updated_at", | ||
| ] | ||
| permission_classes = [IsProjectMemberOrReadOnly] | ||
| require_project = True |
Collaborator
There was a problem hiding this comment.
After #1133 merges: Change to require_project_for_list = False.
PR #1133 makes require_project_for_list = True the default on ProjectMixin, but opts out TaxaListViewSet because taxa lists are global M2M resources (same rationale as TaxonViewSet and TagViewSet). Keep require_project = True for write operations -- only listing should be allowed without a project filter.
This was referenced Feb 18, 2026
ObjectPermission doesn't work for M2M-to-project models because BaseModel.get_project() returns None when get_project_accessor() returns "projects". This caused all write operations (create, update, delete) on taxa lists to be denied for every user. Switch to IsProjectMemberOrReadOnly which resolves the project via ProjectMixin.get_active_project() (from query param) instead of through the model instance. Add 10 permission tests covering member CRUD, anonymous read-only, non-member rejection, and owner access. Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
feat/taxa-lists
branch
from
8990a33 to
4682933
Compare
Collaborator
Code reviewFound 3 issues:
antenna/ui/src/data-services/hooks/entities/utils.ts Lines 8 to 10 in 4682933 Affected serializers at
antenna/ami/main/api/serializers.py Lines 635 to 637 in 4682933 Root cause at Lines 142 to 144 in 4682933
Lines 1686 to 1691 in 4682933 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Collaborator
Additional issues from code reviewTwo more issues to track: 4.
The frontend gates edit/delete buttons on 5. The Lines 1686 to 1691 in 4682933 Note: the frontend currently doesn't call this endpoint (it uses 🤖 Generated with Claude Code |
convertToServerFieldValues was sending `project_id` in the POST body, but the serializers for Sites, Devices, Storage Sources, and SourceImageCollections all declare `project` as the field name. Also pass project_id as a query param so IsProjectMemberOrReadOnly can resolve the active project for permission checks. Co-Authored-By: Claude <noreply@anthropic.com>
TaxaList had no permission entries in Project.Permissions, so non-superuser members could never receive create/update/delete permissions through the guardian role system. Adds create_taxalist, update_taxalist, and delete_taxalist to Project.Permissions, the Meta permissions list, and the ProjectManager role. Co-Authored-By: Claude <noreply@anthropic.com>
Models with an M2M relationship to Project (like TaxaList) return None from get_project(), causing the default permission resolver to return an empty list. This meant non-superuser members never saw edit/delete buttons despite having the correct role permissions. Adds add_m2m_object_permissions() to ami/base/permissions.py, which resolves permissions against the active project from the request context. TaxaListSerializer now calls this instead of the default path. The function is designed to be reused by other M2M-to-Project serializers (Taxon, Pipeline, ProcessingService) as part of #1120. Co-Authored-By: Claude <noreply@anthropic.com>
The UI fetches taxa for a list via the main /taxa/ endpoint with a
taxa_list_id filter, not the nested /taxa/lists/{id}/taxa/ list
endpoint. Remove the custom list() method and corresponding tests.
The viewset now only provides create (POST) and delete (DELETE).
Co-Authored-By: Claude <noreply@anthropic.com>
mihow
force-pushed
the
feat/taxa-lists
branch
from
b21acfd to
d0c8c24
Compare
Merge main into feat/taxa-lists. The only conflict was in ui/src/app.tsx where the taxa-lists routes needed to coexist with the collections-to- capture-sets rename from main. Dropped a stale CollectionDetails route reference (component doesn't exist). Co-Authored-By: Claude <noreply@anthropic.com>
Use STRING.SELECT_TAXON_PLACEHOLDER and STRING.LOADING_DATA instead of hardcoded English strings in taxa list detail pages. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
|
All 3 issues from this review are now resolved:
|
Collaborator
|
Both issues from this comment are also resolved: 4. 5. |
mihow
approved these changes
Feb 27, 2026
mihow
approved these changes
Feb 27, 2026
Collaborator
mihow
left a comment
mihow
left a comment
There was a problem hiding this comment.
Tested once more with different user roles and I think it's ready to go!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Will be a help for the upcoming class masking feature, but also something we have been wanting in general. This PR is still in draft mode.
Related to #997.
Missing BE stuff
taxalists/id/taxa/✅Missing FE stuff
Notes
Screenshots
List view:
Create view:
Edit view:
Delete view:
Detail view:
Summary by CodeRabbit
New Features
UI / Navigation
API / Backend
Hooks
Permissions
Tests