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.

Sign up for GitHub

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

Apply changes for rails-ujs and Turbo compatibility #5545

Closed
wants to merge 1 commit into from
Closed

Apply changes for rails-ujs and Turbo compatibility #5545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/devise/confirmations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def create
if successfully_sent?(resource)
respond_with({}, location: after_resending_confirmation_instructions_path_for(resource_name))
else
respond_with(resource)
respond_with resource, status: :unprocessable_entity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: We will be able to use responders to handle this for us across the board going forward.

Please see https://github.com/heartcombo/responders/blob/fb9f787055a7a842584ce351793b249676290090/CHANGELOG.md#unreleased (still unreleased at the time of this writing)

end
end

Expand Down
4 changes: 2 additions & 2 deletions app/controllers/devise/passwords_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def create
if successfully_sent?(resource)
respond_with({}, location: after_sending_reset_password_instructions_path_for(resource_name))
else
respond_with(resource)
respond_with resource, status: :unprocessable_entity
end
end

Expand Down Expand Up @@ -47,7 +47,7 @@ def update
respond_with resource, location: after_resetting_password_path_for(resource)
else
set_minimum_password_length
respond_with resource
respond_with resource, status: :unprocessable_entity
end
end

Expand Down
6 changes: 3 additions & 3 deletions app/controllers/devise/registrations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def create
else
clean_up_passwords resource
set_minimum_password_length
respond_with resource
respond_with resource, status: :unprocessable_entity
end
end

Expand All @@ -57,7 +57,7 @@ def update
else
clean_up_passwords resource
set_minimum_password_length
respond_with resource
respond_with resource, status: :unprocessable_entity
end
end

Expand All @@ -67,7 +67,7 @@ def 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) }
respond_with_navigational(resource){ redirect_to after_sign_out_path_for(resource_name), status: :see_other }
end

# GET /resource/cancel
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/devise/sessions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def respond_to_on_destroy
# 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) }
format.any(*navigational_formats) { redirect_to after_sign_out_path_for(resource_name), status: :see_other }
end
end
end
2 changes: 1 addition & 1 deletion app/controllers/devise/unlocks_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def create
if successfully_sent?(resource)
respond_with({}, location: after_sending_unlock_instructions_path_for(resource))
else
respond_with(resource)
respond_with resource, status: :unprocessable_entity
end
end

Expand Down
2 changes: 1 addition & 1 deletion app/views/devise/registrations/edit.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,6 @@

<h3>Cancel my account</h3>

<p>Unhappy? <%= button_to "Cancel my account", registration_path(resource_name), data: { confirm: "Are you sure?" }, method: :delete %></p>
<p>Unhappy? <%= button_to "Cancel my account", registration_path(resource_name), data: { confirm: "Are you sure?", turbo_confirm: "Are you sure?" }, method: :delete %></p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good option to support both confirm options.


<%= link_to "Back", :back %>
2 changes: 1 addition & 1 deletion app/views/devise/shared/_links.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@

<%- if devise_mapping.omniauthable? %>
<%- resource_class.omniauth_providers.each do |provider| %>
<%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider), method: :post %><br />
<%= button_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider), data: { turbo: false } %><br />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the best option for this one might be a button since this is not something that should go through turbo I guess?

The only other option that could work would be two methods. (method: :post and data: { turbo_method: :post }), but again I'm not sure about the Turbo behavior for something like this so may be better to disable it entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I tested, Turbo didn't work with OAuth because it raises CORS error.

Here is the result when I click the link generated via <%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider), method: :post, data: { turbo_method: :post } %>:

Screenshot 2023-02-01 at 19 20 54

So I had to change this line from link to button and add data: { turbo: false } option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I hadn't thought about CORS but yeah since it's all happening via JS it will trigger a CORS error when trying to follow the redirect with fetch under the hood.

button_to it is then, to support this correctly on all cases, since there's no way to do POST with turbo like rails-ujs does. Thanks!

<% end %>
<% end %>
2 changes: 1 addition & 1 deletion lib/devise.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ module Test

# Which formats should be treated as navigational.
mattr_accessor :navigational_formats
@@navigational_formats = ["*/*", :html]
@@navigational_formats = ["*/*", :html, :turbo_stream]

# When set to true, signing out a user signs out all other scopes.
mattr_accessor :sign_out_all_scopes
Expand Down
4 changes: 2 additions & 2 deletions lib/devise/failure_app.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def respond
if http_auth?
http_auth
elsif warden_options[:recall]
recall
request_format == :turbo_stream ? redirect : recall
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid that while redirecting works in the sense that it will re-display the form as expected, it will lose some context about the flash / error message and show the more generic "you need to be signed in" instead of the "invalid email or password".

Ideally we'd still recall, but change the status, so it has that same behavior.

else
redirect
end
Expand Down Expand Up @@ -167,7 +167,7 @@ def scope_url
end

def skip_format?
%w(html */*).include? request_format.to_s
%w(html */* turbo_stream).include? request_format.to_s
end

# Choose whether we should respond in an HTTP authentication fashion,
Expand Down
2 changes: 1 addition & 1 deletion lib/generators/templates/simple_form_for/registrations/edit.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@

<h3>Cancel my account</h3>

<p>Unhappy? <%= link_to "Cancel my account", registration_path(resource_name), data: { confirm: "Are you sure?" }, method: :delete %></p>
<p>Unhappy? <%= button_to "Cancel my account", registration_path(resource_name), data: { confirm: "Are you sure?", turbo_confirm: "Are you sure?" }, method: :delete %></p>

<%= link_to "Back", :back %>
19 changes: 19 additions & 0 deletions test/failure_app_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

require 'test_helper'
require 'ostruct'
require 'minitest/mock'

