Feature/35989/miscellaneous updates - #11
Conversation
…d configurable field mapping Introduces bulk instance loading across entire resource trees, accession record handling in PopulateDataGrid, and a data-driven grid/import pipeline that reads columns from DataMapping.lua instead of hardcoded values. Adds ResolveASpaceMapping for composite and dot-notation field resolution, CustomFields import routing, SetDefaultRepository for auto-selecting a repo after login, and separates URI tracking from grid resets to prevent data loss on tree navigation.
Replace client-side field extraction with calls to the companion
ArchivesSpace Data Handler plugin, which returns records already mapped
to Aeon field names:
- GetPluginEndpointUrl/GetPluginData translate a record URI into the
plugin's /aeon/{type}/ endpoint for archival objects, resources,
accessions, and digital objects.
- PopulateDataGrid and PopulateAllInstances build grid rows from the
plugin response's fields and instances arrays instead of walking raw
ArchivesSpace records, keeping the fallback to the parent resource's
instances when an archival object has none.
- Grid columns come from DataMapping.lua's GridColumns table, and the
underlying DataTable columns are created dynamically from whatever
fields the plugin returns.
- Instance import loops over every column in the selected row and
imports each valid Aeon transaction field; citation import reads the
plugin's fields filtered by the CitationFields list.
- DataMapping.lua drops the client-side extraction tables
(InstanceDataImport/CitationDataImport) in favor of GridColumns,
CitationFields, and AutoGroupField.
This is a migration baseline and work in progress: it depends on plugin
endpoints that expose instance data (opt-in via include_instances as of
the current plugin build, which this code does not yet request), and
the legacy extraction functions are unused but not yet removed.
Finish the addon's side of the Data Handler plugin migration on top of the previous baseline commit: - Request instance data explicitly: grid-population calls append include_instances=true&include_digital_objects=true (instance data is opt-in on the plugin's endpoints); citation-import calls fetch only the record-level fields. - Import into Aeon custom fields: IsValidAeonField now validates CustomFields.-prefixed names against the Transaction.CustomFields data source, so plugin mappings targeting custom fields import. - Citation import imports every field the plugin returns; the CitationFields whitelist is removed. Which fields import is entirely the plugin's mapping configuration. - Grid columns are built dynamically from the returned fields. The new GridDisplayFields setting picks which fields get columns and their order (unlisted fields still import); AutoGroupResults grouping is driven by the new AutoGroupField setting. Grid rows no longer carry stale values between instances, and bulk resource loads overlay each archival object's full field set over the resource-level fields. - Imported values are no longer truncated by the addon; the ImportDataSeparator setting is removed. - Remove the now-dead client-side extraction code (citation extractors, creator/top-container/digital-object API fetchers, location mapping, ResolveASpaceMapping) and the unused Utility require. - Bump version to 4.0.0 and rewrite the Readme for the plugin-based mapping model.
The released addon's static grid included a SubTitle column; the migration's dynamic GridDisplayFields default dropped it. Add ItemSubtitle back (after CallNumber, matching master's order) so archival objects and resources show their subtitle by default.
The old AjaxTree path threw under InfiniteTree, so resource/AO grids never populated. Read the selected node's data-uri and repopulate on infiniteTree:nodeSelect instead.
- Add handling for a blank or invalid DefaultRepositoryId so the addon leaves the ArchivesSpace default in place instead of showing an error - Remove unused Utility.lua - Stop logging skipped grid columns when the results grid is just being cleared
mcalsada
left a comment
mcalsada
left a comment
There was a problem hiding this comment.
Reviewed the migration end-to-end, including the companion ArchivesSpaceDataHandlerPlugin and the ArchivesSpace v4.2.0 InfiniteTree source.
The architecture change is the right call — moving field mapping out of DataMapping.lua and into the plugin is a real improvement, and the InfiniteTree integration checks out against the upstream source (container id, .node.current, data-uri, node-id format all match, and .current is moved to the new node before infiniteTree:nodeSelect dispatches, so there's no stale-node race). The DefaultRepositoryId value is also correctly validated against ^%d+$ before being interpolated into the injected script — that's the one spot this PR could have introduced script injection, and it's guarded.
Release coordination: this addon requires the plugin's feature/35989/ClientAddonAccommodations branch. include_instances doesn't exist anywhere on plugin main, where all four endpoints call result.delete("instances") unconditionally — so against main the instance grid never populates. The two need to release together.
Two things I'd like verified before merge:
- Does archival-object selection still work on ArchivesSpace < 4.2? The AjaxTree branch was removed with no fallback. (
Aeon-ArchivesSpace.lua:98) - Does
AutoSearchstill fire withDefaultRepositoryIdblank — the default? (Aeon-ArchivesSpace.lua:226)
One confirmed behavior change to make deliberate: citation import now writes SystemID and Site to the transaction, which the old allowlist never did. (Aeon-ArchivesSpace.lua:792)
One correction worth recording: the comment on ImportField — and the plugin's own comment at aeon_mapping_engine.rb:96 — both state that Aeon's SetFieldValue raises on over-length values. It doesn't. ScriptDataRowWrapper.SetValue catches the ArgumentException and logs it, so the field is silently never set. Details inline at Aeon-ArchivesSpace.lua:876.
The rest is DRY cleanup and dead code.
| // full record URI in its data-uri. (The legacy AjaxTree class is still | ||
| // loaded on these pages, but its `tree` global is gone, so the old | ||
| // `tree.large_tree.current_tree_id` path throws — hence this replaces it.) | ||
| if (typeof InfiniteTree === 'function' && document.getElementById('infinite-tree-container')) { |
There was a problem hiding this comment.
Removing the AjaxTree branch drops pre-4.2 support.
The window.AjaxTree branch was removed rather than kept alongside this one. On ArchivesSpace < 4.2, typeof InfiniteTree === 'function' is false, so we fall through to the else branch and use window.location.pathname — but pathname doesn't change when AjaxTree swaps the record pane, so selecting an archival object stops updating currentRecordUri.
Could you confirm against a pre-4.2 instance? If AO selection is indeed broken there, we either restore the AjaxTree branch or make 4.2 the documented floor.
For what it's worth, I verified the InfiniteTree side against the ArchivesSpace v4.2.0 source and it's correct: container id, .node.current selector, data-uri, and node-id format all match, and setCurrentNode moves .current to the new node before dispatching infiniteTree:nodeSelect, so there's no stale-node race here.
| <Version>4.0.0</Version> | ||
| <Active>True</Active> | ||
| <Type>Addon</Type> | ||
| <Description>This addon performs searches in ArchivesSpace using the staff interface. This addon supports ArchivesSpace versions v2.8.0 and later.</Description> |
There was a problem hiding this comment.
Still reads "supports ArchivesSpace versions v2.8.0 and later", but this release requires the ArchivesSpace Data Handler plugin, and (pending the AjaxTree question on Aeon-ArchivesSpace.lua:98) likely ArchivesSpace 4.2+ as well. This description and the Readme's 4.0.0 notes should state both prerequisites.
| else | ||
| LogDebug("AutoSearch is disabled. Skipping page page handler registration to perform autosearch functionality.") | ||
| end | ||
| catalogSearchForm.Browser:RegisterPageHandler("custom", "IsSignedIn", "SetDefaultRepository", true); |
There was a problem hiding this comment.
AutoSearch may never fire under the default configuration. Worth verifying before merge.
AutoSearchAfterLogin registration moved out of InitializeLoginPageHandler and into SetDefaultRepository, which is itself an IsSignedIn handler. PerformLogin re-registering itself on entry suggests the 4th arg means "remove after firing" — so AutoSearchAfterLogin only gets registered during sign-in dispatch, and can realistically only be evaluated on a subsequent page load.
When DefaultRepositoryId is set, the "Select Repository" click supplies that page load. But it defaults to blank, in which case SetDefaultRepository returns early, nothing navigates, and there may be no further IsSignedIn evaluation.
Test case: AutoSearch=true with DefaultRepositoryId blank.
Separately — SetDefaultRepository doing two unrelated jobs is worth splitting regardless of the outcome. The name doesn't suggest it owns autosearch registration.
|
|
||
| function ExtractArchivalObjectInstanceBarcode(topContainer, digitalObject) | ||
| local barcode = ""; | ||
| function ImportCitation_Clicked() |
There was a problem hiding this comment.
Citation import now writes SystemID and Site to the transaction. Is that intended?
This imports every field the plugin returns that passes IsValidAeonField, and the plugin's extract_system_information unconditionally merges SystemID, Site, ReturnLinkURL, and ReturnLinkSystemName into fields.
I checked these against RequestDataSet.Designer.cs: ReturnLinkURL and ReturnLinkSystemName aren't Transaction columns, so IsValidAeonField filters them out. But SystemID (nvarchar 100) and Site (nvarchar 20) are columns, and will be written.
Master's CitationDataImport allowlist only ever wrote ItemTitle / ItemAuthor / ItemSubtitle / ItemDate / Location, so overwriting a request's Site and SystemID is new behavior. It may well be desirable for ArchivesSpace-sourced requests — but it should be a deliberate choice rather than a side effect of "import everything the plugin returns."
| function ImportField(target, fieldValue, targetSize) | ||
| -- Values are imported untruncated — the client/database handles values that | ||
| -- exceed a field's column length (see MIGRATION_PLAN.md testing notes). | ||
| function ImportField(target, fieldValue) |
There was a problem hiding this comment.
Over-length values are now silently dropped, not truncated.
The comment above says the client/database handles values exceeding a column length. That isn't what happens — and the plugin's own comment (aeon_mapping_engine.rb:96) claiming SetFieldValue raises is also incorrect. Actual behavior, from ScriptDataRowWrapper.SetValue:
try { _row[fieldName] = newFieldValue; ... }
catch (ArgumentException ex) { log.Error(ex); } // swallowedRequestDataSet.Designer.cs sets MaxLength on 209 columns, so an over-length write throws ArgumentException, gets caught, logged, and the field is never set. Nothing surfaces to Lua or to the user.
In practice the plugin's seeded defaults carry stock column lengths, so this is fine out of the box. But the plugin README documents that a rule with a blank max_length is unlimited — so a customer who adds a custom mapping rule and leaves that blank gets a value that silently vanishes. Previously Truncate() guaranteed something landed; now ImportField can't even detect the failure.
Suggest a client-side backstop truncate, or at minimum logging a warning when the value exceeds the column length.
Two smaller things in this function:
- Line 875 cites
MIGRATION_PLAN.md, which doesn't exist in this repo. - Line 879:
target:sub(14)is a magic number for#"CustomFields.". You already have the capture — usetarget:match("^CustomFields%.(.+)")instead offind+sub.
| } | ||
| } | ||
|
|
||
| function getResourceUri() { |
There was a problem hiding this comment.
Unreferenced — getResourceUri() is never called (its only other mention is the comment at line 145). Safe to remove.
| SetImportButtonsDisabled(); | ||
| end | ||
|
|
||
| function UpdateCurrentUri(currentRepositoryPath, selectedResourcePath) |
There was a problem hiding this comment.
UpdateCurrentUri is unreferenced today — but unlike the other dead functions, I'd hold this one until the AjaxTree question on line 98 is settled. "Update the URI without resetting the grid" is exactly what a restored AjaxTree path would need (per e5e5b6e, "separates URI tracking from grid resets to prevent data loss on tree navigation").
| return res; | ||
| end | ||
|
|
||
| function GetWebExceptionMessage(exception) |
There was a problem hiding this comment.
Unreferenced — GetWebExceptionMessage is only ever called by itself. Safe to remove.
If it's kept, two bugs: exception.InnerException.Response ~= "Response" compares an object against a string literal, and the function uses tabs where the rest of the file uses 4 spaces.
| }; | ||
|
|
||
| // Re-populate the grid for whichever node the staff select. | ||
| document.addEventListener('infiniteTree:nodeSelect', populateFromCurrentNode, true); |
There was a problem hiding this comment.
Worth a comment noting that capture: true here is load-bearing, not stylistic.
In ArchivesSpace v4.2.0, InfiniteTree.setCurrentNode dispatches a non-bubbling CustomEvent on #infinite-tree-record-pane — not on document, and not on #infinite-tree-container. A bubble-phase document listener would never fire. Someone will eventually "clean up" that true and silently break record selection.
| gridColumns["Location"] = gridColumn; | ||
| -- Parses the GridDisplayFields setting into an ordered list of field names. | ||
| -- Returns nil when the setting is blank (meaning: display everything). | ||
| function GetGridDisplayFields() |
There was a problem hiding this comment.
Minor: this re-parses the GridDisplayFields setting on every grid rebuild (every node selection). Parse once at init and cache the list.
2 participants
No description provided.