📍 Use the server-side geocoded addresses for place names #1166
Conversation
Since the server now fills in geocoded_address (we may later change this to reverse_geocode), we will only need to perform a client-side nominatim request for unprocessed trips. If either field is present, we'll use that; otherwise schedule a request like before. Updated types: -Since the CompositeTrip type definition is based on the return type of Timeline.readAllCompositeTrips, which is after "unpacking", start_confirmed_place and end_confirmed_place are simply ConfirmedPlace, not BEMData<ConfirmedPlace> -ConfirmedPlace.location is a `Point`, (having `coordinates`) not just a generic `Geometry` -added geocoded_address and reverse_geocode, both as optional since it depends on e-mission/e-mission-server#973
|
This PR works with either If e-mission/e-mission-server#973 is merged first, I can update this PR to only use |
JGreenlee
changed the title
|
I can merge e-mission/e-mission-server#973 soon. I am just waiting to merge the automatic build PRs (e.g. e-mission/op-admin-dashboard#113) so that I could see it work! All the public PRs are merged, the internal ones need to be fixed @MukuFlash03 |
|
Note that even after e-mission/e-mission-server#973 is merged, the older trips will still have only the We should then also write a migration script to copy over all of the |
That is what this PR already does because I anticipated that we may need to support both fields.
Actually, this no longer has anything to do with the footprint calculations. I wanted to use the zip codes, but they aren't reliable enough. Sometimes they are missing from the response, and sometimes there isn't a clear mapping to the urban area or eGRID region. I do think we should write the migration script, but it isn't blocking footprint calculations if we are ok with having both fields temporarily. |
|
Ah I am clearly not on top of the changes to the carbon footprint calculations. |
| const data = place.reverse_geocode || { address: place.geocoded_address }; | ||
| localStorage.setItem(coordsStr, JSON.stringify(data)); | ||
| publish(coordsStr, JSON.stringify(data)); |
There was a problem hiding this comment.
nice! great idea to reuse the data from prior mappings even for trips that don't have the address instead of looking it up.
| @@ -96,7 +102,7 @@ export type CompositeTrip = { | |||
| confirmed_trip: ObjectId; | |||
| distance: number; | |||
| duration: number; | |||
| end_confirmed_place: BEMData<ConfirmedPlace>; | |||
| end_confirmed_place: ConfirmedPlace; | |||
There was a problem hiding this comment.
Curious about what this change does. Why did we have BEMData before and why don't we need it now?
There was a problem hiding this comment.
It was just incorrect. I noticed because Typescript complained when I accessed properties on the place objects while making the changes
Updated types:
-Since the CompositeTrip type definition is based on the return type of Timeline.readAllCompositeTrips, which is after "unpacking", start_confirmed_place and end_confirmed_place are simply ConfirmedPlace, not BEMData
-ConfirmedPlace.location is aPoint, (havingcoordinates) not just a genericGeometry
-added geocoded_address and reverse_geocode, both as optional since it depends on e-mission/e-mission-server#973
Since the server now fills in geocoded_address (we may later change this to reverse_geocode), we will only need to perform a client-side nominatim request for unprocessed trips. If either field is present, we'll use that; otherwise schedule a request like before.
Updated types:
-Since the CompositeTrip type definition is based on the return type of Timeline.readAllCompositeTrips, which is after "unpacking",
start_confirmed_placeandend_confirmed_placeare simplyConfirmedPlace, notBEMData<ConfirmedPlace>-ConfirmedPlace.location is a
Point, (havingcoordinates) not just a genericGeometry-added
geocoded_addressandreverse_geocode, both optional since it depends on e-mission/e-mission-server#973