Can't save events with lat/longs

JChipault commented 3 years ago

Describe the bug Heard from external partner and internal user that live site was not allowing them to save events that had lat/longs. I figured it was because their lat/longs weren't within the county they had selected, but when I played around with it a bit it seems it's not letting through any events with lat/longs.

To Reproduce Steps to reproduce the behavior:

Create Event in the test site
enter minimal event data, with the focus on Cococino County, AZ with this lat/long 36.86572, 111.58650 (no negative sign on the longitude)
Save event
Get this JSON error
Go back up and add negative sign, so now have 36.86572, -111.58650, which is in Cococino County, AZ
Save event
Get this lat/long error:

Can also try with Denton County, Texas and this lat/long: 33.050072, -96.895581 and get

If lat/longs are removed, these events save just fine. Can add in the lat/long in the event details page (but as we discussed before, that's likely because the lat/long check is not run in the event details interface)

Expected behavior For situations where lat/long is not in the county (e.g., when there's no negative sign on the longitude as in the first AZ example), then should get this warning.

If lat/long is in county, then should save event.

JChipault commented 3 years ago

@aaronstephenson - I suspect this would get directed to you

JChipault commented 3 years ago

If it's easier to just turn off this lat/long check for now, that's probably ok. The error message isn't super useful to a user (they don't know what "administrative level two" is) so I suspect people will trip over this a bit even if it's working as expected, unless it's easy to change that error message.

aaronstephenson commented 3 years ago

This is a slightly different problem from the ones solved in #458.

I found that your step 2 resulted in an "index out of range" error (which shows up as a JSON parse error in the client because the server is returning HTML not JSON) because my code was assuming that the Geonames web service would return a JSON response that included an admin level two (code 'ADM2)) value. The lat/lng in your step 2 was a point in China and the Geonames web service doesn't have admin level twos there; instead cities (code 'PPL')) are immediately below admin level ones (code 'ADM1'). I have now implemented a check there to ensure an ADM2 value is present otherwise skip over that validation step. In the future if you expand WHISPers to countries that don't have admin level twos, you may want to revisit this code block... as well as the entire data model surrounding the parent-child relationships of locations (e.g., country -> admin level one -> admin level two).

The error you get in your step 5 is because I was doing an exact match on admin level two names and there was no exact match found in the WHISPers database administrativeleveltwo table (the SQL query here being SELECT * FROM whispers_administrativeleveltwo WHERE name = 'Coconino' AND administrative_level_one_id = 4). Geonames was returning a value of 'Coconino' for the submitted lat/lng, but the DB table doesn't have that exact value; instead it is 'Coconino County', which isn't an exact match to 'Coconino'. Looking through that DB table, this is a widespread phenomenon: some states have just the unadorned name (e.g., 'York') in the Name field, other states have the name plus the descriptor (e.g., 'Coconino County' or 'Powell River Regional District' or even 'Valdez - Cordova Census Area') in the Name field. To solve this problem, I am now doing a 'contains' query rather than an 'exact match' query. Translated to SQL, the query is now SELECT * FROM whispers_administrativeleveltwo WHERE name LIKE '%Coconino%' AND administrative_level_one_id = 4.

Fixed in v2.1.2 42ed18f and pushed to test and prod servers.

JChipault commented 3 years ago

Perfect. Thanks! Thanks also for modifying the error message that pops up when the lat/long is not in the county.

nbaertlein commented 3 years ago

@aaronstephenson We had a report last week of a similar issue seen on the event details - add location form. Do you know if the updates applied above would also apply to this form?

Details: For event 201172, a user tried to add a new location to the already existing event. When trying add both a county (Montgomery, TX) and lat/long, he got an error stating he couldn't save with a lat/long. The user then removed that lat/long and was able to save the new location correctly. After being saved, he went to edit the location and was able to add the lat/long successfully. Unfortunately, we don't have any screenshot of the actual error message.

aaronstephenson commented 3 years ago

Yep, that would use the same validation as when creating a new event. I'll take a look. My guess is that the lat/lng is either not in the county, or there's some county name lookup problem.

nbaertlein commented 3 years ago

I verified the lat/longs as he later entered them for Montgomery county and they check out fine. It's impossible to know if he was misentering on the first go around. Flip-flopping lat and long values or dropping the minus symbol off the longitude are common entry errors that I could see causing issues. As is with the other county name issues you looked at, 'Montgomery' is pretty common. thanks for taking a look.

aaronstephenson commented 3 years ago

I discovered that the problem here is that the Geonames web service is returning Harris County for the submitted lat/lng. Looking on a map, the point is very close to the boundary between Harris and Montgomery, so there may be some issue with disparate projections between the submitted lat/lng and the Geonames web service, or perhaps Geonames is using coarser polygons than the map I used while troubleshooting (the county boundary here is a creek with lots of meanders).

As a solution for now, Neil and I agreed to make this a "soft" validation... if the validation code finds that the submitted lat/lng is not in the county that the user submitted, we'll send an email to the admin team with the details, but still allow the EventLocation creation to proceed without an error to the user.

aaronstephenson commented 3 years ago

Changed in 2.1.3 509c1ac and pushed to test and prod servers.

JChipault commented 3 years ago

@aaronstephenson Running into an issue here again on both test and live sites. County = Santa Cruz, AZ. Lat/Long = 31.3800792,-111.13646. Santa Cruz is a common county name and this lat/long is on the border of Arizona and Mexico so I'm not sure which of the issues listed earlier in this string might be causing the problem. Here's the error message that the user gets:

nbaertlein commented 3 years ago

We received another similar error. The message was different, but I was able replicate the error on the test site. Removal of lat-long fixed the issue. Lat-long is extremely close to the county border.

Error read as: "Error. Event not Submitted. Error message: SyntaxError: Unexpected token K in JSON at position 0"

Data were: { "event_reference": "", "event_type": "2", "complete": false, "public": true, "staff": "4", "event_status": "2", "legal_status": 1, "legal_number": "", "new_organizations": [ 659 ], "new_service_request": { "request_type": 1, "new_comments": [ { "comment": "1 frozen liver for rhdv2 testing" } ] }, "new_event_diagnoses": [], "new_comments": [], "new_eventgroups": [], "new_event_locations": [ { "name": "Boring area", "start_date": "2021-04-29", "end_date": "2021-05-02", "country": "30", "administrative_level_one": 40, "administrative_level_two": 2207, "latitude": "45.46109", "longitude": "-122.3617", "land_ownership": "6", "gnis_name": "", "gnis_id": "", "site_description": "", "history": "Eastern Cottontail (21-RHD-72): Multiple dead or moribund rabbits observed on a property in the Boring area. A veterinarian who lives in the area observed several wild rabbits found dead or moribund on her property over a few days. One rabbit was collected on 5/2/21 and submitted for sampling ", "environmental_factors": "", "clinical_signs": "", "comment": "", "new_location_species": [ { "species": 184, "population_count": null, "sick_count": null, "dead_count": 1, "sick_count_estimated": null, "dead_count_estimated": null, "priority": null, "captive": false, "age_bias": null, "sex_bias": null, "new_species_diagnoses": [] } ], "new_location_contacts": [ { "contact": 4709, "contact_type": 1 } ] } ], "new_read_collaborators": [], "new_write_collaborators": [], "quality_check": null }

aaronstephenson commented 3 years ago

The underlying problem here is the inexact match / noncontiguous topology between the source geographic data (user) and the geographic data being used to confirm topological accuracy (Geonames). At the edges of counties, being off by even a meter between the two datasets could cause an error.

@nbaertlein @JChipault I suggest we just scrap this entire check (lat/lng in submitted county). As long as people try to submit data along administrative boundaries, you're going to run the risk of this error occurring, no matter the dataset you use for validation (even your own).

JChipault commented 3 years ago

@aaronstephenson I see this up above in this string: "As a solution for now, Neil and I agreed to make this a "soft" validation... if the validation code finds that the submitted lat/lng is not in the county that the user submitted, we'll send an email to the admin team with the details, but still allow the EventLocation creation to proceed without an error to the user."

Is that still a possibility?

aaronstephenson commented 3 years ago

That was already implemented, so to be honest I'm mystified why the user gets errors; it should just pass thru. I'll try to debug locally using the example JSON you posted earlier, once I'm finished debugging another issue.

JChipault commented 3 years ago

@nbaertlein thoughts on debugging vs just turning it off altogether? I'm ok either way.

nbaertlein commented 3 years ago

@aaronstephenson I wouldn't spend too much time on it. If we can't get it to work, we'll do some QC on the back end against the points in the county table instead. Main goal is to get the data into the system.

I'd like to keep this segment of code for future development though. I could see a means to auto populate county, state, etc. based on an entered lat/long or from a map picker. We can then leave it in the user's hands if they want to adjust in the case of border locations.

aaronstephenson commented 3 years ago

OK. I'll just take a look over the code. I'm fairly confident that I can find the problem quickly, and failing that it wouldn't take much time to just comment out the relevant code blocks.

aaronstephenson commented 3 years ago

OK, using the JSON that Neil posted here last week, I get an error:

File "whispersservices\whispersapi\serializers.py", line 792, in validate message += f" ({data['administrative_level_two'].name}) when using the latitude" KeyError: 'administrative_level_two'

This is in a code block that is just trying to form an email message to the admins about the lat/lng not being contained in the submitted county. So, the validation code is working correctly, the problem is simply in the email message compilation (I seem to be using the wrong dict key name). Easy fix.

aaronstephenson commented 3 years ago

@nbaertlein @JChipault this is fixed in 077bdf8. I just pushed it to the test server. Can you give it a try now?

JChipault commented 3 years ago

@aaronstephenson Cleared cache, logged in a jennypartner1, created event in Santa Cruz County, AZ with lat/long of 31.3800792,-111.13646 and got this toast:

aaronstephenson commented 3 years ago

So this one is slightly different from the others, it says lat/lng are not in the submitted country, not county, so it's not in the section of code that I've altered. And it looks like Geonames thinks that that coordinate is in Mexico ☹️, hence the validation error.

Guess I'll go thru all these lat/lng validations and change them all to send an email to the admin rather than return an error to the user. Will only take a few minutes.

JChipault commented 3 years ago

Nice catch - I missed that important "r" in the error message!

aaronstephenson commented 3 years ago

OK, I changed all lat/lng validations (country and state, in addition to county that was already changed) to no longer return errors to the user, but send emails to the admins instead. Fixed in b51360e and pushed to the test server just now.

JChipault commented 3 years ago

this seems ok for now. the emails to whispers@usgs.gov seem spotty, but the user is able to enter data without hitting the error message dead end so we'll call that a win! Thank you!

aaronstephenson commented 3 years ago

Great!

I'm curious tho, can you tell me more about spotty emails? If there's an underlying problem I want to fix it. I looked in all the logs and I don't see any problems, so I'd like to reproduce what you did on my laptop to have more debugging options.

JChipault commented 3 years ago

I haven't dug into it much so am a bit reluctant.... but.... whispers@usgs.gov got a "Third Party Service Validation Warming" email when I created an event with the Santa Cruz County, AZ lat/long above (actually we got two identical emails). But when I created an event with the Denton County TX lat/long above, that seemed to not send an email (created event 171024). To that 171024 event, I added the Coconino, AZ location mentioned above and didn't receive an email about that either. So maybe we're getting emails for the countRy validation but not the county? Regardless, I think Neil has some SQL code to check this so we can just do an after-the-fact check of lat/longs entered for now

JChipault commented 3 years ago

maybe the emails have issued and just haven't shown up in our dropbox yet?

aaronstephenson commented 3 years ago

I just tried all three locations on my laptop dev environment, and all three triggered emails. I can't explain why the test server would behave differently, it's the same code. I don't see any errors in any of the server logs, either, but I'll keep poking around up there.

The two emails are because these validations happen both in the Event and in the EventLocations. I'll just comment out the code block in Event to prevent that in the future.

JChipault commented 3 years ago

Coconino emails just came through...

aaronstephenson commented 3 years ago

In a side discussion, we think an Apache Web Server restart on the test server fixed the problem of the country validation emails not triggering.

Also, I just pushed a tiny change (cb97777) to the test environment that prevents the duplicate emails and corrects a typo in the county validation email.

JChipault commented 3 years ago

Created events 171027-171030 with abilenetest account on the test site with some of the country/county/lat/long combos mentioned in this string. Was able to create all events (yay!) so, again, we can stop here and call that a win. However, looks like the only email that came through to whispers@usgs.gov is one that violated the countRy match (171030). There was only one copy of that email this time, so that's nice. 171027 also has a mismatch between countRy of county and countRy of lat/long but perhaps it's just so far outside the country that geonames doesn't register

JChipault commented 3 years ago

Whoops - not out of the woods yet - tried creating an event with the Denton County, Tx lat/longs listed above (33.050072, -96.895581) but put it in the state of Alabama (county Autauga) to see if I'd get an email at whispers@usgs.gov when the state and lat/long don't match and I WAS NOT ABLE TO SAVE EVENT. Got this error message: . Event saved after I removed lat/long (171032)

aaronstephenson commented 3 years ago

171027 didn't send an admin email because its lat/lng (36.865720, 111.586500) is in China, which doesn't use the standard administrative levels one and two like most of the rest of the world. Their analog to 'ADM2' is 'PPL', which apparently is a 'city', and so there's no way for me to directly compare that to our admin level twos, so the code just skips all those validations that would normally happen (this particular code block was written well before the current iteration). And anyway, the WHISPers database doesn't have any admin level two records for China, and only one admin level one record (named "China"). I will change the behavior to send an email in that case:

'Geonames returned data in an unexpected format that could not be validated against data in the WHISPers database when using the latitude and longitude submitted by the user (111.586500, 36.865720).

When I try a lat/lng outside the USA, but in a country that we have records for in the WHISPers DB, the emails do actually get triggered:

'Geonames returned a Country (MX) different from the one submitted by the user (United States) when using the latitude and longitude submitted by the user (-110.136460, 29.380079).

I also tested points in Peru and Brazil, because neither are in the WHISPers database, and I received the expected email:

Geonames returned a Country (BR) that could not be found in the WHISPers database when using the latitude and longitude submitted by the user (-60.586500, -5.865720).

Regarding your last comment, that is an intentional validation error that I purposely have not removed, though upon closer review it is a bit broad. I will keep it to validate the format of the lat/lng data (ensuring they are decimal degrees with no more than 6 decimals), but send an admin email instead of that error message when the response from Geonames returns data in an unexpected format (in this specific case, there is no 'address' or 'geonames' object in the response, which means Geonames things the point is not in a country (likely in an ocean)).

Anyway, when I submit an event with the Denton, TX lat/lng but the Autauga, AL admin levels 2 and 1, I get the expected email:

'Geonames returned an Administrative Level One (Texas) different from the one submitted by the user (Alabama) when using the latitude and longitude submitted by the user (-96.895581, 33.050072).

All changes mentioned here are included in e9b0356a and have been pushed to the test environment. Give it another try when you have a moment @JChipault

nbaertlein commented 3 years ago

Jenny is out of the office so I tested a few of the scenarios listed above. Not sure if I got them all, but here's what I have. All the following scenarios successfully saved AND send the appropriate admin validation emails.

US county and point in China: Event 171033 - Coconino County, AZ 36.865720, 111.586500
US county and borderline point: Event 171034 - Santa Cruz County, AZ 31.380079, -111.136460
US county and point in another US county: Event 171035 - Autauga County, AL 33.050072, -96.895581
US county and point in the ocean: Event 171036 - Honolulu County, HI 25.346546, -152.495607

Unless I missed a scenario, all look good to me.

nbaertlein commented 3 years ago

Not sure if it matters, but looks like this scenario created 2 emails:

US county and point in the ocean: Event 171036 - Honolulu County, HI 25.346546, -152.495607

aaronstephenson commented 3 years ago

Right you are about the 2 emails, I had two different code blocks doing nearly identical geonames validations. Fixed in d5a94eb and pushed to test environment.

JChipault commented 3 years ago

Did a bit of testing and looks good to me. Thanks!

aaronstephenson commented 3 years ago

Fix included in v2.1.4 21520fb and deployed to production environment.

JChipault commented 3 years ago

This one seems to be popping up again with an event in western Alaska.

User Robb Kaler tried on multiple occasions (2 different days - 9/29 and 10/4) to create an event for puffin in Alaska, and he kept getting a JSON error message that said the event was not created.

We later realized that the event does actually save, but not all the data save (e.g., events are missing species and event org). See events 201556, 201570, 201572, and 201573 - I have since edited these four events to make them not visible to the public while we work through this bug. He had same issue with 201555 but this one we'll keep on the production site (and he'll go back and edit it to add in missing information). Each time, a "WHISPERS ADMIN: Third Party Service Validation Warning" email was issued.

He was able to create other events just fine during this time frame (e.g., 201571).

I can recreate this issue on the test site and get a slightly different error message, below. JSON is also below.

If I remove the lat/long, the event submits just fine!

As jchipault user: Form Status: VALID

{ "event_reference": "", "event_type": "1", "complete": false, "public": true, "staff": null, "event_status": "1", "legal_status": 1, "legal_number": "", "new_organizations": [ { "org": 2 } ], "new_service_request": { "request_type": 1, "new_comments": [ { "comment": "test test" } ] }, "new_event_diagnoses": [], "new_comments": [], "new_eventgroups": [], "new_event_locations": [ { "name": "", "start_date": "2020-07-10T05:00:00.000Z", "end_date": "2020-07-17T05:00:00.000Z", "country": "30", "administrative_level_one": 1, "administrative_level_two": 69, "latitude": "53.58", "longitude": "-165.51", "land_ownership": "1", "gnis_name": "", "gnis_id": "", "site_description": "", "history": "test", "environmental_factors": "", "clinical_signs": "", "comment": "", "event_type": 1, "new_location_species": [ { "species": 547, "population_count": null, "sick_count": null, "dead_count": 1, "sick_count_estimated": null, "dead_count_estimated": null, "priority": null, "captive": false, "age_bias": null, "sex_bias": null, "new_species_diagnoses": [] } ], "new_location_contacts": [] } ], "new_read_collaborators": [], "new_write_collaborators": [] }

JChipault commented 3 years ago

Lat/long in this event is near the international date line... is that part of the problem?

JChipault commented 3 years ago

USGS-WiM / whispersservices

Can't save events with lat/longs #468