girldevelopit / gdi-website

This is the official repository for the Girl Develop It website.
https://girldevelopit.com
MIT License
310 stars 378 forks source link

Chapter page fails if a state is null #347

Closed Roenok closed 9 years ago

Roenok commented 9 years ago

Description If for some reason the db state (as in state of the country) is null, the entire Chapters page fails, along with the map on the homepage.

Specifically, the error is on this line: https://github.com/girldevelopit/gdi-new-site/blob/production/app/controllers/chapters_controller.rb#L7

If 'l.state' returns 'nil' the page fails, because there is no appropriate catch

Ask Add a try/catch (or appropriate equivalent Ruby structure) to handle this situation gracefully. The page should simply skip the errored chapter and move on.

To Do

Additional Info The failure state image

therufs commented 9 years ago

Since the chapters page depends on each chapter belonging to a state, my first impression is that it would be better to do a db-side check that will prevent stateless chapters from being created (also/instead). Thoughts?

jbielick commented 9 years ago

@therufs that'd be a good validation. Also, the possibility of the geocoding failing is nonzero, but you may want the chapter to still save. In that case, the query could be updated to only return chapters with state is not null and any other info needed for the view.

Roenok commented 9 years ago

Theoretically it should not be possible, but I just demonstrated that it can happen :)

When you create a chapter, there is a mandatory field for an address. There is then a Geocoding gem that extracts the state from there. It would probably make sense to return a failure message when trying to save a new chapter if the geocoding fails.

I'm not sure how possible it is with our setup, though.

Roenok commented 9 years ago

By the way, thanks to @jbielick for diagnosing the issue so quickly!

kstack7 commented 9 years ago

Let's try tinkering with these required fields before we go too far into the back-end logic. I was able to grab a state even with the letter "p" as a location so I'm reluctant to blame the geocode library just yet. I can apply some other ideas if I can see this fail after the push: https://github.com/girldevelopit/gdi-new-site/pull/348

kstack7 commented 9 years ago

Interesting! Okay, so there must be a difference between dev/prod environments with this behavior because now it's not working with bad entries on staging. Totally seeing it now. Digging resumes :)

jbielick commented 9 years ago

grab a state even with the letter "p" as a location so I'm reluctant to blame the geocode library just yet

The geocoding can fail regardless of the geo input. API outages, rate limits, and timeouts are possible and in such an event, the geocoded fields would not be written during a failure (as likely seen in production). Validating the geo field wouldn't have fixed this original issue, because it had a geo value, but no latitude, longitude, state, etc. values. However, the actual error from the logs should be the single source of truth for this failure and state being nil is just a theory. Can we track down the actual logged error from log/production.log?

kstack7 commented 9 years ago

Yeah, I see the author suggests a 15 second timeout period while we have ours at 5; that's one quick fix re: outages/rate limits. But it's more that these random tests I'm doing don't throw any errors and I've repeated them over and over with the same "successful save" result. Example: "ew, 34gvaz" being a location. It takes it every time but just bombs out the proper fields since that's a crazy entry. We need to intercept it to test for nil before save (yes, I'm definitely exploring the before_save() as well).

Screenshot of the successful garbage-data save that was tested on staging and locally:

screen shot 2015-08-04 at 12 24 44 am

kstack7 commented 9 years ago

Mk so I have a nice before_save() checking for "state" now...seems to be doing the trick. I'll put a pull request in for added feedback or changes, but definitely stops the weird location attempts.

Added is_state_null method to the admin/chapter model: screen shot 2015-08-04 at 1 51 19 am

Result on AA page:

screen shot 2015-08-04 at 1 48 37 am

kstack7 commented 9 years ago

Currently up on staging and seemingly good. Feel free to try and break it!

https://girl-develop-it-staging.herokuapp.com/admin/chapters

screen shot 2015-08-05 at 6 48 58 pm