[Issue 2448] use v1 search endpoint and frontend fetch pattern refactors #2518
Conversation
| @@ -8,6 +8,7 @@ | |||
| "scripts": { | |||
| "build": "next build", | |||
| "dev": "next dev", | |||
| "debug": "NODE_OPTIONS='--inspect' next dev", | |||
There was a problem hiding this comment.
I was not able to get this to work to use the chrome node debugger, and ended up using the VS code debugger, which did work to debug node based / server side next functions. If anyone else can get this to work, we should probably keep this, or we can just get rid of it.
see vercel/next.js#48767 and https://nextjs.org/docs/pages/building-your-application/configuring/debugging
| @@ -15,32 +15,26 @@ import { | |||
| ValidationError, | |||
| } from "src/errors"; | |||
| import { QueryParamData } from "src/services/search/searchfetcher/SearchFetcher"; | |||
| // TODO (#1682): replace search specific references (since this is a generic API file that any | |||
There was a problem hiding this comment.
I believe I've accomplished this now, though the ticket mentioned here was already closed.
| // Root path of API resource without leading slash. | ||
| abstract get basePath(): string; | ||
| // Root path of API resource without leading slash, can be overridden by implementing API classes as necessary | ||
| get basePath() { |
There was a problem hiding this comment.
since we are using the same basePath everywhere, I figured we may as well define it at a higher level
| return headers; | ||
| } | ||
|
|
||
| /** | ||
| * Send an API request. | ||
| */ | ||
| async request( | ||
| async request<ResponseType extends APIResponse>( |
There was a problem hiding this comment.
use a generic to get around having to hardcode search specific typing
| @@ -149,14 +132,14 @@ export function createRequestUrl( | |||
| let url = [...cleanedPaths].join("/"); | |||
| if (method === "GET" && body && !(body instanceof FormData)) { | |||
| // Append query string to URL | |||
| const body: { [key: string]: string } = {}; | |||
| const newBody: { [key: string]: string } = {}; | |||
There was a problem hiding this comment.
I think this was completely broken before, but was unused so nobody noticed
| method: ApiMethod, | ||
| basePath: string, | ||
| namespace: string, |
There was a problem hiding this comment.
we can reference these as class variables, no need to pass in
| eligibility: "applicant_type", | ||
| agency: "agency", | ||
| category: "funding_category", | ||
| } as const; |
There was a problem hiding this comment.
these types are all to support the refactor of buildFilters, which is... admittedly a lot of scope creep for this ticket
| async searchOpportunities(searchInputs: QueryParamData) { | ||
| const { query } = searchInputs; | ||
| const filters = this.buildFilters(searchInputs); | ||
| const pagination = this.buildPagination(searchInputs); |
There was a problem hiding this comment.
these function do not reference the class at all, so can be split out
| if (sortby) { | ||
| sort_direction = sortby?.endsWith("Desc") ? "descending" : "ascending"; | ||
| } | ||
| const sort_direction = |
There was a problem hiding this comment.
the diff is tough here since I removed these functions from the class, but the only real change I made to the pagination function is here, simplifying the sort_direction logic slightly.
| }); | ||
|
|
||
| describe("searchOpportunities", () => { | ||
| beforeEach(() => { | ||
| global.fetch = mockFetch({}); |
There was a problem hiding this comment.
this test was mocking down to the fetch level, while the existing opportunity listing API test was just mocking the request function on the class. In the interest of using inheritance to our benefit and keeping the tests somewhat uniform, I changed this to use the request mocking approach, and left the fetch mocking approach for the base class
doug-s-nava
force-pushed
the
dschrashun/2448-v1-search-endpoint
branch
2 times, most recently
from
2695b69
to
b41c04c
Compare
… timing racy condition thing with promise.all
doug-s-nava
force-pushed
the
dschrashun/2448-v1-search-endpoint
branch
from
b41c04c
to
2915e8b
Compare
Summary
Fixes #2448
Time to review: 45 mins
Changes proposed
DISCLAIMER: there is a lot of scope creep in this PR. Feel free to push back on anything you feel goes to far here.
The primary purpose of this change is to change the search API to use the
v1version of the search endpoint in order to activate the use of Open Search on the backend. This purpose is accomplished in the first commit, which can be split into a separate PR or commit if called for.The rest of the PR contains refactors to the
APIfetch system that:Context for reviewers
Test Steps
Additional information