First edit including lat long change doesn't trigger manual geocoding logic

adrian-lara commented 5 years ago

Expected Behavior

When a user edits an existing record and changes the latitude and longitude values, the Geocoding Confidence and long_lat should be updated.

Actual Behavior

In most cases, this does happen via a pre_save hook. But when a user edits a record for the first time, a completely new -State object is used in order for the originally imported data to persist. This new -State object does not get affected by the pre_save hook.

The affected record would not be pinned on the map page until the user triggers a "Geocode Selected" event on the record.

Also, if the intent of an edit is to remove geocoding, the user would not be able to remove the Geocoding confidence value with the initial edit. This is relevant in the case where a user wants to trigger geocoding by MapQuest for a previously manually geocoded record. This re-geocoding is only allowed on records that don't have a Geocoding confidence value of "Manually geocoded (N/A)".

Steps to Reproduce

Import a record without geocoding it - don't provide a MapQuest API key or don't import a file with address data. On the first edit of this record, change the lat and long. See that the geocoding confidence isn't populated.

Instance Information

all instances

Possible Solution

On first-edit, create a copy of the -State and run that copy through the existing not-first-edit code.

RDmitchell commented 4 years ago

@adrian-lara -- does your 2nd testing option work? Ie, importing a file without address info? Seems like that will generate a geocoding confidence value if you still have the API turned on? Anyway, I think I will test this with the API key absent.

adrian-lara commented 4 years ago

Even without an API key, you should still see the "Manually Geocoded..." within geocoding_confidence once you make that initial edit of populating latitude and longitude.

RDmitchell commented 4 years ago

Instance: dev1 SHA: 758cbdd5 Org: LBNL 201 Cycle: Geocoding test

@adrian-lara -- is this the behavior you are expecting? I couldn't quite tell from the testing desciption.

Imported sample ESPM file that has only Address Line 1 and City (neither are real)
No Mapquest API specified
File imported, Lat, Long, Geocoding confidence are all blank
Click on the "i" icon to go into the detail view of PM Property ID # 6798215 (first record in screenshot above) -- Lat/Long/Geocoding confidence fields are blank before editing
Click the Edit link, add values for Lat and Long
Click the Save Changes button
New values for Lat/Lon are in Master
Geocoding Confidence field is now set to "Manually Geocoded"

adrian-lara commented 4 years ago

Yes - this and the reverse - where a record was geocoded on import, and removing the latitude and longitude will clear out the geocoding confidence.

RDmitchell commented 4 years ago

Instance; dev1 SHA: 6a6d4ba0 Org: LBNL Test 200 Cycle: Lat Long Testing

See this doc https://docs.google.com/document/d/1AwRWe9kQDHWHNRgLnAufGCubmmFkAe3_NrYInTw_t_M/edit?usp=sharing

I am still working on this so it's not ready for prime time yet (RM 2/10/2020)

adrian-lara commented 4 years ago

I agree with the comment about the text in the confirmation modal.

Could you create a separate issue for that and tag me? I coded that modal initially, and I've got some thoughts on how to add more clarity without being overwhelming to read through.

RDmitchell commented 4 years ago

just created a new issue for the message https://github.com/SEED-platform/seed/issues/2105

RDmitchell commented 4 years ago

@adrian-lara -- decided to start over on testing this, with an org with very little history of imports, and all old imports / datasets / inventory deleted.

I believe the import file has the correct information to generate the lat/long, but it didn't appear to do that -- the lat/long Tax Lot fields (all fields were mapped to Tax Lot). Maybe I skipped a step or something.

Here is a doc that shows all my steps with screenshots, etc. https://docs.google.com/document/d/1AwRWe9kQDHWHNRgLnAufGCubmmFkAe3_NrYInTw_t_M/edit?usp=sharing

adrian-lara commented 4 years ago

Did the new organization have a valid API key before you imported the file?

RDmitchell commented 4 years ago

oh, duh, that's probably the issue -- thx !!

RDmitchell commented 4 years ago

@adrian-lara -- after adding API key, tested, and works as designed -- if the lat/long data is deleted, the geocoding confidence is also deleted.

See the updates in the doc https://docs.google.com/document/d/1AwRWe9kQDHWHNRgLnAufGCubmmFkAe3_NrYInTw_t_M/edit?usp=sharing

CLOSLING !!!

SEED-platform / seed