Repository navigation
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
This was referenced Sep 17, 2026
Adds TaxaList.is_public and a manage_public_taxalist permission so a taxa list can be marked available to every project instead of relying on "has zero projects" as an implicit signal. BaseQuerySet.for_project() filters a model's M2M projects field via an Exists subquery (no join, no duplicate rows) with an is_public bypass; visible_for_user() gets the same bypass so a public list is visible to every user, draft projects included. get_or_create_for_project() takes an is_public flag to look up/create the public list of a name instead of only a hidden zero-project one. The 0096 migration backfills is_public=True for every existing zero-project list, except a per-algorithm "Taxa returned by ..." category-map list, which stays hidden. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
Being a member of a project is no longer enough to edit or delete a taxa list, or change its taxa, once that list is public — only a user with the manage_public_taxalist permission (or a superuser) can. Project-scoped lists keep working exactly as before for their own members. IsProjectMemberOrPublicListManagerOrReadOnly relaxes has_permission() for the manage_public_taxalist bypass on update/destroy only (create still needs real project membership, since there's no object yet to check), then has_object_permission() re-verifies once the target list's is_public flag is known. This closes a gap the relaxed has_permission() would otherwise open: without the object-level re-check, a manage_public_taxalist holder who isn't a project member could edit an unrelated project's own list. The nested add/remove-taxon route has no automatic object-permission check (it never calls get_object()), so it runs the same check_taxalist_write_ permission() by hand after resolving the target list. TaxaListViewSet.get_queryset() takes an include_public param (default true, SingleParamSerializer-validated so a bad value is a 400) and now includes public lists via for_project(). The serializer exposes is_public read-only, and add_m2m_object_permissions() reports update/delete for a public list based on the platform permission instead of project membership. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
import_taxa and update_taxa create their taxa list with project=None, which used to mean an ad hoc hidden list with no project. Now that public is an explicit flag, pass is_public=True so those lists are actually available to every project, matching what "global list" meant in comments up to now. Also rewords the per-algorithm category-map list's comment in pipeline.py away from "global", since that word now means something more specific. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…ery cost Adds the permission matrix for public vs. project-scoped TaxaLists (member, non-member, anonymous, superuser, and a plain manage_public_taxalist holder) across update, delete, and add/remove taxon; the include_public query param and its 400 on an invalid value; draft-project visibility for a non-public list; a multi-row assertNumQueries check on the list endpoint; direct unit tests for BaseQuerySet.for_project(); and the 0096 migration's backfill rule. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…'s existence via the nested route The 0096 migration docstring described "Taxa returned by <algorithm>" lists as a category-map detail — they're not: each one holds only the taxa that algorithm has actually returned as a top prediction so far. Rewrites the docstring to say what is_public means, the backfill rule, and that these lists stay non-public because what a project should see or do with them is still undecided. TaxaListTaxonViewSet.get_taxa_list() resolved the parent list via for_project() alone, which does not apply draft-project visibility. A manage_public_taxalist holder bypasses the project-membership check at has_permission() (the target might turn out to be public), so without this fix they could find out a project-scoped list exists in a draft project they have nothing to do with, just by guessing its id and that project's id. get_taxa_list() now runs visible_for_user() first, so a non-public list in an unrelated draft project resolves to 404 for anyone but its members and superusers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…yond TaxaList user_can_manage_public_taxalist() only ever checked one hardcoded permission string. Replaced it with user_can_manage_public(user, model_or_instance), which derives <app_label>.manage_public_<model_name> from the model's own metadata, so any model with an is_public flag can reuse it instead of every model growing its own copy. add_m2m_object_permissions() and IsProjectMemberOrPublicListManager now call the generic helper. check_taxalist_write_permission() stays TaxaList-specific rather than folding into a shared write-check: its non-public fallback is project membership, while ProcessingService's is active-staff status, so the two bodies would diverge immediately. Added the ProcessingService equivalent, check_processingservice_write_permission(), plus IsActiveStaffOrPublicManager and IsActiveStaffOrPublicManagerOrReadOnly, mirroring the TaxaList permission classes' shape (safe methods open; create always needs the base gate since there's no object yet; update/delete/register_pipelines defer to the object-level check once is_public is known) with staff status as the base gate in place of project membership. Also fixed a docstring on IsProjectMemberOrPublicListManager that overstated its own scope: the nested add/remove-taxon route it guards serves ordinary project-scoped lists too, not only public ones. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…nction as TaxaList ProcessingService gets is_public and manage_public_processingservice, the same shape TaxaList already has: ProcessingServiceViewSet.get_queryset() uses for_project(project, include_public=...) instead of a plain projects=project filter, so a public service shows up for every project. Writes go through IsActiveStaffOrPublicManagerOrReadOnly — a project-scoped service keeps its existing staff-only rule unchanged (still no membership check, see #1120), but a public one now needs the platform permission; plain staff status is not enough. status and register_pipelines call self.get_object() instead of a raw ProcessingService.objects.get(pk=pk), so they inherit the same visibility and permission checks as every other detail action instead of bypassing them. Factored the ?include_public query-param parsing (shared with TaxaListViewSet) into ProjectMixin.get_include_public(), and the OpenApiParameter doc for it into include_public_doc_param next to the other shared doc params, instead of duplicating both per viewset. is_public is read-only in ProcessingServiceSerializer. Its get_permissions() now calls add_m2m_object_permissions() (previously it used the DefaultSerializer fallback, which reports empty user_permissions for a public service to anyone but a superuser, whether or not they hold the new permission). Migration 0029 is schema-only: unlike TaxaList, an existing zero-project service does NOT become public — it stays visible to superusers only until someone opts it in. Other call sites filter processing services by project directly and would wrongly exclude a public one; left unchanged, per scope, but worth knowing about: - ami/ml/models/pipeline.py:1287 (pick the lowest-latency service for a project+pipeline during job dispatch) - ami/jobs/tasks.py:74 and :189 (mark an async service as seen; count available async workers for a job) - ami/ml/views.py:116 and :246 (prefetch a pipeline's processing services for PipelineViewSet and ProjectPipelineViewSet) - ami/main/models.py:265 (project.processing_services.exists() decides whether to auto-create a project's default service) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
… and query cost Adds the permission matrix for public vs. project-scoped ProcessingServices (staff, member, non-member, anonymous, superuser, and a staff user granted manage_public_processingservice) across update, delete, status, and register_pipelines; the include_public query param and its 400 on an invalid value; a public service linked to several projects appearing once; a non-public service with zero projects being invisible to non-superusers; and a multi-row assertNumQueries check on the list endpoint. All fixtures use endpoint_url=None so get_status()/create_pipelines() never make a real network call, matching the existing pull-mode test pattern in this file. Fixes _register_pipelines()'s test helper, which never passed project_id: that worked before only because the action did a raw pk lookup bypassing get_queryset() entirely; now that it calls self.get_object(), the same project_id ProcessingServiceViewSet's other actions already require applies here too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…ch the frontend The frontend calls both endpoints without a project_id (usePopulateProcessingService.ts, useTestProcessingServiceConnection.ts), a call shape the previous raw ProcessingService.objects.get(pk=pk) lookup tolerated because it never checked project_id at all. Switching those actions to self.get_object() picked up require_project=True from the rest of the viewset, so both buttons started returning 400. ProcessingServiceViewSet.get_active_project() now passes required=False for exactly these two actions; get_queryset() already falls back to the plain visible_for_user() set when no project is given, so the two actions resolve against every service the user can see instead of one project's services. Every other action (list, create, retrieve, update, destroy) still requires project_id, unchanged. Restored the pre-existing _register_pipelines test helper to its original call shape (no project_id) and added a with_project_id=True variant, plus the same pair for status and a public-service permission check confirming a staff user without manage_public_processingservice is denied register_pipelines under both call shapes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…eak, cut ProcessingService permission cost include_public governed every action's queryset, not just list: a public list/service retrieved, updated, or deleted with ?include_public=false 404'd instead of resolving normally, since for_project() dropped it from the candidate set entirely. TaxaListViewSet and ProcessingServiceViewSet now only apply it when self.action == "list"; every other action always includes public rows. TaxaListSerializer.get_projects() and ProcessingServiceSerializer.get_projects() emitted every linked project id unconditionally. Since a public row bypasses the draft-project visibility filter, an outsider retrieving a public list or service linked to a draft project could learn that project's id even though they can't see the project itself. Both now intersect with Project.objects.visible_for_user(request.user), computed once per request (cached on the serializer instance, the same request-scoped-cache-on-self shape used elsewhere for this exact reason) and reused across every row a ListSerializer renders. ProcessingServiceSerializer.get_permissions() called add_m2m_object_permissions, which ran a membership .exists() query and a guardian get_perms() lookup for every non-public row — pure overhead, since no per-project *_processingservice guardian permission exists anywhere in this codebase (checked Project.Permissions and ami/users/roles.py), so that branch could only ever grant anything to a superuser, which a plain attribute check already covers. New add_processingservice_permissions() keeps only the public-row platform-permission check and the superuser check. get_or_create_for_project(project=X, is_public=True) silently dropped is_public instead of raising: a project-scoped list can't also be the platform's public list of that name, and silently ignoring the caller's is_public=True hid a mistake instead of surfacing it. Raises ValueError now. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…ed is_public for_project() and visible_for_user() decided their public-row bypass with hasattr(model, "is_public") — any model that later grew an attribute of that name would silently pick up public-row visibility rules it never asked for. New abstract PublicScopedModel (ami/base/models.py) declares is_public once; TaxaList and ProcessingService inherit it (ProcessingService overrides the field only to give it its own help text). Both queryset methods now check issubclass(model, PublicScopedModel) instead. Verified the refactor alone is migration-neutral (makemigrations --check: no changes) before touching wording, so drift from moving the field would have been caught separately from the wording fix below. Reworded the field's help text in the same change, since both needed the same scrutiny: TaxaList's "available to every project" already held (no other code path filters TaxaList by project bypassing is_public); a job still won't dispatch to a public ProcessingService that isn't linked to the project, so "available" overclaimed there. Both now say "shown", and ProcessingService's override adds the one line callers actually need. This does need a migration (help_text is part of field state) — 0097 and 0030, schema-only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
… the matrix New coverage: ?include_public=false on a detail/delete URL must not 404 a public row (TaxaList and ProcessingService); a public row linked to both a draft and a non-draft project reports only the visible project's id to an outsider, both ids to a draft-project member; get_or_create_for_project raises when given project and is_public=True together; a project member's own list can be PATCHed successfully while user_permissions reports nothing for it, pinning the known gap between the membership write gate and the guardian-based permission report (#1120) so a future change is deliberate, not accidental. test_public_list_linked_to_project_is_not_duplicated now links to three projects, matching the mechanism it guards: a naive join-based filter (Q(projects=project) | Q(is_public=True)) produces one joined row per linked project, and is_public=True is true on every one of those rows regardless of which project_id it carries, so all three pass the WHERE clause without .distinct() — verified by temporarily swapping the Exists for that Q filter and confirming the test fails with 3 rows instead of 1, then reverting. test_public_list_linked_to_multiple_projects_appears_once now runs as a superuser, since visible_for_user() returns the queryset unchanged for one (no .distinct() applied there) — a non-superuser's request benefits from that .distinct() regardless of what for_project() does, which would let a duplication bug in for_project() pass silently. Dropped ProcessingService's copy of the same guarantee; it's a property of the shared for_project() code, already covered once at the queryset level and once at the API level. Trimmed redundant permission-matrix coverage: kept one of two overlapping authenticated/anonymous "cannot update" checks per model, one of three "cannot update a scoped service" checks (all fail for the same reason — not staff), the register_pipelines allow case over its already-covered deny twin, and dropped a status test whose only difference from its sibling was an unused project_id. Split each "is_public field is read-only and reported" test into two so a failure names which behavior broke. Re-measured both list-endpoint query-count baselines after the permission and get_projects fixes above: TaxaList's 34 -> 35 (get_projects() adds one Project.objects.visible_for_user() query per request, not per row); ProcessingService's 34 -> 21 (add_processingservice_permissions() drops the per-non-public-row membership check and guardian lookup that could never grant anything to begin with). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…nto one check_taxalist_write_permission and check_processingservice_write_permission differed only in the fallback used for a non-public row (project membership vs. active-staff status); IsProjectMemberOrPublicListManager[OrReadOnly] and IsActiveStaffOrPublicManager[OrReadOnly] were the same shape again, parameterised the same way. Both pairs are now one shared function/base class taking that fallback as a predicate, with every existing name kept as a thin subclass or alias so the branches stacked on this one still import cleanly. Nothing was renamed. Also fixes the one remaining bare `# type: ignore` on the superuser check this consolidation touched, matching its siblings' `[union-attr]` form. The four permission classes' full test matrices pass unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
… add the field The branch has not been deployed anywhere, so a separate migration whose only effect is new help text adds a file for nothing and takes migration numbers that stacked branches already use. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
mihow
force-pushed
the
feat/public-taxa-lists-and-services
branch
from
September 18, 2026 19:38
6fecbdf to
38874ef
Compare
Resolves the overlap with the Taxa Lists speedup (#1428): the public-row branch of add_m2m_object_permissions runs first, then main's prefetch-aware membership check and once-per-page project permissions. get_projects keeps the draft-project visibility filter and now reads the prefetched projects. Renumbers the taxa list migration to 0098 after main's 0096/0097. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
…nd check it does not grow with rows The Taxa Lists speedup from main brings the list down from 35 queries to 13. The test now warms process-wide caches first, so the count no longer depends on which tests ran before it, and asserts that ten rows cost the same as five, which catches a per-row lookup on either a public or a project-scoped list. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z
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.
Summary
Some things in Antenna are meant to be shared by every project, such as a reference species list, while others belong to one project. Until now there was no way to say which was which. A taxa list with no project was treated as shared by convention, but in practice nobody except a superuser could see it, so the shared lists that already exist in the database (imported reference lists, for example) never appeared anywhere in the app.
This PR gives taxa lists and processing services an explicit "public" setting. A public list or service shows up inside every project alongside the project's own, clearly marked, and only people with a specific platform-level permission can change it, so a member of one project cannot edit something every other project relies on. Nothing changes for project-owned lists and services.
It is the first step of #1424, which uses public taxa lists to show what each classifier can predict and lets a project copy such a list and curate it. It also lays the ground for platform-level roles such as a taxonomy curator, who would be granted the new permission without being a superuser.
List of Changes
is_publicfield, declared once on an abstractPublicScopedModelbase thatTaxaListandProcessingServiceinherit; shown and filterable in the admin; read-only in the APIfor_project(project, include_public=True); both viewsets use itinclude_publicquery parameter on the list actions only, default true; an invalid value returns 400; detail, update and delete routes always resolve public rowsvisible_for_user()treats public rows as visible; theprojectsfield on both serializers is filtered to projects visible to the requester, computed once per requestmain.manage_public_taxalistandml.manage_public_processingservice, checked without reference to any project; superusers passuser_permissionsreports update and delete only to holders of the platform permissionadd_m2m_object_permissionsshort-circuits on public rows. For project-owned rows nothing changes:user_permissionsstill comes from guardian role permissions while the write gate for taxa lists is plain membership, so a basic member can edit through the API but sees no buttons; that pre-existing gap is #1120's and one test now pins it so any change is deliberateget_or_create_for_project(..., is_public=True)visible_for_user()get_object()instead of a raw id lookup;project_idstays optional for these two actions because the frontend calls them without itRelated Issues
Relates to #1424 (step A of its order of work). Overlaps with #1120 (permissions for things that belong to several projects) and #1081 (only administrators should be able to make a list shared by all projects).
Detailed Description
Why an explicit field. Treating "has no project" as "public" carries a silent risk: deleting a project removes its links, so a private list whose only project is deleted would become visible to everyone. With an explicit field, a row that is neither public nor linked to a project is an orphan that only superusers see.
Why the word "public". In this domain "global" already means geographic scope (the global moth classifier, global species lists). New code and comments say "public"; comments that said "global list" were reworded where touched.
Reading.
BaseQuerySet.for_project()tests project membership with anExistssubquery on the many-to-many table rather than a join. A join combined withOR is_publicwould return a public row once per linked project and force a.distinct(). One test pins that a public list linked to several projects appears once.Writing. The permission check happens at the object, not only at the request. An earlier shape of this change let a holder of the platform permission pass the request-level check and then edit a project-owned list in a project they do not belong to; the object-level check re-verifies membership for every non-public row, and two tests cover it. The add/remove-species route never calls DRF's object permission hook, so it checks explicitly. Creating through the API always produces a project-owned row.
Processing services. Writes on project-owned services stay staff-only, exactly as before; redesigning that belongs to #1120. The only new rule is that a public service needs the platform permission and staff status alone is refused.
Not in this PR. A public processing service is visible but not yet used to run jobs: job dispatch, the async worker heartbeat and count, the pipeline views' prefetch of services, and the check that gives a new project a default service all still filter services by project. These are listed in #1424 and come with the next step. No frontend changes are included; the API adds fields and a parameter and removes nothing.
Known and unchanged. The taxa list endpoint runs queries per row (measured: 34 queries for 5 rows, 58 for 10). That predates this PR and is fixed separately in #1428; the strict query-count test here pins today's number (35 for the 5-row fixture, one more than before for the once-per-request visible-projects lookup) and will be lowered when #1428 merges. The processing service list endpoint dropped from 34 to 21 queries for 5 rows in review, by not running guardian lookups that can never match (no
*_processingservicepermission exists).Permission classes. One base class parameterised by the non-public fallback (project membership for taxa lists, active staff for processing services) backs both viewsets; the previous class names remain as thin subclasses.
Two questions this raises for the other shared models.
is_publicanswers "who may see this row"; a second question is "who owns its contents", and the two are independent. Follow-up work on this feature adds aTaxaList.is_managedfor the second: a list an algorithm points at mirrors that classifier's category map, so the API refuses hand edits to it and the user copies it instead. The same pair looks worth having elsewhere. Pipelines and algorithms are created by registration from what a processing service advertises, and are then edited by hand in the admin with nothing recording that the next registration may overwrite the edit. The default processing service is created from settings rather than by a person. Taxa created from classifier labels carry only a name and a rank, and nothing marks them as awaiting a curator. Declaring both flags on the shared base, with the same read-only serializer fields, would let a client label and lock those rows the same way everywhere instead of each page guessing. That is a direction to discuss rather than something to add here, and it belongs with #1120 and the platform roles it describes.How to Test the Changes
Automated, run locally against the CI compose stack: the taxa list and processing service test classes pass on the final commit (86 across the matrices, re-run by the author after the structural review). The full
ami.main(424 tests) andami.ml(199 tests) modules passed during development, along withmakemigrations --check --dry-run, black, isort and flake8; CI on this PR is the authoritative run. Coverage includes a permission matrix for each model (project member, authenticated non-member, anonymous, superuser, staff, and a plain user granted the platform permission) against a public and a project-owned row, for list, retrieve, update, delete, add and remove species, connection test and pipeline registration.Manual:
?include_public=falsehides it;?include_public=abcreturns 400.Deployment Notes
Two migrations:
main/0096(adds the field and permission, makes a taxa list's projects optional, backfills) andml/0029(adds the field and permission, no backfill). Both are small and quick. After deploying, every taxa list that had no project becomes public and therefore visible in every project, except lists whose name starts with "Taxa returned by". It is worth looking at that set before deploying (TaxaList.objects.filter(projects__isnull=True)) in case any should be attached to a project instead. No processing service becomes public until someone marks it so. The reverse migration drops the field and does not restore anything else.Checklist
makemigrations --check --dry-runis clean🤖 Generated with Claude Code
https://claude.ai/code/session_01TngW6AshkUEhNp9D6z3U9Z