From 7775f86a69f187b8e162280e1f4de1a8b58fd3dd Mon Sep 17 00:00:00 2001
From: Jean-Philippe Lang <jp_lang@yahoo.fr>
Date: Thu, 13 Dec 2012 15:04:11 +0000
Subject: [PATCH] Code cleanup in AuthSource controller and views.

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@10996 e93f8b46-1217-0410-a6f0-8f06a7374b81
---
 app/controllers/auth_sources_controller.rb    | 14 ++--
 app/views/auth_sources/_form.html.erb         | 13 +---
 .../_form_auth_source_ldap.html.erb           | 64 ++++++-------------
 app/views/auth_sources/edit.html.erb          |  4 +-
 app/views/auth_sources/index.html.erb         |  2 +-
 app/views/auth_sources/new.html.erb           |  4 +-
 .../auth_sources_controller_test.rb           | 33 ++++++++--
 7 files changed, 66 insertions(+), 68 deletions(-)

diff --git a/app/controllers/auth_sources_controller.rb b/app/controllers/auth_sources_controller.rb
index 0ba89ec02..ede9e0733 100644
--- a/app/controllers/auth_sources_controller.rb
+++ b/app/controllers/auth_sources_controller.rb
@@ -20,6 +20,7 @@ class AuthSourcesController < ApplicationController
   menu_item :ldap_authentication
 
   before_filter :require_admin
+  before_filter :find_auth_source, :only => [:edit, :update, :test_connection, :destroy]
 
   def index
     @auth_source_pages, @auth_sources = paginate AuthSource, :per_page => 10
@@ -28,6 +29,7 @@ class AuthSourcesController < ApplicationController
   def new
     klass_name = params[:type] || 'AuthSourceLdap'
     @auth_source = AuthSource.new_subclass_instance(klass_name, params[:auth_source])
+    render_404 unless @auth_source
   end
 
   def create
@@ -41,11 +43,9 @@ class AuthSourcesController < ApplicationController
   end
 
   def edit
-    @auth_source = AuthSource.find(params[:id])
   end
 
   def update
-    @auth_source = AuthSource.find(params[:id])
     if @auth_source.update_attributes(params[:auth_source])
       flash[:notice] = l(:notice_successful_update)
       redirect_to auth_sources_path
@@ -55,7 +55,6 @@ class AuthSourcesController < ApplicationController
   end
 
   def test_connection
-    @auth_source = AuthSource.find(params[:id])
     begin
       @auth_source.test_connection
       flash[:notice] = l(:notice_successful_connection)
@@ -66,11 +65,18 @@ class AuthSourcesController < ApplicationController
   end
 
   def destroy
-    @auth_source = AuthSource.find(params[:id])
     unless @auth_source.users.exists?
       @auth_source.destroy
       flash[:notice] = l(:notice_successful_delete)
     end
     redirect_to auth_sources_path
   end
+
+  private
+
+  def find_auth_source
+    @auth_source = AuthSource.find(params[:id])
+  rescue ActiveRecord::RecordNotFound
+    render_404
+  end
 end
diff --git a/app/views/auth_sources/_form.html.erb b/app/views/auth_sources/_form.html.erb
index 79abae7d8..05c6ca9e9 100644
--- a/app/views/auth_sources/_form.html.erb
+++ b/app/views/auth_sources/_form.html.erb
@@ -1,13 +1,6 @@
 <%= error_messages_for 'auth_source' %>
 
-<div class="box">
-<!--[form:auth_source]-->
-<p><label for="auth_source_name"><%=l(:field_name)%> <span class="required">*</span></label>
-<%= text_field 'auth_source', 'name'  %></p>
-
-<p><label for="auth_source_onthefly_register"><%=l(:field_onthefly)%></label>
-<%= check_box 'auth_source', 'onthefly_register' %></p>
+<div class="box tabular">
+  <p><%= f.text_field :name, :required => true %></p>
+  <p><%= f.check_box :onthefly_register, :label => :field_onthefly %></p>
 </div>
-
-<!--[eoform:auth_source]-->
-
diff --git a/app/views/auth_sources/_form_auth_source_ldap.html.erb b/app/views/auth_sources/_form_auth_source_ldap.html.erb
index 2f6d0b0ce..2ffd4d43e 100644
--- a/app/views/auth_sources/_form_auth_source_ldap.html.erb
+++ b/app/views/auth_sources/_form_auth_source_ldap.html.erb
@@ -1,50 +1,24 @@
 <%= error_messages_for 'auth_source' %>
 
-<div class="box">
-<!--[form:auth_source]-->
-<p><label for="auth_source_name"><%=l(:field_name)%> <span class="required">*</span></label>
-<%= text_field 'auth_source', 'name'  %></p>
-
-<p><label for="auth_source_host"><%=l(:field_host)%> <span class="required">*</span></label>
-<%= text_field 'auth_source', 'host'  %></p>
-
-<p><label for="auth_source_port"><%=l(:field_port)%> <span class="required">*</span></label>
-<%= text_field 'auth_source', 'port', :size => 6 %> <%= check_box 'auth_source', 'tls'  %> LDAPS</p>
-
-<p><label for="auth_source_account"><%=l(:field_account)%></label>
-<%= text_field 'auth_source', 'account'  %></p>
-
-<p><label for="auth_source_account_password"><%=l(:field_password)%></label>
-<%= password_field 'auth_source', 'account_password', :name => 'ignore',
-                                           :value => ((@auth_source.new_record? || @auth_source.account_password.blank?) ? '' : ('x'*15)),
-                                           :onfocus => "this.value=''; this.name='auth_source[account_password]';",
-                                           :onchange => "this.name='auth_source[account_password]';" %></p>
-
-<p><label for="auth_source_base_dn"><%=l(:field_base_dn)%> <span class="required">*</span></label>
-<%= text_field 'auth_source', 'base_dn', :size => 60 %></p>
-
-<p><label for="auth_source_custom_filter"><%=l(:field_auth_source_ldap_filter)%></label>
-<%= text_field 'auth_source', 'filter', :size => 60 %></p>
-
-<p><label for="auth_source_timeout"><%=l(:field_timeout)%></label>
-<%= text_field 'auth_source', 'timeout', :size => 4 %></p>
-
-<p><label for="auth_source_onthefly_register"><%=l(:field_onthefly)%></label>
-<%= check_box 'auth_source', 'onthefly_register' %></p>
+<div class="box tabular">
+  <p><%= f.text_field :name, :required => true %></p>
+  <p><%= f.text_field :host, :required => true %></p>
+  <p><%= f.text_field :port, :required => true, :size => 6 %> <%= f.check_box :tls, :no_label => true %> LDAPS</p>
+  <p><%= f.text_field :account %></p>
+  <p><%= f.password_field :account_password, :label => :field_password,
+           :name => 'dummy_password',
+           :value => ((@auth_source.new_record? || @auth_source.account_password.blank?) ? '' : ('x'*15)),
+           :onfocus => "this.value=''; this.name='auth_source[account_password]';",
+           :onchange => "this.name='auth_source[account_password]';" %></p>
+  <p><%= f.text_field :base_dn, :required => true, :size => 60 %></p>
+  <p><%= f.text_field :filter, :size => 60, :label => :field_auth_source_ldap_filter %></p>
+  <p><%= f.text_field :timeout, :size => 4 %></p>
+  <p><%= f.check_box :onthefly_register, :label => :field_onthefly %></p>
 </div>
 
-<fieldset class="box"><legend><%=l(:label_attribute_plural)%></legend>
-<p><label for="auth_source_attr_login"><%=l(:field_login)%> <span class="required">*</span></label>
-<%= text_field 'auth_source', 'attr_login', :size => 20  %></p>
-
-<p><label for="auth_source_attr_firstname"><%=l(:field_firstname)%></label>
-<%= text_field 'auth_source', 'attr_firstname', :size => 20  %></p>
-
-<p><label for="auth_source_attr_lastname"><%=l(:field_lastname)%></label>
-<%= text_field 'auth_source', 'attr_lastname', :size => 20  %></p>
-
-<p><label for="auth_source_attr_mail"><%=l(:field_mail)%></label>
-<%= text_field 'auth_source', 'attr_mail', :size => 20  %></p>
+<fieldset class="box tabular"><legend><%=l(:label_attribute_plural)%></legend>
+  <p><%= f.text_field :attr_login, :required => true, :size => 20 %></p>
+  <p><%= f.text_field :attr_firstname, :size => 20 %></p>
+  <p><%= f.text_field :attr_lastname, :size => 20 %></p>
+  <p><%= f.text_field :attr_mail, :size => 20 %></p>
 </fieldset>
-<!--[eoform:auth_source]-->
-
diff --git a/app/views/auth_sources/edit.html.erb b/app/views/auth_sources/edit.html.erb
index 1673d8336..b84d60053 100644
--- a/app/views/auth_sources/edit.html.erb
+++ b/app/views/auth_sources/edit.html.erb
@@ -1,6 +1,6 @@
 <h2><%=l(:label_auth_source)%> (<%= h(@auth_source.auth_method_name) %>)</h2>
 
