Conversation
adamdickmeiss
changed the title
There was a problem hiding this comment.
Pull request overview
Adds support for passing lender action parameters (via EventData.CustomData) through the action service so outgoing ISO18626 SupplyingAgencyMessages can include fields like note, offeredCosts, and loanCondition.
Changes:
- Introduces
ActionParamsand decodesEventData.CustomDatainto it inhandleLenderAction. - Populates ISO18626
MessageInfo.Note,MessageInfo.OfferedCosts, andDeliveryInfo.LoanConditionfor selected lender actions. - Extends tests to pass
CustomDataand assert the resulting ISO18626 message contents.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| broker/patron_request/service/action.go | Parses action parameters from CustomData and includes them in outgoing SupplyingAgencyMessages (note/costs/loan condition). |
| broker/patron_request/service/action_test.go | Updates/extends lender action tests to supply CustomData and verify outgoing ISO18626 message fields. |
| func (a *PatronRequestActionService) addConditionsLenderRequest(ctx common.ExtendedContext, pr pr_db.PatronRequest, actionParams ActionParams) actionExecutionResult { | ||
| var offeredCosts *iso18626.TypeCosts | ||
| if actionParams.Cost != nil { | ||
| if actionParams.Currency == "" { | ||
| status, result := a.logErrorAndReturnResult(ctx, "currency is required when cost is provided", nil) | ||
| return actionExecutionResult{status: status, result: result, pr: pr} | ||
| } | ||
| _, costBase, costExp := utils.ExtractDecimal(strconv.FormatFloat(*actionParams.Cost, 'f', -1, 64), -1) | ||
| offeredCosts = &iso18626.TypeCosts{ | ||
| CurrencyCode: iso18626.TypeSchemeValuePair{Text: actionParams.Currency}, | ||
| MonetaryValue: utils.XSDDecimal{Base: costBase, Exp: costExp}, | ||
| } |
There was a problem hiding this comment.
ActionParams.Cost is modeled as a float64 and then converted to an XSD decimal via FormatFloat/ExtractDecimal. Using binary floating point for money can produce surprising decimal expansions (and therefore incorrect Base/Exp) for some inputs. Consider accepting cost as a string/decimal representation (or as explicit base/exp) and converting deterministically, to avoid rounding artifacts.
| type ActionParams struct { | ||
| Note string `json:"note,omitempty"` | ||
| LoanCondition string `json:"loanCondition,omitempty"` | ||
| Cost *float64 `json:"cost,omitempty"` | ||
| Currency string `json:"currency,omitempty"` | ||
| ReasonUnfilled string `json:"reasonUnfilled,omitempty"` | ||
| } |
There was a problem hiding this comment.
ActionParams.Cost is modeled as *float64, and later converted via FormatFloat -> ExtractDecimal. Using floating point for monetary values can introduce rounding artifacts (e.g., 12.34 may format to a longer representation), resulting in incorrect MonetaryValue being sent. Consider representing cost as a decimal-safe type in params (e.g., string/json.Number and parse, or accept base/exp directly) to preserve exact values.
| note := actionParams.Note + encodeItemsNote(items) | ||
| result := events.EventResult{} | ||
| status, eventResult, httpStatus := a.sendSupplyingAgencyMessage(ctx, pr, &result, | ||
| iso18626.MessageInfo{ReasonForMessage: iso18626.TypeReasonForMessageStatusChange, Note: note}, | ||
| iso18626.StatusInfo{Status: iso18626.TypeStatusLoaned}) | ||
| iso18626.MessageInfo{ | ||
| ReasonForMessage: iso18626.TypeReasonForMessageStatusChange, | ||
| Note: note, | ||
| }, |
There was a problem hiding this comment.
When composing the shipped-message note, actionParams.Note is concatenated directly with the #MultipleItems#... payload. If a human note is provided, consider inserting a delimiter (e.g., newline) before the encoded items block to keep the note readable and consistent with other code paths that prepend notes before item markers.
| iso18626.MessageInfo{ | ||
| ReasonForMessage: iso18626.TypeReasonForMessageNotification, | ||
| Note: shim.RESHARE_ADD_LOAN_CONDITION, // TODO add action params | ||
| Note: actionParams.Note + shim.RESHARE_ADD_LOAN_CONDITION, | ||
| OfferedCosts: offeredCosts, | ||
| }, |
There was a problem hiding this comment.
actionParams.Note is concatenated directly with the #ReShareAddLoanCondition# marker. If a note is provided, add a separator (space/newline) before the marker to avoid the marker being glued to the last word of the note and to keep the combined field easier to read/debug.
part 1 of https://index-data.atlassian.net/browse/CROSSLINK-236