Turbo compatibility: return status unprocessable_entity

[jankeesvw]

We are using Administrate quite a lot in our project, and a few weeks ago we upgraded from Turbolinks to Turbo. I noticed that our validations weren't triggering anymore in Administrate.

Then I noticed that the Administrate base class doesn't return the right status code when the record can't be saved.

From the Hotwire docs:

Redirecting After a Form Submission

After a stateful request from a form submission, Turbo Drive expects the server to return an HTTP 303 redirect response, which it will then follow and use to navigate and update the page without reloading.

The exception to this rule is when the response is rendered with either a 4xx or 5xx status code. This allows form validation errors to be rendered by having the server respond with 422 Unprocessable Entity and a broken server to display a “Something Went Wrong” screen on a 500 Internal Server Error.

This pull request returns the correct status codes when saving a record.

[pablobm]

Thank you for that PR @jankeesvw! Would you be able to add spec examples for both create and update? Currently our controller specs are a bit disorganised, but I think that https://github.com/thoughtbot/administrate/blob/master/spec/controllers/admin/log_entries_controller_spec.rb would be a good place to put them

[jankeesvw]

Thank you for that PR @jankeesvw! Would you be able to add spec examples for both create and update? Currently our controller specs are a bit disorganised, but I think that https://github.com/thoughtbot/administrate/blob/master/spec/controllers/admin/log_entries_controller_spec.rb would be a good place to put them

I tried adding a spec, but my expectation is failing. Not sure why, because it works in practice. @pablobm do you have some time to help me?

This is my current .patch with a failing spec:

diff --git a/spec/controllers/admin/log_entries_controller_spec.rb b/spec/controllers/admin/log_entries_controller_spec.rb
index 729fbef..162eefb 100644
--- a/spec/controllers/admin/log_entries_controller_spec.rb
+++ b/spec/controllers/admin/log_entries_controller_spec.rb
@@ -135,6 +135,7 @@ describe Admin::LogEntriesController, type: :controller do
         expect(page).to be_instance_of(Administrate::Page::Form)
         expect(page.resource).to eq(log_entry)
         expect(page.resource.action).to eq(new_action)
+        expect(response).to have_http_status(:unprocessable_entity)
       end
     end
   end

[pablobm]

Uh, you are right. For some strange reason, it tries to render the template for :update, instead of for :edit as it's told, and it returns 204 No Content. This is not what it does in development mode. Rails is doing something weird, so let me look into it, hopefully this week.

[jankeesvw]

Uh, you are right. For some strange reason, it tries to render the template for :update, instead of for :edit as it's told, and it returns 204 No Content. This is not what it does in development mode. Rails is doing something weird, so let me look into it, hopefully this week.

Are you willing to merge before we improved the tests, now people won't see errors when using Turbo.

[pablobm]

I looked into this today. After some head scratching, I realised what was going on. This controller test doesn't actually run Rails's test rendering code. Instead it captures the call to render with a mock, extracts the value of :locals, and runs expectations on that. As a result, nothing is actually rendered, and Rails tells us that the result was a :no_content.

The capture is done with capture_view_locals, which can be seen at https://github.com/thoughtbot/administrate/blob/e1ae45cf44a6b7fce13865175bb5e596ad448ba9/spec/support/controller_helpers.rb

To compound the problem, I don't think there is a standard way of achieving this. It's possible to test :locals, but only in a very limited way, by comparing exact values. See:

https://github.com/rails/rails-controller-testing/blob/4b15c86e82ee380f2a7cc009e470368f7520560a/lib/rails/controller/testing/template_assertions.rb#L104-L108

For now, I'm going to accept this MR, as it fixes the problem at hand and its intent I think is obvious enough. Meanwhile I'll file a separate issue to see how we can address this problem properly and provide better tests.

[jankeesvw]

For now, I'm going to accept this MR, as it fixes the problem at hand and its intent I think is obvious enough. Meanwhile I'll file a separate issue to see how we can address this problem properly and provide better tests.

Thanks for looking into this!

[jankeesvw]

@pablobm can you make a new gem release with this change, would be great to get this out.