IN-PROGRESS: Simply 3661 registry search #190
Conversation
|
@leonardr - This is a big set of diffs (there are a number of formatting changes that make the overall diff pretty big), but here are the pieces to look at around geographic search:
I'll flag individual pieces with comments. |
| return decorated | ||
|
|
||
|
|
||
| def uses_location(f): |
There was a problem hiding this comment.
Decorator that looks for location, tries to store it in g.location as a Location instance.
| # Decrease the score based on how far away the library's focus area is. | ||
| score = score * exponential_decrease(1.0 * focus_area_distance_factor * focus_min_distance) | ||
| @classmethod | ||
| def nearby(cls, db_session, target, max_radius=150, production=True): |
There was a problem hiding this comment.
New version of Library.nearby()
| """Raised when a Location is created with invalid input""" | ||
|
|
||
|
|
||
| class Location: |
There was a problem hiding this comment.
New Location class
|
|
||
| GENERATED_SHORT_NAME_REGEX = re.compile(r'^[A-Z]{6}$') | ||
|
|
||
| ############################################################################## |
There was a problem hiding this comment.
Explanation of geographic test fixtures
| db_session.delete(a_library) | ||
| db_session.delete(a_place) | ||
| db_session.commit() | ||
|
|
There was a problem hiding this comment.
Tests of Library.nearby() start here
|
|
||
| return f(*args, **kwargs) | ||
|
|
||
| return decorated |
There was a problem hiding this comment.
Is this duplicative of uses_location_factory in app_helper.py?
|
|
||
| return response | ||
|
|
||
| return decorated |
There was a problem hiding this comment.
I'm getting the impression that you've moved the app_helpers into this file and will be removing or greatly cutting app_helpers.
There was a problem hiding this comment.
Right--I'll remove them from app_helpers once I confirm that nothing uses the older versions.
model.py
Outdated
| """Constant container for library types. | ||
| This is as defined here: | ||
| https://github.com/NYPL-Simplified/Simplified/wiki/LibraryRegistryPublicAPI#the-subject-scheme-for-library-types | ||
| """ |
There was a problem hiding this comment.
Should this be moved into constants.py?
| # use the point-to-polygon-edge distance to order our result set. | ||
| query_obj = query_obj.add_columns(meters_to_near_edge) | ||
| query_obj = query_obj.group_by(Library.id) | ||
| query_obj = query_obj.order_by(meters_to_near_edge.asc()) |
There was a problem hiding this comment.
Just a note for later unless you fixed this, but this GROUP BY might explain why libraries sometimes show up twice in the results -- they have two different service areas that both match the criteria.
There was a problem hiding this comment.
Oh, interesting. I'll have to figure out how to consolidate that and test for it. Good catch.
| [(focus_areas_subquery.c.type==Place.EVERYWHERE, literal_column(str(510000000000000)))], | ||
| else_=func.ST_Area(focus_areas_subquery.c.geometry) | ||
| ) | ||
| ) / 1000000 |
There was a problem hiding this comment.
Since we are handling supralocal libraries separately I'm OK with removing this.
| goal=ExternalIntegration.DRM_GOAL | ||
| ) | ||
| integration.setting(Configuration.ADOBE_VENDOR_ID).value = "VENDORID" | ||
| integration.setting(Configuration.ADOBE_VENDOR_ID_NODE_VALUE).value = "0x685b35c00f05" |
There was a problem hiding this comment.
Just confirming this is a made-up value.
There was a problem hiding this comment.
So far as I know, though I didn't make it up myself--I just ported it directly into a fixture from what's in here:
tests/test_util_search.py
Outdated
| * If truncating to fewer total characters than MAX_SEARCH_STRING_LEN, | ||
| the string should not end in a partial word. | ||
| """ | ||
| assert LibrarySearchQuery._normalize_search_string(search_string) == result |
There was a problem hiding this comment.
We've got code to correct the most common spelling of "library" ("libary") which isn't tested here; the code may not be in _normalize_search_string but I think it should be.
There was a problem hiding this comment.
Weird, I remember adding that at some point because I'd seen it in the existing code. I'll make sure it gets back in.
| return (latitude, longitude, srid) | ||
|
|
||
| @classmethod | ||
| def location_in_ocean(cls, location): |
There was a problem hiding this comment.
Do you plan to make use of this outside of tests?
There was a problem hiding this comment.
I was going to performance test it and use it if it's a cheap operation, which I think it should be. I just wanted some very simple protection against having to do the more expensive geographic operations on obviously unusable locations. Possibly a premature / unnecessary optimization, can take it out if it seems silly.
|
Just to set a baseline, I've reviewed the code so far. For the sake of unblocking mobile development, I think it's perfectly fine to merge this without any working search functionality -- just the proximity matching -- since the mobile apps currently don't use search. |
This is not ready for merge, it's a partial, in-progress view of the work I've done so far on search for the finder. In particular it's so we can discuss the geographic search portion, which is for SIMPLY-3662.