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 rebased #290

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

236 simple api rebased #290

wants to merge 35 commits into from

Conversation

jszwedko
Copy link
Member

@jszwedko jszwedko commented Jan 6, 2018

Rebased #241 to get a cleaner diff

James Fowler and others added 27 commits January 6, 2018 12:12
…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
@jszwedko jszwedko temporarily deployed to adoptadrainsf-staging-pr-290 January 6, 2018 20:18 Inactive
@coveralls
Copy link

Coverage Status

Coverage decreased (-93.4%) to 5.157% when pulling 9bb9fdf on 236-simple-api-rebased into fc5f68a on production.

Copy link
Member Author

@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.

@jafowler49 I rebased #241 as the Github diff was showing other changes that happened on the production branch. I left a couple of comments, but this is looking better. Thank you!

@@ -0,0 +1,46 @@
class AdoptedController < ApplicationController
before_action :authenticate
before_action :get_adopted_things, :make_cur_page, :make_other_pages, only: [:index]
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

There is already an adopted scope on Thing that you could use.

@@ -0,0 +1,5 @@
<%- headers = ['Lat', 'Lng', 'City ID'] -%>
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this used anywhere? It didn't appear to be.

Copy link

Choose a reason for hiding this comment

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

No it's not. I removed CSV rendering, and the API allows for JSON only.

@jszwedko jszwedko temporarily deployed to adoptadrainsf-staging-pr-290 January 9, 2018 03:21 Inactive
@coveralls
Copy link

coveralls commented Jan 9, 2018

Coverage Status

Coverage increased (+0.1%) to 98.71% when pulling d760550 on 236-simple-api-rebased into fc5f68a on production.

@jszwedko jszwedko temporarily deployed to adoptadrainsf-staging-pr-290 January 9, 2018 03:36 Inactive
@ghost
Copy link

ghost commented Jan 9, 2018

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.

@jszwedko jszwedko temporarily deployed to adoptadrainsf-staging-pr-290 February 5, 2018 03:44 Inactive
@ghost
Copy link

ghost commented Feb 5, 2018

Hey @jszwedko, this pull request can be reviewed again to see if its good for merge.

Thanks

@jszwedko
Copy link
Member Author

jszwedko commented Feb 11, 2018

Awesome, thanks for revisiting this @jafowler49 . Apologies that I haven't had a chance to review yet, but aim to do so soon.

Copy link
Member Author

@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.

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

Choose a reason for hiding this comment

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

Feels like we should return a 401 here in addition to the rendered HTML.

@@ -3,9 +3,11 @@ ruby '2.2.3'

gem 'airbrake', '~> 7.1'
gem 'devise', '~> 3.0'
gem 'faker', '~> 1.7.0'
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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: @results

and then the make_cur_pages, make_other_pages and the shared state could be removed.

@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.

2 participants