search

koopjs / koop-socrata

Socrata provider for Koop.
Other
8 stars 7 forks source link

Fix to support returning all fields and numbers #58

Closed sirws closed 8 years ago

sirws commented 8 years ago

@jgravois

Test it out!

If the first record found has fields that are null, socrata doesn’t return the field, so it is missing in the feature service json. This fix ensures all fields are there. It also return numeric fields as numbers instead of strings. We are also storing the fields and socrata field types for better date handling in the future.

jgravois commented 8 years ago

pulled the latest koop-sample-app, reran npm install and now i'm hitting the same error the guy reported in #55 (even after creating a brand new PostGIS datastore)

what version of koop itself have you been testing against? 2.10.4?

sirws commented 8 years ago

@jgravois

Check on the second block in “usage” here https://github.com/koopjs/koop-opendata replace openData:services with socrata:services

Daniel had me do that and it worked for me.

jgravois commented 8 years ago

i have confirmed that the field type casting is improved and that missing fields like year are now present. nothing problematic about your logic jumps out at me and all tests are passing, so i think this is safe to merge.

...replace openData:services with socrata:services

sweet. thanks. can you add this information to the readme in another commit and bump the changelog w/ a mention of your fix?

cc/ @dmfenton lemme know if you're not okay with me merging/tagging @sirws's work as is.

dmfenton commented 8 years ago

@sirws Am I correct in understanding no changes to Koop will need to be made to fix this issue?

@jgravois I'm good with you merging this PR once we can get the tests passing.

jgravois commented 8 years ago

I definitely already confirmed that tests are passing in @sirws' branch.

I'll merge/tag as soon as he has a chance to lay down one more commit with the README update and changelog bump.

dmfenton commented 8 years ago

OK, cool. I'll see if I can get to the bottom of the test failures.

dmfenton commented 8 years ago

Alright, I've fixed the issues that were causing test failures. @sirws can you please rebase your PR and push it back up?

sirws commented 8 years ago

I show the same failure in the master branch as what is in mine, but it is after all of the tests pass. Hold off on merging as I have a few other fixes coming in the socrata provider. I hope to have something tomorrow.

Regarding changes to koop... it isn't required but the date handling in https://github.com/koopjs/koop/pull/277 will make it even better.

jgravois commented 8 years ago

i think @dmfenton has already tracked down the problem with the tests.

I have a few other fixes coming...

if they fix something unrelated, it'd be preferable to review in a seperate PR.

dmfenton commented 8 years ago

@sirws you have an error even after rebasing?

jgravois commented 8 years ago

no need for rebase. i restarted the travis test and its all green now.

sirws commented 8 years ago

No, I am seeing an issue with the table opening in the ArcGIS Online map viewer. Map shows fine, but when I go to view the table, it isn't showing the records. I am trying to resolve that. The fixes are related to the same PR.

dmfenton commented 8 years ago

Ok, either way, let's open a new one for that. We'll review it separately and wait to release to NPM until everything is resolved.

sirws commented 8 years ago

ok