From 849e874dce46f798b4df68579d5a892ce11d52e8 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 31 Jul 2013 16:20:26 -0700 Subject: [PATCH] Refactor user creation actions Create a separate #create action that handles POSTs to /user/new. This simplifies the other actions and ensures that the URL is /user/new when validation errors occur, rather than /user/terms. Fixes #398 --- app/controllers/user_controller.rb | 108 ++++++++++++------------ app/views/user/new.html.erb | 2 +- config/routes.rb | 3 +- test/functional/user_controller_test.rb | 26 +++++- test/integration/user_creation_test.rb | 34 ++++---- 5 files changed, 94 insertions(+), 79 deletions(-) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index b96b66464..57d2f7d13 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -24,63 +24,15 @@ class UserController < ApplicationController if request.xhr? render :partial => "terms" - elsif using_open_id? - # The redirect from the OpenID provider reenters here - # again and we need to pass the parameters through to - # the open_id_authentication function - @user = session.delete(:new_user) - - openid_verify(nil, @user) do |user,verified_email| - user.status = "active" if user.email == verified_email - end - - if @user.openid_url.nil? or @user.invalid? - render :action => 'new' - else - session[:new_user] = @user - render :action => 'terms' - end - elsif params[:user] and Acl.no_account_creation(request.remote_ip, params[:user][:email].split("@").last) - render :action => 'blocked' else - session[:referer] = params[:referer] - @title = t 'user.terms.title' - @user = User.new(params[:user]) if params[:user] - - if params[:user] and params[:user][:openid_url] and @user.pass_crypt.empty? - # We are creating an account with OpenID and no password - # was specified so create a random one - @user.pass_crypt = SecureRandom.base64(16) - @user.pass_crypt_confirmation = @user.pass_crypt - end - - if @user - @user.status = "pending" + @user ||= session[:new_user] - if @user.invalid? - if @user.new_record? - # Something is wrong with a new user, so rerender the form - render :action => :new - else - # Error in existing user, so go to account settings - flash[:errors] = @user.errors - redirect_to :action => :account, :display_name => @user.display_name - end - elsif @user.terms_agreed? - # Already agreed to terms, so just show settings - redirect_to :action => :account, :display_name => @user.display_name - elsif params[:user] and params[:user][:openid_url] and not params[:user][:openid_url].empty? - # Verify OpenID before moving on - session[:new_user] = @user - openid_verify(params[:user][:openid_url], @user) - elsif @user.new_record? - # Save the user record - session[:new_user] = @user - end - else - # Not logged in, so redirect to the login page + if !@user redirect_to :action => :login, :referer => request.fullpath + elsif @user.terms_agreed? + # Already agreed to terms, so just show settings + redirect_to :action => :account, :display_name => @user.display_name end end end @@ -248,7 +200,23 @@ class UserController < ApplicationController @title = t 'user.new.title' @referer = params[:referer] || session[:referer] - if @user + if using_open_id? + # The redirect from the OpenID provider reenters here + # again and we need to pass the parameters through to + # the open_id_authentication function + @user = session.delete(:new_user) + + openid_verify(nil, @user) do |user, verified_email| + user.status = "active" if user.email == verified_email + end + + if @user.openid_url.nil? or @user.invalid? + render :action => 'new' + else + session[:new_user] = @user + redirect_to :action => 'terms' + end + elsif @user # The user is logged in already, so don't show them the signup # page, instead send them to the home page if @referer @@ -268,6 +236,38 @@ class UserController < ApplicationController end end + def create + if params[:user] and Acl.no_account_creation(request.remote_ip, params[:user][:email].split("@").last) + render :action => 'blocked' + + else + session[:referer] = params[:referer] + + @user = User.new(params[:user]) + @user.status = "pending" + + if @user.openid_url.present? && @user.pass_crypt.empty? + # We are creating an account with OpenID and no password + # was specified so create a random one + @user.pass_crypt = SecureRandom.base64(16) + @user.pass_crypt_confirmation = @user.pass_crypt + end + + if @user.invalid? + # Something is wrong with a new user, so rerender the form + render :action => "new" + elsif @user.openid_url.present? + # Verify OpenID before moving on + session[:new_user] = @user + openid_verify(@user.openid_url, @user) + else + # Save the user record + session[:new_user] = @user + redirect_to :action => :terms + end + end + end + def login if params[:username] or using_open_id? session[:remember_me] ||= params[:remember_me] diff --git a/app/views/user/new.html.erb b/app/views/user/new.html.erb index 52abb3311..7324ddd29 100644 --- a/app/views/user/new.html.erb +++ b/app/views/user/new.html.erb @@ -6,7 +6,7 @@ <%= error_messages_for 'user' %> -<%= form_for :user, :url => { :action => 'terms' } do %> +<%= form_for :user, :url => { :action => 'create' } do %> <%= hidden_field_tag('referer', h(@referer)) unless @referer.nil? %>
diff --git a/config/routes.rb b/config/routes.rb index 6896fcc01..bc473a4b0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -134,7 +134,8 @@ OpenStreetMap::Application.routes.draw do match '/key' => 'site#key', :via => :get match '/id' => 'site#id', :via => :get match '/user/new' => 'user#new', :via => :get - match '/user/terms' => 'user#terms', :via => [:get, :post] + match '/user/new' => 'user#create', :via => :post + match '/user/terms' => 'user#terms', :via => :get match '/user/save' => 'user#save', :via => :post match '/user/:display_name/confirm/resend' => 'user#confirm_resend', :via => :get match '/user/:display_name/confirm' => 'user#confirm', :via => [:get, :post] diff --git a/test/functional/user_controller_test.rb b/test/functional/user_controller_test.rb index 7761c0c7e..61286b284 100644 --- a/test/functional/user_controller_test.rb +++ b/test/functional/user_controller_test.rb @@ -55,11 +55,12 @@ class UserControllerTest < ActionController::TestCase ) assert_routing( - { :path => "/user/terms", :method => :get }, - { :controller => "user", :action => "terms" } + { :path => "/user/new", :method => :post }, + { :controller => "user", :action => "create" } ) + assert_routing( - { :path => "/user/terms", :method => :post }, + { :path => "/user/terms", :method => :get }, { :controller => "user", :action => "terms" } ) @@ -202,7 +203,7 @@ class UserControllerTest < ActionController::TestCase end assert_select "body", :count => 1 do assert_select "div#content", :count => 1 do - assert_select "form[action='/user/terms'][method=post]", :count => 1 do + assert_select "form[action='/user/new'][method=post]", :count => 1 do assert_select "input[id=user_email]", :count => 1 assert_select "input[id=user_email_confirmation]", :count => 1 assert_select "input[id=user_display_name]", :count => 1 @@ -322,6 +323,23 @@ class UserControllerTest < ActionController::TestCase assert_select "div#signupForm > fieldset > div.form-row > div.field_with_errors > input#user_display_name" end + def test_user_terms_new_user + get :terms, {}, { "new_user" => User.new } + assert_response :success + assert_template :terms + end + + def test_user_terms_seen + user = users(:normal_user) + + # Set the username cookie + @request.cookies["_osm_username"] = user.display_name + + get :terms, {}, { "user" => user } + assert_response :redirect + assert_redirected_to :action => :account, :display_name => user.display_name + end + def test_user_lost_password # Test fetching the lost password page get :lost_password diff --git a/test/integration/user_creation_test.rb b/test/integration/user_creation_test.rb index 6e253afad..c565afd1e 100644 --- a/test/integration/user_creation_test.rb +++ b/test/integration/user_creation_test.rb @@ -21,7 +21,7 @@ class UserCreationTest < ActionController::IntegrationTest display_name = "#{localer.to_s}_new_tester" assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post '/user/terms', + post '/user/new', {:user => { :email => dup_email, :email_confirmation => dup_email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"}}, {"HTTP_ACCEPT_LANGUAGE" => localer.to_s} end @@ -41,7 +41,7 @@ class UserCreationTest < ActionController::IntegrationTest email = "#{locale.to_s}_new_tester" assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post '/user/terms', + post '/user/new', {:user => {:email => email, :email_confirmation => email, :display_name => dup_display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"}}, {"HTTP_ACCEPT_LANGUAGE" => locale.to_s} end @@ -61,13 +61,12 @@ class UserCreationTest < ActionController::IntegrationTest assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post "/user/terms", + post "/user/new", {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"}} end end - assert_response :success - assert_template 'user/terms' + assert_redirected_to "/user/terms" assert_difference('User.count') do assert_difference('ActionMailer::Base.deliveries.size', 1) do @@ -108,10 +107,9 @@ class UserCreationTest < ActionController::IntegrationTest referer = "/traces/mine" assert_difference('User.count') do assert_difference('ActionMailer::Base.deliveries.size', 1) do - post "/user/terms", + post "/user/new", {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :pass_crypt => password, :pass_crypt_confirmation => password}, :referer => referer } - assert_response :success - assert_template 'terms' + assert_redirected_to "/user/terms" post_via_redirect "/user/save", {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :pass_crypt => password, :pass_crypt_confirmation => password} } end @@ -154,13 +152,12 @@ class UserCreationTest < ActionController::IntegrationTest password = "testtest" assert_difference('User.count') do assert_difference('ActionMailer::Base.deliveries.size', 1) do - post "/user/terms", + post "/user/new", {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :openid_url => "http://localhost:1123/john.doe?openid.success=newuser", :pass_crypt => "", :pass_crypt_confirmation => ""}} assert_response :redirect res = openid_request(@response.redirect_url) - post '/user/terms', res - assert_response :success - assert_template 'terms' + get "/user/new", res + assert_redirected_to "/user/terms" post '/user/save', {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :openid_url => "http://localhost:1123/john.doe?openid.success=newuser", :pass_crypt => password, :pass_crypt_confirmation => password}} assert_response :redirect @@ -181,11 +178,11 @@ class UserCreationTest < ActionController::IntegrationTest password = "testtest2" assert_difference('User.count',0) do assert_difference('ActionMailer::Base.deliveries.size',0) do - post "/user/terms", + post "/user/new", {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :openid_url => "http://localhost:1123/john.doe?openid.failure=newuser", :pass_crypt => "", :pass_crypt_confirmation => ""}} assert_response :redirect res = openid_request(@response.redirect_url) - post '/user/terms', res + get '/user/new', res assert_response :success assert_template 'user/new' end @@ -202,13 +199,12 @@ class UserCreationTest < ActionController::IntegrationTest referer = "/traces/mine" assert_difference('User.count') do assert_difference('ActionMailer::Base.deliveries.size', 1) do - post "/user/terms", + post "/user/new", {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :openid_url => "http://localhost:1123/john.doe?openid.success=newuser", :pass_crypt => "", :pass_crypt_confirmation => ""}, :referer => referer } - assert_response :redirect + assert_response :redirect res = openid_request(@response.location) - post '/user/terms', res - assert_response :success - assert_template 'terms' + get "/user/new", res + assert_redirected_to "/user/terms" post_via_redirect "/user/save", {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :openid_url => "http://localhost:1123/john.doe?openid.success=newuser", :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"} } end -- 2.43.2