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.
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.
# Conflicts: # broker/patron_request/db/prrepo.go # broker/patron_request/service/action_test.go # broker/patron_request/service/message-handler.go # broker/patron_request/service/message-handler_test.go # broker/sqlc/pr_query.sql # broker/test/patron_request/api/api-handler_test.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
| prToUpdate, err := a.prRepo.GetPatronRequestById(ctx, pr.ID) | ||
| if err != nil { | ||
| ctx.Logger().Error("failed to read patron request", "error", err) | ||
| return | ||
| } | ||
| prToUpdate.NeedsAttention = true | ||
| _, err = a.prRepo.UpdatePatronRequest(ctx, pr_db.UpdatePatronRequestParams(prToUpdate)) |
There was a problem hiding this comment.
setNeedsAttention reads the patron request and then calls the full-row UpdatePatronRequest. Because UpdatePatronRequest updates all columns, this introduces a lost-update risk if another concurrent operation updates the same patron_request between the read and this update (those changes could be overwritten). Prefer a single atomic update that only sets needs_attention = true (e.g., a dedicated sqlc query/update method), or update using a WHERE + partial update pattern to avoid overwriting unrelated fields.
| prToUpdate, err := a.prRepo.GetPatronRequestById(ctx, pr.ID) | |
| if err != nil { | |
| ctx.Logger().Error("failed to read patron request", "error", err) | |
| return | |
| } | |
| prToUpdate.NeedsAttention = true | |
| _, err = a.prRepo.UpdatePatronRequest(ctx, pr_db.UpdatePatronRequestParams(prToUpdate)) | |
| if pr.NeedsAttention { | |
| // Already marked; avoid unnecessary update. | |
| return | |
| } | |
| pr.NeedsAttention = true | |
| _, err := a.prRepo.UpdatePatronRequest(ctx, pr_db.UpdatePatronRequestParams(pr)) |
| EXISTS ( | ||
| SELECT 1 | ||
| FROM notification n | ||
| WHERE n.pr_id = pr.id | ||
| ) AS has_notification, |
There was a problem hiding this comment.
The new patron_request_search_view computes has_notification/has_cost/has_unread_notification via correlated EXISTS subqueries on notification. There doesn’t appear to be an index on notification.pr_id, so these checks can degrade list performance significantly (especially with large offsets). Consider adding CREATE INDEX on notification(pr_id) and (optionally) partial indexes for the cost IS NOT NULL and acknowledged_at IS NULL predicates.
No description provided.