search

codeforamerica / srtracker

Open311 Service Request Status Site
BSD 3-Clause "New" or "Revised" License
20 stars 23 forks source link

Generalized version should work without note.extended_attributes #26

Open h4ck3rm1k3 opened 11 years ago

h4ck3rm1k3 commented 11 years ago

I found more code that was obviously never executed. Patch here. https://github.com/h4ck3rm1k3/srtracker/commit/2ef54b1854bbc082c871ce08ea22f6c9ce03eb37#L1R150

note.extended_attributes.closed_datetime is expected to be in a datetime object in the template, there was no check for it or conversion code.

Mr0grog commented 11 years ago

There are certain peculiarities to Chicago's Open311 endpoint (not yet publicly available) that the current code is designed to make use of. Yes, it should be generalized in the future, but for now this code is all going to be Chicago-specific so that we can move a bit more quickly.

h4ck3rm1k3 commented 11 years ago

Well there are no provisions for datetimes in json from what I saw, what does the chicago code return for this field? is it a timestamp string or a ready made date object. I saw that for the other timestamps they are converted before being sent to the template, and that Is what I did here. in my test server I returned a standard iso timestamp like the other timestamps.

https://github.com/h4ck3rm1k3/open311-dancer/blob/master/open311/lib/open311.pm#L76

Mr0grog commented 11 years ago

The SR's notes don't actually come with a closed_datetime field. The loop where you inserted the check is where that field is being created. It's also created as a copy of a field that has already been converted to a datetime, so all that checking is unnecessary.

It's also worth noting that we're adding this field here as a convenience for the template. It may be something that we remove in the future as it was primarily created to demonstrate an issue with the current implementation of Chicago's Open311 endpoint (not yet public).

h4ck3rm1k3 commented 11 years ago

Well I tested the code with my test server, I will create some more detailed tests for this case for with and without the extended fields, and make sure it works with all the cases. Please provide some example responses from chicago for testing purposes.