Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

236 simple api #241

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

236 simple api #241

wants to merge 34 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 2, 2017

This should be ready for initial review

Thanks!

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@jszwedko
Copy link
Member

jszwedko commented May 4, 2017

@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
Copy link
Author

ghost commented May 4, 2017

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

I will test it out

@coveralls
Copy link

Coverage Status

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

before_action :authenticate

def index
@things = Thing.where.not(user_id: !nil)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is true, good catch

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

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@ghost
Copy link
Author

ghost commented Aug 14, 2017

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
Copy link

Coverage Status

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

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Gemfile Outdated
@@ -25,6 +26,7 @@ end
group :development do
gem 'byebug'
gem 'spring'
gem 'kaminari'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need an exact version here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this uncommented?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

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

# API
scope '/api' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to the URL pathing here

db/seeds.rb Outdated
@@ -6,6 +6,7 @@

r = Random.new

=begin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to leave this commented?

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


# 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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 1?

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

class AdoptedControllerTest < ActionController::TestCase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coveralls
Copy link

Coverage Status

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

@ghost
Copy link
Author

ghost commented Dec 16, 2017

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
Copy link

Coverage Status

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

@bensheldon bensheldon changed the base branch from production to master April 7, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants