From 86ba692bf5312b37b6e30778d14daf0a675254bb Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Thu, 30 Sep 2010 18:22:46 +0000 Subject: [PATCH] Refactor: split UsersController#edit into #edit and #update git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@4230 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/users_controller.rb | 69 ++++++++++++++---------- app/helpers/users_helper.rb | 8 +-- app/views/users/_general.rhtml | 2 +- app/views/users/_groups.rhtml | 2 +- config/routes.rb | 2 +- test/functional/users_controller_test.rb | 16 +++--- test/integration/admin_test.rb | 2 +- test/integration/routing_test.rb | 3 +- 8 files changed, 58 insertions(+), 46 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 1fd8347b7..66979d5e2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -116,40 +116,51 @@ class UsersController < ApplicationController @notification_options = @user.valid_notification_options @notification_option = @user.mail_notification - if request.post? - @user.admin = params[:user][:admin] if params[:user][:admin] - @user.login = params[:user][:login] if params[:user][:login] - if params[:password].present? && (@user.auth_source_id.nil? || params[:user][:auth_source_id].blank?) - @user.password, @user.password_confirmation = params[:password], params[:password_confirmation] - end - @user.group_ids = params[:user][:group_ids] if params[:user][:group_ids] - @user.attributes = params[:user] - # Was the account actived ? (do it before User#save clears the change) - was_activated = (@user.status_change == [User::STATUS_REGISTERED, User::STATUS_ACTIVE]) - # TODO: Similar to My#account - @user.mail_notification = params[:notification_option] || 'only_my_events' - @user.pref.attributes = params[:pref] - @user.pref[:no_self_notified] = (params[:no_self_notified] == '1') - - if @user.save - @user.pref.save - @user.notified_project_ids = (params[:notification_option] == 'selected' ? params[:notified_project_ids] : []) - - if was_activated - Mailer.deliver_account_activated(@user) - elsif @user.active? && params[:send_information] && !params[:password].blank? && @user.auth_source_id.nil? - Mailer.deliver_account_information(@user, params[:password]) - end - flash[:notice] = l(:notice_successful_update) - redirect_to :back - end - end @auth_sources = AuthSource.find(:all) @membership ||= Member.new + end + + verify :method => :put, :only => :update, :render => {:nothing => true, :status => :method_not_allowed } + def update + @user = User.find(params[:id]) + @notification_options = @user.valid_notification_options + @notification_option = @user.mail_notification + + @user.admin = params[:user][:admin] if params[:user][:admin] + @user.login = params[:user][:login] if params[:user][:login] + if params[:password].present? && (@user.auth_source_id.nil? || params[:user][:auth_source_id].blank?) + @user.password, @user.password_confirmation = params[:password], params[:password_confirmation] + end + @user.group_ids = params[:user][:group_ids] if params[:user][:group_ids] + @user.attributes = params[:user] + # Was the account actived ? (do it before User#save clears the change) + was_activated = (@user.status_change == [User::STATUS_REGISTERED, User::STATUS_ACTIVE]) + # TODO: Similar to My#account + @user.mail_notification = params[:notification_option] || 'only_my_events' + @user.pref.attributes = params[:pref] + @user.pref[:no_self_notified] = (params[:no_self_notified] == '1') + + if @user.save + @user.pref.save + @user.notified_project_ids = (params[:notification_option] == 'selected' ? params[:notified_project_ids] : []) + + if was_activated + Mailer.deliver_account_activated(@user) + elsif @user.active? && params[:send_information] && !params[:password].blank? && @user.auth_source_id.nil? + Mailer.deliver_account_information(@user, params[:password]) + end + flash[:notice] = l(:notice_successful_update) + redirect_to :back + else + @auth_sources = AuthSource.find(:all) + @membership ||= Member.new + + render :action => :edit + end rescue ::ActionController::RedirectBackError redirect_to :controller => 'users', :action => 'edit', :id => @user end - + def edit_membership @user = User.find(params[:id]) @membership = Member.edit_membership(params[:membership_id], params[:membership], @user) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 757a91a9f..37cecc05c 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -34,14 +34,14 @@ module UsersHelper end def change_status_link(user) - url = {:controller => 'users', :action => 'edit', :id => user, :page => params[:page], :status => params[:status], :tab => nil} + url = {:controller => 'users', :action => 'update', :id => user, :page => params[:page], :status => params[:status], :tab => nil} if user.locked? - link_to l(:button_unlock), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :post, :class => 'icon icon-unlock' + link_to l(:button_unlock), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock' elsif user.registered? - link_to l(:button_activate), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :post, :class => 'icon icon-unlock' + link_to l(:button_activate), url.merge(:user => {:status => User::STATUS_ACTIVE}), :method => :put, :class => 'icon icon-unlock' elsif user != User.current - link_to l(:button_lock), url.merge(:user => {:status => User::STATUS_LOCKED}), :method => :post, :class => 'icon icon-lock' + link_to l(:button_lock), url.merge(:user => {:status => User::STATUS_LOCKED}), :method => :put, :class => 'icon icon-lock' end end diff --git a/app/views/users/_general.rhtml b/app/views/users/_general.rhtml index e962056a2..a08b3cee3 100644 --- a/app/views/users/_general.rhtml +++ b/app/views/users/_general.rhtml @@ -1,4 +1,4 @@ -<% labelled_tabular_form_for :user, @user, :url => { :controller => 'users', :action => "edit", :tab => nil }, :html => { :class => nil } do |f| %> +<% labelled_tabular_form_for :user, @user, :url => { :controller => 'users', :action => "update", :tab => nil }, :html => { :method => :put, :class => nil } do |f| %> <%= render :partial => 'form', :locals => { :f => f } %> <% if @user.active? -%>

