From e1d873cde99eaa6eafaca6659c221ec2b98de36e Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Mon, 19 Mar 2012 11:26:02 +0000 Subject: [PATCH] Add functional tests for user blocks Also fixes various issues in the code discovered while writing the tests, and adds some named routes for user blocks. --- app/controllers/application_controller.rb | 8 +- app/controllers/user_blocks_controller.rb | 80 ++--- config/routes.rb | 6 +- test/fixtures/user_blocks.yml | 29 ++ test/fixtures/user_roles.yml | 5 + test/fixtures/users.yml | 38 ++ .../functional/user_blocks_controller_test.rb | 329 ++++++++++++++++++ test/unit/user_test.rb | 6 +- 8 files changed, 455 insertions(+), 46 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6caed0594..0f9fad45e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -51,7 +51,13 @@ class ApplicationController < ActionController::Base end def require_user - redirect_to :controller => 'user', :action => 'login', :referer => request.fullpath unless @user + unless @user + if request.get? + redirect_to :controller => 'user', :action => 'login', :referer => request.fullpath + else + render :nothing => true, :status => :forbidden + end + end end ## diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb index 2bdafa80e..26b698b60 100644 --- a/app/controllers/user_blocks_controller.rb +++ b/app/controllers/user_blocks_controller.rb @@ -4,7 +4,7 @@ class UserBlocksController < ApplicationController before_filter :authorize_web before_filter :set_locale before_filter :require_user, :only => [:new, :create, :edit, :update, :revoke] - before_filter :require_moderator, :only => [:create, :update, :revoke] + before_filter :require_moderator, :only => [:new, :create, :edit, :update, :revoke] before_filter :lookup_this_user, :only => [:new, :create, :blocks_on, :blocks_by] before_filter :lookup_user_block, :only => [:show, :edit, :update, :revoke] before_filter :require_valid_params, :only => [:create, :update] @@ -34,46 +34,43 @@ class UserBlocksController < ApplicationController end def create - unless @valid_params - redirect_to :action => "new" - return - end - - @user_block = UserBlock.new({ - :user_id => @this_user.id, - :creator_id => @user.id, - :reason => params[:user_block][:reason], - :ends_at => Time.now.getutc() + @block_period.hours, - :needs_view => params[:user_block][:needs_view] - }, :without_protection => true) + if @valid_params + @user_block = UserBlock.new({ + :user_id => @this_user.id, + :creator_id => @user.id, + :reason => params[:user_block][:reason], + :ends_at => Time.now.getutc() + @block_period.hours, + :needs_view => params[:user_block][:needs_view] + }, :without_protection => true) - if @user_block.save - flash[:notice] = t('user_block.create.flash', :name => @this_user.display_name) - redirect_to @user_block + if @user_block.save + flash[:notice] = t('user_block.create.flash', :name => @this_user.display_name) + redirect_to @user_block + else + render :action => "new" + end else - render :action => "new" + redirect_to new_user_block_path(:display_name => params[:display_name]) end end def update - unless @valid_params - redirect_to :action => "edit" - return - end - - if @user_block.creator_id != @user.id - flash[:error] = t('user_block.update.only_creator_can_edit') - redirect_to :action => "edit" - return - end - - if @user_block.update_attributes({ :ends_at => Time.now.getutc() + @block_period.hours, - :reason => params[:user_block][:reason], - :needs_view => params[:user_block][:needs_view] }, :without_protection => true) - flash[:notice] = t('user_block.update.success') - redirect_to(@user_block) + if @valid_params + if @user_block.creator_id != @user.id + flash[:error] = t('user_block.update.only_creator_can_edit') + redirect_to :action => "edit" + elsif @user_block.update_attributes({ + :ends_at => Time.now.getutc() + @block_period.hours, + :reason => params[:user_block][:reason], + :needs_view => params[:user_block][:needs_view] + }, :without_protection => true) + flash[:notice] = t('user_block.update.success') + redirect_to(@user_block) + else + render :action => "edit" + end else - render :action => "edit" + redirect_to edit_user_block_path(:id => params[:id]) end end @@ -114,17 +111,22 @@ class UserBlocksController < ApplicationController # and return them to the blocks index. def require_moderator unless @user.moderator? - flash[:error] = t('user_block.filter.not_a_moderator') - redirect_to :action => 'index' + if request.get? + flash[:error] = t('user_block.filter.not_a_moderator') + redirect_to :action => 'index' + else + render :nothing => true, :status => :forbidden + end end end ## # ensure that there is a "this_user" instance variable def lookup_this_user - @this_user = User.find_by_display_name(params[:display_name]) - rescue ActiveRecord::RecordNotFound - redirect_to :controller => 'user', :action => 'view', :display_name => params[:display_name] unless @this_user + unless @this_user = User.find_by_display_name(params[:display_name]) + @not_found_user = params[:display_name] + render :template => 'user/no_such_user', :status => :not_found + end end ## diff --git a/config/routes.rb b/config/routes.rb index 5408b4e71..fec224bb9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -169,7 +169,7 @@ OpenStreetMap::Application.routes.draw do match '/user/:display_name/diary/:id/hidecomment/:comment' => 'diary_entry#hidecomment', :via => :post, :id => /\d+/, :comment => /\d+/ # user pages - match '/user/:display_name' => 'user#view', :via => :get + match '/user/:display_name' => 'user#view', :via => :get, :as => "user" match '/user/:display_name/make_friend' => 'user#make_friend', :via => :get match '/user/:display_name/remove_friend' => 'user#remove_friend', :via => :get match '/user/:display_name/account' => 'user#account', :via => [:get, :post] @@ -223,7 +223,7 @@ OpenStreetMap::Application.routes.draw do match '/user/:display_name/role/:role/revoke' => 'user_roles#revoke', :via => [:get, :post] match '/user/:display_name/blocks' => 'user_blocks#blocks_on', :via => :get match '/user/:display_name/blocks_by' => 'user_blocks#blocks_by', :via => :get - match '/blocks/new/:display_name' => 'user_blocks#new', :via => :get + match '/blocks/new/:display_name' => 'user_blocks#new', :via => :get, :as => "new_user_block" resources :user_blocks - match '/blocks/:id/revoke' => 'user_blocks#revoke', :via => [:get, :post] + match '/blocks/:id/revoke' => 'user_blocks#revoke', :via => [:get, :post], :as => "revoke_user_block" end diff --git a/test/fixtures/user_blocks.yml b/test/fixtures/user_blocks.yml index e69de29bb..c29252916 100644 --- a/test/fixtures/user_blocks.yml +++ b/test/fixtures/user_blocks.yml @@ -0,0 +1,29 @@ +active_block: + id: 1 + user_id: 13 + creator_id: 5 + reason: "Active block" + reason_format: "markdown" + ends_at: "2012-05-01 11:22:33" + needs_view: false + revoker_id: + +expired_block: + id: 2 + user_id: 14 + creator_id: 15 + reason: "Expired block" + reason_format: "markdown" + ends_at: "2012-05-01 11:22:33" + needs_view: false + revoker_id: + +revoked_block: + id: 3 + user_id: 13 + creator_id: 15 + reason: "Revoked block" + reason_format: "markdown" + ends_at: "2012-05-01 11:22:33" + needs_view: false + revoker_id: 1 diff --git a/test/fixtures/user_roles.yml b/test/fixtures/user_roles.yml index a5f814255..113d1627e 100644 --- a/test/fixtures/user_roles.yml +++ b/test/fixtures/user_roles.yml @@ -9,3 +9,8 @@ moderator: user_id: 5 role: moderator granter_id: 6 + +second_moderator: + user_id: 15 + role: moderator + granter_id: 6 diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 34ac9ce42..de99c00b5 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -162,3 +162,41 @@ confirmed_user: terms_agreed: "2010-01-01 11:22:33" terms_seen: true languages: en + +blocked_user: + id: 13 + email: blocked@openstreetmap.org + status: active + pass_crypt: <%= Digest::MD5.hexdigest('test') %> + creation_time: "2007-01-01 00:00:00" + display_name: blocked + data_public: true + description: test + terms_agreed: "2010-01-01 11:22:33" + terms_seen: true + languages: en + +unblocked_user: + id: 14 + email: unblocked@openstreetmap.org + status: active + pass_crypt: <%= Digest::MD5.hexdigest('test') %> + creation_time: "2007-01-01 00:00:00" + display_name: unblocked + data_public: true + description: test + terms_agreed: "2010-01-01 11:22:33" + terms_seen: true + languages: en + +second_moderator_user: + id: 15 + email: second_moderator@example.com + status: active + pass_crypt: <%= Digest::MD5.hexdigest('test') %> + creation_time: "2008-05-01 01:23:45" + display_name: second_moderator + data_public: true + terms_agreed: "2010-01-01 11:22:33" + terms_seen: true + languages: en diff --git a/test/functional/user_blocks_controller_test.rb b/test/functional/user_blocks_controller_test.rb index adf0f3631..a93e07934 100644 --- a/test/functional/user_blocks_controller_test.rb +++ b/test/functional/user_blocks_controller_test.rb @@ -1,6 +1,8 @@ require File.dirname(__FILE__) + '/../test_helper' class UserBlocksControllerTest < ActionController::TestCase + fixtures :users, :user_roles, :user_blocks + ## # test all routes which lead to this controller def test_routes @@ -55,4 +57,331 @@ class UserBlocksControllerTest < ActionController::TestCase { :controller => "user_blocks", :action => "blocks_by", :display_name => "username" } ) end + + ## + # test the index action + def test_index + get :index + assert_response :success + assert_select "table#block_list", :count => 1 do + assert_select "tr", 4 + assert_select "a[href='#{user_block_path(user_blocks(:active_block))}']", 1 + assert_select "a[href='#{user_block_path(user_blocks(:expired_block))}']", 1 + assert_select "a[href='#{user_block_path(user_blocks(:revoked_block))}']", 1 + end + end + + ## + # test the show action + def test_show + get :show + assert_response :not_found + assert_template "not_found" + assert_select "p", "Sorry, the user block with ID could not be found." + + get :show, :id => 99999 + assert_response :not_found + assert_template "not_found" + assert_select "p", "Sorry, the user block with ID 99999 could not be found." + + get :show, :id => user_blocks(:active_block) + assert_response :success + + get :show, :id => user_blocks(:expired_block) + assert_response :success + + get :show, :id => user_blocks(:revoked_block) + assert_response :success + end + + ## + # test the new action + def test_new + get :new, :display_name => users(:normal_user).display_name + assert_redirected_to login_path(:referer => new_user_block_path(:display_name => users(:normal_user).display_name)) + + session[:user] = users(:public_user).id + cookies["_osm_username"] = users(:public_user).display_name + + get :new, :display_name => users(:normal_user).display_name + assert_redirected_to user_blocks_path + assert_equal "You need to be a moderator to perform that action.", flash[:error] + + session[:user] = users(:moderator_user).id + cookies["_osm_username"] = users(:moderator_user).display_name + + get :new, :display_name => users(:normal_user).display_name + assert_response :success + assert_select "form#new_user_block", :count => 1 do + assert_select "textarea#user_block_reason", :count => 1 + assert_select "select#user_block_period", :count => 1 + assert_select "input#user_block_needs_view[type='checkbox']", :count => 1 + assert_select "input#display_name[type='hidden']", :count => 1 + assert_select "input[type='submit'][value='Create block']", :count => 1 + end + + get :new + assert_response :not_found + assert_template "user/no_such_user" + assert_select "h2", "The user does not exist" + + get :new, :display_name => "non_existent_user" + assert_response :not_found + assert_template "user/no_such_user" + assert_select "h2", "The user non_existent_user does not exist" + end + + ## + # test the edit action + def test_edit + get :edit, :id => user_blocks(:active_block).id + assert_redirected_to login_path(:referer => edit_user_block_path(:id => user_blocks(:active_block).id)) + + session[:user] = users(:public_user).id + cookies["_osm_username"] = users(:public_user).display_name + + get :edit, :id => user_blocks(:active_block).id + assert_redirected_to user_blocks_path + assert_equal "You need to be a moderator to perform that action.", flash[:error] + + session[:user] = users(:moderator_user).id + cookies["_osm_username"] = users(:moderator_user).display_name + + get :edit, :id => user_blocks(:active_block).id + assert_response :success + assert_select "form#edit_user_block_#{user_blocks(:active_block).id}", :count => 1 do + assert_select "textarea#user_block_reason", :count => 1 + assert_select "select#user_block_period", :count => 1 + assert_select "input#user_block_needs_view[type='checkbox']", :count => 1 + assert_select "input[type='submit'][value='Update block']", :count => 1 + end + + get :edit + assert_response :not_found + assert_template "not_found" + assert_select "p", "Sorry, the user block with ID could not be found." + + get :edit, :id => 99999 + assert_response :not_found + assert_template "not_found" + assert_select "p", "Sorry, the user block with ID 99999 could not be found." + end + + ## + # test the create action + def test_create + post :create + assert_response :forbidden + + session[:user] = users(:public_user).id + cookies["_osm_username"] = users(:public_user).display_name + + post :create + assert_response :forbidden + + session[:user] = users(:moderator_user).id + cookies["_osm_username"] = users(:moderator_user).display_name + + assert_no_difference "UserBlock.count" do + post :create, + :display_name => users(:unblocked_user).display_name, + :user_block_period => "99" + end + assert_redirected_to new_user_block_path(:display_name => users(:unblocked_user).display_name) + assert_equal "The blocking period must be one of the values selectable in the drop-down list.", flash[:error] + + assert_difference "UserBlock.count", 1 do + post :create, + :display_name => users(:unblocked_user).display_name, + :user_block_period => "12", + :user_block => { :needs_view => false, :reason => "Vandalism" } + end + assert_redirected_to user_block_path(:id => 4) + assert_equal "Created a block on user #{users(:unblocked_user).display_name}.", flash[:notice] + b = UserBlock.find(4) + assert_in_delta Time.now, b.created_at, 1 + assert_in_delta Time.now, b.updated_at, 1 + assert_in_delta Time.now + 12.hour, b.ends_at, 1 + assert_equal false, b.needs_view + assert_equal "Vandalism", b.reason + assert_equal "markdown", b.reason_format + assert_equal users(:moderator_user).id, b.creator_id + + post :create + assert_response :not_found + assert_template "user/no_such_user" + assert_select "h2", "The user does not exist" + + post :create, :display_name => "non_existent_user" + assert_response :not_found + assert_template "user/no_such_user" + assert_select "h2", "The user non_existent_user does not exist" + end + + ## + # test the update action + def test_update + put :update + assert_response :forbidden + + session[:user] = users(:public_user).id + cookies["_osm_username"] = users(:public_user).display_name + + put :update + assert_response :forbidden + + session[:user] = users(:second_moderator_user).id + cookies["_osm_username"] = users(:second_moderator_user).display_name + + assert_no_difference "UserBlock.count" do + put :update, + :id => user_blocks(:active_block).id, + :user_block_period => "12", + :user_block => { :needs_view => true, :reason => "Vandalism" } + end + assert_redirected_to edit_user_block_path(:id => user_blocks(:active_block).id) + assert_equal "Only the moderator who created this block can edit it.", flash[:error] + + session[:user] = users(:moderator_user).id + cookies["_osm_username"] = users(:moderator_user).display_name + + assert_no_difference "UserBlock.count" do + put :update, + :id => user_blocks(:active_block).id, + :user_block_period => "99" + end + assert_redirected_to edit_user_block_path(:id => user_blocks(:active_block).id) + assert_equal "The blocking period must be one of the values selectable in the drop-down list.", flash[:error] + + assert_no_difference "UserBlock.count" do + put :update, + :id => user_blocks(:active_block).id, + :user_block_period => "12", + :user_block => { :needs_view => true, :reason => "Vandalism" } + end + assert_redirected_to user_block_path(:id => user_blocks(:active_block).id) + assert_equal "Block updated.", flash[:notice] + b = UserBlock.find(user_blocks(:active_block).id) + assert_in_delta Time.now, b.updated_at, 1 + assert_equal true, b.needs_view + assert_equal "Vandalism", b.reason + + put :update + assert_response :not_found + assert_template "not_found" + assert_select "p", "Sorry, the user block with ID could not be found." + + put :update, :id => 99999 + assert_response :not_found + assert_template "not_found" + assert_select "p", "Sorry, the user block with ID 99999 could not be found." + end + + ## + # test the revoke action + def test_revoke + get :revoke, :id => user_blocks(:active_block).id + assert_redirected_to login_path(:referer => revoke_user_block_path(:id => user_blocks(:active_block).id)) + + session[:user] = users(:public_user).id + cookies["_osm_username"] = users(:public_user).display_name + + get :revoke, :id => user_blocks(:active_block).id + assert_redirected_to user_blocks_path + assert_equal "You need to be a moderator to perform that action.", flash[:error] + + session[:user] = users(:moderator_user).id + cookies["_osm_username"] = users(:moderator_user).display_name + + get :revoke, :id => user_blocks(:active_block).id + assert_response :success + assert_template "revoke" + assert_select "form", :count => 1 do + assert_select "input#confirm[type='checkbox']", :count => 1 + assert_select "input[type='submit'][value='Revoke!']", :count => 1 + end + + post :revoke, :id => user_blocks(:active_block).id, :confirm => true + assert_redirected_to user_block_path(:id => user_blocks(:active_block).id) + b = UserBlock.find(user_blocks(:active_block).id) + assert_in_delta Time.now, b.ends_at, 1 + + get :revoke + assert_response :not_found + assert_template "not_found" + assert_select "p", "Sorry, the user block with ID could not be found." + + get :revoke, :id => 99999 + assert_response :not_found + assert_template "not_found" + assert_select "p", "Sorry, the user block with ID 99999 could not be found." + end + + ## + # test the blocks_on action + def test_blocks_on + get :blocks_on + assert_response :not_found + assert_template "user/no_such_user" + assert_select "h2", "The user does not exist" + + get :blocks_on, :display_name => "non_existent_user" + assert_response :not_found + assert_template "user/no_such_user" + assert_select "h2", "The user non_existent_user does not exist" + + get :blocks_on, :display_name => users(:normal_user).display_name + assert_response :success + assert_select "table#block_list", false + assert_select "p", "#{users(:normal_user).display_name} has not been blocked yet." + + get :blocks_on, :display_name => users(:blocked_user).display_name + assert_response :success + assert_select "table#block_list", :count => 1 do + assert_select "tr", 3 + assert_select "a[href='#{user_block_path(user_blocks(:active_block))}']", 1 + assert_select "a[href='#{user_block_path(user_blocks(:revoked_block))}']", 1 + end + + get :blocks_on, :display_name => users(:unblocked_user).display_name + assert_response :success + assert_select "table#block_list", :count => 1 do + assert_select "tr", 2 + assert_select "a[href='#{user_block_path(user_blocks(:expired_block))}']", 1 + end + end + + ## + # test the blocks_by action + def test_blocks_by + get :blocks_by + assert_response :not_found + assert_template "user/no_such_user" + assert_select "h2", "The user does not exist" + + get :blocks_by, :display_name => "non_existent_user" + assert_response :not_found + assert_template "user/no_such_user" + assert_select "h2", "The user non_existent_user does not exist" + + get :blocks_by, :display_name => users(:moderator_user).display_name + assert_response :success + assert_select "table#block_list", :count => 1 do + assert_select "tr", 2 + assert_select "a[href='#{user_block_path(user_blocks(:active_block))}']", 1 + end + + get :blocks_by, :display_name => users(:second_moderator_user).display_name + assert_response :success + assert_select "table#block_list", :count => 1 do + assert_select "tr", 3 + assert_select "a[href='#{user_block_path(user_blocks(:expired_block))}']", 1 + assert_select "a[href='#{user_block_path(user_blocks(:revoked_block))}']", 1 + end + + get :blocks_by, :display_name => users(:normal_user).display_name + assert_response :success + assert_select "table#block_list", false + assert_select "p", "#{users(:normal_user).display_name} has not made any blocks yet." + end end diff --git a/test/unit/user_test.rb b/test/unit/user_test.rb index eb2f2a959..bc0b7573b 100644 --- a/test/unit/user_test.rb +++ b/test/unit/user_test.rb @@ -155,7 +155,7 @@ class UserTest < ActiveSupport::TestCase end def test_visible - assert_equal 10, User.visible.count + assert_equal 13, User.visible.count assert_raise ActiveRecord::RecordNotFound do User.visible.find(users(:suspended_user).id) end @@ -165,7 +165,7 @@ class UserTest < ActiveSupport::TestCase end def test_active - assert_equal 9, User.active.count + assert_equal 12, User.active.count assert_raise ActiveRecord::RecordNotFound do User.active.find(users(:inactive_user).id) end @@ -178,7 +178,7 @@ class UserTest < ActiveSupport::TestCase end def test_public - assert_equal 11, User.public.count + assert_equal 14, User.public.count assert_raise ActiveRecord::RecordNotFound do User.public.find(users(:normal_user).id) end -- 2.39.5