[Issue #3464] Better agency display/sort in search #3738
Conversation
…at the API expects as sorting fields
mdragon
requested review from
acouch,
andycochran,
btabaska,
doug-s-nava and
chouinar
as code owners
| agency: "agency_code", | ||
| postedDate: "post_date", | ||
| closeDate: "close_date", | ||
| relevancy: ["relevancy"], |
There was a problem hiding this comment.
Shouldn't this also have post_date? I see you had a default pagination sort order below, but if someone intentionally chose relevancy (not sure how that links / if that's possible) you'd lose the post_date bit, right?
| relevancy: ["relevancy"], | |
| relevancy: ["relevancy", "post_date"], |
There was a problem hiding this comment.
Yes, I'm torn on how to best handle the default and the post_date backup. Part of me thinks we should always have a post_date fallback, so even if sorting by agency, we show the ones closing sooner earlier amongst the peers from that same agency. But I think In the implementation you'll see that if it's hitting on relevancy it's adding the post_date backup in the pagination builder. But agree, I'm not sure if that's the right way, except to note, anything we default here, doesn't affect the "no sort" route which is possible and does happen (primarily if you've just never touched sort upon landing on the page).
There was a problem hiding this comment.
[tldr: I'm good with this change as is] it seems like based on the changes to the fetcher, the default is being set the way we want with the secondary sort. Beyond that, I feel like if users want to update the sort away from the default I think it's best to not implement a hidden secondary sort behind the scenes at the moment.
| relevancy: ["relevancy"], | ||
| opportunityNumber: ["opportunity_number"], | ||
| opportunityTitle: ["opportunity_title"], | ||
| agency: ["top_level_agency_name", "agency_name"], |
There was a problem hiding this comment.
To make this fully work, we'd also need the backend to always populate top level agency name, as long as you're making the API changes, could add the fallback to the property on the ORM model
There was a problem hiding this comment.
@chouinar Is this what you mean by the fallback? I think this is saying to not return the top level if the agency is set, so this change should make it always populate when top level is available?
There was a problem hiding this comment.
Don't modify that line, you'll hit a null pointer error if you do. That full check needs to be there to check if the agency record exists first, and then if the top level agency exists off of that.
Add a new if statement below it that is:
if self.agency_record is not None:
return self.agency_record.agency_nameThere was a problem hiding this comment.
Right, thanks. I realized I was thinking about the Not None backwards after I asked you here.
| for (const [key, value] of Object.entries(orderByFieldLookup)) { | ||
| if (sortby.startsWith(key)) { | ||
| order_by = value as PaginationOrderBy; | ||
| break; // Stop searching after the first match is found | ||
| const sort_direction = |
There was a problem hiding this comment.
I feel like I'm missing why this was moved into the body of the for loop
There was a problem hiding this comment.
I was originally going to go bigger and try to support multiple arguments via the URL argument so we'd need to be parsing each argument for it's direction. I stopped short of that since we have no plans for how the UI for that would work, and it would have required a more fundamental overhaul of what this was already doing.
| agency: "agency_code", | ||
| postedDate: "post_date", | ||
| closeDate: "close_date", | ||
| relevancy: ["relevancy"], |
There was a problem hiding this comment.
[tldr: I'm good with this change as is] it seems like based on the changes to the fetcher, the default is being set the way we want with the secondary sort. Beyond that, I feel like if users want to update the sort away from the default I think it's best to not implement a hidden secondary sort behind the scenes at the moment.
Summary
Fixes #3464
Time to review: 10 mins
🚨🚨🚨 Diffing against Michael's API change branch for now to highlight the further changes I made, but we will probably merge direct to main not into his branch to go to main.
Changes proposed
Added support for the API change that takes an array of sort criteria instead of individual arguments for filed and direction
Additional information
With some logging in the API layer I was able to demonstrate we were getting the correct format passed in from the FE and results sorted as expected:
Default/relevancy sort:
sort_order: [SortOrderParams(order_by='relevancy', sort_direction=<SortDirection.DESCENDING: 'descending'>), SortOrderParams(order_by='post_date', sort_direction=<SortDirection.DESCENDING: 'descending'>)]Agency sort:
sort_order: [SortOrderParams(order_by='top_level_agency_name', sort_direction=<SortDirection.DESCENDING: 'descending'>), SortOrderParams(order_by='agency_name', sort_direction=<SortDirection.DESCENDING: 'descending'>)]