Open josh-chamberlain opened 4 months ago
This will be a simplified version of the current quick_search_query. All the potential fields for trying to match a search term in the where clause will be replaced by record_type = '{coarse_record_type}'
This will be a simplified version of the current quick_search_query. All the potential fields for trying to match a search term in the where clause will be replaced by record_type = '{coarse_record_type}'
@josh-chamberlain @mbodeantor So this will be replacing the quick search and associated API endpoint?
Additionally, will users only be able to search for one coarse record type at a time, or multiple?
@maxachis In order to maintain backwards compatibility, I would suggest a new endpoint with similar functionality
@maxachis let's let them multi-select coarse record types, why not? Even if the UI doesn't support it. Agreed about keeping the quick search endpoint—we are likely still going to have that option
@josh-chamberlain @mbodeantor PR has been created, incorporating both the API changes described here and #258 (which as I indicated there, is a very trivial add and is quick to reverse if nonfunctional). Next questions:
quick_search_query_logs
table. Should there be a search_query_logs
table as well, or a table that consolidates both Quick Search and Search?@maxachis
quick
and other
searches.@maxachis Any code you merge into the dev branch will get deployed in the dev instance of the API on DigitalOcean. It uses the same database, but is otherwise separate from the live prod app
@mbodeantor Once I get further along in setting up the dev database, it may be prudent to switch the dev instance to point to the dev database. That probably also encourages us to copy the data as well as the schema to the dev database, to better emulate it.
Hey @josh-chamberlain I'm pretty close to starting on the updated UI here. A couple questions:
we should structure the URL like /search/{record_type}/{state}/{county}/{muni}
- Would it make more sense to pass
record_type
,state
,county
, andmunicipality
, as well assearchId
, as data in the request body, rather than trying to add all of them to the URL string? Particularly since all of those fields could benull
when we pass a saved searchsearchId
, it seems less complex to use the request body here.
For instance, if we hit /search/ohio
or /search/9b7ddde7-5871-4f45-a8a1-3dce52847736
, the API would need to check the value against some static list of states to know whether state
or searchId
were being passed here, no?
@joshuagraber nice!
yes, @maxachis is working on the back end, potentially with help from others. If you would like to be involved, that can work, as the front end is the primary consumer of the API. It's mostly happening on this fork: https://github.com/Police-Data-Accessibility-Project/data-sources-app-v2
Good point about the URL structure.
search/id/123
? search/oh/hamilton/cincinnati
.search/oh/hamilton/cincinnati?=arrests
muni
, the state
and county
are just inferred—so it's really just record type
and location
. It won't be possible to create other kinds of searches yet.Ah, gotcha @josh-chamberlain, that all makes sense to me.
If you have a muni, the state and county are just inferred—so it's really just record type and location. It won't be possible to create other kinds of searches yet.
I think we will have edge cases here. I just did a basic filtering of all data source records and it shows that we have duplicate municipality names in different states. But I suppose as long as we're okay with that, it's nbd.
As far as flexibility of API and sharability of links, the easiest path forward here, to me, is to use query params.
So that way/search?state=oh&county=franklin&muni=cincinnati&term=arrests
, /search?muni=cincinnati&term=arrests
, /search?term=arrests
could all be valid requests, and we don't have to worry about ugliness like /search/undefined/undefined/cincinatti/arrests
or w/e
@joshuagraber yes, the duplicate muni names are why you would need the /state/county as a disambiguation.
To be clear: The goal of the typeahead is to treat places
as objects
and stop trying to match on strings. I would like to not allow people to create /undefined/undefined/cincinnati
—we should only let people find a municipality object, which would require having the state and county. One problem we found with quick search was that people would search for a term like "Dallas" and get results from every "Dallas" in the country, rather than the typeahead helping them choose between Dallases.
That said, you quickly get into trouble if you let someone search for multiple record types (or, more accurate to the wireframe, record type categories). So far, I think the cleanest way is still /state/county/muni?category={record_type_category}.
@joshuagraber @josh-chamberlain From the backend perspective, creating a typeahead endpoint will be fun! But it will be fun in the same way running the Pittsburgh Half Marathon was fun.
I've never developed a typeahead endpoint, which automatically adds to the fun.
Additional points of fun:
Cincinnati
is a great example, because I would probably get it wrong on my first attempt. We may want to ignore that for now, as it adds a lot of additional dimensions.The goal of the typeahead is to treat places as objects and stop trying to match on strings.
@josh-chamberlain gotcha, that makes sense. Sorry, I think the design is a bit misleading to me in this respect, because it appears that the user can select state / county / municipality, then search.
That said, you quickly get into trouble if you let someone search for multiple record types
Hmm, I think I also misread the design here. So rather than checkboxes, these will need to be radio buttons with only one selectable, no? I'll need to make another update to design-system
. Will create an issue for that shortly.
How do we handle possible misspellings?
@maxachis would this library be potentially helpful? I know this is more or less Google's strategy as well, although their algorithm is almost certainly proprietary.
Also, we probably want to implement debouncing, which will make sure we wait for some delay before we start sending requests.
Yes, debouncing is a best practice in client engineering for this type of work. I won't bother implementing it though, because we use lodash utils, and their debounce function is well tested and consistently maintained.
When should typeahead kick in?
I think we should just wait for the debounced function to fire a request, rather than set a hardcoded number of characters.
We may need to implement rate limiting, because otherwise we might send a lot of requests.
Unless this endpoint will be available publicly, we shouldn't strictly need rate limiting if we're debouncing properly on the client. But if you feel like doing it, I suppose it's a good performance guardrail anyways.
@joshuagraber
the design is a bit misleading
Yes—the design has not yet been updated to reflect that we want to do a typeahead. Sorry about that. Vikram has responded and will be back online starting ~Monday to make edits
multiple record types
Sorry again for the confusion! I think the design is correct, and I was just thinking out loud—we should allow multi-select. Current favorite answer is a url path for /search/state/county/muni/?type={args}
so that we have a nice solid URL for geography (most common search facet) with room for flexible args. Hopefully that works...I updated the issue.
misspellings
IMO, typeahead helps here—if someone gets a "no locations found" message, that's a clue that they mistyped. We can get better at this over time.
I think Joshua covered the rest. Thanks for helping think it through, y'all
@josh-chamberlain Ah, gotcha, that makes more sense now, thank you for explaining.
Current favorite answer is a url path for
/search/state/county/muni/?type={args}
so that we have a nice solid URL for geography (most common search facet) with room for flexible args
I like that. We could just pass an array of record type values: /search/state/county/muni/?types={serialized record_type[]}
IMO, typeahead helps here—if someone gets a "no locations found" message, that's a clue that they mistyped. We can get better at this over time.
True enough, and if we collect a db of search queries by session, it could be an interesting problem to throw at ML.
Putting together potentially useful articles on the matter:
@josh-chamberlain Regarding pulling locality from the agencies
table, what column would that be? municipality
? name
?
@maxachis yep, municipality
is what we currently call that. switching to locality
is generally more inclusive; for now, we can have a front end and back end terminology adjustment, which is not the end of the world, but creates tech/information debt we should still fix: https://github.com/Police-Data-Accessibility-Project/data-sources-app/issues/332
@joshuagraber @josh-chamberlain Just to confirm -- debouncing is done on the frontend through the Iodash
library that Joshua mentioned, correct? So no need to do that on the backend?
@maxachis @josh-chamberlain Yes, we'll debounce client-side, but IMO it might not hurt to add some rate limiting on the API side as well (for all endpoints, really) if it's not too big of a lift, just as a protective measure against a surprise hosting bill.
rate limiting: #353
part of #248
see wireframe
Front end
places
as objects people can predictably search, not strings we have to try to match.states
andcounties
table, withlocality
from the agencies table.contains
(if the endpoint supports that)Al
yieldsAlabama
beforeTopal County
No results found. Search for "string"?
and load quick search resultsAPI changes