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

diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index b96b66464445575fabf8dc26436fa9658c185590..57d2f7d132c299f4f323959b491cc8b644b2b597 100644 (file)
--- 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"
 
     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
     else

-      session[:referer] = params[:referer]
-

       @title = t 'user.terms.title'
       @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
         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
       end
     end
   end


@@ -248,7 +200,23 @@ class UserController < ApplicationController
     @title = t 'user.new.title'
     @referer = params[:referer] || session[:referer]
 
     @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
       # 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
 
     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]
   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 52abb33113e3837c6824a2c251556b953c97e5f1..7324ddd29773a8edb0c585afa122511245d1a289 100644 (file)
--- a/app/views/user/new.html.erb
+++ b/app/views/user/new.html.erb
@@ -6,7 +6,7 @@
 
 <%= error_messages_for 'user' %>
 
 
 <%= 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? %>
 
   <div id="signupForm" class="standard-form">
   <%= hidden_field_tag('referer', h(@referer)) unless @referer.nil? %>
 
   <div id="signupForm" class="standard-form">

diff --git a/config/routes.rb b/config/routes.rb
index 6896fcc01f7bc39d3cf51094d61d77f1b2124d5c..bc473a4b0d74d86724add2413aef7d6f05addc63 100644 (file)
--- 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 '/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]
   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 7761c0c7e30f1563fe7ced1cb2890d3bec38a24c..61286b2845e94c660e5bc930f8649e6841aba262 100644 (file)
--- a/test/functional/user_controller_test.rb
+++ b/test/functional/user_controller_test.rb
@@ -55,11 +55,12 @@ class UserControllerTest < ActionController::TestCase
     )
 
     assert_routing(
     )
 
     assert_routing(

-      { :path => "/user/terms", :method => :get },
-      { :controller => "user", :action => "terms" }
+      { :path => "/user/new", :method => :post },
+      { :controller => "user", :action => "create" }

     )
     )

+

     assert_routing(
     assert_routing(

-      { :path => "/user/terms", :method => :post },
+      { :path => "/user/terms", :method => :get },

       { :controller => "user", :action => "terms" }
     )
 
       { :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
       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
             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
 
     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
   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 6e253afade6176951c3bffac53985aed7489b576..c565afd1ed132961f2ae4f49175b3995f7c0d275 100644 (file)
--- 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
       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
             {: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
       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
           {: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
 
       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
 
             {: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
 
       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
     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 }
         {: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
         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
     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)
           {: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
         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
     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)
           {: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
         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
     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 }
           {: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)
         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
         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