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 failed. Why did it fail? →
|
❌ Deploy Preview for antenna-ssec failed. Why did it fail? →
|
|
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 | ✅ 3✅ Passed checks (3 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>
|
Exciting! I will look into the missing BE parts. But would love to merge some version of this sooner and continue to improve |
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")... 
|
|
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 🙏 |
|
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>
|
@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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ami/ml/views.py (1)
150-190:⚠️ Potential issue | 🟠 MajorPrevent orphan ProcessingService records by validating project before database persistence.
With
require_project = True,get_active_project()raisesHttp404when the project is missing or invalid. However,serializer.save()is called first, persisting the ProcessingService record to the database. Ifget_active_project()then raises an exception, an orphan record remains in the database without project assignment.Proposed fix
- instance = serializer.save() - project = self.get_active_project() + project = self.get_active_project() + instance = serializer.save() if project: instance.projects.add(project)
🤖 Fix all issues with AI agents
In `@ami/main/api/views.py`:
- Around line 1691-1713: The create method may raise Taxon.DoesNotExist between
validation and Taxon.objects.get; to fix, catch that race by replacing or
wrapping the Taxon.objects.get(pk=taxon_id) call: use Django's
get_object_or_404(Taxon, pk=taxon_id) or try/except around Taxon.objects.get to
return a 404 Response when not found, then proceed to taxa_list.taxa.add(taxon)
and return self.get_serializer(taxon).data; update references in this method
(create, TaxaListTaxonInputSerializer, Taxon.objects.get, taxa_list.taxa.add,
get_serializer) accordingly.
- Around line 1662-1670: The class TaxaListTaxonViewSet currently sets
permission_classes = [], allowing unrestricted access; remove this override so
it inherits the default permissions (from DefaultViewSet/TaxaListViewSet) or
explicitly set permission_classes to the same tuple used by TaxaListViewSet
(e.g., [IsAuthenticated, <project-specific update permission class>); also
ensure the create() and destroy() handlers in TaxaListTaxonViewSet perform the
"can update" check (reuse the same permission check or call the parent/utility
that enforces the taxa-list "can_update" permission) so add explicit permission
checks if they aren’t already present.
In `@ui/src/data-services/hooks/taxa-lists/useTaxaListDetails.ts`:
- Around line 17-20: Change the query generic to match the server payload and
include projectId in the cache key: update the useAuthorizedQuery generic from
<TaxaList> to <ServerTaxaList> (the hook receives ServerTaxaList JSON which is
then converted via convertServerRecord) and add projectId to the queryKey array
passed to useAuthorizedQuery (alongside API_ROUTES.TAXA_LISTS and id) to prevent
cache collisions across projects.
In `@ui/src/data-services/models/entity.ts`:
- Around line 21-28: The createdAt getter incorrectly checks
this._data.updated_at before formatting; change the condition to check
this._data.created_at instead so that createdAt returns undefined only when
created_at is missing, and continue to call getFormatedDateTimeString with new
Date(this._data.created_at) inside the createdAt getter to format the existing
timestamp.
In `@ui/src/pages/species/species.tsx`:
- Around line 197-204: The onOpenAutoFocus handler on Dialog.Content currently
calls e.preventDefault(), which prevents focus from moving into the modal;
update that handler so after calling e.preventDefault() you manually transfer
focus into the dialog (e.g., call focus() on the Dialog.Content element or its
dialogRef/current DOM node, or focus its first focusable child) so keyboard
focus lands inside the modal; locate the onOpenAutoFocus prop on Dialog.Content
and implement the manual focus transfer (using e.currentTarget.focus() or
dialogRef.current.focus()/focusFirstDescendant) immediately after
preventDefault().
In `@ui/src/pages/taxa-list-details/add-taxa-list-taxon/add-taxa-list-taxon.tsx`:
- Around line 44-55: The Add button currently only checks for taxon presence, so
rapid clicks can fire duplicate addTaxaListTaxon calls; update the button to
also disable while the mutation is in flight (e.g., add disabled={!taxon ||
isAdding}), derive isAdding from the mutation's loading flag returned by the
hook that provides addTaxaListTaxon (use the isLoading/isMutating boolean from
the useMutation or API hook), and add a defensive guard at the top of the
onClick handler (if (isAdding) return) before awaiting addTaxaListTaxon({
taxaListId, taxonId: taxon.id }) to ensure a single inflight request; keep the
existing setTimeout(setTaxon...) behavior unchanged.
In
`@ui/src/pages/taxa-list-details/remove-taxa-list-taxon/remove-taxa-list-taxon-dialog.tsx`:
- Around line 56-60: The confirm Button currently only uses isSuccess to
disable, so rapid clicks can trigger duplicate requests; update the Button's
disabled prop to also check the mutation's in-flight state (e.g., isLoading or
isPending) returned by the removeTaxaListTaxon hook. Concretely, modify the
disabled expression to something like disabled={isSuccess || isLoading} (or
isPending) so the Button is disabled while removeTaxaListTaxon({ taxaListId,
taxonId }) is executing; ensure you reference the mutation status variable you
already get from the hook alongside isSuccess.
In `@ui/src/pages/taxa-list-details/taxa-list-details.tsx`:
- Around line 60-69: The AddTaxaListTaxonPopover is always rendered even when
the list is read-only; update the JSX to only render AddTaxaListTaxonPopover
when taxaList?.canUpdate is truthy (same gating used for remove), e.g.
conditionally render AddTaxaListTaxonPopover using the taxaList?.canUpdate flag
so the "Add taxon" action is hidden for read-only lists; ensure you reference
the existing props/ids (AddTaxaListTaxonPopover taxaListId={id as string}) and
leave SortControl unchanged.
- Around line 71-79: The Table's isLoading prop uses an inverted guard (!id &&
isLoading) so the loading state never shows when an id exists; update the
isLoading prop on the Table (in taxa-list-details.tsx where Table is rendered)
to a correct expression such as isLoading={isLoading || !id} so the table shows
loading when data is loading or when the id is not yet available (reference the
Table component and the id and isLoading variables).
🧹 Nitpick comments (6)
ui/src/pages/project/entities/new-entity-dialog.tsx (1)
2-2: Remove unused import.
API_ROUTESis imported but never used in this file.♻️ Proposed fix
import classNames from 'classnames' -import { API_ROUTES } from 'data-services/constants' import { useCreateEntity } from 'data-services/hooks/entities/useCreateEntity'ui/src/components/breadcrumbs/breadcrumbs.tsx (1)
24-32: Logic change approved; consider addingsetMainBreadcrumbto dependency array.The inverted condition correctly prevents setting
mainBreadcrumbfor the 'project' item. Note thatsetMainBreadcrumbis missing from the dependency array. While context setters are typically stable, adding it ensures exhaustive deps compliance.♻️ Proposed fix
- }, [navItems, activeNavItem]) + }, [navItems, activeNavItem, setMainBreadcrumb])ui/src/data-services/hooks/taxa-lists/useAddTaxaListTaxon.ts (1)
35-35: Minor inconsistency:resetis returned here but not inuseRemoveTaxaListTaxon.This hook returns
resetin its return object, but the sibling hookuseRemoveTaxaListTaxondoes not. Consider aligning the return signatures for consistency.ui/src/pages/project/sidebar/useSidebarSections.tsx (1)
147-150: Remove dead code.This expression computes a value but doesn't assign or return it. It appears to be leftover debug code or an incomplete refactor.
🧹 Proposed fix
- sidebarSections - .map(({ items }) => items) - .flat() - .find((item) => !!matchPath(item.path, location.pathname)) - return { sidebarSections, activeItem }ami/ml/serializers.py (1)
133-158: Consider prefetchingprojectsto avoid N+1 on list endpoints.
SerializerMethodFieldaccessesobj.projectsper instance; without prefetching in the viewset, list responses can trigger extra queries. Ensure the queryset prefetches this relation (or annotate IDs) to keep list performance stable.ami/main/api/views.py (1)
1672-1678: Useraise ... fromfor proper exception chaining.When re-raising as a different exception type, use
fromto preserve the original traceback for debugging.♻️ Proposed fix
def get_taxa_list(self): """Get the parent taxa list from URL parameters.""" taxa_list_id = self.kwargs.get("taxalist_pk") try: return TaxaList.objects.get(pk=taxa_list_id) except TaxaList.DoesNotExist: - raise api_exceptions.NotFound("Taxa list not found.") + raise api_exceptions.NotFound("Taxa list not found.") from None
| <Button | ||
| disabled={!taxon} | ||
| onClick={async () => { | ||
| if (taxon) { | ||
| await addTaxaListTaxon({ taxaListId, taxonId: taxon.id }) | ||
| setTimeout(() => setTaxon(undefined), SUCCESS_TIMEOUT) | ||
| } | ||
| }} | ||
| size="small" | ||
| variant="success" | ||
| > | ||
| <span>{translate(STRING.ADD)}</span> |
There was a problem hiding this comment.
Disable the Add action while the mutation is in flight.
Right now the button is enabled during loading, so rapid clicks can enqueue duplicate add requests.
🛠️ Proposed fix
- disabled={!taxon}
+ disabled={!taxon || isLoading}🤖 Prompt for AI Agents
In `@ui/src/pages/taxa-list-details/add-taxa-list-taxon/add-taxa-list-taxon.tsx`
around lines 44 - 55, The Add button currently only checks for taxon presence,
so rapid clicks can fire duplicate addTaxaListTaxon calls; update the button to
also disable while the mutation is in flight (e.g., add disabled={!taxon ||
isAdding}), derive isAdding from the mutation's loading flag returned by the
hook that provides addTaxaListTaxon (use the isLoading/isMutating boolean from
the useMutation or API hook), and add a defensive guard at the top of the
onClick handler (if (isAdding) return) before awaiting addTaxaListTaxon({
taxaListId, taxonId: taxon.id }) to ensure a single inflight request; keep the
existing setTimeout(setTaxon...) behavior unchanged.
There was a problem hiding this comment.
@annavik there are a couple suggestions here about guarding against double form submissions. I wonder if we can implement that in general - I was able to do it for new jobs with the keyboard.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ui/src/pages/taxa-list-details/remove-taxa-list-taxon/remove-taxa-list-taxon-dialog.tsx
Show resolved
Hide resolved
|
We are going to need an import option after this too :) |
|
Hm, I guess we are not ready for the Here are some fixes in response to the other feedback: #1119 |
Yes! |
I
Thanks for the updates! I tested everything in this branch, I found one small thing needed related to For the PR to address feedback, see comments: #1119 |
* 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>
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.
| class TaxaListViewSet(DefaultViewSet, ProjectMixin): | ||
| queryset = TaxaList.objects.all() | ||
| serializer_class = TaxaListSerializer | ||
| ordering_fields = [ | ||
| "name", | ||
| "description", | ||
| "annotated_taxa_count", | ||
| "created_at", | ||
| "updated_at", | ||
| ] | ||
| require_project = True | ||
|
|
||
| def get_queryset(self): | ||
| qs = super().get_queryset() | ||
| # Annotate with taxa count for better performance | ||
| qs = qs.annotate(annotated_taxa_count=models.Count("taxa")) | ||
| project = self.get_active_project() | ||
| if project: | ||
| return qs.filter(projects=project) | ||
| return qs | ||
|
|
||
| serializer_class = TaxaListSerializer | ||
| 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) | ||
|
|
There was a problem hiding this comment.
TaxaListViewSet inherits IsActiveStaffOrReadOnly — inconsistent with other project-scoped viewsets.
Every other project-scoped viewset in this file (ProjectViewSet, DeploymentViewSet, SourceImageViewSet, SourceImageCollectionViewSet, SiteViewSet, DeviceViewSet) explicitly sets permission_classes = [ObjectPermission]. TaxaListViewSet inherits the default IsActiveStaffOrReadOnly from DefaultViewSetMixin, which means only staff users can create/edit/delete taxa lists — regular project members cannot.
Given the PR objective of scoping taxa list management to project members, consider adding permission_classes = [ObjectPermission] (or [IsProjectMemberOrReadOnly]) to match the rest of the project-scoped viewsets:
Proposed fix
class TaxaListViewSet(DefaultViewSet, ProjectMixin):
queryset = TaxaList.objects.all()
serializer_class = TaxaListSerializer
ordering_fields = [
"name",
"description",
"annotated_taxa_count",
"created_at",
"updated_at",
]
+ permission_classes = [IsProjectMemberOrReadOnly]
require_project = True#!/bin/bash
# Verify permission_classes across project-scoped viewsets
rg -n 'permission_classes' ami/main/api/views.py🧰 Tools
🪛 Ruff (0.15.1)
[warning] 1630-1636: Mutable default value for class attribute
(RUF012)
🤖 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 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.
There was a problem hiding this comment.
@mihow I think there might be some crossed wires here!
This comment thread is about the permission classes on TaxaListViewSet (lines 1627-1661 in ami/main/api/views.py). The bot flagged that it inherits IsActiveStaffOrReadOnly from DefaultViewSet, which means only staff users can create/edit/delete taxa lists — whereas other project-scoped viewsets like DeploymentViewSet, SiteViewSet, etc. use permission_classes = [ObjectPermission] to allow project members to manage them.
Your response about lowercase rank="species" in tests sounds like it's for a different comment thread (possibly about test file changes).
For this permissions issue: do you want to update TaxaListViewSet to use ObjectPermission or IsProjectMemberOrReadOnly to match the other project-scoped viewsets, or is there a reason to keep the staff-only restriction?
ui/src/pages/taxa-list-details/add-taxa-list-taxon/add-taxa-list-taxon.tsx
Show resolved
Hide resolved
| }) => { | ||
| const { projectId } = useParams() | ||
| const [isOpen, setIsOpen] = useState(false) | ||
| const { removeTaxaListTaxon, isLoading, isSuccess, error } = | ||
| useRemoveTaxaListTaxon(projectId as string) | ||
| const errorMessage = useFormError({ error }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the Dialog implementation
fd "dialog.tsx" ui/src/design-system --exec cat {}Repository: RolnickLab/antenna
Length of output: 2157
🏁 Script executed:
#!/bin/bash
# Examine the remove-taxa-list-taxon-dialog.tsx file
cat ui/src/pages/taxa-list-details/remove-taxa-list-taxon/remove-taxa-list-taxon-dialog.tsxRepository: RolnickLab/antenna
Length of output: 2651
🏁 Script executed:
#!/bin/bash
# Find and examine the useRemoveTaxaListTaxon hook
fd "useRemoveTaxaListTaxon" ui/src --exec cat {}Repository: RolnickLab/antenna
Length of output: 1126
🏁 Script executed:
#!/bin/bash
# Find the SUCCESS_TIMEOUT constant value
rg "SUCCESS_TIMEOUT" ui/src/data-services/constants --max-count=5 -A 2 -B 2Repository: RolnickLab/antenna
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Find the constants file
fd "constants" --type f --path "*/data-services/*" | head -20Repository: RolnickLab/antenna
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Find the constants file
fd "constants" --type f | grep "data-services"Repository: RolnickLab/antenna
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check the SUCCESS_TIMEOUT constant value
rg "SUCCESS_TIMEOUT" ui/src/data-services/constants.ts -A 2 -B 2Repository: RolnickLab/antenna
Length of output: 208
Dialog content unmounts on close; state resets automatically after 1 second.
The custom Dialog.Content wraps Radix UI's Dialog.Portal and Dialog.Content, which unmounts children when the dialog closes. Additionally, useRemoveTaxaListTaxon includes automatic state reset via setTimeout(reset, SUCCESS_TIMEOUT) in the onSuccess callback (1-second timeout), so stale "Confirmed" state does not persist across typical dialog open/close cycles.
A narrow edge case remains: if a user closes and immediately reopens the dialog within ~1 second before the automatic reset fires, they could briefly see stale success state. If UX polish is desired, consider resetting mutation state when the dialog opens.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ui/src/pages/taxa-list-details/remove-taxa-list-taxon/remove-taxa-list-taxon-dialog.tsx`
around lines 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.
# Conflicts: # ami/main/tests.py # ami/ml/serializers.py
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>
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. |
| "created_at", | ||
| "updated_at", | ||
| ] | ||
| permission_classes = [IsProjectMemberOrReadOnly] |
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 |
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.
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
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 👎. |
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>
End-to-end tests covering the full taxa list CRUD workflow as a ProjectManager member: create, view detail, add/remove taxa, rename, and delete. Also verifies that permission-gated UI elements (create button, edit/delete row buttons, add-taxon button) are visible for project members. Co-Authored-By: Claude <noreply@anthropic.com>
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
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