236 simple api #241

ghost · 2017-05-02T19:28:00Z

This should be ready for initial review

Thanks!

…on code for API using basic auth

…he back-end, Added admin check, Render json for adopted drains

… output

coveralls · 2017-05-02T19:29:44Z

Coverage decreased (-93.6%) to 4.902% when pulling 15fae71 on 236-simple-api into c0ec6a6 on production.

coveralls · 2017-05-02T19:46:19Z

Coverage decreased (-3.004%) to 95.486% when pulling 7287643 on 236-simple-api into c0ec6a6 on production.

coveralls · 2017-05-02T22:29:13Z

Coverage increased (+0.1%) to 98.611% when pulling 160a045 on 236-simple-api into c0ec6a6 on production.

jszwedko · 2017-05-04T20:06:49Z

@jafowler49 👍 this looks good.

I'm a little concerned about how it will fare as the number of adopted drains grows though. Would you be able to test this with, say, 10000 adopted drains (noting that the Heroku request timeout is 60s)? We may need to introduce pagination of the responses.

ghost · 2017-05-04T21:04:18Z

oh ok, sounds like scalability/response times might be an issue. good call

I will test it out

coveralls · 2017-06-29T19:47:22Z

Coverage increased (+0.1%) to 98.611% when pulling ceca853 on 236-simple-api into c0ec6a6 on production.

turino · 2017-07-17T00:45:52Z

app/controllers/adopted_controller.rb

+  before_action :authenticate
+
+  def index
+    @things = Thing.where.not(user_id: !nil)


I don't think you want !nil, but just nil

Interestingly -- not that anyone cares -- !nil evaluates to true and true gets converted to 1 in the sql, so you end up with WHERE user_id != 1. (I only know this because I checked the console)

this is true, good catch

…s. I created this initially to test out how the API would handle load

coveralls · 2017-07-24T17:44:47Z

Coverage decreased (-93.6%) to 4.892% when pulling b8fff22 on 236-simple-api into c0ec6a6 on production.

coveralls · 2017-07-24T17:49:34Z

Coverage increased (+0.1%) to 98.611% when pulling 88a33a7 on 236-simple-api into c0ec6a6 on production.

coveralls · 2017-07-26T21:13:58Z

Coverage increased (+0.1%) to 98.611% when pulling 973efac on 236-simple-api into c0ec6a6 on production.

…an paged results

ghost · 2017-08-14T01:03:20Z

hey @jszwedko , sorry this took so long... anyways added some work on this. hoping you can check it and see what you think. thanks

@jeanwalshie @jasonlally

coveralls · 2017-08-14T01:07:43Z

Coverage increased (+0.2%) to 98.653% when pulling 01c0719 on 236-simple-api into c0ec6a6 on production.

jszwedko

Thanks @jafowler49 !

Overall, this is is shaping up well, but I'm still a little concerned about the CSV API route returning all of the records rather than paginating like the other formats. I realize it is difficult to encode the pagination information directly in the response body, but we can leverage HTTP response headers to indicate the paging information. I also think we could opt to just serve JSON for now since that is the most common API format these days and drop XML and CSV.

Could we also configure the max_per_page to be 1000 and drop the default to 100? Or whatever makes sense with your testing; I just want to make sure we have a maximum.

jszwedko · 2017-08-28T18:53:46Z

Gemfile

@@ -25,6 +26,7 @@ end
 group :development do
  gem 'byebug'
  gem 'spring'
+  gem 'kaminari'


We'll need this gem in production too, no? It feels like it should be at the top level. Do you mind locking it to a major version as well (I locked the rest in #259) so that it is easier to update without breaking compatibility.

~>1.0 includes supported versions. the older version dropped support for an earlier version of Ruby

jszwedko · 2017-08-28T18:54:00Z

Gemfile

@@ -3,6 +3,7 @@ ruby '2.2.3'

 gem 'dotenv-rails', groups: [:development, :test]
 gem 'rails', '~> 4.2.4'
+gem 'faker', '1.7.3'


Do we need an exact version here?

Changed that to have the version ~> 1.7.0... These are the sure compatible versions, not sure if they will change support in the future.

jszwedko · 2017-08-28T18:56:37Z

config/initializers/devise.rb

@@ -59,7 +59,7 @@
  # given strategies, for example, `config.http_authenticatable = [:database]` will
  # enable it only for database authentication. The supported strategies are:
  # :database      = Support basic authentication with authentication key + password
-  # config.http_authenticatable = false
+  config.http_authenticatable = false


Why was this uncommented?

Never mind, I see that the API is locked to authenticated users.

jszwedko · 2017-08-28T18:57:00Z

config/routes.rb

@@ -19,4 +19,13 @@
  resource :things
  mount RailsAdmin::Engine => '/admin', :as => 'rails_admin'
  root to: 'main#index'
+
+  # API
+  scope '/api' do


👍 to the URL pathing here

jszwedko · 2017-08-28T18:57:41Z

db/seeds.rb

@@ -6,6 +6,7 @@

 r = Random.new

+=begin


Did you mean to leave this commented?

jszwedko · 2017-08-28T18:59:41Z

lib/tasks/data.rake

+    unless Rails.env.production?
+      Thing.first(10_000).each do |t|
+        if t.user_id.blank?
+          t.user_id = User.find_by('id' => Random.rand(1..User.last.id)).id


I think something like User.order('RANDOM()').limit(1) is more idiomatic here. Could we maybe print out an error message if the user attempts to run this in production?

jszwedko · 2017-08-28T19:02:05Z

app/controllers/adopted_controller.rb

+
+  # Determine if the user supplied a valid page number, if not they get first page
+  def make_cur_page
+    page = params[:page].blank? ? 2 : params[:page]


