shakacode / old-react-on-rails-examples

Various example apps for React on Rails, outdated
1 stars 0 forks source link

remove html format views & logic #16

Closed Judahmeek closed 7 years ago

Judahmeek commented 7 years ago

This change is Reviewable

robwise commented 7 years ago

Reviewed 5 of 6 files at r1, 2 of 2 files at r2. Review status: all files reviewed at latest revision, 4 unresolved discussions.


todo-app-production/app/controllers/todos_controller.rb, line 39 at r2 (raw file):

        format.json { render json: @todo, status: :created, location: @todo }
      else
        format.json { render json: @todo.errors, status: :unprocessable_entity }

since we have the defaults set that means we can now just do render json: #... without the format thing


todo-app-production/spec/controllers/todos_controller_spec.rb, line 50 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…
`:deleted` isn't actually a valid HTTP status. Other than that, we should be good to go. I'm not sure why the capybara test suddenly started failing though. Reverting `routes.rb` doesn't help. Is it related to the removal of `pure/input`?

oh right (duh) lol

Did you get this fixed? You seem to be passing now


Comments from Reviewable

Judahmeek commented 7 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions.


todo-app-production/app/controllers/todos_controller.rb, line 39 at r2 (raw file):

Previously, robwise (Rob Wise) wrote…
since we have the defaults set that means we can now just do `render json: #...` without the format thing

I'll clean that up then.


todo-app-production/spec/controllers/todos_controller_spec.rb, line 22 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…
Use your factory_girl factory here. This will ensure that your tests will always stay in sync with the model: ```ruby todo_attrs = attributes_for(:todo) post :create, params: { todo: todo_attrs }, format: :json expect(response).to have_http_status(:created) expect(response.body).to eql(todo.to_json) ```

Done.


todo-app-production/spec/controllers/todos_controller_spec.rb, line 42 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…
give a bad id

Giving a bad id fails the before_action filter and not the action itself.


todo-app-production/spec/controllers/todos_controller_spec.rb, line 50 at r1 (raw file):

Previously, robwise (Rob Wise) wrote…
oh right (duh) lol Did you get this fixed? You seem to be passing now

No, it failed locally and works on circleci. weird.


Comments from Reviewable

Judahmeek commented 7 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions.


todo-app-production/spec/controllers/todos_controller_spec.rb, line 42 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…
Giving a bad id fails the before_action filter and not the action itself.

Although that might actually be enough. I've never checked the status codes when rails spits out DB errors.


Comments from Reviewable

Judahmeek commented 7 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions.


todo-app-production/spec/controllers/todos_controller_spec.rb, line 42 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…
Although that might actually be enough. I've never checked the status codes when rails spits out DB errors.

No luck. Bad id results in a fatal error, not a failed test:

2) TodosController#update renders a Todo JSON object upon failure
     Failure/Error: @todo = Todo.find(params[:id])

     ActiveRecord::RecordNotFound:
       Couldn't find Todo with 'id'=3
     # ./app/controllers/todos_controller.rb:59:in `find_todo'```

Comments from Reviewable

Judahmeek commented 7 years ago

Review status: 6 of 7 files reviewed at latest revision, 4 unresolved discussions.


todo-app-production/spec/controllers/todos_controller_spec.rb, line 50 at r1 (raw file):

Previously, Judahmeek (Judah Meek) wrote…
No, it failed locally and works on circleci. weird.

Still failing locally after rebundling and running yarn in client so I have no idea.


Comments from Reviewable