Add bulk delete to query builder#8120
Open
alesan99 wants to merge 23 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 7 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a backend atomic bulk-delete POST endpoint accepting explicit IDs or a stored query and a frontend two-step bulk-delete UI with permissions, result updates, localization, and OpenAPI documentation. ChangesBulk Delete Feature
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Triggered by 5aa0849 on branch refs/heads/issue-6882
Triggered by 3957e98 on branch refs/heads/issue-6882
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@specifyweb/backend/bulk_copy/bulk_copy.py`:
- Around line 94-124: The code is using spquery["contexttableid"] to fetch IDs
but then deletes rows from the URL {model}, allowing mismatched-table deletes;
update the logic in the block that builds the query (the session_context /
build_query path using fields_from_json and build_query) to verify the resolved
query target matches the route model before proceeding: after obtaining entity =
query.column_descriptions[0]["entity"] (or inspecting pk_col via
inspect(entity)) compare the entity/table identifier to the route model's table
identity (e.g., model.__tablename__ or model's table id), and if they differ
raise/return a 400 error (abort the operation) rather than continuing to call
delete_resource; keep the check local to the code that computes ids (the
session_context / build_query / ids logic) so deletions only proceed when the
query resolves to the same model.
- Around line 84-90: The handler currently assumes json.loads(request.body)
returns a dict and calls data.get(...), which will crash for list/scalar bodies;
update the code in bulk_copy.py (the section handling data, ids, spquery and
model) to first verify that data is a dict and return a 400 (bad request) if
not, then validate that ids (if present) is a list and spquery (if present) is
the expected type (e.g., str or dict per your API) before using .get; keep the
existing model handling (isinstance(model, str) -> get_model_or_404(model)) but
perform these body/type checks immediately after parsing to reject invalid
bodies and provide clear error messages.
In `@specifyweb/frontend/js_src/lib/components/QueryBuilder/BulkDelete.tsx`:
- Around line 132-156: The final confirmation Dialog (rendered when
isWarningOpen) only shows generic text; update the bulk-delete confirmation UI
to surface dependent/related records and potential deletion blockers before the
delete is committed: compute or fetch the dependent records (e.g., a new
dependentRecords array derived alongside totalDeleteCount in the BulkDelete
logic or via an async call in the component), then render a clear section inside
the Dialog (near formsText.bulkDeleteFinalConfirmationDescription()) listing
each dependent record type/count and any blockers, and disable the Button.Danger
(handleClick) unless the user has acknowledged or resolved blockers; update the
Dialog content to include this dependent-record summary and any required user
acknowledgement state so handleClick only runs when safe.
- Around line 34-36: The button is incorrectly disabled when queryResource is
undefined even if there are selected rows; change the disabled logic on
Button.Small to only be true when there are no rows to delete (e.g. replace
disabled={totalCount === 0 || queryResource === undefined} with something like
disabled={totalCount === 0 && (selectedIds?.length ?? 0) === 0} so the button is
enabled when selectedIds (or your actual selected-row array/ids variable) has
entries even if queryResource is undefined; update the expression to use the
actual selected-row variable name present in this file (e.g. selectedIds,
selectedRowIds, or selectedRows) and keep onClick={handleOpen} as-is.
In `@specifyweb/frontend/js_src/lib/components/QueryBuilder/Results.tsx`:
- Around line 407-420: The bulk-delete button renders even for distinct queries
which makes recordIds() unsafe; update the rendering guard so QueryBulkDelete
(used here with props table, totalCount, onDeleted, recordIds, queryResource) is
only shown when canBulkDeleteTable is true AND the result set is not distinct
(e.g., !isDistinct) or when a guaranteed primary-key derivation is available;
specifically, change the condition around the QueryBulkDelete component to check
isDistinct (or a new safeBulkDelete boolean) and skip rendering QueryBulkDelete
when isDistinct is true unless you can derive real primary keys for each row
instead of casting display values via queryIdField from
loadedResults/selectedRows.
- Around line 221-238: The bug is that totalRemoveCount is accumulated with a
negative sign (totalRemoveCount = totalRemoveCount - removeCount) so later
subtracting it in setTotalCount increases totalCount; change the accumulator to
add positives (use totalRemoveCount += removeCount) and ensure the "no ids
provided" branch sets totalRemoveCount to the positive count (totalRemoveCount =
totalCount ?? 0) so the final setTotalCount(... Math.max(0, totalCount -
totalRemoveCount)) correctly decreases totalCount; update references in the
block handling recordIds, the loop using removeCount and deletingRef, and the
no-ids branch so all usages treat totalRemoveCount as a positive number of
removed rows.
- Around line 240-244: The selection filter currently uses a stale recordIds and
ends up preserving deleted row ids; update the newSelectedRows implementation so
it computes the current set of existing IDs (e.g., map the current records to
currentIds) and then return new Set(Array.from(selectedRows).filter(id =>
currentIds.includes(id))) so setSelectedRows(newSelectedRows(selectedRows))
drops any ids that no longer exist; modify newSelectedRows to reference that
fresh currentIds (instead of the old recordIds) to ensure deleted selections are
removed.
- Around line 199-247: The handleDelete closure uses stale
results/selectedRows/totalCount because those values aren't in its deps; change
it to read latest state from refs and use functional state updates: inside
handleDelete, read the current list from resultsRef.current (fallback to [])
instead of the closed-over results, compute filteredResults and
totalRemoveCount, then call setResults(prev => /* use prev or resultsRef.current
to compute new array */) and setTotalCount(prev => /* compute from prev and
totalRemoveCount */) and setSelectedRows(prev => new
Set(Array.from(prev).filter(id => !recordIds.includes(id)))), and keep
deletingRef usage; update the dependency list to only include stable setters and
refs (setResults, setTotalCount, setSelectedRows, deletingRef, resultsRef) so
the callback no longer captures stale snapshots.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 427c36b6-8c21-45e8-af63-818c2fc6b464
📒 Files selected for processing (9)
specifyweb/backend/bulk_copy/bulk_copy.pyspecifyweb/backend/bulk_copy/urls.pyspecifyweb/backend/bulk_copy/views.pyspecifyweb/frontend/js_src/lib/components/Permissions/definitions.tsspecifyweb/frontend/js_src/lib/components/QueryBuilder/BulkDelete.tsxspecifyweb/frontend/js_src/lib/components/QueryBuilder/Results.tsxspecifyweb/frontend/js_src/lib/localization/forms.tsspecifyweb/frontend/js_src/lib/localization/query.tsspecifyweb/specify/models_utils/schema.py
Comment on lines
+132
to
+156
| <div className="mb-4 flex flex-col gap-4"> | ||
| <section>{formsText.deleteConfirmationDescription()}</section> | ||
| </div> | ||
| </Dialog> | ||
| { | ||
| isWarningOpen ? | ||
| ( | ||
| <Dialog | ||
| buttons={ | ||
| <> | ||
| <Button.DialogClose>{commonText.close()}</Button.DialogClose> | ||
| <Button.Danger | ||
| disabled={confirmationText !== totalDeleteCount.toString()} | ||
| onClick={handleClick} | ||
| > | ||
| {formsText.bulkDeleteFinalConfirmationOption({ count: totalDeleteCount })} | ||
| </Button.Danger> | ||
| </> | ||
| } | ||
| header={formsText.bulkDeleteFinalConfirmation()} | ||
| onClose={onClose} | ||
| > | ||
| <div className="mb-4 flex flex-col gap-4"> | ||
| <section>{formsText.deleteConfirmationDescription()}</section> | ||
| <section>{formsText.bulkDeleteFinalConfirmationDescription()}</section> |
There was a problem hiding this comment.
Show dependent-record impact before the destructive confirmation.
This dialog still only shows generic warning text and the typed count. The linked issue/PR objective requires surfacing related records/deletion blockers before confirmation, so users can see what else will be affected before committing the delete.
🤖 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 `@specifyweb/frontend/js_src/lib/components/QueryBuilder/BulkDelete.tsx` around
lines 132 - 156, The final confirmation Dialog (rendered when isWarningOpen)
only shows generic text; update the bulk-delete confirmation UI to surface
dependent/related records and potential deletion blockers before the delete is
committed: compute or fetch the dependent records (e.g., a new dependentRecords
array derived alongside totalDeleteCount in the BulkDelete logic or via an async
call in the component), then render a clear section inside the Dialog (near
formsText.bulkDeleteFinalConfirmationDescription()) listing each dependent
record type/count and any blockers, and disable the Button.Danger (handleClick)
unless the user has acknowledged or resolved blockers; update the Dialog content
to include this dependent-record summary and any required user acknowledgement
state so handleClick only runs when safe.
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
Verify query table matches bulk_delete table
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Disable bulk delete for distinct queries
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
2 participants
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.
Partially fixes issue #6882
Adds a "Bulk Delete" button to Query Builder. Also adds a new "bulk_delete" permission.
Implemented through the new bulk_delete endpoint.
TODO:
Checklist
self-explanatory (or properly documented)
Testing instructions
Summary by CodeRabbit
New Features
Localization
Documentation