CROSSLINK-191 NCIP supplier side #365

adamdickmeiss · 2026-01-08T13:36:23Z

https://index-data.atlassian.net/browse/CROSSLINK-191

Copilot

Pull request overview

This pull request implements NCIP support for supplier-side operations in the Crosslink ILL brokering service. The changes enable supplier libraries to validate their institutional patron accounts and perform LMS operations via NCIP when acting as lenders.

Key changes:

Refactored LmsCreator interface to accept string symbols instead of pgtype.Text for cleaner API design
Added InstitutionalPatron() method to LmsAdapter interface to support supplier-side validation
Consolidated ILL request parsing and LMS adapter creation to reduce code duplication in action handlers

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
broker/patron_request/service/action.go	Refactored action handling to parse ILL requests and create LMS adapters early; added supplier-side NCIP calls for validate, will-supply, ship, and mark-received actions
broker/patron_request/service/action_test.go	Updated tests for new method signatures; added edge case tests for missing LMS creator, invalid ILL requests, and missing symbols; removed duplicate/redundant tests
broker/lms/lms_creator.go	Changed GetAdapter interface signature from pgtype.Text to string
broker/lms/lms_creator_impl.go	Simplified implementation by removing symbol validation (now handled by callers)
broker/lms/lms_creator_test.go	Updated tests to use string symbols instead of pgtype.Text
broker/lms/lms_adapter.go	Added InstitutionalPatron() method to interface
broker/lms/lms_adapter_ncip.go	Implemented InstitutionalPatron() with configurable institutional_patron field
broker/lms/lms_adapter_manual.go	Added stub InstitutionalPatron() implementation returning empty string
go.work.sum	Transitive dependency updates from Go workspace

broker/patron_request/service/action.go

broker/patron_request/service/action_test.go

Copilot · 2026-01-08T13:40:00Z

broker/patron_request/service/action.go

+	var illRequest iso18626.Request
+	err = json.Unmarshal(pr.IllRequest, &illRequest)
+	if err != nil {
+		return events.LogErrorAndReturnResult(ctx, "failed to parse ILL request", err)
+	}


The ILL request parsing at lines 110-114 happens unconditionally for all actions, but not all actions require the ILL request data. Actions like BorrowerActionCancelRequest, BorrowerActionAcceptCondition, BorrowerActionRejectCondition, LenderActionCannotSupply, LenderActionAddCondition, and LenderActionMarkCancelled don't use the illRequest parameter. This means these actions will fail if pr.IllRequest contains invalid JSON, even though they don't need it. Consider moving the ILL request parsing to only happen when needed, or ensure pr.IllRequest is always valid JSON even for actions that don't require it.

Premature optimization. The illRequest bytes are stored in the DB anyway, so this makes no difference for the user.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.

Copilot · 2026-01-08T14:07:22Z

broker/patron_request/service/action.go

+		return a.validateBorrowingRequest(ctx, pr, lmsAdapter)
 	case BorrowerActionSendRequest:
-		return a.sendBorrowingRequest(ctx, pr)
+		return a.sendBorrowingRequest(ctx, pr, illRequest)


The sendBorrowingRequest action is not receiving the lmsAdapter parameter even though it's retrieved in handleBorrowingAction. While this action may not currently need it, the inconsistency in parameter passing could lead to confusion. Consider either passing the adapter to all actions or restructuring to only retrieve it when needed.

Copilot · 2026-01-08T14:07:23Z

broker/patron_request/service/action.go

+	// TODO set these values properly
+	itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId
+	requestId := ""
+	borrowerBarcode := ""
+	externalReferenceValue := ""


Using empty strings as placeholders for required NCIP parameters (requestId, borrowerBarcode, externalReferenceValue) may cause the LMS CheckOutItem operation to fail or behave unexpectedly. These TODO comments indicate incomplete implementation that should be addressed before merging.

Suggested change

// TODO set these values properly

itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId

requestId := ""

borrowerBarcode := ""

externalReferenceValue := ""

itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId

// Use non-empty values for required LMS parameters; fall back to itemId as a stable surrogate.

requestId := itemId

borrowerBarcode := itemId

externalReferenceValue := itemId

Copilot · 2026-01-08T14:07:23Z

broker/patron_request/service/action.go

+	// TODO set these values properly
+	itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId


Using an empty itemId placeholder may cause the LMS CheckInItem operation to fail. This TODO comment indicates incomplete implementation that should be addressed before merging.

Suggested change

// TODO set these values properly

itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId

itemId := strings.TrimSpace(illRequest.BibliographicInfo.SupplierUniqueRecordId)

if itemId == "" {

return events.LogErrorAndReturnResult(ctx, "LMS CheckInItem failed", errors.New("missing supplier unique record id for item check-in"))

}

Copilot · 2026-01-08T14:07:23Z

broker/patron_request/service/action_test.go

+func TestHandleInvokeActionNoLms(t *testing.T) {
+	mockPrRepo := new(MockPrRepo)
+	prAction := CreatePatronRequestActionService(mockPrRepo, *new(events.EventBus), new(handler.Iso18626Handler), nil)
+	mockPrRepo.On("GetPatronRequestById", patronRequestId).Return(pr_db.PatronRequest{RequesterSymbol: pgtype.Text{Valid: true, String: "ISIL:x"}, State: BorrowerStateNew, Side: SideBorrowing, Tenant: pgtype.Text{Valid: true, String: "testlib"}}, nil)
+
+	status, resultData := prAction.handleInvokeAction(appCtx, events.Event{PatronRequestID: patronRequestId, EventData: events.EventData{CommonEventData: events.CommonEventData{Action: &actionValidate}}})
+
+	assert.Equal(t, events.EventStatusError, status)
+	assert.Equal(t, "LMS creator not configured", resultData.EventError.Message)
+}


This test expects the error "LMS creator not configured" but doesn't provide an IllRequest field in the PatronRequest. According to the new code logic, it would fail earlier at line 107-114 with "failed to parse ILL request" before reaching the LMS creator check. The test should include a valid IllRequest field.

Copilot · 2026-01-08T14:07:23Z

broker/patron_request/service/action.go

 	if !a.actionMappingService.GetActionMapping(pr).IsActionAvailable(pr, action) {
 		return events.LogErrorAndReturnResult(ctx, "state "+string(pr.State)+" does not support action "+string(action), errors.New("invalid action"))
 	}
+	if a.lmsCreator == nil {


The LMS creator validation happens for all actions and sides, but some actions like cancelBorrowingRequest, acceptConditionBorrowingRequest, and rejectConditionBorrowingRequest don't use the LMS adapter. This check should be moved to only apply when LMS operations are actually needed, or those actions should be handled before this validation.

Suggested change

if a.lmsCreator == nil {

requiresLMS := true

switch action {

case "cancelBorrowingRequest", "acceptConditionBorrowingRequest", "rejectConditionBorrowingRequest":

requiresLMS = false

}

if requiresLMS && a.lmsCreator == nil {

Copilot · 2026-01-08T14:07:24Z

broker/patron_request/service/action.go

+	lmsAdapter, err := a.lmsCreator.GetAdapter(ctx, pr.RequesterSymbol.String)
+	if err != nil {
+		return events.LogErrorAndReturnResult(ctx, "failed to create LMS adapter", err)
+	}


The LMS adapter is retrieved for all borrowing actions, but BorrowerActionSendRequest doesn't use it. The lmsAdapter parameter should only be passed to actions that actually need it, or the GetAdapter call should be moved to the individual action handlers that require it.

Copilot · 2026-01-08T14:07:24Z

broker/patron_request/service/action.go

+func (a *PatronRequestActionService) willSupplyLenderRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, lmsAdapter lms.LmsAdapter, illRequest iso18626.Request) (events.EventStatus, *events.EventResult) {
+	// TODO set these values properly
+	itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId
+	requestId := ""
+	borrowerBarCode := ""
+	pickupLocation := ""
+	itemLocation := ""
+	err := lmsAdapter.RequestItem(requestId, itemId, borrowerBarCode, pickupLocation, itemLocation)
+	if err != nil {


Using empty strings as placeholders for required NCIP parameters (requestId, borrowerBarCode, pickupLocation, itemLocation) may cause the LMS RequestItem operation to fail or behave unexpectedly. These TODO comments indicate incomplete implementation that should be addressed before merging.

Suggested change

func (a *PatronRequestActionService) willSupplyLenderRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, lmsAdapter lms.LmsAdapter, illRequest iso18626.Request) (events.EventStatus, *events.EventResult) {

// TODO set these values properly

itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId

requestId := ""

borrowerBarCode := ""

pickupLocation := ""

itemLocation := ""

err := lmsAdapter.RequestItem(requestId, itemId, borrowerBarCode, pickupLocation, itemLocation)

if err != nil {

type lmsRequestItemParams struct {

RequestID string

ItemID string

BorrowerBarcode string

PickupLocation string

ItemLocation string

}

// buildLmsRequestItemParams constructs the parameters required for the LMS

// RequestItem operation. It returns an error if the parameters cannot be

// safely derived yet.

func buildLmsRequestItemParams(pr pr_db.PatronRequest, illRequest iso18626.Request) (lmsRequestItemParams, error) {

params := lmsRequestItemParams{

ItemID: illRequest.BibliographicInfo.SupplierUniqueRecordId,

}

if params.ItemID == "" {

return params, errors.New("missing item ID for LMS RequestItem")

}

// TODO: Derive RequestID, BorrowerBarcode, PickupLocation and ItemLocation

// from the patron request and/or ISO18626 request once the data mapping

// is defined. For now, signal that this is not fully implemented instead

// of calling the LMS with placeholder values.

return params, errors.New("building LMS RequestItem parameters not fully implemented")

}

func (a *PatronRequestActionService) willSupplyLenderRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, lmsAdapter lms.LmsAdapter, illRequest iso18626.Request) (events.EventStatus, *events.EventResult) {

params, err := buildLmsRequestItemParams(pr, illRequest)

if err != nil {

return events.LogErrorAndReturnResult(ctx, "LMS RequestItem parameters invalid", err)

}

err = lmsAdapter.RequestItem(params.RequestID, params.ItemID, params.BorrowerBarcode, params.PickupLocation, params.ItemLocation)

if err != nil {

Copilot · 2026-01-08T14:07:24Z

broker/patron_request/service/action.go

+	institutionalPatron := lms.InstitutionalPatron()
+	_, err := lms.LookupUser(institutionalPatron)
+	if err != nil {
+		return events.LogErrorAndReturnResult(ctx, "LMS LookupUser failed", err)
+	}


If InstitutionalPatron returns an empty string (as it does in LmsAdapterManual), this will call LookupUser with an empty string which may not be the intended behavior. Consider checking if institutionalPatron is empty and handling that case explicitly, or documenting that an empty string is valid for manual adapters.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

adamdickmeiss added 8 commits January 5, 2026 17:24

CROSSLINK-191 WIP1

d93a738

institutional_patron configuation

b91b1ae

Common LMS check for supplier

85db114

Common LMS check for requester

de467c4

LMS GetAdapter takes string again

2d24dc0

one place for lms creator check; redundant tests going away

6242d8a

3 more NCIP calls in place (with dummy parms)

efae03b

Unmarshal once

d6460f4

adamdickmeiss requested a review from jakub-id as a code owner January 8, 2026 13:36

Copilot AI review requested due to automatic review settings January 8, 2026 13:36

adamdickmeiss requested a review from JanisSaldabols as a code owner January 8, 2026 13:36

Copilot started reviewing on behalf of adamdickmeiss January 8, 2026 13:36 View session

adamdickmeiss marked this pull request as draft January 8, 2026 13:36

Copilot AI reviewed Jan 8, 2026

View reviewed changes

Test fixes, TestHandleInvokeActionCancelRequest back

618bde6

adamdickmeiss requested a review from Copilot January 8, 2026 14:00

Copilot started reviewing on behalf of adamdickmeiss January 8, 2026 14:01 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

Pass illRequest for TestHandleInvokeActionNoLms

4192929

		// TODO set these values properly
		itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId

-	// TODO set these values properly
-	itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId
+	itemId := strings.TrimSpace(illRequest.BibliographicInfo.SupplierUniqueRecordId)
+	if itemId == "" {
+		return events.LogErrorAndReturnResult(ctx, "LMS CheckInItem failed", errors.New("missing supplier unique record id for item check-in"))
+	}

-	if a.lmsCreator == nil {
+	requiresLMS := true
+	switch action {
+	case "cancelBorrowingRequest", "acceptConditionBorrowingRequest", "rejectConditionBorrowingRequest":
+		requiresLMS = false
+	}
+	if requiresLMS && a.lmsCreator == nil {

-func (a *PatronRequestActionService) willSupplyLenderRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, lmsAdapter lms.LmsAdapter, illRequest iso18626.Request) (events.EventStatus, *events.EventResult) {
-	// TODO set these values properly
-	itemId := illRequest.BibliographicInfo.SupplierUniqueRecordId
-	requestId := ""
-	borrowerBarCode := ""
-	pickupLocation := ""
-	itemLocation := ""
-	err := lmsAdapter.RequestItem(requestId, itemId, borrowerBarCode, pickupLocation, itemLocation)
-	if err != nil {
+type lmsRequestItemParams struct {
+	RequestID       string
+	ItemID          string
+	BorrowerBarcode string
+	PickupLocation  string
+	ItemLocation    string
+}
+// buildLmsRequestItemParams constructs the parameters required for the LMS
+// RequestItem operation. It returns an error if the parameters cannot be
+// safely derived yet.
+func buildLmsRequestItemParams(pr pr_db.PatronRequest, illRequest iso18626.Request) (lmsRequestItemParams, error) {
+	params := lmsRequestItemParams{
+		ItemID: illRequest.BibliographicInfo.SupplierUniqueRecordId,
+	}
+	if params.ItemID == "" {
+		return params, errors.New("missing item ID for LMS RequestItem")
+	}
+	// TODO: Derive RequestID, BorrowerBarcode, PickupLocation and ItemLocation
+	// from the patron request and/or ISO18626 request once the data mapping
+	// is defined. For now, signal that this is not fully implemented instead
+	// of calling the LMS with placeholder values.
+	return params, errors.New("building LMS RequestItem parameters not fully implemented")
+}
+func (a *PatronRequestActionService) willSupplyLenderRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, lmsAdapter lms.LmsAdapter, illRequest iso18626.Request) (events.EventStatus, *events.EventResult) {
+	params, err := buildLmsRequestItemParams(pr, illRequest)
+	if err != nil {
+		return events.LogErrorAndReturnResult(ctx, "LMS RequestItem parameters invalid", err)
+	}
+	err = lmsAdapter.RequestItem(params.RequestID, params.ItemID, params.BorrowerBarcode, params.PickupLocation, params.ItemLocation)
+	if err != nil {

CROSSLINK-191 NCIP supplier side #365

Are you sure you want to change the base?

CROSSLINK-191 NCIP supplier side #365

Uh oh!

Conversation

adamdickmeiss commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

adamdickmeiss Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

adamdickmeiss commented Jan 8, 2026 •

edited

Loading