CROSSLINK-229 populate item table on supplier side by adamdickmeiss · Pull Request #437 · indexdata/crosslink

adamdickmeiss · 2026-03-06T16:14:12Z

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

Get callnumber, item barcode from RequestItemResponse.

Copilot

Pull request overview

Updates supplier-side flow to capture item metadata returned by NCIP RequestItem and persist it so later actions (e.g., Ship/Checkout) can use the LMS-assigned item barcode/call number.

Changes:

Change LmsAdapter.RequestItem to return (itemBarcode, callNumber, error) and plumb these values through the NCIP adapter and tests.
Persist a new item row on lender WillSupply using the returned barcode/call number, and use saved item barcode during Ship checkout.
Extend action service tests and mocks to cover new item persistence and retrieval behavior.

Reviewed changes

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

Show a summary per file

File	Description
illmock/app/ncipmock.go	Adjusts illmock NCIP `RequestItemResponse` to include an `ItemId` type/value.
broker/patron_request/service/action.go	Saves item on `WillSupply`; loads items and uses barcode for `Ship` checkout.
broker/patron_request/service/action_test.go	Adds tests for item save failure and item lookup behaviors; updates mocks.
broker/lms/lms_adapter.go	Updates `LmsAdapter` interface signatures for `RequestItem` and `CheckOutItem`.
broker/lms/lms_adapter_ncip.go	Parses barcode/call number from NCIP `RequestItemResponse`; renames checkout param to barcode.
broker/lms/lms_adapter_manual.go	Updates manual adapter to match new interface return values/param names.
broker/lms/lms_adapter_test.go	Expands `RequestItem` test to assert returned barcode/call number and updates mock response.

Copilot · 2026-03-06T16:20:38Z

broker/patron_request/service/action.go

+		return actionExecutionResult{status: status, result: result, outcome: ActionOutcomeFailure, pr: pr}
+	}
+	if len(items) == 0 {
+		status, result := events.LogErrorAndReturnResult(ctx, "no item found for patron request", nil)


events.LogErrorAndReturnResult(ctx, "no item found for patron request", nil) logs at error level with error=<nil>, which makes operational logs misleading. Prefer passing a real error (e.g., errors.New(...)) or using a non-error logging path for this expected validation failure.

Suggested change

status, result := events.LogErrorAndReturnResult(ctx, "no item found for patron request", nil)

status, result := events.LogErrorAndReturnResult(ctx, "no item found for patron request", errors.New("no item found for patron request"))

Copilot · 2026-03-06T16:20:39Z

illmock/app/ncipmock.go

+		res.RequestItemResponse.ItemId = &ncip.ItemId{
+			ItemIdentifierType:  &ncip.SchemeValuePair{Text: "Item Barcode"},
+			ItemIdentifierValue: req.RequestItem.ItemId[0].ItemIdentifierValue,
+		}


handleRequestItem only populates RequestItemResponse.ItemId when the incoming request has RequestItem.ItemId. The NCIP adapter’s RequestItem call typically sends a BibliographicId (and may omit ItemId), so this mock will return no barcode in that common path. Consider always returning an ItemId in the response (e.g., derived from the bibliographic identifier when ItemId is absent), so supplier-side item population can be exercised against illmock as well.

Suggested change

}

}

} else if len(req.RequestItem.BibliographicId) > 0 {

// For bibliographic-only requests, synthesize an ItemId so that

// supplier-side item population can be exercised against illmock.

res.RequestItemResponse.ItemId = &ncip.ItemId{

ItemIdentifierType: &ncip.SchemeValuePair{Text: "Item Barcode"},

ItemIdentifierValue: "mock-item",

}

Copilot · 2026-03-06T16:20:39Z

broker/lms/lms_adapter_ncip.go

+	if response.ItemId != nil && response.ItemId.ItemIdentifierType != nil &&
+		response.ItemId.ItemIdentifierType.Text == NCIPItemBarcode {
+		barCode = response.ItemId.ItemIdentifierValue
+	}
+	if response.ItemOptionalFields != nil && response.ItemOptionalFields.ItemDescription != nil {
+		callNumber = response.ItemOptionalFields.ItemDescription.CallNumber
+	}
+	return barCode, callNumber, err


RequestItem returns an empty barcode with no error when the response doesn’t contain an ItemId typed as "Item Barcode". That empty barcode is later persisted and used for checkout, which is very likely to fail in Ship. Consider treating “missing barcode” as an error (or at least falling back to ItemIdentifierValue regardless of type / supporting additional identifier types) so the caller can fail fast and avoid persisting unusable item records.

Suggested change

if response.ItemId != nil && response.ItemId.ItemIdentifierType != nil &&

response.ItemId.ItemIdentifierType.Text == NCIPItemBarcode {

barCode = response.ItemId.ItemIdentifierValue

}

if response.ItemOptionalFields != nil && response.ItemOptionalFields.ItemDescription != nil {

callNumber = response.ItemOptionalFields.ItemDescription.CallNumber

}

return barCode, callNumber, err

// Prefer an ItemId typed as "Item Barcode", but fall back to any non-empty

// ItemIdentifierValue so callers can still use a usable identifier.

if response.ItemId != nil {

if response.ItemId.ItemIdentifierType != nil &&

response.ItemId.ItemIdentifierType.Text == NCIPItemBarcode {

barCode = response.ItemId.ItemIdentifierValue

} else if response.ItemId.ItemIdentifierValue != "" {

barCode = response.ItemId.ItemIdentifierValue

}

}

if response.ItemOptionalFields != nil && response.ItemOptionalFields.ItemDescription != nil {

callNumber = response.ItemOptionalFields.ItemDescription.CallNumber

}

if barCode == "" {

return "", callNumber, fmt.Errorf("ncip RequestItem: missing item identifier value in response")

}

return barCode, callNumber, nil

Copilot · 2026-03-06T16:20:39Z

broker/lms/lms_adapter_ncip.go

+	barCode := ""
+	callNumber := ""
+	if response.ItemId != nil && response.ItemId.ItemIdentifierType != nil &&
+		response.ItemId.ItemIdentifierType.Text == NCIPItemBarcode {
+		barCode = response.ItemId.ItemIdentifierValue
+	}
+	if response.ItemOptionalFields != nil && response.ItemOptionalFields.ItemDescription != nil {
+		callNumber = response.ItemOptionalFields.ItemDescription.CallNumber
+	}
+	return barCode, callNumber, err


Minor naming/style: local variable barCode is not idiomatic Go casing. Prefer barcode for consistency with Go naming conventions and the rest of the codebase.

Suggested change

barCode := ""

callNumber := ""

if response.ItemId != nil && response.ItemId.ItemIdentifierType != nil &&

response.ItemId.ItemIdentifierType.Text == NCIPItemBarcode {

barCode = response.ItemId.ItemIdentifierValue

}

if response.ItemOptionalFields != nil && response.ItemOptionalFields.ItemDescription != nil {

callNumber = response.ItemOptionalFields.ItemDescription.CallNumber

}

return barCode, callNumber, err

barcode := ""

callNumber := ""

if response.ItemId != nil && response.ItemId.ItemIdentifierType != nil &&

response.ItemId.ItemIdentifierType.Text == NCIPItemBarcode {

barcode = response.ItemId.ItemIdentifierValue

}

if response.ItemOptionalFields != nil && response.ItemOptionalFields.ItemDescription != nil {

callNumber = response.ItemOptionalFields.ItemDescription.CallNumber

}

return barcode, callNumber, err

Copilot · 2026-03-06T16:20:40Z

broker/patron_request/service/action.go

+	itemBarcode, callNumber, err := lmsAdapter.RequestItem(requestId, itemId, userId, pickupLocation, itemLocation)
 	if err != nil {
 		status, result := events.LogErrorAndReturnResult(ctx, "LMS RequestItem failed", err)
 		return actionExecutionResult{status: status, result: result, outcome: ActionOutcomeFailure, pr: pr}
 	}
+	_, err = a.prRepo.SaveItem(ctx, pr_db.SaveItemParams{
+		ID:         uuid.NewString(),
+		CreatedAt:  pgtype.Timestamp{Valid: true, Time: time.Now()},
+		PrID:       pr.ID,
+		ItemID:     getDbText(itemId),
+		Title:      pgtype.Text{}, // no title
+		CallNumber: getDbText(callNumber),
+		Barcode:    itemBarcode,
+	})


willSupplyLenderRequest saves an item even if itemBarcode is empty. Since Ship now relies on the saved item’s Barcode for checkout, this can result in persisting an unusable item and a later checkout failure that’s harder to diagnose. Consider validating itemBarcode (fail the action if missing, or use a deterministic fallback such as the requested item id) before calling SaveItem.

broker/patron_request/service/action.go

Copilot · 2026-03-06T16:20:40Z

broker/patron_request/service/action.go

+		status, result := events.LogErrorAndReturnResult(ctx, "no item found for patron request", nil)
+		return actionExecutionResult{status: status, result: result, outcome: ActionOutcomeFailure, pr: pr}
+	}
+	err = lmsAdapter.CheckOutItem(requestId, items[0].Barcode, userId, externalReferenceValue)


