-
Notifications
You must be signed in to change notification settings - Fork 11
Configurable taxa lists #1094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Configurable taxa lists #1094
Changes from 28 commits
fc68985
bd2d08d
e9d79dd
9a23bf5
466dc2c
24d8fe5
08babab
b126349
279dd4c
0e8a09f
4aec469
4bfec3d
e73ff00
3fdbbd8
6778081
f9a5378
30459e0
fea81bb
1d3e7a5
84cb25b
dffa27d
1f5e5d4
6aaf0ea
c591ae3
bf3f304
4892c11
a86308f
8b8cdcc
e68e47f
17cf37e
2716251
05f60f0
d947756
4cd067f
d19885b
8f5144e
4682933
312e362
73f6e08
3d2eb1e
d0c8c24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,8 @@ | |
| StorageSourceSerializer, | ||
| StorageStatusSerializer, | ||
| TaxaListSerializer, | ||
| TaxaListTaxonInputSerializer, | ||
| TaxaListTaxonSerializer, | ||
| TaxonListSerializer, | ||
| TaxonSearchResultSerializer, | ||
| TaxonSerializer, | ||
|
|
@@ -1261,11 +1263,15 @@ def list(self, request, *args, **kwargs): | |
|
|
||
| class TaxonTaxaListFilter(filters.BaseFilterBackend): | ||
| """ | ||
| Filters taxa based on a TaxaList Similar to `OccurrenceTaxaListFilter`. | ||
| Filters taxa based on a TaxaList. | ||
|
|
||
| Queries for all taxa that are either: | ||
| - Directly in the requested TaxaList. | ||
| - A descendant (child or deeper) of any taxon in the TaxaList, recursively. | ||
| By default, queries for taxa that are directly in the TaxaList. | ||
| If include_descendants=true, also includes descendants (children or deeper) recursively. | ||
|
|
||
| Query parameters: | ||
| - taxa_list_id: ID of the taxa list to filter by | ||
| - include_descendants: Set to 'true' to include descendants (default: false) | ||
| - not_taxa_list_id: ID of taxa list to exclude | ||
| """ | ||
|
|
||
| query_param = "taxa_list_id" | ||
|
|
@@ -1277,11 +1283,20 @@ def filter_queryset(self, request, queryset, view): | |
| request.query_params.get(self.query_param_exclusive) | ||
| ) | ||
|
|
||
| include_descendants_default = True | ||
| include_descendants = request.query_params.get("include_descendants", include_descendants_default) | ||
| if include_descendants is not None: | ||
| include_descendants = BooleanField(required=False).clean(include_descendants) | ||
|
|
||
| def _get_filter(taxa_list: TaxaList) -> models.Q: | ||
| taxa = taxa_list.taxa.all() # Get taxa in the taxa list | ||
| query_filter = Q(id__in=taxa) | ||
| for taxon in taxa: | ||
| query_filter |= Q(parents_json__contains=[{"id": taxon.pk}]) | ||
|
|
||
| # Only include descendants if explicitly requested | ||
| if include_descendants: | ||
| for taxon in taxa: | ||
| query_filter |= Q(parents_json__contains=[{"id": taxon.pk}]) | ||
|
|
||
| return query_filter | ||
|
|
||
| if taxalist_id: | ||
|
|
@@ -1608,17 +1623,110 @@ def list(self, request, *args, **kwargs): | |
| return super().list(request, *args, **kwargs) | ||
|
|
||
|
|
||
| class TaxaListViewSet(viewsets.ModelViewSet, ProjectMixin): | ||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After #1133 merges: Change to PR #1133 makes |
||
|
|
||
| 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) | ||
|
|
||
|
Comment on lines
1627
to
1660
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Every other project-scoped viewset in this file ( Given the PR objective of scoping taxa list management to project members, consider adding 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This comment thread is about the permission classes on Your response about lowercase For this permissions issue: do you want to update |
||
|
|
||
| class TaxaListTaxonViewSet(viewsets.GenericViewSet, ProjectMixin): | ||
| """ | ||
| Nested ViewSet for managing taxa in a taxa list. | ||
| Accessed via /taxa/lists/{taxa_list_id}/taxa/ | ||
| """ | ||
|
|
||
| serializer_class = TaxaListTaxonSerializer | ||
| permission_classes = [] # Allow public access for now | ||
| require_project = True | ||
mihow marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| 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.") | ||
|
|
||
| def get_queryset(self): | ||
| """Return taxa in the specified taxa list.""" | ||
| taxa_list = self.get_taxa_list() | ||
| return taxa_list.taxa.all() | ||
|
|
||
| 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}) | ||
|
|
||
mihow marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| def create(self, request, taxalist_pk=None): | ||
| """Add a taxon to the taxa list.""" | ||
| taxa_list = self.get_taxa_list() | ||
|
|
||
| # Validate input | ||
| input_serializer = TaxaListTaxonInputSerializer(data=request.data) | ||
| input_serializer.is_valid(raise_exception=True) | ||
| taxon_id = input_serializer.validated_data["taxon_id"] | ||
|
|
||
| # Check if already exists | ||
| if taxa_list.taxa.filter(pk=taxon_id).exists(): | ||
| return Response( | ||
| {"non_field_errors": ["Taxon is already in this taxa list."]}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| # Add taxon | ||
| taxon = Taxon.objects.get(pk=taxon_id) | ||
| taxa_list.taxa.add(taxon) | ||
|
|
||
| # Return the added taxon | ||
| serializer = self.get_serializer(taxon) | ||
| return Response(serializer.data, status=status.HTTP_201_CREATED) | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| @action(detail=False, methods=["delete"], url_path=r"(?P<taxon_id>\d+)") | ||
| def delete_by_taxon(self, request, taxalist_pk=None, taxon_id=None): | ||
| """ | ||
| Remove a taxon from the taxa list by taxon ID. | ||
| DELETE /taxa/lists/{taxa_list_id}/taxa/{taxon_id}/ | ||
| """ | ||
| taxa_list = self.get_taxa_list() | ||
|
|
||
| # Check if taxon exists in list | ||
| if not taxa_list.taxa.filter(pk=taxon_id).exists(): | ||
| raise api_exceptions.NotFound("Taxon is not in this taxa list.") | ||
|
|
||
| # Remove taxon | ||
| taxa_list.taxa.remove(taxon_id) | ||
| return Response(status=status.HTTP_204_NO_CONTENT) | ||
|
|
||
|
|
||
| class TagViewSet(DefaultViewSet, ProjectMixin): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # Tests for ami.main app |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| """ | ||
| Tests for TaxaList taxa management API endpoints (without through model). | ||
| """ | ||
|
|
||
| from django.test import TestCase | ||
| from rest_framework import status | ||
| from rest_framework.test import APIClient | ||
|
|
||
| from ami.main.models import Project, TaxaList, Taxon | ||
| from ami.users.models import User | ||
|
|
||
|
|
||
| class TaxaListTaxonAPITestCase(TestCase): | ||
| """Test TaxaList taxa management operations via API.""" | ||
|
|
||
| def setUp(self): | ||
| """Set up test data.""" | ||
| self.user = User.objects.create_user(email="[email protected]", password="testpass") | ||
| self.project = Project.objects.create(name="Test Project", owner=self.user) | ||
| self.taxa_list = TaxaList.objects.create(name="Test Taxa List", description="Test description") | ||
| self.taxa_list.projects.add(self.project) | ||
| self.taxon1 = Taxon.objects.create(name="Taxon 1", rank="species") | ||
| self.taxon2 = Taxon.objects.create(name="Taxon 2", rank="species") | ||
| self.client = APIClient() | ||
| self.client.force_authenticate(self.user) | ||
| self.base_url = f"/api/v2/taxa/lists/{self.taxa_list.pk}/taxa/?project_id={self.project.pk}" | ||
|
|
||
| def test_add_taxon_returns_201(self): | ||
| """Test adding taxon to taxa list returns 201.""" | ||
| response = self.client.post(self.base_url, {"taxon_id": self.taxon1.pk}) | ||
| self.assertEqual(response.status_code, status.HTTP_201_CREATED) | ||
| self.assertTrue(self.taxa_list.taxa.filter(pk=self.taxon1.pk).exists()) | ||
| self.assertEqual(response.data["id"], self.taxon1.pk) | ||
|
|
||
| def test_add_duplicate_returns_400(self): | ||
| """Test adding duplicate taxon returns 400.""" | ||
| self.taxa_list.taxa.add(self.taxon1) | ||
| response = self.client.post(self.base_url, {"taxon_id": self.taxon1.pk}) | ||
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn("already in this taxa list", str(response.data).lower()) | ||
|
|
||
| def test_add_nonexistent_taxon_returns_400(self): | ||
| """Test adding non-existent taxon returns 400.""" | ||
| response = self.client.post(self.base_url, {"taxon_id": 999999}) | ||
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| def test_list_taxa_in_list(self): | ||
| """Test listing taxa in a taxa list.""" | ||
| self.taxa_list.taxa.add(self.taxon1, self.taxon2) | ||
|
|
||
| response = self.client.get(self.base_url) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertEqual(response.data["count"], 2) | ||
| taxon_ids = [item["id"] for item in response.data["results"]] | ||
| self.assertIn(self.taxon1.pk, taxon_ids) | ||
| self.assertIn(self.taxon2.pk, taxon_ids) | ||
|
|
||
| def test_delete_by_taxon_id(self): | ||
| """Test deleting by taxon ID returns 204.""" | ||
| self.taxa_list.taxa.add(self.taxon1) | ||
| url = f"/api/v2/taxa/lists/{self.taxa_list.pk}/taxa/{self.taxon1.pk}/?project_id={self.project.pk}" | ||
| response = self.client.delete(url) | ||
| self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) | ||
| self.assertFalse(self.taxa_list.taxa.filter(pk=self.taxon1.pk).exists()) | ||
|
|
||
| def test_delete_nonexistent_returns_404(self): | ||
| """Test deleting non-existent taxon returns 404.""" | ||
| url = f"/api/v2/taxa/lists/{self.taxa_list.pk}/taxa/{self.taxon1.pk}/?project_id={self.project.pk}" | ||
| response = self.client.delete(url) | ||
| self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) | ||
|
|
||
| def test_list_empty_taxa_list(self): | ||
| """Test listing taxa in an empty taxa list.""" | ||
| response = self.client.get(self.base_url) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertEqual(response.data["count"], 0) | ||
| self.assertEqual(response.data["results"], []) | ||
|
|
||
| def test_m2m_relationship_works(self): | ||
| """Test that M2M relationship still works correctly.""" | ||
| self.taxa_list.taxa.add(self.taxon1) | ||
| # Should be accessible via M2M relationship | ||
| self.assertEqual(self.taxa_list.taxa.count(), 1) | ||
| self.assertIn(self.taxon1, self.taxa_list.taxa.all()) | ||
| # Test reverse relationship | ||
| self.assertIn(self.taxa_list, self.taxon1.lists.all()) | ||
|
|
||
| def test_add_multiple_taxa(self): | ||
| """Test adding multiple taxa to the same list.""" | ||
| response1 = self.client.post(self.base_url, {"taxon_id": self.taxon1.pk}) | ||
| self.assertEqual(response1.status_code, status.HTTP_201_CREATED) | ||
|
|
||
| response2 = self.client.post(self.base_url, {"taxon_id": self.taxon2.pk}) | ||
| self.assertEqual(response2.status_code, status.HTTP_201_CREATED) | ||
|
|
||
| self.assertEqual(self.taxa_list.taxa.count(), 2) | ||
|
|
||
| def test_remove_one_taxon_keeps_others(self): | ||
| """Test that removing one taxon doesn't affect others.""" | ||
| self.taxa_list.taxa.add(self.taxon1, self.taxon2) | ||
|
|
||
| url = f"/api/v2/taxa/lists/{self.taxa_list.pk}/taxa/{self.taxon1.pk}/?project_id={self.project.pk}" | ||
| response = self.client.delete(url) | ||
| self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) | ||
|
|
||
| # taxon1 should be removed | ||
| self.assertFalse(self.taxa_list.taxa.filter(pk=self.taxon1.pk).exists()) | ||
| # taxon2 should still be there | ||
| self.assertTrue(self.taxa_list.taxa.filter(pk=self.taxon2.pk).exists()) | ||
| self.assertEqual(self.taxa_list.taxa.count(), 1) | ||
|
|
||
|
|
||
| class TaxaListTaxonValidationTestCase(TestCase): | ||
| """Test validation and error cases.""" | ||
|
|
||
| def setUp(self): | ||
| """Set up test data.""" | ||
| self.user = User.objects.create_user(email="[email protected]", password="testpass") | ||
| self.project = Project.objects.create(name="Test Project", owner=self.user) | ||
| self.taxa_list = TaxaList.objects.create(name="Test Taxa List") | ||
| self.taxa_list.projects.add(self.project) | ||
| self.taxon = Taxon.objects.create(name="Test Taxon", rank="species") | ||
| self.client = APIClient() | ||
| self.client.force_authenticate(self.user) | ||
| self.base_url = f"/api/v2/taxa/lists/{self.taxa_list.pk}/taxa/?project_id={self.project.pk}" | ||
|
|
||
| def test_add_without_taxon_id_returns_400(self): | ||
| """Test adding without taxon_id returns 400.""" | ||
| response = self.client.post(self.base_url, {}) | ||
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| def test_add_with_invalid_taxon_id_returns_400(self): | ||
| """Test adding with invalid taxon_id returns 400.""" | ||
| response = self.client.post(self.base_url, {"taxon_id": "invalid"}) | ||
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| def test_nonexistent_taxa_list_returns_404(self): | ||
| """Test accessing non-existent taxa list returns 404.""" | ||
| url = f"/api/v2/taxa/lists/999999/taxa/?project_id={self.project.pk}" | ||
| response = self.client.get(url) | ||
| self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) |
Uh oh!
There was an error while loading. Please reload this page.