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.
- Restore AjaxTree support so ArchivesSpace before 4.2 still populates the grid (InfiniteTree, then AjaxTree, then pathname fallback) - Register the AutoSearch page handler before sign-in so it fires when no default repository is configured (critical handlers are snapshotted per page load; mid-check registrations are skipped) - Skip SystemID, Site, and ReturnLink fields on import so record data cannot overwrite Aeon system/routing fields - Extract the duplicated grid-build logic into PopulateGridFromPluginData and cache the parsed GridDisplayFields - Comment out unwired PopulateAllInstances pending a product decision; remove dead getResourceUri, UpdateCurrentUri, GetWebExceptionMessage - Correct the ImportField over-length comment (values fail to set, not raise) and document the load-bearing capture-phase listener - Note the Data Handler plugin requirement in Config.xml and Readme
Consumer keys: the Data Handler plugin now keeps an independent mapping set per consumer and rejects data requests that do not name one. Grid and instance calls pass aspace_client_addon_instance_import. Citation import passes aspace_client_addon_citation_import. This requires the plugin's per-consumer release, so the two ship together. Import Instance button: ImportCitation_Clicked disables both import buttons as a double-click guard but only re-enabled the citation button, and the grid's focused-row event fires only when the row changes. Re-enable the button when the grid still has a focused row. AutoSearch race: critical page handlers all run in the same page-load check without stopping after a match, so a search registered on IsSignedIn fired in the same check in which SetDefaultRepository starts the repository switch and the two navigations raced. The handler now matches on ReadyForAutoSearch, which stays unmatched (and therefore registered) until the switch completes or was never started. Blank and invalid defaults still search immediately.
asmith-atlas
left a comment
asmith-atlas
left a comment
There was a problem hiding this comment.
Review of the consumer-key + client-fix commits (c4585268, 30b191f0), cross-checked against the Data Handler plugin's per-consumer PR (ArchivesSpaceDataHandlerPlugin #2).
Verdict: approve with suggestions. The consumer integration is exactly right against the plugin contract, and the two client-side fixes are thoughtful work with honest comments. The findings are one silent-compatibility gap worth closing before ship, one auto-search dead-end edge, and a handful of small items.
Consumer integration — correct against the plugin contract
Verified against DataHandler PR #2's implementation directly: the constants match the seeded protected keys character-for-character; all five instance-flow call sites (AO grid, AO→resource fallback, accession grid, plus the two in the commented-out bulk loader) send aspace_client_addon_instance_import, and the citation import sends aspace_client_addon_citation_import — precisely the per-import-type split the plugin's plan prescribes. Citation calls correctly omit include_instances; grid calls request instances + digital objects. The commit message's "the two ship together" coordination matches the plugin side's expectations.
Findings
1. MEDIUM — Silent degradation against a pre-consumer plugin (v1.0.x): the partition split quietly doesn't happen. ArchivesSpace's endpoint DSL ignores undeclared query params, so addon 4.0.0 pointed at a released v1.0.x Data Handler doesn't fail — it works, with consumer ignored and both import types served from the single unpartitioned rule set. That silently resurrects the exact ItemTitle-style collision the two-consumer design exists to fix, with nothing in any log to say so. Config.xml/Readme now state the plugin is required but not which version, so a site that installs addon 4.0.0 against plugin 1.0.3 gets subtly wrong mappings instead of a clear error. Cheapest fix: state the minimum plugin version in the Config description and Readme installation section; sturdier fix: detect it (e.g. the plugin's consumer-aware release could echo the consumer key in the response envelope, and the addon warns when it's absent — needs a one-line plugin-side addition, worth deciding together).
2. LOW-MEDIUM — Auto-search can dead-end for the whole session if the repository switch never verifiably completes. Once defaultRepositorySelected is set, ReadyForAutoSearch returns true only when CurrentRepositoryMatchesDefault() — if the switch click errors ArchivesSpace-side, or the staff member manually lands somewhere the repo-link selector doesn't resolve, the handler stays unmatched forever and auto-search silently never fires. The race-avoidance design is good (and the comments explaining the snapshot/capture-phase constraints are genuinely excellent); it just lacks a fallback. A page-load counter that gives up waiting after a few loads and proceeds with the search would cap the failure at "searched in the wrong repo" instead of "never searched."
3. LOW — GetPluginData concatenates consumer unguarded. A future call site that forgets the new argument gets a Lua "attempt to concatenate a nil value" runtime error rather than a useful message. A one-line nil guard (log + return nil, or an explicit error) makes the contract self-enforcing.
4. LOW — The truncation comment now contradicts the Data Handler README. This PR says over-length SetFieldValue "silently fails to set the field (caught and logged, not raised)"; the plugin README's max_length rationale says it raises. One of the two repos is documenting the wrong behavior — worth reconciling (if silent-fail is the tested truth, the plugin README should be corrected, since its seeded max-lengths are justified by the raise claim).
5. NIT — PopulateAllInstances kept as a --[==[ ]==] block. The bracket-level is valid Lua (no ]==] inside the body), and the "pending a decision with Katie / would hang on large finding aids" note is honest — but the repo's own code standards say commented-out code goes to version control instead. Given the sync-per-AO hang warning argues it can't ship as-is anyway, deleting it (git keeps it; the commit message can name the recovery point) would follow the house rule.
6. NIT — Readme names the consumers only by display label. The labels are staff-editable; the keys are what the addon actually sends and what an admin will see in the plugin's 400 errors while troubleshooting. Worth listing both.
Verified clean / positives
- The AjaxTree restore is correctly guarded (
window.AjaxTree && typeof tree !== 'undefined' && tree.large_tree) so it can't reintroduce the 4.2 throw, and callingPopulateDataGriddirectly from the hook is safe since grid population reads the API, not the record pane DOM. Pre-4.2 support is genuinely back. - The capture-phase comment on
infiniteTree:nodeSelectdocuments a real, non-obvious constraint — future maintainers will thank you. SYSTEM_FIELDS_NOT_IMPORTEDis the right call with the right rationale (Site drives routing), and matches the plugin's system-field merge exactly.- The auto-search handler re-ordering (register before sign-in, match on
ReadyForAutoSearch) correctly solves both the never-fires-without-default-repo bug and the navigation race, per the snapshot semantics documented. - Dead-code removals (
UpdateCurrentUri,GetWebExceptionMessage) verified unreferenced; thePopulateGridFromPluginData/StringifyFieldsdedup is a faithful extraction; theGridDisplayFieldscache handles the nil/blank cases correctly; the instance-button re-enable after citation import fixes a real focused-row-event gap.
Cross-repo note: findings #1 and #4 both need small decisions on the plugin side, so they may be better handled as follow-ups on ArchivesSpaceDataHandlerPlugin #2 than as blockers here.
🤖 Generated with Claude Code
3 participants
No description provided.