Extend object-reference checks to all versions and add bulk-archive action#3749
Extend object-reference checks to all versions and add bulk-archive action#3749SmittieC wants to merge 3 commits into
Conversation
… action Working and published experiment versions already blocked deletion of referenced service providers, assistants, pipelines, and collections. Non-published snapshot versions (not working, not published, not archived) were silently ignored, letting deletions corrupt those references. Changes: - Add is_blocking_object / is_bulk_archiveable helpers in apps/utils/deletion.py - Update get_related_experiments(_with_pipeline)_queryset methods in assistants and documents models to return ALL non-archived versions, not just working/published - Categorise blocking references in every delete view into 'manual-only' (working + published) and 'bulk-archiveable' (non-working, non-published, non-archived) - Add bulk_archive_experiment_versions endpoint (experiments:bulk_archive_versions) that archives a supplied list of non-working, non-published versions atomically - Extend the render_referenced_objects_modal helper (and referenced_objects.html) to render the bulk-archive section with a form button, threading request through for CSRF - Add tests for deletion blocking and the bulk-archive endpoint Ports the changes from PR #3198 (branch claude/issue-3197-20260422-0552) onto current main, adapting to the refactored render_referenced_objects_modal helper. Co-authored-by: Chris Smit <SmittieC@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Lowercase form method value (POST -> post) in referenced_objects.html - Collapse three-way conditional to experiment_objects or assistant_objects (equivalent, since those lists partition all blocking objects), which also clears the CodeScene complex-conditional advisory Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Fixed the CI failure and addressed the CodeScene advisory:
Verified locally: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
📝 WalkthroughWalkthroughThis PR broadens related-experiment queries to include all non-archived versions (not just default/published), introduces Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/pipelines/views.py (1)
218-231: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSplit static-trigger experiments by bulk-archive eligibility too.
get_static_trigger_experiment_ids()can return non-working, non-published versions, but they’re always passed as manual-only here, so they miss the bulk-archive path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pipelines/views.py` around lines 218 - 231, The pipeline referenced-objects modal currently builds static-trigger experiments only into the manual list, so versions returned by get_static_trigger_experiment_ids() never reach the bulk-archive flow. Update the logic in views.py around the pipeline modal assembly to split static-trigger experiments by is_bulk_archiveable the same way other experiments are handled, using the existing helpers like render_referenced_objects_modal, bulk_archiveable_experiments, and bulk_archiveable_ids so eligible static-trigger versions are included in the bulk-archive path.
🧹 Nitpick comments (7)
apps/utils/deletion.py (1)
198-204: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
is_bulk_archiveablesilently mislabels non-Experimentobjects.
is_default_versiononly exists onExperiment(perapps/experiments/models.py);OpenAiAssistantandCollectiondon't define it. Because of thegetattr(..., False)default, calling this on any non-Experimentversion object with aworking_version_idset would incorrectly returnTrue. All current call sites happen to pre-filter toExperimentinstances, so this isn't triggered today, but the function's generic signature invites future misuse.🛡️ Proposed defensive fix
def is_bulk_archiveable(obj) -> bool: """Returns True if an object can be bulk-archived (non-working, non-published, non-archived version).""" return ( - not getattr(obj, "is_archived", False) + isinstance(obj, Experiment) + and not getattr(obj, "is_archived", False) and getattr(obj, "working_version_id", None) is not None and not getattr(obj, "is_default_version", False) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/utils/deletion.py` around lines 198 - 204, `is_bulk_archiveable` currently assumes every object has `is_default_version`, which can wrongly return True for non-`Experiment` objects that only have `working_version_id`. Update the helper in `apps/utils/deletion.py` to defensively require an `Experiment`-style object before checking `is_default_version`, or otherwise gate the logic so it only applies to objects that actually define that attribute. Keep the existing archive/working-version checks, but make the function safe against `OpenAiAssistant` and `Collection` instances by using a type/attribute guard tied to `is_bulk_archiveable` and the `is_default_version` access.apps/service_providers/tests/test_views.py (2)
273-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parametrizing bulk-archive eligibility tests.
test_bulk_archive_non_published_versions,test_bulk_archive_ignores_published_version, andtest_bulk_archive_ignores_working_versionall exercise the same endpoint with different version states, again relying on inline comments to convey the intent.As per path instructions,
**/test_*.py: "Usepytest.mark.parametrizefor tests with varying inputs over enumerated data, with readable IDs viapytest.param(..., id=\"...\")rather than inline comments".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/service_providers/tests/test_views.py` around lines 273 - 320, The bulk-archive eligibility checks are split across three nearly identical tests, so consolidate them into a single parametrized test using pytest.mark.parametrize with readable ids. Update the test logic around test_bulk_archive_non_published_versions, test_bulk_archive_ignores_published_version, and test_bulk_archive_ignores_working_version to cover each version state via enumerated inputs, while keeping the shared setup and endpoint call in one place.Source: Path instructions
179-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parametrizing the blocking-vs-allowed scenarios.
TestDeleteServiceProviderReferenceCheckshas four tests (working / non-published / published / archived-only) that differ mainly in how the referencing experiment version is set up, using inline comments (e.g. "v1 auto-becomes published") to explain the setup instead of readable test IDs.As per path instructions,
**/test_*.py: "Usepytest.mark.parametrizefor tests with varying inputs over enumerated data, with readable IDs viapytest.param(..., id=\"...\")rather than inline comments".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/service_providers/tests/test_views.py` around lines 179 - 267, The `TestDeleteServiceProviderReferenceChecks` class has several near-duplicate delete-behavior tests that only vary by how the referencing experiment version is configured. Consolidate the blocking/allowed scenarios into a single `pytest.mark.parametrize`-driven test using readable `pytest.param(..., id="...")` values, and move the setup differences into per-case fixtures or inline setup branches tied to the parametrized case. Keep the assertions against `authed_client.delete`, `VoiceProvider`, and the modal response the same, but eliminate the separate working/non-published/published/archived-only test methods and the explanatory inline comments.Source: Path instructions
apps/service_providers/views.py (1)
122-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
is_bulk_archiveablecalls.
is_bulk_archiveable(e)is invoked once per experiment in three separate list comprehensions (manual_experiments, bulk_archiveable_experiments, and bulk_archiveable_ids). Partitionexperiment_objectsonce and derive all three downstream values from that single pass.♻️ Proposed refactor
- manual_experiments = [ - Chip( - label=( - f"{e.name} [{e.get_version_name()}]" - if e.is_working_version - else f"{e.name} {e.get_version_name()} [published]" - ), - url=e.get_absolute_url(), - ) - for e in experiment_objects - if not is_bulk_archiveable(e) - ] - bulk_archiveable_experiments = [ - Chip(label=f"{e.name} {e.get_version_name()}", url=e.get_absolute_url()) - for e in experiment_objects - if is_bulk_archiveable(e) - ] + archiveable, manual = [], [] + for e in experiment_objects: + (archiveable if is_bulk_archiveable(e) else manual).append(e) + + manual_experiments = [ + Chip( + label=( + f"{e.name} [{e.get_version_name()}]" + if e.is_working_version + else f"{e.name} {e.get_version_name()} [published]" + ), + url=e.get_absolute_url(), + ) + for e in manual + ] + bulk_archiveable_experiments = [ + Chip(label=f"{e.name} {e.get_version_name()}", url=e.get_absolute_url()) for e in archiveable + ] related_assistants = [ Chip(label=assistant.name, url=assistant.get_absolute_url()) for assistant in assistant_objects ] if manual_experiments or bulk_archiveable_experiments or related_assistants: return render_referenced_objects_modal( "service provider", request=request, experiments=manual_experiments, assistants=related_assistants, bulk_archiveable_experiments=bulk_archiveable_experiments, - bulk_archiveable_ids=[e.id for e in experiment_objects if is_bulk_archiveable(e)], + bulk_archiveable_ids=[e.id for e in archiveable], bulk_archive_url=reverse("experiments:bulk_archive_versions", args=[team_slug]), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/service_providers/views.py` around lines 122 - 150, Remove the repeated is_bulk_archiveable(e) checks in the referenced view by partitioning experiment_objects once in the service provider modal logic, then derive manual_experiments, bulk_archiveable_experiments, and bulk_archiveable_ids from that single split. Update the code near the Chip list comprehensions and the render_referenced_objects_modal call so the same categorization result is reused instead of recalculating it three times.apps/experiments/views/experiment.py (2)
890-898: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBulk-archive eligibility is re-implemented inline instead of reusing the
is_bulk_archiveablepredicate.Per the PR stack,
apps/utils/deletion.pyintroducesis_bulk_archiveable/is_blocking_objecthelpers specifically to define this eligibility. This view instead hardcodes the same conditions (working_version_id__isnull=False,is_default_version=False,is_archived=False) directly in a queryset filter. If the predicate's definition changes later, this endpoint's behavior can silently diverge from what the modal displays as "bulk-archiveable," letting the UI and the actual archive action disagree.Consider deriving this filter from (or validating against) the shared helper to keep a single source of truth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/experiments/views/experiment.py` around lines 890 - 898, The bulk-archive eligibility logic in the experiment view is duplicated inline instead of using the shared predicate. Update the queryset in experiment.py to reuse or validate against is_bulk_archiveable from apps/utils/deletion.py (and its related is_blocking_object helper if needed) so the archive action matches the same single source of truth as the modal. Keep the filtering behavior centralized around Experiment.objects.get_all() and the bulk-archiveable check rather than hardcoding the conditions in the view.
879-881: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDecorator order opens a DB transaction before the auth check.
Decorators apply bottom-up, so
login_and_team_required(innermost) runs inside thetransaction.atomicblock, which itself runs insiderequire_POST. This means a transaction is opened for every POST before authentication/team-membership is verified — wasted transaction overhead (and a stray BEGIN/ROLLBACK) for every unauthenticated/unauthorized request.♻️ Proposed reorder
-@require_POST -@transaction.atomic -@login_and_team_required +@login_and_team_required +@require_POST +@transaction.atomic def bulk_archive_experiment_versions(request, team_slug: str):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/experiments/views/experiment.py` around lines 879 - 881, The decorator order on the view opens a database transaction before the authentication/team check because decorators run bottom-up. Reorder the decorators on the affected view so `login_and_team_required` is applied outermost, `require_POST` next, and `transaction.atomic` innermost only around the actual view body; use the view’s unique identifier in this diff to update the decorator stack without changing its behavior.apps/generics/referenced_objects.py (1)
8-29: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winContract between
requestandbulk_archive_urlisn't enforced at runtime.The docstring correctly notes
requestis required for CSRF rendering whenbulk_archive_urlis set, but nothing enforces this. If a future caller passesbulk_archive_urlwithoutrequest,{% csrf_token %}silently renders empty (no context processors run), and the archive form will fail CSRF validation — a hard-to-diagnose failure.🛡️ Proposed guard
``request`` must be supplied when ``bulk_archive_url`` is set so the archive form can render a valid CSRF token. """ + if bulk_archive_url and request is None: + raise ValueError("`request` is required when `bulk_archive_url` is set") html = render_to_string(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/generics/referenced_objects.py` around lines 8 - 29, Add a runtime guard in the referenced_objects modal-rendering function so the request/bulk_archive_url contract is enforced, since the CSRF form depends on request being present. In the function that calls render_to_string for referenced_objects_modal.html, check for bulk_archive_url without request and fail fast with a clear error before rendering. Keep the validation close to the existing request=request usage so future callers of this helper cannot accidentally generate an archive form without CSRF context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/assistants/views.py`:
- Around line 232-259: The manual/bulk archive split for experiments is
duplicated here and in the other delete views, and it re-evaluates
is_bulk_archiveable(e) multiple times. Extract the classification and
Chip-labeling logic into a shared helper (for example, a utility used by
DeleteCollection.delete, DeletePipeline.delete, delete_service_provider, and
this assistant view) that takes all_experiments and returns manual chips, bulk
chips, and bulk archiveable ids in one pass. Keep the existing label formatting
and use the helper’s results directly in render_referenced_objects_modal so all
four call sites stay consistent.
In `@apps/experiments/views/experiment.py`:
- Around line 887-892: The `version_ids` handling in `experiment.py` should
validate POSTed values before they reach the
`Experiment.objects.get_all().filter(id__in=version_ids)` query. Add
parsing/validation in the request handler that only accepts numeric IDs (or
safely coerces them) and return `HttpResponse(status=400)` for any invalid input
instead of letting the ORM raise `ValueError`. Keep the fix near the
`version_ids` retrieval and the `Experiment` lookup so the existing early-return
pattern still applies.
In `@apps/service_providers/views.py`:
- Around line 117-142: The delete flow in the related-objects handling should
not proceed when any live relation exists. In the blocking_objects branch of the
service_providers view, either add the remaining related object types to the
modal content or change the condition so service_config.delete() is only reached
when blocking_objects is empty. Use the existing blocking_objects,
manual_experiments, bulk_archiveable_experiments, and related_assistants logic
to locate and update the guard.
---
Outside diff comments:
In `@apps/pipelines/views.py`:
- Around line 218-231: The pipeline referenced-objects modal currently builds
static-trigger experiments only into the manual list, so versions returned by
get_static_trigger_experiment_ids() never reach the bulk-archive flow. Update
the logic in views.py around the pipeline modal assembly to split static-trigger
experiments by is_bulk_archiveable the same way other experiments are handled,
using the existing helpers like render_referenced_objects_modal,
bulk_archiveable_experiments, and bulk_archiveable_ids so eligible
static-trigger versions are included in the bulk-archive path.
---
Nitpick comments:
In `@apps/experiments/views/experiment.py`:
- Around line 890-898: The bulk-archive eligibility logic in the experiment view
is duplicated inline instead of using the shared predicate. Update the queryset
in experiment.py to reuse or validate against is_bulk_archiveable from
apps/utils/deletion.py (and its related is_blocking_object helper if needed) so
the archive action matches the same single source of truth as the modal. Keep
the filtering behavior centralized around Experiment.objects.get_all() and the
bulk-archiveable check rather than hardcoding the conditions in the view.
- Around line 879-881: The decorator order on the view opens a database
transaction before the authentication/team check because decorators run
bottom-up. Reorder the decorators on the affected view so
`login_and_team_required` is applied outermost, `require_POST` next, and
`transaction.atomic` innermost only around the actual view body; use the view’s
unique identifier in this diff to update the decorator stack without changing
its behavior.
In `@apps/generics/referenced_objects.py`:
- Around line 8-29: Add a runtime guard in the referenced_objects
modal-rendering function so the request/bulk_archive_url contract is enforced,
since the CSRF form depends on request being present. In the function that calls
render_to_string for referenced_objects_modal.html, check for bulk_archive_url
without request and fail fast with a clear error before rendering. Keep the
validation close to the existing request=request usage so future callers of this
helper cannot accidentally generate an archive form without CSRF context.
In `@apps/service_providers/tests/test_views.py`:
- Around line 273-320: The bulk-archive eligibility checks are split across
three nearly identical tests, so consolidate them into a single parametrized
test using pytest.mark.parametrize with readable ids. Update the test logic
around test_bulk_archive_non_published_versions,
test_bulk_archive_ignores_published_version, and
test_bulk_archive_ignores_working_version to cover each version state via
enumerated inputs, while keeping the shared setup and endpoint call in one
place.
- Around line 179-267: The `TestDeleteServiceProviderReferenceChecks` class has
several near-duplicate delete-behavior tests that only vary by how the
referencing experiment version is configured. Consolidate the blocking/allowed
scenarios into a single `pytest.mark.parametrize`-driven test using readable
`pytest.param(..., id="...")` values, and move the setup differences into
per-case fixtures or inline setup branches tied to the parametrized case. Keep
the assertions against `authed_client.delete`, `VoiceProvider`, and the modal
response the same, but eliminate the separate
working/non-published/published/archived-only test methods and the explanatory
inline comments.
In `@apps/service_providers/views.py`:
- Around line 122-150: Remove the repeated is_bulk_archiveable(e) checks in the
referenced view by partitioning experiment_objects once in the service provider
modal logic, then derive manual_experiments, bulk_archiveable_experiments, and
bulk_archiveable_ids from that single split. Update the code near the Chip list
comprehensions and the render_referenced_objects_modal call so the same
categorization result is reused instead of recalculating it three times.
In `@apps/utils/deletion.py`:
- Around line 198-204: `is_bulk_archiveable` currently assumes every object has
`is_default_version`, which can wrongly return True for non-`Experiment` objects
that only have `working_version_id`. Update the helper in
`apps/utils/deletion.py` to defensively require an `Experiment`-style object
before checking `is_default_version`, or otherwise gate the logic so it only
applies to objects that actually define that attribute. Keep the existing
archive/working-version checks, but make the function safe against
`OpenAiAssistant` and `Collection` instances by using a type/attribute guard
tied to `is_bulk_archiveable` and the `is_default_version` access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bcf2b181-8f8b-45b2-b4f1-a5dabe281b65
📒 Files selected for processing (13)
apps/assistants/models.pyapps/assistants/views.pyapps/documents/models.pyapps/documents/views.pyapps/experiments/urls.pyapps/experiments/views/__init__.pyapps/experiments/views/experiment.pyapps/generics/referenced_objects.pyapps/pipelines/views.pyapps/service_providers/tests/test_views.pyapps/service_providers/views.pyapps/utils/deletion.pytemplates/generic/referenced_objects.html
| all_experiments = list( | ||
| assistant.get_related_experiments_with_pipeline_queryset(assistant_ids=version_query) | ||
| ) | ||
| manual_experiments = [ | ||
| Chip( | ||
| label=f"{experiment.name} {experiment.get_version_name()} [published]", | ||
| url=experiment.get_absolute_url(), | ||
| label=( | ||
| f"{e.name} [{e.get_version_name()}]" | ||
| if e.is_working_version | ||
| else f"{e.name} {e.get_version_name()} [published]" | ||
| ), | ||
| url=e.get_absolute_url(), | ||
| ) | ||
| for experiment in assistant.get_related_experiments_with_pipeline_queryset(assistant_ids=version_query) | ||
| for e in all_experiments | ||
| if not is_bulk_archiveable(e) | ||
| ] | ||
| bulk_archiveable_experiments = [ | ||
| Chip(label=f"{e.name} {e.get_version_name()}", url=e.get_absolute_url()) | ||
| for e in all_experiments | ||
| if is_bulk_archiveable(e) | ||
| ] | ||
| return render_referenced_objects_modal( | ||
| "assistant", | ||
| request=request, | ||
| pipeline_nodes=pipeline_nodes, | ||
| experiments_with_pipeline_nodes=experiments_with_pipeline_nodes, | ||
| experiments_with_pipeline_nodes=manual_experiments, | ||
| bulk_archiveable_experiments=bulk_archiveable_experiments, | ||
| bulk_archiveable_ids=[e.id for e in all_experiments if is_bulk_archiveable(e)], | ||
| bulk_archive_url=reverse("experiments:bulk_archive_versions", args=[team_slug]), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the manual/bulk-archiveable split into a shared helper.
This exact block — all_experiments split into manual_experiments/bulk_archiveable_experiments Chips plus a re-filtered bulk_archiveable_ids list, with identical label formatting — is duplicated verbatim in apps/documents/views.py (DeleteCollection.delete), apps/pipelines/views.py (DeletePipeline.delete), and apps/service_providers/views.py (delete_service_provider). is_bulk_archiveable(e) is also evaluated redundantly (once for the manual filter, once for the bulk chip list, once again for bulk_archiveable_ids).
Extracting a shared helper (e.g. in apps/utils/deletion.py) that classifies all_experiments once and returns (manual_chips, bulk_chips, bulk_ids) would eliminate the duplication across all four call sites and prevent future divergence when the labeling logic changes.
♻️ Proposed helper extraction
# apps/utils/deletion.py
def split_experiments_for_deletion_modal(experiments) -> tuple[list[Chip], list[Chip], list[int]]:
manual, bulk, bulk_ids = [], [], []
for e in experiments:
label = f"{e.name} {e.get_version_name()}"
if is_bulk_archiveable(e):
bulk.append(Chip(label=label, url=e.get_absolute_url()))
bulk_ids.append(e.id)
else:
label = f"{e.name} [{e.get_version_name()}]" if e.is_working_version else f"{label} [published]"
manual.append(Chip(label=label, url=e.get_absolute_url()))
return manual, bulk, bulk_ids- all_experiments = list(
- assistant.get_related_experiments_with_pipeline_queryset(assistant_ids=version_query)
- )
- manual_experiments = [
- Chip(
- label=(
- f"{e.name} [{e.get_version_name()}]"
- if e.is_working_version
- else f"{e.name} {e.get_version_name()} [published]"
- ),
- url=e.get_absolute_url(),
- )
- for e in all_experiments
- if not is_bulk_archiveable(e)
- ]
- bulk_archiveable_experiments = [
- Chip(label=f"{e.name} {e.get_version_name()}", url=e.get_absolute_url())
- for e in all_experiments
- if is_bulk_archiveable(e)
- ]
+ all_experiments = list(
+ assistant.get_related_experiments_with_pipeline_queryset(assistant_ids=version_query)
+ )
+ manual_experiments, bulk_archiveable_experiments, bulk_archiveable_ids = (
+ split_experiments_for_deletion_modal(all_experiments)
+ )
return render_referenced_objects_modal(
"assistant",
request=request,
pipeline_nodes=pipeline_nodes,
experiments_with_pipeline_nodes=manual_experiments,
bulk_archiveable_experiments=bulk_archiveable_experiments,
- bulk_archiveable_ids=[e.id for e in all_experiments if is_bulk_archiveable(e)],
+ bulk_archiveable_ids=bulk_archiveable_ids,
bulk_archive_url=reverse("experiments:bulk_archive_versions", args=[team_slug]),
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/assistants/views.py` around lines 232 - 259, The manual/bulk archive
split for experiments is duplicated here and in the other delete views, and it
re-evaluates is_bulk_archiveable(e) multiple times. Extract the classification
and Chip-labeling logic into a shared helper (for example, a utility used by
DeleteCollection.delete, DeletePipeline.delete, delete_service_provider, and
this assistant view) that takes all_experiments and returns manual chips, bulk
chips, and bulk archiveable ids in one pass. Keep the existing label formatting
and use the helper’s results directly in render_referenced_objects_modal so all
four call sites stay consistent.
| version_ids = request.POST.getlist("version_ids") | ||
| if not version_ids: | ||
| return HttpResponse(status=400) | ||
| experiments = list( | ||
| Experiment.objects.get_all().filter( | ||
| id__in=version_ids, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unvalidated version_ids can raise an unhandled ValueError (500) instead of a clean 400.
request.POST.getlist("version_ids") returns raw strings passed straight into id__in=version_ids on an integer PK field. A malformed (non-numeric) value will raise ValueError inside the ORM during query construction, resulting in an unhandled 500 rather than a graceful error response.
🛡️ Proposed validation
version_ids = request.POST.getlist("version_ids")
if not version_ids:
return HttpResponse(status=400)
+ try:
+ version_ids = [int(v) for v in version_ids]
+ except (TypeError, ValueError):
+ return HttpResponse(status=400)
experiments = list(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| version_ids = request.POST.getlist("version_ids") | |
| if not version_ids: | |
| return HttpResponse(status=400) | |
| experiments = list( | |
| Experiment.objects.get_all().filter( | |
| id__in=version_ids, | |
| version_ids = request.POST.getlist("version_ids") | |
| if not version_ids: | |
| return HttpResponse(status=400) | |
| try: | |
| version_ids = [int(v) for v in version_ids] | |
| except (TypeError, ValueError): | |
| return HttpResponse(status=400) | |
| experiments = list( | |
| Experiment.objects.get_all().filter( | |
| id__in=version_ids, |
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 888-888: Lack of sanitization of user data
Context: HttpResponse(status=400)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(http-response-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/experiments/views/experiment.py` around lines 887 - 892, The
`version_ids` handling in `experiment.py` should validate POSTed values before
they reach the `Experiment.objects.get_all().filter(id__in=version_ids)` query.
Add parsing/validation in the request handler that only accepts numeric IDs (or
safely coerces them) and return `HttpResponse(status=400)` for any invalid input
instead of letting the ORM raise `ValueError`. Keep the fix near the
`version_ids` retrieval and the `Experiment` lookup so the existing early-return
pattern still applies.
| blocking_objects = [obj for obj in related_objects if is_blocking_object(obj)] | ||
| if blocking_objects: | ||
| experiment_objects = [obj for obj in blocking_objects if isinstance(obj, Experiment)] | ||
| assistant_objects = [obj for obj in blocking_objects if isinstance(obj, OpenAiAssistant)] | ||
|
|
||
| manual_experiments = [ | ||
| Chip( | ||
| label=( | ||
| f"{experiment.name} [{experiment.get_version_name()}]" | ||
| if experiment.is_working_version | ||
| else f"{experiment.name} {experiment.get_version_name()} [published]" | ||
| f"{e.name} [{e.get_version_name()}]" | ||
| if e.is_working_version | ||
| else f"{e.name} {e.get_version_name()} [published]" | ||
| ), | ||
| url=experiment.get_absolute_url(), | ||
| url=e.get_absolute_url(), | ||
| ) | ||
| for experiment in [obj for obj in filtered_objects if isinstance(obj, Experiment)] | ||
| for e in experiment_objects | ||
| if not is_bulk_archiveable(e) | ||
| ] | ||
| bulk_archiveable_experiments = [ | ||
| Chip(label=f"{e.name} {e.get_version_name()}", url=e.get_absolute_url()) | ||
| for e in experiment_objects | ||
| if is_bulk_archiveable(e) | ||
| ] | ||
| related_assistants = [ | ||
| Chip(label=assistant.name, url=assistant.get_absolute_url()) | ||
| for assistant in [obj for obj in filtered_objects if isinstance(obj, OpenAiAssistant)] | ||
| Chip(label=assistant.name, url=assistant.get_absolute_url()) for assistant in assistant_objects | ||
| ] | ||
| if related_experiments or related_assistants: | ||
| if manual_experiments or bulk_archiveable_experiments or related_assistants: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect what object types get_related_objects can surface for service providers
rg -n -A 30 "_get_related_objects_querysets" apps/utils/deletion.pyRepository: dimagi/open-chat-studio
Length of output: 2292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/service_providers/views.py around the reported lines =="
sed -n '90,170p' apps/service_providers/views.py | cat -n
echo
echo "== is_blocking_object and related deletion helpers =="
rg -n -A 20 -B 10 "def is_blocking_object|class Node|get_related_objects\(" apps/utils/deletion.py apps -g '*.py'
echo
echo "== service provider delete view / delete call sites =="
rg -n -A 30 -B 15 "service_config\.delete\(|blocking_objects|related_objects" apps/service_providers/views.py apps -g '*.py'Repository: dimagi/open-chat-studio
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== provider model classes =="
rg -n -A 120 -B 20 "class .*ProviderModel|class .*ProviderType|class ServiceProvider" apps/service_providers/models.py apps/service_providers/utils.py -g '*.py'
echo
echo "== reverse relations into provider models =="
rg -n -A 3 -B 3 "ForeignKey\(.*service_providers|related_name=.*(llm|voice|embedding)|on_delete=models\.SET_NULL|on_delete=models\.CASCADE" apps -g '*.py' | sed -n '1,220p'
echo
echo "== delete methods and has_related_objects usage on provider models =="
rg -n -A 15 -B 10 "def delete\(|has_related_objects\(" apps/service_providers/models.py apps -g '*.py' | sed -n '1,220p'Repository: dimagi/open-chat-studio
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Foreign keys / reverse relations to provider models =="
rg -n -A 4 -B 4 "ForeignKey\(.*(LlmProvider|VoiceProvider|MessagingProvider|AuthProvider|TraceProvider)|related_name=.*(provider|providers|voice|llm|messaging|auth|trace)|OneToOneField\(.*(LlmProvider|VoiceProvider|MessagingProvider|AuthProvider|TraceProvider)" apps -g '*.py' | sed -n '1,260p'
echo
echo "== get_related_objects consumers for provider deletes =="
rg -n -A 20 -B 10 "delete_service_provider|has_related_objects\(self|get_related_objects\(self|get_related_objects\(service_config" apps/service_providers apps -g '*.py' | sed -n '1,260p'
echo
echo "== provider delete overrides =="
rg -n -A 20 -B 5 "class (LlmProvider|VoiceProvider|MessagingProvider|AuthProvider|TraceProvider)|def delete\(" apps/service_providers/models.py -g '*.py' | sed -n '1,260p'Repository: dimagi/open-chat-studio
Length of output: 1969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== provider reverse FKs =="
rg -n -A 3 -B 3 "models\.ForeignKey\(.*(VoiceProvider|LlmProvider|MessagingProvider|AuthProvider|TraceProvider)|models\.OneToOneField\(.*(VoiceProvider|LlmProvider|MessagingProvider|AuthProvider|TraceProvider)" apps -g '*.py' | sed -n '1,220p'
echo
echo "== SyntheticVoice model section =="
rg -n -A 80 -B 20 "class SyntheticVoice" apps -g '*.py' | sed -n '1,220p'
echo
echo "== experiment assistant/provider references =="
rg -n -A 4 -B 4 "OpenAiAssistant|Experiment.*ForeignKey|related_name=.*assistant|related_name=.*experiment" apps -g '*.py' | sed -n '1,220p'Repository: dimagi/open-chat-studio
Length of output: 1969
Gate deletion on all live related objects. blocking_objects already contains every non-archived related record, but the modal only handles Experiment and OpenAiAssistant. Any other related type will fall through to service_config.delete(), so extend the modal to cover the remaining relations or return early whenever blocking_objects is non-empty.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/service_providers/views.py` around lines 117 - 142, The delete flow in
the related-objects handling should not proceed when any live relation exists.
In the blocking_objects branch of the service_providers view, either add the
remaining related object types to the modal content or change the condition so
service_config.delete() is only reached when blocking_objects is empty. Use the
existing blocking_objects, manual_experiments, bulk_archiveable_experiments, and
related_assistants logic to locate and update the guard.
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 file with Complex Method)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| views.py | 1 advisory rule | 9.39 → 9.34 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| deletion.py | 8.53 → 9.08 | Overall Code Complexity |
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
1 participant
Product Description
When deleting an object (service provider, assistant, pipeline, collection), deletion is now blocked when any non-archived version — not just working/published — references it. The "Cannot delete" modal now offers a one-click bulk-archive button for non-working, non-published snapshot versions, while working and published versions must still be cleaned up manually.
Resolves #3197. Ports #3198 (branch claude/issue-3197-20260422-0552) onto current main. Closes #3198.
Technical Description
is_blocking_object/is_bulk_archiveablehelpers inapps/utils/deletion.py.get_related_experiments*querysets (assistants, documents) now return all non-archived versions.experiments:bulk_archive_versionsendpoint archives non-working, non-published versions atomically (team-scoped).render_referenced_objects_modalhelper +referenced_objects.htmlfor the bulk-archive section (threads request for CSRF).Migrations
Docs and Changelog
Generated with Claude Code