class FailureTest < ActiveSupport::TestCase
class RootFailureApp < Devise::FailureApp
Expand Down Expand Up @@ -65,6 +66,10 @@ def fake_engine
end
end

class SessionStorableFailureApp < Devise::FailureApp
def store_location_for(*); end
end

class RequestWithoutFlashSupport < ActionDispatch::Request
undef_method :flash
end
Expand Down Expand Up @@ -355,6 +360,20 @@ def call_failure(env_params = {})
assert_includes @response.third.body, 'Your account is not activated yet.'
end

test 'returns to the default redirect location for turbo_stream' do
Mime::Type.register "text/vnd.turbo-stream.html", :turbo_stream
env = {
"warden.options" => { recall: "devise/sessions#new", attempted_path: "/users/sign_in" },
"devise.mapping" => Devise.mappings[:user],
"warden" => stub_everything,
"formats" => Mime[:turbo_stream],
app: SessionStorableFailureApp
}
call_failure(env)
assert_equal 302, @response.first
assert_equal 'http://test.host/users/sign_in', @response.second['Location']
end

if Rails.application.config.respond_to?(:relative_url_root)
test 'calls the original controller with the proper environment considering the relative url root' do
swap Rails.application.config, relative_url_root: "/sample" do
Expand Down
4 changes: 2 additions & 2 deletions test/integration/authenticatable_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class AuthenticationSanityTest < Devise::IntegrationTest

delete destroy_admin_session_path
assert_response :redirect
assert_redirected_to root_path
assert_see_other_to root_path

get root_path
assert_contain 'Signed out successfully'
Expand All @@ -129,7 +129,7 @@ class AuthenticationSanityTest < Devise::IntegrationTest
test 'unauthenticated admin set message on sign out' do
delete destroy_admin_session_path
assert_response :redirect
assert_redirected_to root_path
assert_see_other_to root_path

get root_path
assert_contain 'Signed out successfully'
Expand Down
12 changes: 8 additions & 4 deletions test/integration/omniauthable_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,19 @@ def stub_action!(name)
end
end

test "generates a link to authenticate with provider" do
test "generates a button to authenticate with provider" do
visit "/users/sign_in"
assert_select "a[href=?][data-method='post']", "/users/auth/facebook", text: "Sign in with FaceBook"
assert_select "form[action=?]", "/users/auth/facebook" do
assert_select "input[value=?]", "Sign in with FaceBook"
end
end

test "generates a proper link when SCRIPT_NAME is set" do
test "generates a proper button when SCRIPT_NAME is set" do
header 'SCRIPT_NAME', '/q'
visit "/users/sign_in"
assert_select "a[href=?][data-method='post']", "/q/users/auth/facebook", text: "Sign in with FaceBook"
assert_select "form[action=?]", "/q/users/auth/facebook" do
assert_select "input[value=?]", "Sign in with FaceBook"
end
end

test "handles callback error parameter according to the specification" do
Expand Down
12 changes: 6 additions & 6 deletions test/integration/recoverable_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def reset_password(options = {}, &block)
fill_in 'email', with: '[email protected]'
end

assert_response :success
assert_response :unprocessable_entity
assert_current_url '/users/password'
assert_have_selector "input[type=email][value='[email protected]']"
assert_contain 'not found'
Expand All @@ -102,7 +102,7 @@ def reset_password(options = {}, &block)
fill_in 'email', with: ' [email protected] '
end

assert_response :success
assert_response :unprocessable_entity
assert_current_url '/users/password'
assert_have_selector "input[type=email][value=' [email protected] ']"
assert_contain 'not found'
Expand Down Expand Up @@ -132,7 +132,7 @@ def reset_password(options = {}, &block)
fill_in 'email', with: '[email protected]'
end

assert_response :success
assert_response :unprocessable_entity
assert_current_url '/users/password'
assert_have_selector "input[type=email][value='[email protected]']"
assert_contain 'not found'
Expand All @@ -156,7 +156,7 @@ def reset_password(options = {}, &block)
user = create_user
reset_password reset_password_token: 'invalid_reset_password'

assert_response :success
assert_response :unprocessable_entity
assert_current_url '/users/password'
assert_have_selector '#error_explanation'
assert_contain %r{Reset password token(.*)invalid}
Expand All @@ -170,7 +170,7 @@ def reset_password(options = {}, &block)
fill_in 'Confirm new password', with: 'other_password'
end

assert_response :success
assert_response :unprocessable_entity
assert_current_url '/users/password'
assert_have_selector '#error_explanation'
assert_contain "Password confirmation doesn't match Password"
Expand All @@ -192,7 +192,7 @@ def reset_password(options = {}, &block)
request_forgot_password

reset_password { fill_in 'Confirm new password', with: 'other_password' }
assert_response :success
assert_response :unprocessable_entity
assert_have_selector '#error_explanation'
refute user.reload.valid_password?('987654321')

Expand Down
4 changes: 2 additions & 2 deletions test/integration/timeoutable_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def last_request_at
delete destroy_user_session_path

assert_response :redirect
assert_redirected_to root_path
assert_see_other_to root_path
follow_redirect!
assert_contain 'Signed out successfully'
end
Expand All @@ -109,7 +109,7 @@ def last_request_at
follow_redirect!

assert_response :success
assert_contain 'Sign in'
assert_contain 'Log in'
refute warden.authenticated?(:user)
end

Expand Down
7 changes: 7 additions & 0 deletions test/support/integration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,13 @@ def assert_redirected_to(url)
assert_url url, @integration_session.headers["Location"]
end

def assert_see_other_to(url)
assert_equal 303, @integration_session.status,
"Expected status to be 303, got #{@integration_session.status}"

assert_url url, @integration_session.headers["Location"]
end

def assert_current_url(expected)
assert_url expected, current_url
end
Expand Down