search

heartcombo / responders

A set of Rails responders to dry up your application
http://blog.plataformatec.com.br/
MIT License
2.05k stars 156 forks source link

System tests with Hotwire #234

Closed excid3 closed 2 years ago

excid3 commented 2 years ago

This PR adds system tests for Responders with a dummy Rails app that's using Hotwire.

Hotwire currently handles non-GET requests slightly differently if you submit a form with a _method or if you have a link using data-turbo-method. This adds a test for both of those cases to confirm that the redirect is handled correctly using a 303 to safely handle both cases.

With a 302, the data-turbo-method will actually forward the request method to the redirect URL. This results in a DELETE /posts/1 redirecting to DELETE /posts. A 303 will change the redirect to a GET /posts correctly.

Why is this necessary? button_to will generate a form with method="post" and _method="delete" as a hidden field. It sends a POST request to Rails, which then gets handled like a DELETE.

data-turbo-method="delete" actually makes a DELETE request. https://github.com/hotwired/turbo/blob/8221c1e5f2021c41c3ecada0dde55bae9ee18380/src/core/session.ts#L143-L147

Related: heartcombo/devise#5410

rafaelfranca commented 2 years ago

I think an integration test for this is overkill. It is true that we only need this change because of hotwire but I don't to exercise hotwire to make sure the response status we are sending in a 303 not a 302. We are probably better served with an unit test.

Also, I don't think we should change the behavior for all the applications. We should be returning 302 by default to not break other people tests, but if configured, we should return 303.

rafaelfranca commented 2 years ago

Closes #227

excid3 commented 2 years ago

@rafaelfranca I was thinking the same thing. You got it. 👍

excid3 commented 2 years ago

This has been refactored to a config variable and keeps the default of 302.

We will probably need Devise to configure the 303 status to be Hotwire compatible out of the box if it doesn't change in Responders. That was the main reason why I wanted to make these changes. It's frustrating that Devise doesn't work with Rails 7 by default.

rafaelfranca commented 2 years ago

We will probably need Devise to configure the 303 status to be Hotwire compatible out of the box if it doesn't change in Responders

I think that is fine. I didn't look on what is require to make devise work, but maybe even that needs to be configurable otherwise will be a breaking change.