School district auto-complete API - #18890
#18890School district auto-complete API#18890jopolsky merged 3 commits intostagingcode-dot-org/code-dot-org:stagingfrom school-district-autocompletecode-dot-org/code-dot-org:school-district-autocompleteCopy head branch name to clipboard
Conversation
There was a problem hiding this comment.
Why is this different from the MIN_QUERY_LENGTH in school_autocomplete?
There was a problem hiding this comment.
We can probably remove MIN_QUERY_LENGTH from school_autocomplete, I just didn't want to mess with that existing code much.
There was a problem hiding this comment.
The get_matches code for school_autocomplete and school_district_autocomplete is almost identical. Can it be moved into the base class?
There was a problem hiding this comment.
Almost but one does the zip code search.
There was a problem hiding this comment.
If that's the only difference, then perhaps it could still be common code with a parameter for the included search fields as a string array, which could be injected into the query with ? and then included_fields.join(',')
There was a problem hiding this comment.
Well it's right in the middle. I think it would be preferable to see how the auto-complete evolves before refactoring and adding complexity around conditionals. I can take another look later today if I still have time as well to see if there is any more opportunity to refactor.
There was a problem hiding this comment.
Do you need this constraint? I see it's specified on the next line too, but if I'm reading it correctly it's asking for at least one character that's not a /. Isn't that how Rails would parse the path anyway? I don't think this does anything.
I'm fairly certain that, for instance, districtsearch/foo/bar would already parse into params {q: 'foo', limit: 'bar'}.
There was a problem hiding this comment.
@drewsamnick can you comment on this comment? This was added after I did the initial school auto-complete.
There was a problem hiding this comment.
The constraint was put in place for schools to allow matching on a period. By default it won't match and that caused errors when searching for things like "St. Paul"
There was a problem hiding this comment.
See the comment on line 535-536.
There was a problem hiding this comment.
Gotcha. Since that is pretty far down in the linked constraint description, let's add a small comment about the purpose too, e.g. "Constrain the q param to allow dots" before that link
3c9b3d3 to
bd2589c
Compare
6088cba to
7f274d2
Compare
API and implementation for school districts auto-complete.