diff --git a/app/views/users/_groups.rhtml b/app/views/users/_groups.rhtml index 4bca77c00..0ab2f11eb 100644 --- a/app/views/users/_groups.rhtml +++ b/app/views/users/_groups.rhtml @@ -1,4 +1,4 @@ -<% form_for(:user, :url => { :action => 'edit' }) do %> +<% form_for(:user, :url => { :action => 'update' }, :html => {:method => :put}) do %>

<% Group.all.sort.each do |group| %>
diff --git a/config/routes.rb b/config/routes.rb index d9c22d6cc..a15f06a07 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -148,11 +148,11 @@ ActionController::Routing::Routes.draw do |map| end users.with_options :conditions => {:method => :post} do |user_actions| user_actions.connect 'users/new', :action => 'create' - user_actions.connect 'users/:id/edit', :action => 'edit' user_actions.connect 'users/:id/memberships', :action => 'edit_membership' user_actions.connect 'users/:id/memberships/:membership_id', :action => 'edit_membership' user_actions.connect 'users/:id/memberships/:membership_id/destroy', :action => 'destroy_membership' end + users.connect 'users/:id/edit', :action => 'update', :conditions => {:method => :put} end # For nice "roadmap" in the url for the index action diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 577f54b07..5e288d445 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -153,9 +153,9 @@ class UsersControllerTest < ActionController::TestCase end - def test_edit + def test_update ActionMailer::Base.deliveries.clear - post :edit, :id => 2, :user => {:firstname => 'Changed'}, :notification_option => 'all', :pref => {:hide_mail => '1', :comments_sorting => 'desc'} + put :update, :id => 2, :user => {:firstname => 'Changed'}, :notification_option => 'all', :pref => {:hide_mail => '1', :comments_sorting => 'desc'} user = User.find(2) assert_equal 'Changed', user.firstname @@ -165,7 +165,7 @@ class UsersControllerTest < ActionController::TestCase assert ActionMailer::Base.deliveries.empty? end - def test_edit_with_activation_should_send_a_notification + def test_update_with_activation_should_send_a_notification u = User.new(:firstname => 'Foo', :lastname => 'Bar', :mail => 'foo.bar@somenet.foo', :language => 'fr') u.login = 'foo' u.status = User::STATUS_REGISTERED @@ -173,7 +173,7 @@ class UsersControllerTest < ActionController::TestCase ActionMailer::Base.deliveries.clear Setting.bcc_recipients = '1' - post :edit, :id => u.id, :user => {:status => User::STATUS_ACTIVE} + put :update, :id => u.id, :user => {:status => User::STATUS_ACTIVE} assert u.reload.active? mail = ActionMailer::Base.deliveries.last assert_not_nil mail @@ -181,12 +181,12 @@ class UsersControllerTest < ActionController::TestCase assert mail.body.include?(ll('fr', :notice_account_activated)) end - def test_edit_with_password_change_should_send_a_notification + def test_updat_with_password_change_should_send_a_notification ActionMailer::Base.deliveries.clear Setting.bcc_recipients = '1' u = User.find(2) - post :edit, :id => u.id, :user => {}, :password => 'newpass', :password_confirmation => 'newpass', :send_information => '1' + put :update, :id => u.id, :user => {}, :password => 'newpass', :password_confirmation => 'newpass', :send_information => '1' assert_equal User.hash_password('newpass'), u.reload.hashed_password mail = ActionMailer::Base.deliveries.last @@ -195,13 +195,13 @@ class UsersControllerTest < ActionController::TestCase assert mail.body.include?('newpass') end - test "POST :edit with a password change to an AuthSource user switching to Internal authentication" do + test "put :update with a password change to an AuthSource user switching to Internal authentication" do # Configure as auth source u = User.find(2) u.auth_source = AuthSource.find(1) u.save! - post :edit, :id => u.id, :user => {:auth_source_id => ''}, :password => 'newpass', :password_confirmation => 'newpass' + put :update, :id => u.id, :user => {:auth_source_id => ''}, :password => 'newpass', :password_confirmation => 'newpass' assert_equal nil, u.reload.auth_source assert_equal User.hash_password('newpass'), u.reload.hashed_password diff --git a/test/integration/admin_test.rb b/test/integration/admin_test.rb index dd52859cf..0b736199b 100644 --- a/test/integration/admin_test.rb +++ b/test/integration/admin_test.rb @@ -35,7 +35,7 @@ class AdminTest < ActionController::IntegrationTest assert_kind_of User, logged_user assert_equal "Paul", logged_user.firstname - post "users/edit", :id => user.id, :user => { :status => User::STATUS_LOCKED } + put "users/#{user.id}/edit", :id => user.id, :user => { :status => User::STATUS_LOCKED } assert_redirected_to "/users/#{ user.id }/edit" locked_user = User.try_to_login("psmith", "psmith09") assert_equal nil, locked_user diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb index 341efe3cd..030f6b18a 100644 --- a/test/integration/routing_test.rb +++ b/test/integration/routing_test.rb @@ -251,10 +251,11 @@ class RoutingTest < ActionController::IntegrationTest should_route :get, "/users/222/edit/membership", :controller => 'users', :action => 'edit', :id => '222', :tab => 'membership' should_route :post, "/users/new", :controller => 'users', :action => 'create' - should_route :post, "/users/444/edit", :controller => 'users', :action => 'edit', :id => '444' should_route :post, "/users/123/memberships", :controller => 'users', :action => 'edit_membership', :id => '123' should_route :post, "/users/123/memberships/55", :controller => 'users', :action => 'edit_membership', :id => '123', :membership_id => '55' should_route :post, "/users/567/memberships/12/destroy", :controller => 'users', :action => 'destroy_membership', :id => '567', :membership_id => '12' + + should_route :put, "/users/444/edit", :controller => 'users', :action => 'update', :id => '444' end # TODO: should they all be scoped under /projects/:project_id ? -- 2.39.5