236 simple api rebased - #290
Conversation
…on code for API using basic auth
…he back-end, Added admin check, Render json for adopted drains
…s. I created this initially to test out how the API would handle load
| @@ -0,0 +1,46 @@ | |||
| class AdoptedController < ApplicationController | |||
| before_action :authenticate | |||
| before_action :get_adopted_things, :make_cur_page, :make_other_pages, only: [:index] | |||
There was a problem hiding this comment.
Given these filters are only used for the index action, what do you think of just moving the logic in there? You could still leave the helper functions and call them.
| private | ||
|
|
||
| def get_adopted_things | ||
| @adopted_things = Thing.where.not(user_id: nil) |
There was a problem hiding this comment.
There is already an adopted scope on Thing that you could use.
| @@ -0,0 +1,5 @@ | |||
| <%- headers = ['Lat', 'Lng', 'City ID'] -%> | |||
There was a problem hiding this comment.
Is this used anywhere? It didn't appear to be.
There was a problem hiding this comment.
No it's not. I removed CSV rendering, and the API allows for JSON only.
|
Hey @jszwedko Made the changes you requested and one other change - adding seed data to an API Test Case. And also made changes to make Rubocop and Rails tests pass. |
|
Hey @jszwedko, this pull request can be reviewed again to see if its good for merge. Thanks |
|
Awesome, thanks for revisiting this @jafowler49 . Apologies that I haven't had a chance to review yet, but aim to do so soon. |
jszwedko
left a comment
jszwedko
left a comment
There was a problem hiding this comment.
@jafowler49 thanks so much for continuing to push this forward! I left a few more comments, but I think this is almost there.
| user = User.find_by(email: username) | ||
| if user && user.valid_password?(password) | ||
| return true if user.admin? | ||
| render html: '<div> You must be an admin to access this page </div>' |
There was a problem hiding this comment.
Feels like we should return a 401 here in addition to the rendered HTML.
|
|
||
| gem 'airbrake', '~> 7.1' | ||
| gem 'devise', '~> 3.0' | ||
| gem 'faker', '~> 1.7.0' |
There was a problem hiding this comment.
Is this needed for all environments? It feels like it is only needed for test and maybe development.
| # Determine if the user supplied a valid page number, if not they get first page | ||
| def make_cur_page | ||
| page = params[:page].blank? || params[:page].to_i.zero? ? 1 : params[:page] | ||
| @cur_page = @adopted_things.page(page) |
There was a problem hiding this comment.
This coupling feels a bit weird as it requires the developer to have called adopted_things first.
I think this can be simplified a bit if def index was like:
adopted_things = Things.adopted.order(:created_at).page(params[:page])
results = {
next_page: adopted_things.next_page, # I think it's fine if this is null to the caller
prev_page: adopted_things.prev_page,
total_pages: adopted_things.total_pages,
things: format_fields(things)
}
render json: @resultsand then the make_cur_pages, make_other_pages and the shared state could be removed.
2 participants
Rebased #241 to get a cleaner diff