feat(attachments): add "match by mapping file" support#8311
Open
grantfitzsimmons wants to merge 35 commits into
Open
feat(attachments): add "match by mapping file" support#8311grantfitzsimmons wants to merge 35 commits into
grantfitzsimmons wants to merge 35 commits into
Conversation
…ileSelection utilities
…hmentFiles, Datasets, and useEagerDataset
… in mapping-file mode
…sReferenceMappingFiles byName map
…in MappingFileSetup
…erenceMappingFiles
…ad of storing 'filename'
This comment was marked as outdated.
This comment was marked as outdated.
|
Warning One or more dependencies are approaching or past End-of-Life. |
![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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
…ancelled type and fileMissing reason Replace three duplicated reason-only fileMissing checks with the shared isMappingFilePlaceholder helper, which requires both status.type === 'cancelled' AND status.reason === 'fileMissing'. This prevents skipped/fileMissing rows from being incorrectly treated as mapping placeholders.
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name='spattachmentdataset', |
Member
Author
There was a problem hiding this comment.
Waiting a bit on the way we finalize creating schema captions for fields. I imagine we may want to have a schema config entry for this field? No real substantive benefit for it, but the sync fields command would create one, so maybe better to add here.
Triggered by dcd25e6 on branch refs/heads/issue-8302
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/Import.tsx (1)
293-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEliminate duplicate logic and fix missing dependencies.
The
recordcolumn definition is identical in both branches ofisMapping. We can extract it to make the logic DRY. Additionally,isMappingModeandcurrentBaseTableshould be included in theuseMemodependency array to ensure the headers update reliably if those values change.Here is a cleaner approach that condenses the conditionally included
matchValuecolumn and resolves the dependencies:♻️ Proposed refactor
- const mainHeaders = React.useMemo(() => { - const isMapping = isMappingMode; - const baseHeaders: IR<JSX.Element | LocalizedString> = isMapping - ? { - selectedFileName: commonText.selectedFileName(), - matchValue: attachmentsText.matchValue(), - fileSize: attachmentsText.fileSize(), - record: ( - <div className="flex min-w-fit items-center gap-2"> - {currentBaseTable === undefined ? ( - userText.resource() - ) : ( - <> - <TableIcon label name={currentBaseTable} /> - {eagerDataSet.uploadplan.staticPathKey === undefined - ? '' - : strictGetTable(currentBaseTable).strictGetField( - staticAttachmentImportPaths[ - eagerDataSet.uploadplan.staticPathKey - ].path - ).label} - </> - )} - </div> - ), - progress: attachmentsText.progress(), - } - : { - selectedFileName: commonText.selectedFileName(), - fileSize: attachmentsText.fileSize(), - record: ( - <div className="flex min-w-fit items-center gap-2"> - {currentBaseTable === undefined ? ( - userText.resource() - ) : ( - <> - <TableIcon label name={currentBaseTable} /> - {eagerDataSet.uploadplan.staticPathKey === undefined - ? '' - : strictGetTable(currentBaseTable).strictGetField( - staticAttachmentImportPaths[ - eagerDataSet.uploadplan.staticPathKey - ].path - ).label} - </> - )} - </div> - ), - progress: attachmentsText.progress(), - }; - let headers = baseHeaders; - if (process.env.NODE_ENV === 'development') - headers = { ...headers, attachmentId: attachmentsText.attachmentId() }; - return headers; - }, [eagerDataSet.uploadplan.staticPathKey, eagerDataSet.uploadplan.matchingMode]); + const mainHeaders = React.useMemo(() => { + const recordHeader = ( + <div className="flex min-w-fit items-center gap-2"> + {currentBaseTable === undefined ? ( + userText.resource() + ) : ( + <> + <TableIcon label name={currentBaseTable} /> + {eagerDataSet.uploadplan.staticPathKey === undefined + ? '' + : strictGetTable(currentBaseTable).strictGetField( + staticAttachmentImportPaths[ + eagerDataSet.uploadplan.staticPathKey + ].path + ).label} + </> + )} + </div> + ); + + let headers: IR<JSX.Element | LocalizedString> = { + selectedFileName: commonText.selectedFileName(), + ...(isMappingMode ? { matchValue: attachmentsText.matchValue() } : {}), + fileSize: attachmentsText.fileSize(), + record: recordHeader, + progress: attachmentsText.progress(), + }; + + if (process.env.NODE_ENV === 'development') + headers = { ...headers, attachmentId: attachmentsText.attachmentId() }; + return headers; + }, [eagerDataSet.uploadplan.staticPathKey, currentBaseTable, isMappingMode]);🤖 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/AttachmentsBulkImport/Import.tsx` around lines 293 - 346, Refactor the headers construction around baseHeaders so the shared record column is defined once and reused in both mapping and non-mapping modes, while conditionally adding only matchValue for mapping mode. Update the useMemo dependency array to include isMappingMode and currentBaseTable alongside the existing dependencies so headers recompute when any referenced value changes.
🤖 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.
Nitpick comments:
In `@specifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/Import.tsx`:
- Around line 293-346: Refactor the headers construction around baseHeaders so
the shared record column is defined once and reused in both mapping and
non-mapping modes, while conditionally adding only matchValue for mapping mode.
Update the useMemo dependency array to include isMappingMode and
currentBaseTable alongside the existing dependencies so headers recompute when
any referenced value changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e692a47f-54e5-47d4-9f13-f6871f3782eb
📒 Files selected for processing (5)
specifyweb/backend/attachment_gw/models.pyspecifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/Import.tsxspecifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/MappingFileSetup.tsxspecifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/ViewAttachmentFiles.tsxspecifyweb/frontend/js_src/lib/localization/attachments.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- specifyweb/frontend/js_src/lib/localization/attachments.ts
- specifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/ViewAttachmentFiles.tsx
- specifyweb/frontend/js_src/lib/components/AttachmentsBulkImport/MappingFileSetup.tsx
| </option> | ||
| {headers.map((header, index) => ( | ||
| <option key={index} value={index}> | ||
| {header || `Column ${index + 1}`} |
Contributor
There was a problem hiding this comment.
Make sure to localize the default header before merging 👀
| </option> | ||
| {headers.map((header, index) => ( | ||
| <option key={index} value={index}> | ||
| {header || `Column ${index + 1}`} |
Contributor
There was a problem hiding this comment.
Same thing as the above comment, make sure the default header is localized before merging
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
3 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.
Fixes #8302
This PR adds a "Match by mapping file" mode to the Bulk Attachment Uploader. Users can upload a CSV mapping file that explicitly pairs filenames with record identifiers (catalog numbers, etc.), enabling multi-attachment-per-record uploads without requiring filename structure conventions.
This lets you attach multiple files to the same record, use files with names that don't follow a pattern, and see exactly what's expected before you start uploading.
When you first open the uploader, a dialog asks which matching method you want. If you choose mapping file, you upload your CSV and pick which columns contain the filename and the record identifier. The table then shows all your expected rows — rows waiting for a file show "Awaiting file", and any files you add are paired with the correct row automatically.
Warning
This PR affects database migrations. See migration testing instructions.
Migration changes
matchingmodefield toSpattachmentdatasetmodel with choices[('mappingFile', 'Mapping File')](null= filename mode). New migration0002_add_matchingmode.pyinattachment_gwDjango app.Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
Test sets
You can use this set of test-files.zip, tested with the KU Entomology database. Please also test a variety of other cases!
1.
catalog-number/: Collection Object → Catalog NumberTest both the standard mapping and multi-mapping.
Steps:
catalog-number/mapping.csvfileName+catalogNumber.jpgfiles → status should clearmapping-multi.csv2.
taxon/: Taxon → Full NameSteps:
(must have a Taxon table with matching records in your collection)
taxon/mapping.csvFile Name+Full Name(based on schema captions).jpgfiles3.
reference-work/: Reference Work → TitleSteps:
(must have a Reference Work table with matching records)
reference-work/mapping.csvfileName+TitleSpecial Cases
CSV row without a matching file — shows "Awaiting file" forever; Validate warns
Make sure that files not in the CSV are handled (they will not be matched or uploaded)
Make sure that duplicate filenames in CSV are handled, showing only one row after import
Verify that when re-opening a saved dataset the mapping CSV data and column selections are preserved
Test rollback and random interruptions (cancel during actions), make sure behavior is consistent with our current bulk attachment uploader
Make sure the matching mode dialog hows on first visit (and subsequent visits if the data set is empty), but stays hidden after choosing (even on refresh/re-open)
Cases that are not handled
8.jpgand list it multiple times in the mapping for different linked objects.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements