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.

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 6caed05940a0bf024dbd51259f67fe04a68bbf92..0f9fad45e860ab40a6687b8eece4de1d59286cad 100644 (file)
--- 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 2bdafa80e482aace89ced8123091255fb82fc9c1..26b698b607bc8b3808cd678cd186bc6d5dc44746 100644 (file)
--- 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 5408b4e71bd401801690221f54a8ed1efce62c1f..fec224bb90dcbe48f89bb9c6ca057435de151a08 100644 (file)
--- 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 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..c29252916e6a42469a10680d5833539eda72e0db 100644 (file)
--- 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 a5f81425547c3b4fe2ace25a8e3b017222d9ab29..113d1627e04f9d9627d6dbce32ccaf6f3ff3f5fd 100644 (file)
--- 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 34ac9ce421f0255acfd8ce9addb21e482c613408..de99c00b5664867666691e0ea964d85186d96356 100644 (file)
--- 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 adf0f3631f703bd01c1f58d74aa7935e4013ac8b..a93e079348bb9027690c34da3aacc4ce3a4a85f1 100644 (file)
--- 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 eb2f2a95984b71e3446dbbd15b655404b9068f73..bc0b7573b2527aae482c3757abb36343c261d89d 100644 (file)
--- 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