Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return 303 on redirect for Turbo #5410

Closed
wants to merge 2 commits into from
Closed

Conversation

coder2000
Copy link

@coder2000 coder2000 commented Oct 23, 2021

Turbo uses the DELETE verb directly so when redirecting with a 302 it retains that verb and the routing fails. This change will use status 303 which will force a change to GET which will allow proper redirecting after sign out.

More information can be found in the following issues:
hotwired/turbo#84
hotwired/turbo-rails#259
rails/rails#43430

Turbo uses the DELETE verb directly when using `data-turbo-method: :delete` in rails so the redirect fails when returning the standard status of 302. This will return a 303 which will force the verb to change to GET. Should not affect any other usage with UJS.
fix: return status 303 for redirect
@JunichiIto
Copy link
Contributor

This PR helped my issue. Thank you very much.

However, I had to add notice: option in order to display signed-out message:

  def respond_to_on_destroy
    # We actually need to hardcode this as Rails default responder doesn't
    # support returning empty response on GET request
    respond_to do |format|
      format.all { head :no_content }
      format.any(*navigational_formats) { redirect_to after_sign_out_path_for(resource_name), status: 303, notice: find_message(:signed_out) }
    end
  end

And the same issue can happen registrations#destroy action. I override destroy action in Devise::RegistrationsController too:

  def destroy
    resource.destroy
    Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)
    set_flash_message! :notice, :destroyed
    yield resource if block_given?
    respond_with_navigational(resource){ redirect_to after_sign_out_path_for(resource_name), status: 303, notice: find_message(:destroyed) }
  end

@Genkilabs
Copy link

I think this is still relevant, as seen here #5458

carlosantoniodasilva added a commit that referenced this pull request Jan 31, 2023
Treat `:turbo_stream` request format as a navigational format, much like
HTML, so Devise/responders can work properly.

Allow configuring the `error_status` and `redirect_status` using the
latest responders features, via a new custom Devise responder, so we can
customize the both responses to match Hotwire/Turbo behavior, for
example with `422 Unprocessable Entity` and `303 See Other`,
respectively. The defaults aren't changing in Devise itself (yet), so it
still responds on errors cases with `200 OK`, and redirects on non-GET
requests with `302 Found`, but new apps are generated with the new
statuses and existing apps can opt-in. Please note that these defaults
might change in a future release of Devise.

PRs/Issues references:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request Jan 31, 2023
Treat `:turbo_stream` request format as a navigational format, much like
HTML, so Devise/responders can work properly.

Allow configuring the `error_status` and `redirect_status` using the
latest responders features, via a new custom Devise responder, so we can
customize the both responses to match Hotwire/Turbo behavior, for
example with `422 Unprocessable Entity` and `303 See Other`,
respectively. The defaults aren't changing in Devise itself (yet), so it
still responds on errors cases with `200 OK`, and redirects on non-GET
requests with `302 Found`, but new apps are generated with the new
statuses and existing apps can opt-in. Please note that these defaults
might change in a future release of Devise.

PRs/Issues references:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request Jan 31, 2023
Treat `:turbo_stream` request format as a navigational format, much like
HTML, so Devise/responders can work properly.

Allow configuring the `error_status` and `redirect_status` using the
latest responders features, via a new custom Devise responder, so we can
customize the both responses to match Hotwire/Turbo behavior, for
example with `422 Unprocessable Entity` and `303 See Other`,
respectively. The defaults aren't changing in Devise itself (yet), so it
still responds on errors cases with `200 OK`, and redirects on non-GET
requests with `302 Found`, but new apps are generated with the new
statuses and existing apps can opt-in. Please note that these defaults
might change in a future release of Devise.

PRs/Issues references:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
carlosantoniodasilva added a commit that referenced this pull request Jan 31, 2023
Treat `:turbo_stream` request format as a navigational format, much like
HTML, so Devise/responders can work properly.

Allow configuring the `error_status` and `redirect_status` using the
latest responders features, via a new custom Devise responder, so we can
customize the both responses to match Hotwire/Turbo behavior, for
example with `422 Unprocessable Entity` and `303 See Other`,
respectively. The defaults aren't changing in Devise itself (yet), so it
still responds on errors cases with `200 OK`, and redirects on non-GET
requests with `302 Found`, but new apps are generated with the new
statuses and existing apps can opt-in. Please note that these defaults
might change in a future release of Devise.

PRs/Issues references:

#5545
#5529
#5516
#5499
#5487
#5467
#5440
#5410
#5340

#5542
#5530
#5519
#5513
#5478
#5468
#5463
#5458
#5448
#5446
#5439
@carlosantoniodasilva
Copy link
Member

The main branch should contain all that's necessary for fully working with Turbo now, based on the changes here and a few others. A new version will be released soon, but feel free to test it out from the main branch in the meantime, and report back on any issues. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants