sketch-city / harvey-api

Harvey Needs API
http://harveyneeds.org
GNU General Public License v3.0
23 stars 39 forks source link

Dev data import for needs and shelters from live API #32

Closed nihonjinrxs closed 7 years ago

nihonjinrxs commented 7 years ago

Closes #20.

I used HTTParty instead of HTTP, since it has a nicer interface to create a class around the response.

nihonjinrxs commented 7 years ago

Regarding the auto-run question, @jwo's PR #17 removes all the Google Sheets import-related stuff, including the app hooks and endpoints. We discussed in Slack merging that one first. This one creates a replacement Rake task, but no hooks or endpoints for production use. Happy to add some docs if that helps.

On tests, I wasn't sure how best to test this one. Happy to write something, but may need some help in that direction.

ironcoconut commented 7 years ago

Got it on PR #17. Will removing that code create a merge conflict here?

For tests, I think just run it and make sure it does not error and that it adds records. You can wrap it in a VCR block so that it runs faster.

Do we have a no merge label? I'm on mobile.

Looks good.

nihonjinrxs commented 7 years ago

OK, can add a test that does that. Never used VCR -- what would that look like?

ironcoconut commented 7 years ago

I think the gem is in the repo.

https://github.com/vcr/vcr/blob/master/README.md

jwo commented 7 years ago

For VCR, I think tests might be overboard honestly. But if your curious, I use VCR tests to prevent the Test suite from actually calling out to Amazon each test run.

@ironcoconut -- will you take this pull and merge when you're happy?

I do think we should add to the README on how devs should use this.

I would also be cool removing all classes and just moving code into db/seeds. I don't want people to think it's more important than it is.

nihonjinrxs commented 7 years ago

Added basic tests that run the import and check that the number of records added to the database match what you get back from the API. Used VCR to capture and store the request data.

Happy to move this into seeds if that makes more sense, but I think that would also need some explanation, since you don't generally need an internet connection when setting up the database for an app. Seems better to have an explicit task to run for that.

Anyway, let me know what else you'd like for me to do.

nihonjinrxs commented 7 years ago

Ah, crap. Travis couldn't load HTTParty? Not getting that error locally when I run tests. Any ideas?

nihonjinrxs commented 7 years ago

OK, not sure what's happening here. Help appreciated. @jwo @ironcoconut

jwo commented 7 years ago

@nihonjinrxs that's weird ... I'll take a look and see if I can figure out

ironcoconut commented 7 years ago

Looks good. If you could also add a short blurb to the readme about how to run and why, it would be appreciated. I'll approve once the tests pass.

ironcoconut commented 7 years ago

@jwo do we need to wait for your 2 prs before merging?

jwo commented 7 years ago

@ironcoconut No, we can merge this in and then update the other PRs to keep them up to date.

nihonjinrxs commented 7 years ago

Also added some info to the README. Build is passing. Let me know if more is needed. Thanks!

cc @ironcoconut @jwo

jwo commented 7 years ago

@nihonjinrxs Will you make one final change here? There's a conflict in Gemfile that'll be easy to solve.

Make your Gemfile look like this.. you're adding redis. (that's already in master, but git can't figure out what to do)

gem 'connection_pool'
gem 'redis', '~> 3.2'
gem "httparty", "~> 0.15.6"

group :development, :test do

@nihonjinrxs UPDATE: I could use the github editor and solve. \o/ .. no need for you to do anything.

nihonjinrxs commented 7 years ago

Sorry I missed the last mention there, glad you could fix it. Happy to help! Looking at #21 next.