Skip to content

ILLDEV-515 LMS vendor profiles - #754

Merged
adamdickmeiss merged 26 commits into
mainfrom
ILLDEV-515-lms-vendor-profiles
Sep 16, 2026
Merged

adamdickmeiss merged 26 commits into
mainfrom
ILLDEV-515-lms-vendor-profiles

Conversation

@adamdickmeiss

@adamdickmeiss adamdickmeiss commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Copilot AI balanced review requested due to automatic review settings September 11, 2026 10:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Profile-only responses retain empty endpoints, and FOLIO temporary locations are dropped before supplier selection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds configurable LMS/vendor and catalog profiles, resolving defaults in the broker without materializing them in Directory records.

Changes:

  • Adds profile persistence, migration, API schema, and documentation.
  • Implements profile resolution, catalog parsers, NCIP namespace handling, and Sierra ID normalization.
  • Adds integration and unit coverage for profile behavior.
File summaries
File Description
directory/test/host_profiles_test.go Tests profile persistence and patching.
directory/test/apifixtures/entry-with-lmsconfig.post.refetch.json Updates omitted-default expectations.
directory/test/apifixtures/entry-new-lmsconfig-incomplete.patch.refetch.json Removes empty endpoint expectations.
directory/RELEASE-NOTES.md Documents the release behavior.
directory/README.md Links profile documentation.
directory/query.sql Persists profile and override fields.
directory/migrations/008_host_profiles.up.sql Adds and migrates profile columns.
directory/migrations/008_host_profiles.down.sql Removes profile columns.
directory/host-profiles.md Documents configuration and precedence.
directory/api/host_profiles_test.go Tests raw override conversion and merging.
directory/api/entries.go Serializes and persists profile settings.
directory/api/catalog_config.go Merges catalog overrides.
directory/api.yaml Extends the Directory API schema.
broker/profiles/resolve.go Resolves effective host profiles.
broker/profiles/resolve_test.go Tests resolution and validation.
broker/profiles/defaults.go Applies runtime defaults and diagnostics.
broker/ncipclient/ncipclient_test.go Updates client construction.
broker/ncipclient/ncipclient_impl.go Supports namespace-free NCIP.
broker/ncipclient/namespace_test.go Tests namespace-free exchanges.
broker/lms/profiles_test.go Tests LMS profile behavior.
broker/lms/lms_creator_test.go Tests profile-only LMS configuration.
broker/lms/lms_creator_impl.go Resolves profiles before adapter creation.
broker/lms/lms_adapter_test.go Updates validation assertions.
broker/lms/lms_adapter_ncip.go Adds profile defaults and ID normalization.
broker/catalog/profiles_test.go Tests catalog profile behavior.
broker/catalog/holdings_parser_opac.go Adds configurable OPAC mappings.
broker/catalog/holdings_parser_marc.go Adds MARC availability predicates.
broker/catalog/creator_impl.go Resolves profiles before lookup creation.
broker/catalog/catalog.go Adds temporary shelving-location data.
broker/catalog/adapter_metaproxy.go Selects schema from record syntax.
Review details
  • Files reviewed: 30/30 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread broker/catalog/catalog.go Outdated
Comment thread directory/api/entries.go
@adamdickmeiss
adamdickmeiss marked this pull request as draft September 11, 2026 10:54
Comment thread broker/catalog/catalog.go Outdated
@adamdickmeiss
adamdickmeiss marked this pull request as ready for review September 15, 2026 09:45
@adamdickmeiss
adamdickmeiss requested a balanced review from Copilot September 15, 2026 09:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Strict parser validation can invalidate accepted legacy configurations, and Generic catalog behavior has compatibility regressions.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 39/39 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread broker/profiles/resolve.go
Comment thread broker/catalog/holdings_parser_opac.go
Comment thread broker/profiles/resolve.go Outdated
Comment thread broker/profiles/resolve.go
Comment thread broker/profiles/resolve.go
Comment thread directory/api.yaml

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The data-preserving migration needs a direct upgrade regression test before it can be safely approved.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 40/40 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread directory/migrations/009_host_profiles.up.sql

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

It combines database migration, public API, adapter selection, XML transformation, and vendor-specific parsing behavior requiring final human validation.

Review details
  • Files reviewed: 46/46 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Comment thread directory/api/catalog_config.go
Comment thread broker/profiles/resolve.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Legacy configurations can fail after migration, empty Generic holdings are rejected, and complete imports cannot restore the new settings.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

broker/profiles/resolve.go:211

  • Migration 009 deliberately preserves every enabled legacy parser key (and its migration test covers mixed selections), but this validation rejects those migrated records. Existing entries that previously relied on the established marcopacreservoirmarc21plus1 precedence will therefore fail catalog adapter creation after upgrade. Normalize legacy mixed data to that precedence during migration or resolution while retaining rejection for newly submitted ambiguous configs.
			if n != 1 {
				return bad("holdingsFormat must set marc, opac, reservoir, or marc21plus1 (exactly one parser)")
  • Files reviewed: 47/47 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread broker/profiles/resolve.go
Comment thread broker/profiles/resolve.go Outdated
Comment thread directory/api.yaml
Comment thread directory/RELEASE-NOTES.md Outdated
Comment thread directory/host-profiles.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Migration compatibility and incomplete cross-field validation can leave catalog configurations unusable.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 54/54 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread directory/migrations/009_host_profiles.up.sql
Comment thread directory/api/catalog_config.go
Comment thread directory/import/model/config.go
@adamdickmeiss
adamdickmeiss merged commit c361b2a into main Sep 16, 2026
9 checks passed
@adamdickmeiss
adamdickmeiss deleted the ILLDEV-515-lms-vendor-profiles branch September 16, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants