Skip to content

Conversation

@adamdickmeiss
Copy link
Contributor

@adamdickmeiss adamdickmeiss commented Dec 4, 2025

Copilot AI review requested due to automatic review settings December 4, 2025 11:02
@adamdickmeiss adamdickmeiss marked this pull request as draft December 4, 2025 11:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements NCIP (NISO Circulation Interchange Protocol) integration for patron request validation (CROSSLINK-190). The changes introduce user lookup validation via NCIP during the patron request workflow, along with the infrastructure to support multi-tenant operations via the X-Okapi-Tenant header.

Key changes:

  • New NcipAdapter for performing NCIP user lookups during patron request validation
  • Enhanced validation logic in validateBorrowingRequest to check user credentials via NCIP
  • Added X-Okapi-Tenant header parameter support across all patron request API endpoints

Reviewed changes

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

Show a summary per file
File Description
broker/patron_request/service/ncip_adapter.go New adapter implementing NCIP user lookup functionality by querying directory for NCIP configuration
broker/patron_request/service/action.go Enhanced validation logic to perform NCIP user lookup before transitioning patron requests to validated state
broker/patron_request/service/action_test.go Updated all test cases to include NcipAdapter dependency and added validation error test cases
broker/patron_request/oapi/open-api.yaml Added optional X-Okapi-Tenant header parameter to all patron request API endpoints
broker/patron_request/api/api-handler.go Updated handler signatures to accept tenant parameters from OpenAPI spec
broker/patron_request/api/api-handler_test.go Updated test calls to include new parameter objects
broker/ncipclient/ncipclient_test.go Refactored test setup to avoid cyclic dependencies by inlining mock app startup
broker/app/app.go Integrated NcipAdapter into application initialization
broker/adapter/api_directory.go Added TODO comment regarding NCIP entries without ISO18626 endpoints

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (1)

broker/patron_request/service/action_test.go:90

  • The test TestHandleInvokeActionValidate may not adequately cover the new NCIP validation logic in validateBorrowingRequest. The test should verify that the NCIP adapter is called correctly and that different NCIP response scenarios (success, failure, no NCIP info, empty user) are properly handled.
func TestHandleInvokeActionValidate(t *testing.T) {
	mockPrRepo := new(MockPrRepo)
	prAction := CreatePatronRequestActionService(mockPrRepo, new(MockIllRepo), *new(events.EventBus), new(handler.Iso18626Handler), CreateMockNcipAdapter())
	mockPrRepo.On("GetPatronRequestById", patronRequestId).Return(pr_db.PatronRequest{State: BorrowerStateNew, 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.EventStatusSuccess, status)
	assert.Nil(t, resultData)

@adamdickmeiss adamdickmeiss force-pushed the CROSSLINK-190-ncip-client-requester-side branch from 00e1ded to de60824 Compare December 12, 2025 15:00
@adamdickmeiss adamdickmeiss marked this pull request as ready for review December 16, 2025 12:17
@adamdickmeiss adamdickmeiss changed the title CROSSLINK-190 WIP CROSSLINK-190 NCIP client requester side Dec 16, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.

"strings"
)

type TenantToSymbol struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

@adamdickmeiss I would rename this to Tenant

return TenantToSymbol{mapping: tenantSymbol}
}

func (t *TenantToSymbol) TenantMode() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to isSpecified

return t.mapping != ""
}

func (t *TenantToSymbol) GetSymbolFromTenant(tenant string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to GetSymbol

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