JSON API #71
5 participants
wezm
commented on an outdated diff
| @@ -0,0 +1,54 @@ | ||
| +module Api | ||
| + class PredictionsController < ApplicationController | ||
| + PREDICTIONS_LIMIT = 1000 | ||
| + | ||
| + before_filter :authenticate_by_api_token | ||
| + before_filter :build_predictions, only: [:index] | ||
| + | ||
| + def index | ||
| + render json: @predictions, status: 200 |
|
|
wezm
commented on an outdated diff
| + def authenticate_by_api_token | ||
| + @user = User.find_by_api_token(params[:api_token]) rescue nil | ||
| + | ||
| + if @user.nil? || params[:api_token].nil? | ||
| + render json: invalid_message, status: 401 | ||
| + end | ||
| + end | ||
| + | ||
| + def build_new_prediction | ||
| + prediction_params = params[:prediction] || {} | ||
| + | ||
| + unless prediction_params[:private] && @user | ||
| + prediction_params[:private] = @user.private_default | ||
| + end | ||
| + | ||
| + @prediction = Prediction.new(prediction_params.merge(creator: @user)) |
|
|
wezm
commented on an outdated diff
| + end | ||
| + | ||
| + def create | ||
| + @prediction = build_new_prediction | ||
| + | ||
| + if @prediction.save | ||
| + render json: @prediction, status: 200 | ||
| + else | ||
| + render json: @prediction.errors, status: 422 | ||
| + end | ||
| + end | ||
| + | ||
| + private | ||
| + | ||
| + def authenticate_by_api_token | ||
| + @user = User.find_by_api_token(params[:api_token]) rescue nil |
wezm
|
wezm
commented on an outdated diff
| + @prediction = build_new_prediction | ||
| + | ||
| + if @prediction.save | ||
| + render json: @prediction, status: 200 | ||
| + else | ||
| + render json: @prediction.errors, status: 422 | ||
| + end | ||
| + end | ||
| + | ||
| + private | ||
| + | ||
| + def authenticate_by_api_token | ||
| + @user = User.find_by_api_token(params[:api_token]) rescue nil | ||
| + | ||
| + if @user.nil? || params[:api_token].nil? | ||
| + render json: invalid_message, status: 401 |
|
wezm
Convention is to use the named statuses. See http://guides.rubyonrails.org/layouts_and_rendering.html#the-status-option |
wezm
commented on an outdated diff
| +module Api | ||
| + class PredictionsController < ApplicationController | ||
| + PREDICTIONS_LIMIT = 1000 | ||
| + | ||
| + before_filter :authenticate_by_api_token | ||
| + before_filter :build_predictions, only: [:index] | ||
| + | ||
| + def index | ||
| + render json: @predictions, status: 200 | ||
| + end | ||
| + | ||
| + def create | ||
| + @prediction = build_new_prediction | ||
| + | ||
| + if @prediction.save | ||
| + render json: @prediction, status: 200 |
|
|
wezm
commented on an outdated diff
| + PREDICTIONS_LIMIT = 1000 | ||
| + | ||
| + before_filter :authenticate_by_api_token | ||
| + before_filter :build_predictions, only: [:index] | ||
| + | ||
| + def index | ||
| + render json: @predictions, status: 200 | ||
| + end | ||
| + | ||
| + def create | ||
| + @prediction = build_new_prediction | ||
| + | ||
| + if @prediction.save | ||
| + render json: @prediction, status: 200 | ||
| + else | ||
| + render json: @prediction.errors, status: 422 |
|
|
wezm
commented on an outdated diff
| + render json: invalid_message, status: 401 | ||
| + end | ||
| + end | ||
| + | ||
| + def build_new_prediction | ||
| + prediction_params = params[:prediction] || {} | ||
| + | ||
| + unless prediction_params[:private] && @user | ||
| + prediction_params[:private] = @user.private_default | ||
| + end | ||
| + | ||
| + @prediction = Prediction.new(prediction_params.merge(creator: @user)) | ||
| + end | ||
| + | ||
| + def build_predictions | ||
| + if params[:limit] && params[:limit].to_i <= PREDICTIONS_LIMIT |
|
wezm
You can call If (1..PREDICTIONS_LIMIT).include?(params[:limit].to_i) |
wezm
commented on an outdated diff
| + | ||
| + def build_new_prediction | ||
| + prediction_params = params[:prediction] || {} | ||
| + | ||
| + unless prediction_params[:private] && @user | ||
| + prediction_params[:private] = @user.private_default | ||
| + end | ||
| + | ||
| + @prediction = Prediction.new(prediction_params.merge(creator: @user)) | ||
| + end | ||
| + | ||
| + def build_predictions | ||
| + if params[:limit] && params[:limit].to_i <= PREDICTIONS_LIMIT | ||
| + @predictions = Prediction.limit(params[:limit].to_i).recent | ||
| + else | ||
| + @predictions = Prediction.limit(100).recent |
|
|
@wezm, thanks for reviewing my code. I've attempted to address each of your comments and the PR is ready for another look now.
Thanks for the improvements Jayson. It's looking good. I had to stop reviewing mid-way on Friday to go to a meeting so I have a couple more comments.
wezm
commented on an outdated diff
| @@ -71,6 +71,18 @@ def due_for_judgement | ||
| @predictions = @predictions.not_private unless current_user == @user | ||
| @predictions = @predictions.select { |x| x.due_for_judgement? } | ||
| end | ||
| + | ||
| + def generate_api_token | ||
| + @user = User.find_by_login(params[:login]) | ||
| + | ||
| + if @user && @user.update_attributes(api_token: User.generate_api_token) | ||
| + flash[:notice] = "Generated a new API token!" | ||
| + else | ||
| + flash[:error] = "We couldn't generate a new API token, sorry." |
|
wezm
When providing an error message like this it's good to keep in mind what it would be like as the user to receive the message. In this case it does not give them any idea what to do to remedy the situation (I'm, aware that the other messages in this controller aren't great but may as well not make it worse). First of all I would suggest adding With that change made you know that it was the "Unable to generate new API token due to these errors: #{current_user.errors.full_messages.to_sentence}. Please ensure your user profile is complete" |
wezm
commented on an outdated diff
| @@ -14,3 +14,9 @@ | ||
| <%= f.submit 'Change password' %> | ||
| <% end -%> | ||
| <h2><%= link_to 'View notifications', [@user,:deadline_notifications] %></h2> | ||
| +<br> | ||
| +<% if @user.api_token.present? %> | ||
| + <h2>API Token: <%= @user.api_token %></h2> | ||
| +<% else %> | ||
| + <%= button_to("Generate API Token", generate_api_token_user_url(login: @user.login)) %> |
|
wezm
With the previous change made the login param is no longer necessary. Also convention is to use the |
wezm
and 1 other
commented on an outdated diff
| @@ -47,5 +48,9 @@ | ||
| root :to => 'predictions#home' | ||
| match '/healthcheck' => 'content#healthcheck' | ||
| + | ||
| + namespace :api do | ||
| + resources :predictions |
jaysonvirissimo
I am confused about why it shows up this way on Github. My editor (Atom) shows no such whitespace. |
wezm
commented on an outdated diff
| + | ||
| +describe Api::PredictionsController, type: :controller do | ||
| + before(:each) do | ||
| + controller.stub(:set_timezone) | ||
| + end | ||
| + | ||
| + describe 'index' do | ||
| + before(:each) do | ||
| + @prediction = build(:prediction) | ||
| + @predictions = [@prediction] | ||
| + end | ||
| + | ||
| + context 'with valid API token' do | ||
| + before(:each) do | ||
| + @user = build(:user_with_email) | ||
| + @user.stub(:api_token).and_return('token') |
|
wezm
Don't think you need to stub |
wezm
commented on an outdated diff
| + end | ||
| + | ||
| + it 'should respond with JSON content type' do | ||
| + get :index, api_token: @user.api_token | ||
| + response.content_type == Mime::JSON | ||
| + end | ||
| + | ||
| + it 'should respond with predictions' do | ||
| + get :index, api_token: @user.api_token | ||
| + response.body.should == @recent.to_json | ||
| + end | ||
| + end | ||
| + | ||
| + context 'with invalid API token' do | ||
| + before(:each) do | ||
| + User.stub(:find_by_api_token).and_return(nil) |
|
wezm
The test description and implementation aren't quite aligned here. The description is invalid API token so I think that there is no need to stub |
wezm
commented on an outdated diff
| + get :index | ||
| + response.response_code.should == 401 | ||
| + end | ||
| + | ||
| + it 'should respond with JSON content type' do | ||
| + get :index | ||
| + response.content_type == Mime::JSON | ||
| + end | ||
| + end | ||
| + end | ||
| + | ||
| + describe 'create' do | ||
| + context 'with valid API token' do | ||
| + before(:each) do | ||
| + @user = build(:user_with_email) | ||
| + @user.stub(:api_token).and_return('token') |
|
|
wezm
commented on an outdated diff
| + | ||
| + it 'should respond with HTTP failure' do | ||
| + post :create, prediction: @prediction, api_token: @user.api_token | ||
| + response.response_code.should == 422 | ||
| + end | ||
| + | ||
| + it 'should respond with error messages' do | ||
| + post :create, prediction: @prediction, api_token: @user.api_token | ||
| + response.body.should include('a probability is between 0 and 100%') | ||
| + end | ||
| + end | ||
| + end | ||
| + | ||
| + context 'with invalid API token' do | ||
| + before(:each) do | ||
| + User.stub(:find_by_api_token).and_return(nil) |
|
|
Ok I'm done now. A few more tweaks and it should be good to go.
@wezm, I've attempted to implement all of the suggestions you made regarding my pull request. I really appreciate your detailed feedback. Please let me know if there are any other areas that need improvement and I'll see what I can do.
@wezm, do you have any idea when you might have the time to take a look at my changes? I'm eager to get working on a CLI that makes use of the JSON API. Thanks!
Hi @jaysonvirissimo - @wezm is on leave until Monday week. I'll try to get one of the other guys to take a look at your PR today or on Monday. Cheers.
RenWenshan
and 1 other
commented on an outdated diff
| @@ -14,3 +14,9 @@ | ||
| <%= f.submit 'Change password' %> | ||
| <% end -%> | ||
| <h2><%= link_to 'View notifications', [@user,:deadline_notifications] %></h2> | ||
| +<br> | ||
| +<% if @user.api_token.present? %> | ||
| + <h2>API Token: <%= @user.api_token %></h2> | ||
| +<% else %> | ||
| + <%= button_to("Generate API Token", generate_api_token_user_path) %> |
|
|
Is there a way to add the outcome via the API? Seems like the next logical step.
This pull request addresses this issue. It adds a RESTful API for getting recent predictions and creating new ones. If it is adequate for this purpose, I will extend it to cover the remaining CRUD operations on predictions.
It works by allowing the user to generate an API key on the settings page and then supplying the key along with each new request. The endpoints, parameter names, and parameter types can be found in the API markdown file in the top level directory.
RSpec tests have been added for the new API controller and the settings view.