[Issue #3464] Better agency display/sort in search #3738
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,19 +4,20 @@ import { fetchOpportunitySearch } from "src/services/fetch/fetchers/fetchers"; | |
| import { | ||
| PaginationOrderBy, | ||
| PaginationRequestBody, | ||
| PaginationSortOrder, | ||
| QueryParamData, | ||
| SearchFetcherActionType, | ||
| SearchFilterRequestBody, | ||
| SearchRequestBody, | ||
| } from "src/types/search/searchRequestTypes"; | ||
|
|
||
| const orderByFieldLookup = { | ||
| 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. |
||
| postedDate: ["post_date"], | ||
| closeDate: ["close_date"], | ||
| }; | ||
|
|
||
| type FrontendFilterNames = | ||
|
|
@@ -98,24 +99,38 @@ export const buildPagination = ( | |
| ? 1 | ||
| : page; | ||
|
|
||
| let sort_order: PaginationSortOrder = [ | ||
| { order_by: "relevancy", sort_direction: "descending" }, | ||
| { order_by: "post_date", sort_direction: "descending" }, | ||
| ]; | ||
|
|
||
| if (sortby) { | ||
| sort_order = []; | ||
| // this will need to change in a future where we allow the user to set an ordered set of sort columns. | ||
| // for now we're just using the multiple internally behind a single column picker drop down so this is fine. | ||
| for (const [key, value] of Object.entries(orderByFieldLookup)) { | ||
| if (sortby.startsWith(key)) { | ||
| 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. |
||
| sortby && sortby.endsWith("Asc") ? "ascending" : "descending"; | ||
| value.forEach((item) => { | ||
| sort_order.push({ | ||
| order_by: <PaginationOrderBy>item, | ||
| sort_direction, | ||
| }); | ||
| if (item === "relevancy") { | ||
| sort_order.push({ | ||
| order_by: "post_date", | ||
| sort_direction, | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| page_offset, | ||
| page_size: 25, | ||
| sort_order, | ||
| }; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.