-<%= form_tag({:action => 'update', :id => @auth_source}, :method => :put, :class => "tabular") do %>
-  <%= render :partial => auth_source_partial_name(@auth_source) %>
+<%= form_for @auth_source, :as => :auth_source, :url => auth_source_path(@auth_source), :html => {:id => 'auth_source_form'} do |f| %>
+  <%= render :partial => auth_source_partial_name(@auth_source), :locals => { :f => f } %>
   <%= submit_tag l(:button_save) %>
 <% end %>
diff --git a/app/views/auth_sources/index.html.erb b/app/views/auth_sources/index.html.erb
index 045207a28..3d7fa8ff6 100644
--- a/app/views/auth_sources/index.html.erb
+++ b/app/views/auth_sources/index.html.erb
@@ -20,7 +20,7 @@
     <td align="center"><%= h source.host %></td>
     <td align="center"><%= h source.users.count %></td>
     <td class="buttons">
-      <%= link_to l(:button_test), {:action => 'test_connection', :id => source}, :class => 'icon icon-test' %>
+      <%= link_to l(:button_test), try_connection_auth_source_path(source), :class => 'icon icon-test' %>
       <%= delete_link auth_source_path(source) %>
     </td>
   </tr>
diff --git a/app/views/auth_sources/new.html.erb b/app/views/auth_sources/new.html.erb
index b585c0aa0..d6d6bc5b3 100644
--- a/app/views/auth_sources/new.html.erb
+++ b/app/views/auth_sources/new.html.erb
@@ -1,7 +1,7 @@
 <h2><%=l(:label_auth_source_new)%> (<%= h(@auth_source.auth_method_name) %>)</h2>
 
-<%= form_tag({:action => 'create'}, :class => "tabular") do %>
+<%= labelled_form_for @auth_source, :as => :auth_source, :url => auth_sources_path, :html => {:id => 'auth_source_form'} do |f| %>
   <%= hidden_field_tag 'type', @auth_source.type %>
-  <%= render :partial => auth_source_partial_name(@auth_source) %>
+  <%= render :partial => auth_source_partial_name(@auth_source), :locals => { :f => f } %>
   <%= submit_tag l(:button_create) %>
 <% end %>
diff --git a/test/functional/auth_sources_controller_test.rb b/test/functional/auth_sources_controller_test.rb
index ff3c5047c..c64dca66d 100644
--- a/test/functional/auth_sources_controller_test.rb
+++ b/test/functional/auth_sources_controller_test.rb
@@ -42,8 +42,15 @@ class AuthSourcesControllerTest < ActionController::TestCase
     assert_equal AuthSourceLdap, source.class
     assert source.new_record?
 
-    assert_tag 'input', :attributes => {:name => 'type', :value => 'AuthSourceLdap'}
-    assert_tag 'input', :attributes => {:name => 'auth_source[host]'}
+    assert_select 'form#auth_source_form' do
+      assert_select 'input[name=type][value=AuthSourceLdap]'
+      assert_select 'input[name=?]', 'auth_source[host]'
+    end
+  end
+
+  def test_new_with_invalid_type_should_respond_with_404
+    get :new, :type => 'foo'
+    assert_response 404
   end
 
   def test_create
@@ -52,7 +59,7 @@ class AuthSourcesControllerTest < ActionController::TestCase
       assert_redirected_to '/auth_sources'
     end
 
-    source = AuthSourceLdap.first(:order => 'id DESC')
+    source = AuthSourceLdap.order('id DESC').first
     assert_equal 'Test', source.name
     assert_equal '127.0.0.1', source.host
     assert_equal 389, source.port
@@ -74,7 +81,23 @@ class AuthSourcesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template 'edit'
 
-    assert_tag 'input', :attributes => {:name => 'auth_source[host]'}
+    assert_select 'form#auth_source_form' do
+      assert_select 'input[name=?]', 'auth_source[host]'
+    end
+  end
+
+  def test_edit_should_not_contain_password
+    AuthSource.find(1).update_column :account_password, 'secret'
+
+    get :edit, :id => 1
+    assert_response :success
+    assert_select 'input[value=secret]', 0
+    assert_select 'input[name=dummy_password][value=?]', /x+/
+  end
+
+  def test_edit_invalid_should_respond_with_404
+    get :edit, :id => 99
+    assert_response 404
   end
 
   def test_update
@@ -96,6 +119,7 @@ class AuthSourcesControllerTest < ActionController::TestCase
   def test_destroy
     assert_difference 'AuthSourceLdap.count', -1 do
       delete :destroy, :id => 1
+      assert_redirected_to '/auth_sources'
     end
   end
 
@@ -104,6 +128,7 @@ class AuthSourcesControllerTest < ActionController::TestCase
 
     assert_no_difference 'AuthSourceLdap.count' do
       delete :destroy, :id => 1
+      assert_redirected_to '/auth_sources'
     end
   end