Citizen science #381
Citizen science #381
Conversation
app/controllers/errors_controller.rb
Outdated
|
|
||
| render_error( | ||
| :method_not_allowed, | ||
| 'HTTP method not allowed for this resource.2', |
There was a problem hiding this comment.
| 'HTTP method not allowed for this resource.2', | |
| 'HTTP method not allowed for this resource., |
app/controllers/errors_controller.rb
Outdated
| :method_not_allowed, | ||
| 'HTTP method not allowed for this resource.2', | ||
| nil, | ||
| 'method_not_allowed_error_1', |
There was a problem hiding this comment.
What does the _1 suffix denote? Choose a better name please
|
|
||
| def question_params | ||
| # empty array is replaced with nil by rails. Revert to empty array | ||
| # to avoid errors with strong parameters |
There was a problem hiding this comment.
can you add in that link to the rails issue that describes this problem and the recommended work around?
|
|
||
| @responses, opts = Settings.api_response.response_advanced( | ||
| api_filter_params, | ||
| Access::ByPermission.responses(current_user, params[:study_id]), |
There was a problem hiding this comment.
if params[:study_id] is nil is that a problem?
You special-cased it in the Questions controller
There was a problem hiding this comment.
I think it was only a problem in questions because it was an array (due to HABTM).
|
|
||
| def response_params | ||
| # params[:response] = params[:response] || {} | ||
| # params[:response][:study_id] = params[:study_id] |
There was a problem hiding this comment.
General comment:
Code is commented out with no space:
#variable = value
Comments are space separated.
Is this code or a comment? It looks like code, formatted like a comment, but to my limited understanding imparts no meaning. Please clarify
| create_entire_hierarchy | ||
|
|
||
| describe "GET #index" do | ||
| it "returns http success" do |
There was a problem hiding this comment.
the next three test cases seem to have mislabeled descriptions with their results.
also, why do we expect 401 responses here?
| @@ -0,0 +1,54 @@ | |||
| require 'rails_helper' | |||
|
|
|||
| describe QuestionsController, type: :controller do | |||
There was a problem hiding this comment.
I can't really determine the point of this class.
Can you add one line of comment context to each controller test explaining why/what we are testing?
| @@ -0,0 +1,43 @@ | |||
| require 'rails_helper' | |||
|
|
|||
| describe ResponsesController, type: :controller do | |||
There was a problem hiding this comment.
same comments as previous for controller specs: add context and fix string labels mismatching with tests
| it { expect(post('/studies/filter')).to route_to('studies#filter', format: 'json') } | ||
| it { expect(post('/studies')).to route_to('studies#create', format: 'json') } | ||
| it { expect(put('/studies/1')).to route_to('studies#update', id: '1', format: 'json') } | ||
| it { expect(delete('/studies/1')).to route_to('studies#destroy', id: '1', format: 'json') } |
There was a problem hiding this comment.
in the controller, there were comments suggesting questions and responses can be nested under studies. please add route tests for the nested routed.
| resources :responses, except: :update, defaults: {format: 'json'} | ||
| get '/studies/:study_id/questions' => 'questions#index', defaults: {format: 'json'} | ||
| get '/studies/:study_id/responses' => 'responses#index', defaults: {format: 'json'} | ||
| post '/studies/:study_id/questions/:question_id/responses' => 'responses#create', defaults: {format: 'json'} |
There was a problem hiding this comment.
please redefine this whole block of routes in terms of resources definitions, as is the style in the rest of this file
closes #346
All tests passing