diff --git a/broker/README.md b/broker/README.md index a6613baf3..4cc1aaa32 100644 --- a/broker/README.md +++ b/broker/README.md @@ -797,22 +797,25 @@ an item ID. The selected holding's call number, when available, is persisted with its item ID and sent in `supplierInfo.callNumber`. Both fields are cleared when a new availability lookup invalidates the previous selection. -Patron requests carry the shared-index record identifier separately in -`bib_record_id` (API: `bibRecordId`). Clients should supply the VuFind UUID or -Trove ANBD record ID when creating the borrowing request. Updates omit the field -to preserve it or send an empty string to clear it. This identifier is preserved -through retries and rerequests. Outgoing ISO18626 requests include it in -`bibliographicInfo.bibliographicRecordId` with -`bibliographicRecordIdentifierCode` set to `ReShare`; incoming lending requests -extract and persist that identifier. Other record codes are preserved, and -conflicting ReShare identifiers are rejected. - -The “view in shared index” UI should use `bibRecordId` with its configured -VuFind/Trove URL and hide the link when the ID is absent or empty. It must not -substitute the supplier's local bibliographic or copy ID. Existing requests -remain without a shared-index ID unless it is explicitly supplied; the migration -does not infer one from local identifiers. Import bundles also accept -`patronRequest.bibRecordId`. +`bibRecordId` is a read-only shared-index ID (VuFind UUID or Trove ANBD record ID), +derived from `illRequest`: `bibliographicInfo.supplierUniqueRecordId` for borrowing, +or the ReShare-coded `bibliographicInfo.bibliographicRecordId` for lending. +The borrowing value follows creation, edits, metadata updates, and corrected +retry requests automatically. Clients cannot set `bibRecordId` directly; create +and update requests ignore that property, and import bundles do not accept it. +Import the source identifiers in `illRequest` instead. The legacy `bib_record_id` +database column is retained for schema compatibility and is no longer used to +determine the API value or the outgoing shared-index identifier. + +The broker replaces `supplierUniqueRecordId` with the supplier's local ID in the +requests it sends to suppliers, so it also sends the shared-index ID as a +`bibliographicRecordId` with code `ReShare`. Other record codes are preserved; +incoming requests with conflicting ReShare IDs are rejected. + +Clients linking to the shared index should use `bibRecordId` with their +configured VuFind/Trove URL and hide the link when it is absent. They must not +fall back to `supplierUniqueRecordId`, which on a lending request is the +supplier's local ID. When supplier Request Item is disabled (including manual LMS workflows), a Loan automatically attaches the selected copy as a volume before proceeding to diff --git a/broker/import/service/importer.go b/broker/import/service/importer.go index 92dd55d66..fdd4fa1fa 100644 --- a/broker/import/service/importer.go +++ b/broker/import/service/importer.go @@ -431,8 +431,7 @@ func (i Importer) normalizePatronRequest(owner string, apiBundle importoapi.Impo bundle := importdb.PatronRequestBundle{PatronRequest: pr_db.CreatePatronRequestParams{ ID: request.Id, CreatedAt: pgTimestamp(request.CreatedAt), UpdatedAt: pgTimestamp(request.UpdatedAt), IllRequest: request.IllRequest, IllResponse: responseValue, DueAt: dueAt, - BibRecordID: pgTextFromPtr(request.BibRecordId), - State: pr_db.PatronRequestState(request.State), Side: side, + State: pr_db.PatronRequestState(request.State), Side: side, Patron: pgTextFromPtr(request.Patron), RequesterSymbol: pgTextFromString(request.RequesterSymbol), SupplierSymbol: pgTextFromPtr(request.SupplierSymbol), Tenant: pgTextFromPtr(request.Tenant), RequesterReqID: pgTextFromString(request.RequesterRequestId), NeedsAttention: request.NeedsAttention, diff --git a/broker/import/service/importer_test.go b/broker/import/service/importer_test.go index 91d4aacbd..a67438a02 100644 --- a/broker/import/service/importer_test.go +++ b/broker/import/service/importer_test.go @@ -1175,7 +1175,7 @@ func TestImportNotificationRejectsLegacyCondition(t *testing.T) { assert.Contains(t, err.Error(), "condition") } -func TestImportPatronRequestStoresBibRecordID(t *testing.T) { +func TestImportPatronRequestRejectsBibRecordID(t *testing.T) { repo := &recordingImportRepo{patronResult: importdb.Result{Outcome: importdb.OutcomeImported}} cache := &recordingPeerCache{peers: []ill_db.Peer{{ID: "peer-requester"}, {ID: "peer-supplier"}}} importer := newImporter(repo, cache, nil, &recordingStateValidator{terminal: true}, fixedClock) @@ -1183,6 +1183,6 @@ func TestImportPatronRequestStoresBibRecordID(t *testing.T) { bundle["patronRequest"].(map[string]any)["bibRecordId"] = "shared-record" }) _, _, err := importer.importPatronRequest(testCtx(), importdb.ConflictPolicyUpdate, "ISIL:REQ", data) - require.NoError(t, err) - assert.Equal(t, pgText("shared-record"), repo.patron.PatronRequest.BibRecordID) + require.ErrorContains(t, err, "bibRecordId") + assert.Empty(t, repo.patron.PatronRequest.ID) } diff --git a/broker/oapi/open-api.yaml b/broker/oapi/open-api.yaml index bcf8dc88b..ae43fb027 100644 --- a/broker/oapi/open-api.yaml +++ b/broker/oapi/open-api.yaml @@ -231,12 +231,6 @@ components: type: string minLength: 1 format: date-time - bibRecordId: - type: string - description: >- - Shared-index bibliographic record ID (VuFind UUID or Trove ANBD record ID), - independent of supplier bibliographic and item IDs. Sent to suppliers as - bibliographicInfo.bibliographicRecordId with identifier code ReShare. illRequest: type: object description: JSON of ISO18626 request @@ -925,10 +919,11 @@ components: description: Timestamp of the patron request last update bibRecordId: type: string + readOnly: true description: >- - Shared-index bibliographic record ID (VuFind UUID or Trove ANBD record ID), - independent of supplier bibliographic and item IDs. Sent to suppliers as - bibliographicInfo.bibliographicRecordId with identifier code ReShare. + Read-only shared-index record ID (VuFind UUID or Trove ANBD record ID). + Borrowing requests use illRequest.bibliographicInfo.supplierUniqueRecordId; + lending requests use the ReShare-coded bibliographicRecordId. illRequest: type: object description: JSON of ISO18626 request @@ -1065,13 +1060,6 @@ components: On creation, ID of the previous borrowing request belonging to the same requester. The previous request must not already have a successor. Both requests are linked atomically. Not supported on update. - bibRecordId: - type: string - description: >- - Shared-index bibliographic record ID (VuFind UUID or Trove ANBD record ID), - independent of supplier bibliographic and item IDs. Sent to suppliers as - bibliographicInfo.bibliographicRecordId with identifier code ReShare. - On update, omit to preserve the current ID or send an empty string to clear it. illRequest: type: object description: JSON of ISO18626 request diff --git a/broker/patron_request/api/api-handler.go b/broker/patron_request/api/api-handler.go index c76960a87..907e5bf37 100644 --- a/broker/patron_request/api/api-handler.go +++ b/broker/patron_request/api/api-handler.go @@ -570,9 +570,6 @@ func (a *PatronRequestApiHandler) PutPatronRequestsId(w http.ResponseWriter, r * existingPr.RequesterReqID = getDbText(&requesterReqId) existingPr.IllRequest = illRequest - if newPr.BibRecordId != nil { - existingPr.BibRecordID = getDbText(newPr.BibRecordId) - } if newPr.RequesterPickupLocationId.IsSpecified() { existingPr.RequesterPickupLocationID = getPickupLocationID(newPr.RequesterPickupLocationId) if !a.validatePickupLocation(w, ctx, existingPr) { @@ -1475,6 +1472,10 @@ func toApiPatronRequest(r *http.Request, request pr_db.PatronRequestSearchView) value := api.Link(r, api.Path("ill_transactions"), api.Query("requester_req_id", request.RequesterReqID.String)) illTransactionLink = &value } + var bibRecordID *string + if id := prservice.BibRecordID(request.Side, request.IllRequest); id != "" { + bibRecordID = &id + } pr := proapi.PatronRequest{ Id: request.ID, @@ -1488,7 +1489,7 @@ func toApiPatronRequest(r *http.Request, request pr_db.PatronRequestSearchView) DueDate: toLoanDate(request.DueAt), SupplierSymbol: toString(request.SupplierSymbol), IllRequest: request.IllRequest, - BibRecordId: toString(request.BibRecordID), + BibRecordId: bibRecordID, RequesterRequestId: toString(request.RequesterReqID), NeedsAttention: request.NeedsAttention, HasCost: request.HasCost, @@ -1646,7 +1647,6 @@ func buildDbPatronRequest( RequesterPickupLocationID: getPickupLocationID(request.RequesterPickupLocationId), SupplierSymbol: getDbText(nil), IllRequest: illRequest, - BibRecordID: getDbText(request.BibRecordId), Tenant: getDbText(tenant), RequesterReqID: getDbText(&requesterReqId), InternalNote: getDbText(request.InternalNote), diff --git a/broker/patron_request/api/api-handler_test.go b/broker/patron_request/api/api-handler_test.go index e92f1e414..d76488425 100644 --- a/broker/patron_request/api/api-handler_test.go +++ b/broker/patron_request/api/api-handler_test.go @@ -2306,36 +2306,91 @@ func TestCreateValidatesPickupBeforePersistence(t *testing.T) { } } -func TestPatronRequestBibRecordIDMapping(t *testing.T) { - for _, id := range []*string{nil, new("9a18bd66-8db6-4afd-95e6-157b2d3b6d12"), new("36457383"), new("")} { - request := proapi.CreatePatronRequest{BibRecordId: id, IllRequest: validIllRequest()} - pr := buildDbPatronRequest(&request, nil, pgtype.Timestamp{}, "request-1", request.IllRequest, "", "default") - result := toApiPatronRequest(httptest.NewRequest(http.MethodGet, "/patron_requests/request-1", nil), patronRequestSearchViewFromPatronRequest(pr, false)) - assert.Equal(t, id, result.BibRecordId) - } -} - -func TestPutPatronRequestBibRecordID(t *testing.T) { +func TestPatronRequestBibRecordID(t *testing.T) { + reShare := []iso18626.BibliographicRecordId{{ + BibliographicRecordIdentifierCode: iso18626.TypeSchemeValuePair{Text: "ReShare"}, + BibliographicRecordIdentifier: "shared-record", + }} for _, tc := range []struct { name string - supplied *string - expected string + side pr_db.PatronRequestSide + info iso18626.BibliographicInfo + expected *string }{ - {name: "omitted preserves ID", expected: "original"}, - {name: "supplied replaces ID", supplied: new("replacement"), expected: "replacement"}, - {name: "empty clears ID", supplied: new("")}, + {name: "borrowing uses system ID", side: prservice.SideBorrowing, info: iso18626.BibliographicInfo{SupplierUniqueRecordId: "shared-record", BibliographicRecordId: []iso18626.BibliographicRecordId{{BibliographicRecordIdentifierCode: iso18626.TypeSchemeValuePair{Text: "ReShare"}, BibliographicRecordIdentifier: "other"}}}, expected: new("shared-record")}, + {name: "borrowing without system ID", side: prservice.SideBorrowing}, + {name: "lending uses ReShare record ID", side: prservice.SideLending, info: iso18626.BibliographicInfo{SupplierUniqueRecordId: "local-bib", BibliographicRecordId: reShare}, expected: new("shared-record")}, + {name: "lending without ReShare record ID", side: prservice.SideLending, info: iso18626.BibliographicInfo{SupplierUniqueRecordId: "local-bib"}}, } { t.Run(tc.name, func(t *testing.T) { - repo := &PrRepoUpdateCapture{bibRecordID: pgtype.Text{String: "original", Valid: true}} - handler := NewPrApiHandler(repo, mockEventBus, mockEventRepo, tenant.NewResolver(), nil, 10) - id := "3" - body, err := json.Marshal(proapi.CreatePatronRequest{Id: &id, RequesterSymbol: &symbol, IllRequest: validIllRequest(), BibRecordId: tc.supplied}) - require.NoError(t, err) - rr := httptest.NewRecorder() - handler.PutPatronRequestsId(rr, httptest.NewRequest(http.MethodPut, "/", bytes.NewReader(body)), id, proapi.PutPatronRequestsIdParams{}) - require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) - require.NotNil(t, repo.lastUpdateParams) - assert.Equal(t, tc.expected, repo.lastUpdateParams.BibRecordID.String) + pr := pr_db.PatronRequest{ID: "request-1", Side: tc.side, BibRecordID: pgtype.Text{String: "stale-column-value", Valid: true}, IllRequest: iso18626.Request{BibliographicInfo: tc.info}} + result := toApiPatronRequest(httptest.NewRequest(http.MethodGet, "/patron_requests/request-1", nil), patronRequestSearchViewFromPatronRequest(pr, false)) + assert.Equal(t, tc.expected, result.BibRecordId) }) } } + +type bibRecordIDRepo struct { + PrRepoUpdateCapture + createdPr *pr_db.PatronRequest +} + +func (r *bibRecordIDRepo) CreatePatronRequest(ctx common.ExtendedContext, params pr_db.CreatePatronRequestParams) (pr_db.PatronRequest, error) { + pr := pr_db.PatronRequest(params) + r.createdPr = &pr + return pr, nil +} + +func (r *bibRecordIDRepo) GetPatronRequestSearchView(ctx common.ExtendedContext, id string) (pr_db.PatronRequestSearchView, error) { + if r.createdPr != nil { + return patronRequestSearchViewFromPatronRequest(*r.createdPr, false), nil + } + return patronRequestSearchViewFromPatronRequest(pr_db.PatronRequest(*r.lastUpdateParams), false), nil +} + +func TestPatronRequestBibRecordIDReadOnly(t *testing.T) { + for _, method := range []string{http.MethodPost, http.MethodPut} { + for _, source := range []string{"source-record", ""} { + for _, supplied := range []string{"omitted", `"override"`, `""`, "null"} { + t.Run(method+"/source="+source+"/supplied="+supplied, func(t *testing.T) { + repo := &bibRecordIDRepo{PrRepoUpdateCapture: PrRepoUpdateCapture{bibRecordID: pgtype.Text{String: "stale-column-value", Valid: true}}} + handler := NewPrApiHandler(repo, mockEventBus, mockEventRepo, tenant.NewResolver(), nil, 10) + request := validIllRequest() + request.BibliographicInfo.SupplierUniqueRecordId = source + id := "3" + body, err := json.Marshal(proapi.CreatePatronRequest{Id: &id, RequesterSymbol: &symbol, IllRequest: request}) + require.NoError(t, err) + var payload map[string]json.RawMessage + require.NoError(t, json.Unmarshal(body, &payload)) + if supplied != "omitted" { + payload["bibRecordId"] = json.RawMessage(supplied) + } + body, err = json.Marshal(payload) + require.NoError(t, err) + rr := httptest.NewRecorder() + req := httptest.NewRequest(method, "/patron_requests/"+id, bytes.NewReader(body)) + if method == http.MethodPost { + handler.PostPatronRequests(rr, req, proapi.PostPatronRequestsParams{}) + require.Equal(t, http.StatusCreated, rr.Code, rr.Body.String()) + require.NotNil(t, repo.createdPr) + assert.False(t, repo.createdPr.BibRecordID.Valid) + } else { + handler.PutPatronRequestsId(rr, req, id, proapi.PutPatronRequestsIdParams{}) + require.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) + require.NotNil(t, repo.lastUpdateParams) + assert.Equal(t, repo.bibRecordID, repo.lastUpdateParams.BibRecordID) + } + var response proapi.PatronRequest + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &response)) + if source == "" { + assert.Nil(t, response.BibRecordId) + assert.NotContains(t, rr.Body.String(), `"bibRecordId"`) + } else { + require.NotNil(t, response.BibRecordId) + assert.Equal(t, source, *response.BibRecordId) + } + }) + } + } + } +} diff --git a/broker/patron_request/service/action.go b/broker/patron_request/service/action.go index 1df5749d8..2a06b4ed3 100644 --- a/broker/patron_request/service/action.go +++ b/broker/patron_request/service/action.go @@ -1439,7 +1439,6 @@ func (a *PatronRequestActionService) createSuccessorBorrowingRequest(ctx common. successorPr.Side = pr.Side successorPr.RequesterSymbol = pr.RequesterSymbol successorPr.RequesterPickupLocationID = pr.RequesterPickupLocationID - successorPr.BibRecordID = pr.BibRecordID if retry { successorPr.SupplierSymbol = pr.SupplierSymbol } diff --git a/broker/patron_request/service/bib_record_id.go b/broker/patron_request/service/bib_record_id.go index a139e517c..741633f4d 100644 --- a/broker/patron_request/service/bib_record_id.go +++ b/broker/patron_request/service/bib_record_id.go @@ -4,11 +4,30 @@ import ( "fmt" "strings" + pr_db "github.com/indexdata/crosslink/broker/patron_request/db" "github.com/indexdata/crosslink/iso18626" ) const sharedIndexRecordCode = "ReShare" +// BibRecordID returns a patron request's shared-index record ID, or "" if it has none +// or it is ambiguous. On the borrowing side that is supplierUniqueRecordId. The broker +// replaces that field with the supplier's local ID in the requests it sends and adds the +// shared-index ID under code ReShare, so on the lending side it is read from there. +func BibRecordID(side pr_db.PatronRequestSide, illRequest iso18626.Request) string { + if side == SideBorrowing { + return illRequest.BibliographicInfo.SupplierUniqueRecordId + } + if side != SideLending { + return "" + } + id, err := sharedIndexRecordID(illRequest.BibliographicInfo) + if err != nil { + return "" + } + return id +} + // sharedIndexRecordID reads only the consortium's record identifier. Supplier // bibliographic IDs and copy IDs belong to different namespaces. func sharedIndexRecordID(info iso18626.BibliographicInfo) (string, error) { @@ -30,7 +49,7 @@ func sharedIndexRecordID(info iso18626.BibliographicInfo) (string, error) { } // setSharedIndexRecordID replaces only ReShare identifiers, including removing -// stale IDs when the local field has been cleared. It does not mutate the input slice. +// stale IDs when the source identifier has been cleared. It does not mutate the input slice. func setSharedIndexRecordID(info *iso18626.BibliographicInfo, id string) { var records []iso18626.BibliographicRecordId for _, record := range info.BibliographicRecordId { diff --git a/broker/patron_request/service/bib_record_id_test.go b/broker/patron_request/service/bib_record_id_test.go index 41f4b17eb..8726eea71 100644 --- a/broker/patron_request/service/bib_record_id_test.go +++ b/broker/patron_request/service/bib_record_id_test.go @@ -4,10 +4,14 @@ import ( "encoding/xml" "testing" + "github.com/indexdata/crosslink/broker/catalog" "github.com/indexdata/crosslink/broker/events" + "github.com/indexdata/crosslink/broker/ill_db" pr_db "github.com/indexdata/crosslink/broker/patron_request/db" + dirapi "github.com/indexdata/crosslink/directory/api" "github.com/indexdata/crosslink/iso18626" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -16,7 +20,7 @@ func TestSharedIndexRecordIDExchange(t *testing.T) { t.Run(id, func(t *testing.T) { request := iso18626.Request{ BibliographicInfo: iso18626.BibliographicInfo{ - SupplierUniqueRecordId: "local-bib", + SupplierUniqueRecordId: id, BibliographicRecordId: []iso18626.BibliographicRecordId{ {BibliographicRecordIdentifierCode: iso18626.TypeSchemeValuePair{Text: "OCLC"}, BibliographicRecordIdentifier: "oclc-id"}, {BibliographicRecordIdentifierCode: iso18626.TypeSchemeValuePair{Text: "ReShare"}, BibliographicRecordIdentifier: "stale-index-id"}, @@ -26,11 +30,13 @@ func TestSharedIndexRecordIDExchange(t *testing.T) { } sender := PatronRequestMessageSender{eventBus: new(MockEventBus), iso18626Handler: new(MockIso18626Handler)} status, result, err := sender.sendBorrowingRequest(appCtx, "send", pr_db.PatronRequest{ - ID: "request", RequesterSymbol: getDbText("ISIL:REQ1"), BibRecordID: getDbTextPtr(&id), + ID: "request", RequesterSymbol: getDbText("ISIL:REQ1"), }, request) require.NoError(t, err) require.Equal(t, events.EventStatusSuccess, status) // Exercise the wire format used between brokers and suppliers. + // The transaction client substitutes the selected supplier's local ID before delivery. + result.OutgoingMessage.Request.BibliographicInfo.SupplierUniqueRecordId = "local-bib" wire, err := xml.Marshal(result.OutgoingMessage) require.NoError(t, err) var received iso18626.ISO18626Message @@ -50,8 +56,7 @@ func TestSharedIndexRecordIDExchange(t *testing.T) { repo := new(MockPrRepo) created, err := CreateLendingRequest(appCtx, repo, pr_db.PatronRequest{ID: "supply", Side: SideLending, IllRequest: *received.Request}) require.NoError(t, err) - assert.Equal(t, id, created.BibRecordID.String) - assert.Equal(t, id != "", created.BibRecordID.Valid) + assert.Equal(t, id, BibRecordID(SideLending, created.IllRequest)) assert.Equal(t, "local-bib", created.IllRequest.BibliographicInfo.SupplierUniqueRecordId) assert.Equal(t, "local-copy", created.IllRequest.SupplierInfo[0].BibliographicRecordId.BibliographicRecordIdentifier) assert.Equal(t, "stale-index-id", request.BibliographicInfo.BibliographicRecordId[1].BibliographicRecordIdentifier) @@ -59,6 +64,50 @@ func TestSharedIndexRecordIDExchange(t *testing.T) { } } +func TestBibRecordIDFollowsMetadataUpdate(t *testing.T) { + for _, tc := range []struct { + name, before, catalogID, expected string + }{ + {name: "fills missing ID", catalogID: "catalog-record", expected: "catalog-record"}, + {name: "replaces ID", before: "original-record", catalogID: "catalog-record", expected: "catalog-record"}, + {name: "preserves ID when catalog omits it", before: "original-record", expected: "original-record"}, + {name: "missing ID stays absent"}, + } { + t.Run(tc.name, func(t *testing.T) { + mode := dirapi.Auto + peer := peerWithMetadataMode(&mode) + peer.Vendor = string(dirapi.CrossLink) + illRepo := new(IllRepoMock) + illRepo.On("GetCachedPeersBySymbols", []string{"ISIL:REQ1"}, mock.Anything).Return([]ill_db.Peer{peer}, "", nil) + svc := PatronRequestActionService{ + illRepo: illRepo, + lookupAdapterFactory: lookupFactoryWithAdapter(&catalog.MockLookupAdapter{Metadata: catalog.Metadata{ + Identifier: tc.catalogID, Title: "Catalog title", + }}), + } + pr := pr_db.PatronRequest{ + ID: "request", Side: SideBorrowing, RequesterSymbol: getDbText("ISIL:REQ1"), + BibRecordID: getDbText("stale-column-value"), + IllRequest: iso18626.Request{BibliographicInfo: iso18626.BibliographicInfo{ + SupplierUniqueRecordId: tc.before, Title: "Original title", + }}, + } + assert.Equal(t, tc.before, BibRecordID(pr.Side, pr.IllRequest)) + updated := svc.updateMetadataBorrowingRequest(appCtx, pr, pr.IllRequest) + require.Equal(t, events.EventStatusSuccess, updated.status) + assert.Equal(t, tc.expected, BibRecordID(updated.pr.Side, updated.pr.IllRequest)) + sender := PatronRequestMessageSender{eventBus: new(MockEventBus), iso18626Handler: new(MockIso18626Handler)} + status, result, err := sender.sendBorrowingRequest(appCtx, "send", updated.pr, updated.pr.IllRequest) + require.NoError(t, err) + require.Equal(t, events.EventStatusSuccess, status) + id, err := sharedIndexRecordID(result.OutgoingMessage.Request.BibliographicInfo) + require.NoError(t, err) + assert.Equal(t, tc.expected, id) + illRepo.AssertExpectations(t) + }) + } +} + func TestLendingRequestReShareRecordIDs(t *testing.T) { for _, tc := range []struct { name string @@ -85,7 +134,7 @@ func TestLendingRequestReShareRecordIDs(t *testing.T) { assert.Empty(t, repo.createdPr.ID) } else { require.NoError(t, err) - assert.Equal(t, tc.expected, created.BibRecordID.String) + assert.Equal(t, tc.expected, BibRecordID(SideLending, created.IllRequest)) } }) } diff --git a/broker/patron_request/service/create.go b/broker/patron_request/service/create.go index d8baa7d8b..eaed1f350 100644 --- a/broker/patron_request/service/create.go +++ b/broker/patron_request/service/create.go @@ -54,11 +54,9 @@ func CreateBorrowingRequest(ctx common.ExtendedContext, repo pr_db.PrRepo, reque // CreateLendingRequest creates a supply request and atomically links a retry or rerequest to // its local predecessor, if held by the same supplier for the same requester. func CreateLendingRequest(ctx common.ExtendedContext, repo pr_db.PrRepo, request pr_db.PatronRequest) (pr_db.PatronRequest, error) { - bibRecordID, err := sharedIndexRecordID(request.IllRequest.BibliographicInfo) - if err != nil { + if _, err := sharedIndexRecordID(request.IllRequest.BibliographicInfo); err != nil { return pr_db.PatronRequest{}, err } - request.BibRecordID = getDbTextPtr(&bibRecordID) info := request.IllRequest.ServiceInfo if info == nil || info.RequestingAgencyPreviousRequestId == "" { return repo.CreatePatronRequest(ctx, pr_db.CreatePatronRequestParams(request)) @@ -72,7 +70,7 @@ func CreateLendingRequest(ctx common.ExtendedContext, repo pr_db.PrRepo, request return repo.CreatePatronRequest(ctx, pr_db.CreatePatronRequestParams(request)) } var created pr_db.PatronRequest - err = repo.WithTxFunc(ctx, func(txRepo pr_db.PrRepo) error { + err := repo.WithTxFunc(ctx, func(txRepo pr_db.PrRepo) error { previous, err := txRepo.GetLendingPredecessorForUpdate(ctx, pr_db.GetLendingPredecessorForUpdateParams{ SupplierSymbol: request.SupplierSymbol, RequesterSymbol: request.RequesterSymbol, RequesterReqID: getDbText(info.RequestingAgencyPreviousRequestId), Tenant: request.Tenant, diff --git a/broker/patron_request/service/message_sender.go b/broker/patron_request/service/message_sender.go index c837c2247..e6d1f7dc3 100644 --- a/broker/patron_request/service/message_sender.go +++ b/broker/patron_request/service/message_sender.go @@ -149,7 +149,7 @@ func (ms *PatronRequestMessageSender) sendBorrowingRequest(ctx common.ExtendedCo AgencyIdValue: reqSymbol, } illRequest.Header.RequestingAgencyRequestId = pr.ID - setSharedIndexRecordID(&illRequest.BibliographicInfo, pr.BibRecordID.String) + setSharedIndexRecordID(&illRequest.BibliographicInfo, BibRecordID(SideBorrowing, illRequest)) illRequest.Header.Timestamp = utils.XSDDateTime{Time: time.Now()} if illRequest.PatronInfo == nil { illRequest.PatronInfo = &iso18626.PatronInfo{} diff --git a/broker/patron_request/service/rerequest_test.go b/broker/patron_request/service/rerequest_test.go index f51bca4b2..24e736853 100644 --- a/broker/patron_request/service/rerequest_test.go +++ b/broker/patron_request/service/rerequest_test.go @@ -26,8 +26,7 @@ func TestRerequest(t *testing.T) { original := pr_db.PatronRequest{ ID: "REQ1-1", Side: SideBorrowing, State: state, TerminalState: true, RequesterSymbol: getDbText("ISIL:REQ1"), SupplierSymbol: getDbText("ISIL:SUP1"), - BibRecordID: getDbText("shared-record"), - Patron: getDbText("patron"), Tenant: getDbText("tenant"), StateModel: "default", + Patron: getDbText("patron"), Tenant: getDbText("tenant"), StateModel: "default", IllRequest: iso18626.Request{ Header: iso18626.Header{RequestingAgencyRequestId: "REQ1-1", SupplyingAgencyRequestId: "old-supplier-id"}, BibliographicInfo: iso18626.BibliographicInfo{Title: "Original title"}, @@ -57,7 +56,6 @@ func TestRerequest(t *testing.T) { assert.False(t, next.TerminalState) assert.Equal(t, original.Patron, next.Patron) assert.Equal(t, original.Tenant, next.Tenant) - assert.Equal(t, original.BibRecordID, next.BibRecordID) assert.Equal(t, original.RequesterPickupLocationID, next.RequesterPickupLocationID) assert.False(t, next.SupplierSymbol.Valid) assert.Empty(t, next.SupplierSymbol.String) @@ -161,9 +159,12 @@ func TestSuccessorPersistsRequestType(t *testing.T) { } original := pr_db.PatronRequest{ ID: "REQ1-1", Side: SideBorrowing, State: "CUSTOM_STATE", - BibRecordID: getDbText("shared-record"), RequesterSymbol: getDbText("ISIL:REQ1"), SupplierSymbol: getDbText("ISIL:SUP1"), + IllRequest: iso18626.Request{ + BibliographicInfo: iso18626.BibliographicInfo{SupplierUniqueRecordId: "shared-record"}, + }, + RetryBibInfo: &iso18626.BibliographicInfo{SupplierUniqueRecordId: "proposed-record"}, } if !legacy { original.IllRequest.ServiceInfo = &iso18626.ServiceInfo{ServiceType: iso18626.TypeServiceTypeLoan, RequestType: &inheritedType} @@ -171,11 +172,12 @@ func TestSuccessorPersistsRequestType(t *testing.T) { result := service.createSuccessorBorrowingRequest(appCtx, original, retry) require.Equal(t, events.EventStatusSuccess, result.status) next := result.successorPr - assert.Equal(t, original.BibRecordID, next.BibRecordID) expectedType := iso18626.TypeRequestTypeNew expectedPrevious := original.ID + expectedRecord := "shared-record" if retry { expectedType = iso18626.TypeRequestTypeRetry + expectedRecord = "proposed-record" assert.Equal(t, original.SupplierSymbol, next.SupplierSymbol) } else { assert.False(t, next.SupplierSymbol.Valid) @@ -193,6 +195,9 @@ func TestSuccessorPersistsRequestType(t *testing.T) { require.Equal(t, events.EventStatusSuccess, status) assert.Equal(t, expectedType, *sent.OutgoingMessage.Request.ServiceInfo.RequestType) assert.Equal(t, expectedPrevious, sent.OutgoingMessage.Request.ServiceInfo.RequestingAgencyPreviousRequestId) + sharedID, err := sharedIndexRecordID(sent.OutgoingMessage.Request.BibliographicInfo) + require.NoError(t, err) + assert.Equal(t, expectedRecord, sharedID) repo.AssertNotCalled(t, "GetPatronRequestById", original.ID) }) } diff --git a/broker/test/patron_request/api/api-handler_test.go b/broker/test/patron_request/api/api-handler_test.go index 337893ac2..3bed191e3 100644 --- a/broker/test/patron_request/api/api-handler_test.go +++ b/broker/test/patron_request/api/api-handler_test.go @@ -136,6 +136,8 @@ func TestCrud(t *testing.T) { assert.NoError(t, err, "failed to unmarshal patron request") assert.Equal(t, *newPr.Id, foundPr.Id) + require.NotNil(t, foundPr.BibRecordId) + assert.Equal(t, "WILLSUPPLY_LOANED", *foundPr.BibRecordId) assert.True(t, foundPr.State != "") assert.Equal(t, "default", foundPr.StateModel) assert.Equal(t, proapi.PatronRequestSide(prservice.SideBorrowing), foundPr.Side) @@ -188,6 +190,8 @@ func TestCrud(t *testing.T) { assert.Equal(t, int64(1), foundPrs.About.Count) assert.Equal(t, *newPr.Id, foundPrs.Items[0].Id) + require.NotNil(t, foundPrs.Items[0].BibRecordId) + assert.Equal(t, "WILLSUPPLY_LOANED", *foundPrs.Items[0].BibRecordId) assert.Nil(t, foundPrs.About.LastLink) assert.NotNil(t, foundPrs.Items[0].NotificationsLink) assert.Equal(t, "/patron_requests/"+*newPr.Id+"/notifications?symbol="+url.QueryEscape(*newPr.RequesterSymbol), *foundPrs.Items[0].NotificationsLink) @@ -418,6 +422,7 @@ func TestNeedsReviewAndUpdate(t *testing.T) { assert.Equal(t, prservice.ActionOutcomeReview, *foundPr.LastActionOutcome) } assert.True(t, foundPr.NeedsAttention) + assert.Nil(t, foundPr.BibRecordId) // PUT without SupplierUniqueRecordId: state must remain NEEDS_REVIEW prId := foundPr.Id @@ -459,6 +464,7 @@ func TestNeedsReviewAndUpdate(t *testing.T) { require.NotNil(t, foundPr.LastActionOutcome) assert.Equal(t, prservice.ActionOutcomeReview, *foundPr.LastActionOutcome) assert.True(t, foundPr.NeedsAttention) + assert.Nil(t, foundPr.BibRecordId) // PUT with SupplierUniqueRecordId: PUT only persists data, state stays NEEDS_REVIEW updateWithId := proapi.CreatePatronRequest{ @@ -481,10 +487,25 @@ func TestNeedsReviewAndUpdate(t *testing.T) { assert.NoError(t, err) assert.Equal(t, string(prservice.BorrowerStateNeedsReview), foundPr.State) assert.Equal(t, "default", foundPr.StateModel) + require.NotNil(t, foundPr.BibRecordId) + assert.Equal(t, "WILLSUPPLY_LOANED", *foundPr.BibRecordId) assertPatronRequestIllRequest(t, foundPr.IllRequest, func(r iso18626.Request) { assert.Equal(t, "WILLSUPPLY_LOANED", r.BibliographicInfo.SupplierUniqueRecordId) }) + // Clearing the source ID also clears the derived API field after persistence. + respBytes = httpRequest(t, "PUT", prPath, updateNoIdBytes, 200) + foundPr = proapi.PatronRequest{} + require.NoError(t, json.Unmarshal(respBytes, &foundPr)) + assert.Nil(t, foundPr.BibRecordId) + respBytes = httpRequest(t, "GET", prPath+queryParams, nil, 200) + require.NoError(t, json.Unmarshal(respBytes, &foundPr)) + assert.Nil(t, foundPr.BibRecordId) + respBytes = httpRequest(t, "PUT", prPath, updateWithIdBytes, 200) + require.NoError(t, json.Unmarshal(respBytes, &foundPr)) + require.NotNil(t, foundPr.BibRecordId) + assert.Equal(t, "WILLSUPPLY_LOANED", *foundPr.BibRecordId) + // Retrying metadata update uses the saved identifier and resumes the pre-send chain. respBytes = httpRequest(t, "POST", prPath+"/action"+queryParams, updateActionBytes, 200) err = json.Unmarshal(respBytes, &pResult) @@ -501,6 +522,8 @@ func TestNeedsReviewAndUpdate(t *testing.T) { assert.NotEqual(t, string(prservice.BorrowerStateMetadataUpdated), foundPr.State) assert.NotEqual(t, string(prservice.BorrowerStateReadyToSend), foundPr.State) assert.Equal(t, "WILLSUPPLY_LOANED", foundPr.IllRequest.BibliographicInfo.SupplierUniqueRecordId) + require.NotNil(t, foundPr.BibRecordId) + assert.Equal(t, "WILLSUPPLY_LOANED", *foundPr.BibRecordId) } func TestActionsToCompleteState(t *testing.T) { diff --git a/bruno/crosslink/PR Happy flow/Create borrowing side PR.bru b/bruno/crosslink/PR Happy flow/Create borrowing side PR.bru index 3d7aa1b29..dd3b67318 100644 --- a/bruno/crosslink/PR Happy flow/Create borrowing side PR.bru +++ b/bruno/crosslink/PR Happy flow/Create borrowing side PR.bru @@ -22,7 +22,6 @@ auth:basic { body:json { { - "bibRecordId": "9a18bd66-8db6-4afd-95e6-157b2d3b6d12", "illRequest": { "bibliographicInfo": { "supplierUniqueRecordId": "return-{{supplierSymbol}}::{{itemId}}", @@ -45,7 +44,8 @@ script:post-response { test("Capture patron request id", () => { expect(responseData.id).to.be.a("string").and.not.empty; bru.setVar("prId", responseData.id); - expect(responseData.bibRecordId).to.equal("9a18bd66-8db6-4afd-95e6-157b2d3b6d12"); + expect(responseData.bibRecordId).to.equal(responseData.illRequest.bibliographicInfo.supplierUniqueRecordId); + bru.setVar("bibRecordId", responseData.bibRecordId); }); } diff --git a/bruno/crosslink/PR Happy flow/Find lending PR id.bru b/bruno/crosslink/PR Happy flow/Find lending PR id.bru index 4d66d7bc7..b00ea2c3c 100644 --- a/bruno/crosslink/PR Happy flow/Find lending PR id.bru +++ b/bruno/crosslink/PR Happy flow/Find lending PR id.bru @@ -39,7 +39,7 @@ script:post-response { // for cleanup/diagnostics even if the assertion fails. bru.setVar("lendingPrId", lendingPr.id); expect(lendingPr.lastActionResult).to.equal("SUCCESS") - expect(lendingPr.bibRecordId).to.equal("9a18bd66-8db6-4afd-95e6-157b2d3b6d12"); + expect(lendingPr.bibRecordId).to.equal(bru.getVar("bibRecordId")); const recordIds = lendingPr.illRequest.bibliographicInfo.bibliographicRecordId; const sharedRecord = recordIds.find(record => record.bibliographicRecordIdentifierCode["#text"] === "ReShare"); expect(sharedRecord.bibliographicRecordIdentifier).to.equal(lendingPr.bibRecordId);