shipLenderRequest selects items[0] without checking whether the barcode is non-empty or whether there are multiple items for the patron request. Since the item table supports multiple rows per PR, this can lead to checking out the wrong item (or an empty barcode). Consider selecting the item deterministically (e.g., matching illRequest.BibliographicInfo.SupplierUniqueRecordId against ItemID, or explicitly erroring when there isn’t exactly one matching item) and validating Barcode before calling CheckOutItem.

Suggested change

err = lmsAdapter.CheckOutItem(requestId, items[0].Barcode, userId, externalReferenceValue)

if len(items) != 1 {

status, result := events.LogErrorAndReturnResult(ctx, "multiple items found for patron request; cannot determine which to check out", nil)

return actionExecutionResult{status: status, result: result, outcome: ActionOutcomeFailure, pr: pr}

}

barcode := strings.TrimSpace(items[0].Barcode)

if barcode == "" {

status, result := events.LogErrorAndReturnResult(ctx, "item barcode is empty; cannot check out item", nil)

return actionExecutionResult{status: status, result: result, outcome: ActionOutcomeFailure, pr: pr}

}

err = lmsAdapter.CheckOutItem(requestId, barcode, userId, externalReferenceValue)

And get callnumber, item barcode from RequestItemResponse.

adamdickmeiss requested review from JanisSaldabols and jakub-id as code owners March 6, 2026 16:14

Copilot AI review requested due to automatic review settings March 6, 2026 16:14

adamdickmeiss marked this pull request as draft March 6, 2026 16:14

Copilot started reviewing on behalf of adamdickmeiss March 6, 2026 16:14 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

adamdickmeiss force-pushed the CROSSLINK-229-item-table-supplier-side branch from 023184c to 8b7fb65 Compare March 6, 2026 16:33

CROSSLINK-229 populate item table on supplier side

8b7fb65

And get callnumber, item barcode from RequestItemResponse.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CROSSLINK-229 populate item table on supplier side#437

CROSSLINK-229 populate item table on supplier side#437
adamdickmeiss wants to merge 1 commit intomainfrom
CROSSLINK-229-item-table-supplier-side

adamdickmeiss commented Mar 6, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

	status, result := events.LogErrorAndReturnResult(ctx, "no item found for patron request", nil)
	status, result := events.LogErrorAndReturnResult(ctx, "no item found for patron request", errors.New("no item found for patron request"))

-		}
+		}
+	} else if len(req.RequestItem.BibliographicId) > 0 {
+		// For bibliographic-only requests, synthesize an ItemId so that
+		// supplier-side item population can be exercised against illmock.
+		res.RequestItemResponse.ItemId = &ncip.ItemId{
+			ItemIdentifierType:  &ncip.SchemeValuePair{Text: "Item Barcode"},
+			ItemIdentifierValue: "mock-item",
+		}

-	err = lmsAdapter.CheckOutItem(requestId, items[0].Barcode, userId, externalReferenceValue)
+	if len(items) != 1 {
+		status, result := events.LogErrorAndReturnResult(ctx, "multiple items found for patron request; cannot determine which to check out", nil)
+		return actionExecutionResult{status: status, result: result, outcome: ActionOutcomeFailure, pr: pr}
+	}
+	barcode := strings.TrimSpace(items[0].Barcode)
+	if barcode == "" {
+		status, result := events.LogErrorAndReturnResult(ctx, "item barcode is empty; cannot check out item", nil)
+		return actionExecutionResult{status: status, result: result, outcome: ActionOutcomeFailure, pr: pr}
+	}
+	err = lmsAdapter.CheckOutItem(requestId, barcode, userId, externalReferenceValue)

Conversation

adamdickmeiss commented Mar 6, 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!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

adamdickmeiss commented Mar 6, 2026 •

edited

Loading