Should this be 1?

jszwedko · 2017-08-28T19:03:38Z

test/controllers/adopted_controller_test.rb

@@ -0,0 +1,44 @@
+require 'test_helper'
+
+class AdoptedControllerTest < ActionController::TestCase


I like checking the content-type, but could we make a few assertions on the response content to make sure the right records are returned? It'd also be nice to exercise the paging logic a little here.

…d_drains var

… had to change it on my local env)

…Rubocop

coveralls · 2017-12-16T04:20:14Z

Coverage increased (+0.2%) to 98.701% when pulling 57f6f42 on 236-simple-api into c0ec6a6 on production.

ghost · 2017-12-16T04:25:16Z

Hi @jszwedko

Finally worked on this. Sorry for all the commits, I merged Prod in to my local branch. Last 1 was fixing merge conflicts and Rubocop requests, next 10 were actual changes, and the rest were just commits from Prod.

Let me know what further changes I should make to the API. I added basic tests. Is there an easy way to test 1000 or so items within a Test Case?

Hope this helps

Also, since I left the City I've been working on my iOS and UI skills. So if you guys need an iOS app for Adopt a Drain, I'm your guy.

Best!

coveralls · 2018-01-06T20:12:06Z

Coverage increased (+0.2%) to 98.701% when pulling 739d86c on 236-simple-api into c0ec6a6 on production.

sferik and others added 9 commits October 29, 2015 13:48

Update minitest to version 5.8.2

1ad6247

Update webmock to version 1.22.2

900eae4

Create Adopted controller for API, and wrote initial auth/authorizati…

43f7697

…on code for API using basic auth

Add API route, and setting to allow for basic_auth

3b65084

Forgot to add the helper file

69d6af7

Edited authentication controller so that a session isn't created on t…

b1b4305

…he back-end, Added admin check, Render json for adopted drains

Adopted Drains API controller test

91b0e3b

Trying to fix Travis CI errors

7371587

Added formatting for different content types, and edited fields to be…

15fae71

… output

(Finally realized I can run Rubocop on my local machine)

7287643

ghost mentioned this pull request May 2, 2017

Create simple API to access locations of adopted drains #236

Open

4 tasks

More controller tests

160a045

root added 2 commits June 20, 2017 14:39

Add Faker gem in order to make sample users

0b4dab3

Adding some user seeds

ceca853

turino reviewed Jul 17, 2017

View reviewed changes

Added a rake task for DEV so mock users can automatically adopt drain…

b8fff22

…s. I created this initially to test out how the API would handle load

Passing rubocop

88a33a7

root added 2 commits July 25, 2017 16:20

Changing !nil to nil... !nil evaluates to true

b893346

Initial comments for cursor funcionality

973efac

James Fowler added 2 commits August 13, 2017 18:00

Adding Kaminari gem for pagination

5f868d1

added pagination functionality to API

5772058

James Fowler added 2 commits August 13, 2017 18:01

Made is so that a CSV template is rendered with all results rather th…

da2f4b1

…an paged results

auto_adopt task comment for further clarification

01c0719

jszwedko requested changes Aug 28, 2017

View reviewed changes

James Fowler added 12 commits December 15, 2017 13:36

Updated dev-envt config file

343a600

temporarily changed asset host

78c0c75

Added a total_pages variable, changed default page to 1, and a adopte…

cfeba19

…d_drains var

Updating versions

417c95f

Removed format options - JSON is the only format available now

2953cf3

Updated gem versions

e24cea2

Change local_time to local-time... (I saw it was already changed, but…

dcc6277

… had to change it on my local env)

Updated pagination defaults

e66a624

Uncommenting so I could add drains

4032d8d

Making this clearer, and printing out info

c375680

Response data tests for pagination counts and drain data

ea959a8

Fixed a lot of merge conflicts from prod... And made changes to pass …

57f6f42

…Rubocop

Merge remote-tracking branch 'origin/master' into 236-simple-api

739d86c

jszwedko mentioned this pull request Jan 6, 2018

236 simple api rebased #290

Open

bensheldon changed the base branch from production to master April 7, 2023 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

236 simple api #241

236 simple api #241

ghost commented May 2, 2017

coveralls commented May 2, 2017

coveralls commented May 2, 2017

coveralls commented May 2, 2017

jszwedko commented May 4, 2017

ghost commented May 4, 2017

coveralls commented Jun 29, 2017

turino Jul 17, 2017

ghost Jul 25, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 26, 2017

ghost commented Aug 14, 2017

coveralls commented Aug 14, 2017

jszwedko left a comment

jszwedko Aug 28, 2017

ghost Dec 16, 2017

jszwedko Aug 28, 2017

ghost Dec 16, 2017

jszwedko Aug 28, 2017

jszwedko Aug 28, 2017

ghost Dec 16, 2017

jszwedko Aug 28, 2017

jszwedko Aug 28, 2017

jszwedko Aug 28, 2017

jszwedko Aug 28, 2017

jszwedko Aug 28, 2017

coveralls commented Dec 16, 2017

ghost commented Dec 16, 2017

coveralls commented Jan 6, 2018

		@@ -0,0 +1,44 @@
		require 'test_helper'

		class AdoptedControllerTest < ActionController::TestCase

236 simple api #241

Are you sure you want to change the base?

236 simple api #241

Conversation

ghost commented May 2, 2017

coveralls commented May 2, 2017

coveralls commented May 2, 2017

coveralls commented May 2, 2017

jszwedko commented May 4, 2017

ghost commented May 4, 2017

coveralls commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 26, 2017

ghost commented Aug 14, 2017

coveralls commented Aug 14, 2017

jszwedko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 16, 2017

ghost commented Dec 16, 2017

coveralls commented Jan 6, 2018