CROSSLINK-210 Add more search parameters#434
Conversation
There was a problem hiding this comment.
Pull request overview
Adds additional patron request search/filter capabilities by extending the patron_request schema and introducing a search view used by the list endpoint/CQL filtering. Also tightens supplier/requester lookup to include “side” and exposes the new needsAttention field through the API.
Changes:
- Add
needs_attentioncolumn and introducepatron_request_search_viewwith derived search fields. - Switch patron request listing to read from the new view and expand supported CQL fields.
- Update
GetPatronRequestBySupplierSymbolAndRequesterReqIdto includeside, and propagate through service/tests; exposeneedsAttentionin API responses.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| broker/test/patron_request/api/api-handler_test.go | Validates API returns default needsAttention=false; updates repo lookup call to include side. |
| broker/sqlc/pr_schema.sql | Adds needs_attention and defines patron_request_search_view used for searching. |
| broker/sqlc/pr_query.sql | Lists patron requests from the new view; adds needs_attention to create/update; adds side constraint to supplier+requesterReqId lookup. |
| broker/patron_request/service/message-handler.go | Updates supplier-side lookups to include SideLending. |
| broker/patron_request/service/action_test.go | Updates mock repo method signature to include side. |
| broker/patron_request/db/prrepo.go | Adapts list scanning/mapping to new list row shape; adds side parameter to lookup. |
| broker/patron_request/db/prcql.go | Adds additional CQL fields for filtering patron requests. |
| broker/patron_request/api/api-handler.go | Exposes needsAttention in API responses; ensures RequesterReqID is set on create. |
| broker/oapi/open-api.yaml | Documents new needsAttention field and CQL-filterable fields on GET /patron_requests. |
| broker/migrations/019_add_attention_field.up.sql | Migration adds needs_attention and creates the search view. |
| broker/migrations/019_add_attention_field.down.sql | Migration rollback drops the column and view. |
You can also share your feedback on Copilot code review. Take the survey.
| @@ -123,7 +123,7 @@ func (m *PatronRequestMessageHandler) getPatronRequest(ctx common.ExtendedContex | |||
| return m.prRepo.GetPatronRequestById(ctx, msg.RequestingAgencyMessage.Header.SupplyingAgencyRequestId) | |||
There was a problem hiding this comment.
@JanisSaldabols this function should also ensure the side
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
| f = pgcql.NewFieldString().WithExact() | ||
| def.AddField("needs_attention", f) | ||
|
|
||
| f = pgcql.NewFieldString().WithExact() | ||
| def.AddField("has_notification", f) | ||
|
|
||
| f = pgcql.NewFieldString().WithExact() | ||
| def.AddField("has_cost", f) | ||
|
|
||
| f = pgcql.NewFieldString().WithExact() | ||
| def.AddField("has_unreaded_notification", f) |
There was a problem hiding this comment.
The CQL fields needs_attention, has_notification, has_cost, and has_unreaded_notification are boolean columns in patron_request_search_view, but they’re registered with pgcql.NewFieldString().WithExact(). Depending on how pgcql binds query arguments, this can produce boolean = text comparisons (and runtime SQL errors) when filtering. Consider using (or adding) a boolean field generator that parses the term into a bool and binds a boolean argument (or explicitly casts in the generated SQL).
| def.AddField("has_cost", f) | ||
|
|
||
| f = pgcql.NewFieldString().WithExact() | ||
| def.AddField("has_unreaded_notification", f) |
There was a problem hiding this comment.
has_unreaded_notification uses an incorrect word (“unreaded”). Since this name is now part of the public CQL field list, it’s worth renaming before it becomes a stable API surface (e.g., has_unread_notification), and updating the view alias and OpenAPI docs accordingly.
| def.AddField("has_unreaded_notification", f) | |
| def.AddField("has_unread_notification", f) |
| SELECT 1 | ||
| FROM notification n | ||
| WHERE n.pr_id = pr.id and acknowledged_at is null | ||
| ) AS has_unreaded_notification, |
There was a problem hiding this comment.
The view exposes a column named has_unreaded_notification, but “unreaded” is not correct English. Since this becomes part of the search surface (CQL field name), consider renaming the alias (e.g., has_unread_notification) before clients start depending on it.
| ) AS has_unreaded_notification, | |
| ) AS has_unread_notification, |
No description provided.