Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Deletion handling uses incomplete or ineligible identifiers, can clear guards before queued jobs run, and validates unused file references, risking incorrect relationships and blocked imports.
5 open issues
What changed in this PR
This PR aligns guided CSV validation with Bulkrax’s delete and re-import behavior, addressing the unrecognized delete column reported in #1243.
Changes:
- Recognizes row-action columns and adds deletion warnings, checks, counts, and tree badges.
- Tracks deleted identifiers to prevent relationships to deleted rows.
- Adds regression coverage and updates documentation and translations.
| File | Description |
|---|---|
| spec/validators/bulkrax/csv_row/required_values_spec.rb | Tests skipping required values for delete rows. |
| spec/validators/bulkrax/csv_row/parent_reference_spec.rb | Tests deletion-aware parent checks. |
| spec/validators/bulkrax/csv_row/duplicate_identifier_spec.rb | Tests delete and re-import warnings. |
| spec/validators/bulkrax/csv_row/delete_record_spec.rb | Covers the new deletion validator. |
| spec/validators/bulkrax/csv_row/controlled_vocabulary_spec.rb | Tests skipping vocabulary checks for delete rows. |
| spec/validators/bulkrax/csv_row/child_reference_spec.rb | Tests deletion-aware child checks. |
| spec/services/bulkrax/stepper_response_formatter_spec.rb | Tests deletion-count passthrough. |
| spec/services/bulkrax/csv_template/csv_parser_template_spec.rb | Covers deletion validation and response data. |
| spec/parsers/bulkrax/csv_parser/csv_validation_hierarchy_spec.rb | Tests deletion markers and relationship filtering. |
| spec/parsers/bulkrax/csv_parser/csv_validation_helpers_spec.rb | Tests action flags, graph filtering, and lookup failures. |
| spec/parsers/bulkrax/csv_parser_spec.rb | Tests saving deleted identifiers before job submission. |
| spec/models/bulkrax/csv_entry_spec.rb | Tests suppressing relationships to deleted identifiers. |
| lib/bulkrax.rb | Registers the deletion validator. |
| docs/VALIDATION_RESPONSE_QUICK_REF.json | Documents deletion fields and error categories. |
| docs/STEPPER_IMPLEMENTATION.md | Updates the documented validator list. |
| docs/CSV_SERVICE_ARCHITECTURE.md | Documents deletion validation and shared context. |
| config/locales/bulkrax.zh.yml | Adds Chinese deletion messages. |
| config/locales/bulkrax.pt-BR.yml | Adds Brazilian Portuguese deletion messages. |
| config/locales/bulkrax.it.yml | Adds Italian deletion messages. |
| config/locales/bulkrax.fr.yml | Adds French deletion messages. |
| config/locales/bulkrax.es.yml | Adds Spanish deletion messages. |
| config/locales/bulkrax.en.yml | Adds English deletion messages. |
| config/locales/bulkrax.de.yml | Adds German deletion messages. |
| app/validators/bulkrax/csv_row/required_values.rb | Skips required-value checks for delete rows. |
| app/validators/bulkrax/csv_row/relationship_ids.rb | Builds deleted-reference errors. |
| app/validators/bulkrax/csv_row/parent_reference.rb | Rejects references to deleted parents. |
| app/validators/bulkrax/csv_row/duplicate_identifier.rb | Distinguishes updates, deletions, and re-imports. |
| app/validators/bulkrax/csv_row/delete_record.rb | Warns about deletions and rejects unmatched targets. |
| app/validators/bulkrax/csv_row/controlled_vocabulary.rb | Skips vocabulary checks for delete rows. |
| app/validators/bulkrax/csv_row/child_reference.rb | Rejects references to deleted children. |
| app/services/bulkrax/stepper_response_formatter.rb | Exposes the deletion count. |
| app/parsers/concerns/bulkrax/csv_parser/csv_validation.rb | Supplies deletion context to validation and hierarchy building. |
| app/parsers/concerns/bulkrax/csv_parser/csv_validation_hierarchy.rb | Marks delete rows and filters their relationships. |
| app/parsers/concerns/bulkrax/csv_parser/csv_validation_helpers.rb | Parses action flags and builds deletion-aware context. |
| app/parsers/bulkrax/csv_parser.rb | Saves deleted identifiers before queuing jobs. |
| app/parsers/bulkrax/application_parser.rb | Shares boolean flag interpretation with validation. |
| app/models/concerns/bulkrax/import_behavior.rb | Suppresses relationships to deleted identifiers. |
| app/assets/stylesheets/bulkrax/stepper/_summary.scss | Styles deletion badges. |
| app/assets/javascripts/bulkrax/importers_stepper.js | Displays deletion badges and counts. |
Review effort: Balanced
laritakr
force-pushed
the
validate-delete-rows
branch
from
November 5, 2026 23:15
438e4ba to
39f0b8e
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Deletion tracking can lose current deletions or retain stale ones, causing incorrect relationship handling, while preflight and header validation still block deletion-only CSVs with blank or omitted required metadata.
4 open issues
5 resolved since last review
Review effort: Balanced
laritakr
force-pushed
the
validate-delete-rows
branch
from
November 6, 2026 00:18
39f0b8e to
b58f23c
Compare
laritakr
force-pushed
the
validate-delete-rows
branch
from
November 6, 2026 00:34
b58f23c to
9cdc9df
Compare
Guided validation now accepts delete and remove_and_rerun columns, says which records a row will delete or re-import, and blocks delete rows that match nothing or that other rows still relate to; import no longer writes relationships to rows it deletes. Validation called the delete column unrecognized, gave delete rows the update warning and held them to checks import never runs on them, while a relationship naming a deleted row was queued and failed after its retries. Assisted by: Claude Opus 5.5
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Deletion-target tracking omits stored source identifiers and identifiers generated later for blank delete rows, allowing surviving relationships to reference records that will be deleted.
2 open issues
2 resolved since last review
Review effort: Balanced
laritakr
marked this pull request as ready for review
November 6, 2026 01:44
This was referenced Nov 6, 2026
ShanaLMoore
added a commit
that referenced
this pull request
Nov 6, 2026
When several file set rows for one work import at once, some file sets drop off the work. ValkyrieObjectFactory#create_file_set loaded the work outside any lock and ran Hyrax's change_set.update_work, whose first step saves the work's member_ids as loaded, erasing file sets other rows had attached meanwhile. hyrax#7240 fixed only the attach step, on 5.0-flexible, and that step runs after the stale save. On the Valkyrie path, each file set row now creates its file set without touching the work: with the work's permissions, the row's visibility, or else the work's visibility, embargo and lease, and its upload left unqueued. The factory builds the file set as Hyrax 5.3's WorkUploadsHandler does rather than calling it, since Hyrax 4's handler queues the ingest itself. The row records a pending relationship to its first parent, the one the file set is created for, skipping a parent the import deletes once #1279's deleted_identifier? is there. A row whose parent is not found fails before anything is saved. A re-import of a row with no visibility gives the file set its work's current visibility, embargo and lease, as creating it does, unless the file set's own embargo or lease is still running. CreateRelationshipsJob then adds every file set to the work in one pass under the work's lock, setting the representative and thumbnail on the first as Hyrax did on attach. Once the work is saved it queues each upload's ingest once, through the object factory, and only then destroys that file set's relationship, so a pass killed or failing before that leaves it for the retry, which queues the ingest of a file set already attached without its file. An enqueue that is refused without raising is a failure. ActiveFedora is unchanged: its file sets are attached when created, its entries record no relationship, and its factory holds no ingests. Ref: - #570 - samvera/hyrax#7240 - notch8/hykuup_knapsack#567 - notch8/utk_knapsack#435 Assisted by: Opus 5.5
ShanaLMoore
added a commit
that referenced
this pull request
Nov 6, 2026
When several file set rows for one work import at once, some file sets drop off the work. ValkyrieObjectFactory#create_file_set loaded the work outside any lock and ran Hyrax's change_set.update_work, whose first step saves the work's member_ids as loaded, erasing file sets other rows had attached meanwhile. hyrax#7240 fixed only the attach step, on 5.0-flexible, and that step runs after the stale save. On the Valkyrie path, each file set row now creates its file set without touching the work: with the work's permissions, the row's visibility, or else the work's visibility, embargo and lease, and its upload left unqueued. The factory builds the file set as Hyrax 5.3's WorkUploadsHandler does rather than calling it, since Hyrax 4's handler queues the ingest itself. The row records a pending relationship to its first parent, the one the file set is created for, skipping a parent the import deletes once #1279's deleted_identifier? is there. A row whose parent is not found, or is not a work, fails before anything is saved. A re-import of a row with no visibility gives the file set its work's current visibility, embargo and lease, as creating it does, unless the file set's own embargo or lease is still running. An ended embargo or lease on the file set is cleared first, so Hyrax's expiry jobs cannot later release it to its after-visibility, and the file set is reindexed so its indexed embargo and lease match. CreateRelationshipsJob then adds every file set to the work in one pass under the work's lock, setting the representative and thumbnail on the first as Hyrax did on attach. Once the work is saved it queues each upload's ingest once, through the object factory, and only then destroys that file set's relationship, so a pass killed or failing before that leaves it for the retry, which queues the ingest of a file set already attached without its file. An enqueue that is refused without raising is a failure. ActiveFedora is unchanged: its file sets are attached when created, its entries record no relationship, and its factory holds no ingests. Ref: - #570 - samvera/hyrax#7240 - notch8/hykuup_knapsack#567 - notch8/utk_knapsack#435 Assisted by: Opus 5.5
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.
Refs #1243
🐛 Validate delete rows the way import runs them
Guided validation now accepts delete and remove_and_rerun columns, says
which records a row will delete or re-import, and blocks delete rows that
match nothing or that other rows still relate to; import no longer
writes relationships to rows it deletes.
Validation called the delete column unrecognized, gave delete rows the
update warning and held them to checks import never runs on them, while
a relationship naming a deleted row was queued and failed after its
retries.