From: Anton Khorev Date: Tue, 24 Dec 2024 04:50:29 +0000 (+0300) Subject: Add revoke all actions to received blocks resource X-Git-Tag: live~55^2 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/df1c59280f423004b8ade2f7d2a01994ce791b94?hp=2df389c97e52437f964e4b4ee22c8493a9cc7fbc Add revoke all actions to received blocks resource --- diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index fe899d09d..6638016d7 100644 --- a/app/abilities/ability.rb +++ b/app/abilities/ability.rb @@ -56,7 +56,7 @@ class Ability can [:read, :resolve, :ignore, :reopen], Issue can :create, IssueComment can [:create, :update, :destroy], Redaction - can [:create, :revoke_all], UserBlock + can [:create, :destroy], UserBlock can :update, UserBlock, :creator => user can :update, UserBlock, :revoker => user can :update, UserBlock, :active? => true diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb index 81b0cbcba..a526f529e 100644 --- a/app/controllers/user_blocks_controller.rb +++ b/app/controllers/user_blocks_controller.rb @@ -9,11 +9,11 @@ class UserBlocksController < ApplicationController authorize_resource - before_action :lookup_user, :only => [:new, :create, :revoke_all] + before_action :lookup_user, :only => [:new, :create] before_action :lookup_user_block, :only => [:show, :edit, :update] before_action :require_valid_params, :only => [:create, :update] before_action :check_database_readable - before_action :check_database_writable, :only => [:create, :update, :revoke_all] + before_action :check_database_writable, :only => [:create, :update] def index @params = params.permit @@ -105,16 +105,6 @@ class UserBlocksController < ApplicationController end end - ## - # revokes all active blocks - def revoke_all - if request.post? && params[:confirm] - @user.blocks.active.each { |block| block.revoke!(current_user) } - flash[:notice] = t ".flash" - redirect_to user_received_blocks_path(@user) - end - end - private ## diff --git a/app/controllers/users/received_blocks_controller.rb b/app/controllers/users/received_blocks_controller.rb index a47b56a58..6f6cabd26 100644 --- a/app/controllers/users/received_blocks_controller.rb +++ b/app/controllers/users/received_blocks_controller.rb @@ -12,6 +12,7 @@ module Users before_action :lookup_user before_action :check_database_readable + before_action :check_database_writable, :only => :destroy ## # shows a list of all the blocks on the given user @@ -27,5 +28,21 @@ module Users render :partial => "user_blocks/page" if turbo_frame_request_id == "pagination" end + + ## + # shows revoke all active blocks page + def edit; end + + ## + # revokes all active blocks + def destroy + if params[:confirm] + @user.blocks.active.each { |block| block.revoke!(current_user) } + flash[:notice] = t ".flash" + redirect_to user_received_blocks_path(@user) + else + render :action => :edit + end + end end end diff --git a/app/views/user_blocks/revoke_all.html.erb b/app/views/users/received_blocks/edit.html.erb similarity index 88% rename from app/views/user_blocks/revoke_all.html.erb rename to app/views/users/received_blocks/edit.html.erb index f9a17d3e6..ff1a38be5 100644 --- a/app/views/user_blocks/revoke_all.html.erb +++ b/app/views/users/received_blocks/edit.html.erb @@ -6,7 +6,7 @@ <% unless @user.blocks.active.empty? %> - <%= bootstrap_form_for :revoke_all, :url => { :action => "revoke_all" } do |f| %> + <%= bootstrap_form_for :revoke_all, :url => { :action => :destroy }, :method => :delete do |f| %>
<%= check_box_tag "confirm", "yes", false, { :class => "form-check-input" } %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 8b09c751e..c168972aa 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -105,9 +105,9 @@ <% end %> - <% if can?(:revoke_all, UserBlock) and @user.blocks.active.exists? %> + <% if can?(:destroy, UserBlock) and @user.blocks.active.exists? %>
  • - <%= link_to t(".revoke_all_blocks"), revoke_all_user_blocks_path(@user) %> + <%= link_to t(".revoke_all_blocks"), edit_user_received_blocks_path(@user) %>
    • <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index e4489ff60..6504f5856 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2856,6 +2856,17 @@ en: title: "Blocks on %{name}" heading_html: "List of Blocks on %{name}" empty: "%{name} has not been blocked yet." + edit: + title: "Revoking all blocks on %{block_on}" + heading_html: "Revoking all blocks on %{block_on}" + empty: "%{name} has no active blocks." + confirm: "Are you sure you wish to revoke %{active_blocks}?" + active_blocks: + one: "%{count} active block" + other: "%{count} active blocks" + revoke: "Revoke!" + destroy: + flash: "All active blocks have been revoked." lists: show: title: Users @@ -2932,16 +2943,6 @@ en: title: "User blocks" heading: "List of user blocks" empty: "No blocks have been made yet." - revoke_all: - title: "Revoking all blocks on %{block_on}" - heading_html: "Revoking all blocks on %{block_on}" - empty: "%{name} has no active blocks." - confirm: "Are you sure you wish to revoke %{active_blocks}?" - active_blocks: - one: "%{count} active block" - other: "%{count} active blocks" - revoke: "Revoke!" - flash: "All active blocks have been revoked." helper: time_future_html: "Ends in %{time}." until_login: "Active until the user logs in." diff --git a/config/routes.rb b/config/routes.rb index 806e4ba85..e6a1aacda 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -273,7 +273,7 @@ OpenStreetMap::Application.routes.draw do resource :role, :controller => "user_roles", :path => "roles/:role", :only => [:create, :destroy] scope :module => :users do resource :issued_blocks, :path => "blocks_by", :only => :show - resource :received_blocks, :path => "blocks", :only => :show + resource :received_blocks, :path => "blocks", :only => [:show, :edit, :destroy] end end get "/user/:display_name/account", :to => redirect(:path => "/account/edit") @@ -335,7 +335,6 @@ OpenStreetMap::Application.routes.draw do # banning pages resources :user_blocks, :path_names => { :new => "new/:display_name" } - match "/user/:display_name/blocks/revoke_all" => "user_blocks#revoke_all", :via => [:get, :post], :as => "revoke_all_user_blocks" # issues and reports resources :issues do diff --git a/test/controllers/user_blocks_controller_test.rb b/test/controllers/user_blocks_controller_test.rb index 0b9c82f6a..ddb33e903 100644 --- a/test/controllers/user_blocks_controller_test.rb +++ b/test/controllers/user_blocks_controller_test.rb @@ -36,15 +36,6 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest { :path => "/user_blocks/1", :method => :delete }, { :controller => "user_blocks", :action => "destroy", :id => "1" } ) - - assert_routing( - { :path => "/user/username/blocks/revoke_all", :method => :get }, - { :controller => "user_blocks", :action => "revoke_all", :display_name => "username" } - ) - assert_routing( - { :path => "/user/username/blocks/revoke_all", :method => :post }, - { :controller => "user_blocks", :action => "revoke_all", :display_name => "username" } - ) end ## @@ -598,83 +589,6 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest assert_equal other_moderator_user, block.revoker end - ## - # test the revoke all page - def test_revoke_all_page - blocked_user = create(:user) - create(:user_block, :user => blocked_user) - - # Asking for the revoke all blocks page with a bogus user name should fail - get user_received_blocks_path("non_existent_user") - assert_response :not_found - - # Check that the revoke all blocks page requires us to login - get revoke_all_user_blocks_path(blocked_user) - assert_redirected_to login_path(:referer => revoke_all_user_blocks_path(blocked_user)) - - # Login as a normal user - session_for(create(:user)) - - # Check that normal users can't load the revoke all blocks page - get revoke_all_user_blocks_path(blocked_user) - assert_redirected_to :controller => "errors", :action => "forbidden" - - # Login as a moderator - session_for(create(:moderator_user)) - - # Check that the revoke all blocks page loads for moderators - get revoke_all_user_blocks_path(blocked_user) - assert_response :success - assert_select "h1 a[href='#{user_path blocked_user}']", :text => blocked_user.display_name - end - - ## - # test the revoke all action - def test_revoke_all_action - blocked_user = create(:user) - active_block1 = create(:user_block, :user => blocked_user) - active_block2 = create(:user_block, :user => blocked_user) - expired_block1 = create(:user_block, :expired, :user => blocked_user) - blocks = [active_block1, active_block2, expired_block1] - moderator_user = create(:moderator_user) - - assert_predicate active_block1, :active? - assert_predicate active_block2, :active? - assert_not_predicate expired_block1, :active? - - # Login as a normal user - session_for(create(:user)) - - # Check that normal users can't load the block revoke page - get revoke_all_user_blocks_path(:blocked_user) - assert_redirected_to :controller => "errors", :action => "forbidden" - - # Login as a moderator - session_for(moderator_user) - - # Check that revoking blocks using GET should fail - get revoke_all_user_blocks_path(blocked_user, :confirm => true) - assert_response :success - assert_template "revoke_all" - - blocks.each(&:reload) - assert_predicate active_block1, :active? - assert_predicate active_block2, :active? - assert_not_predicate expired_block1, :active? - - # Check that revoking blocks works using POST - post revoke_all_user_blocks_path(blocked_user, :confirm => true) - assert_redirected_to user_received_blocks_path(blocked_user) - - blocks.each(&:reload) - assert_not_predicate active_block1, :active? - assert_not_predicate active_block2, :active? - assert_not_predicate expired_block1, :active? - assert_equal moderator_user, active_block1.revoker - assert_equal moderator_user, active_block2.revoker - assert_not_equal moderator_user, expired_block1.revoker - end - ## # test changes to end/deactivation dates def test_dates_when_viewed_before_end diff --git a/test/controllers/users/received_blocks_controller_test.rb b/test/controllers/users/received_blocks_controller_test.rb index b254f9ec3..a48a7e2cd 100644 --- a/test/controllers/users/received_blocks_controller_test.rb +++ b/test/controllers/users/received_blocks_controller_test.rb @@ -12,6 +12,14 @@ module Users { :path => "/user/username/blocks", :method => :get }, { :controller => "users/received_blocks", :action => "show", :user_display_name => "username" } ) + assert_routing( + { :path => "/user/username/blocks/edit", :method => :get }, + { :controller => "users/received_blocks", :action => "edit", :user_display_name => "username" } + ) + assert_routing( + { :path => "/user/username/blocks", :method => :delete }, + { :controller => "users/received_blocks", :action => "destroy", :user_display_name => "username" } + ) end def test_show @@ -91,5 +99,81 @@ module Users assert_redirected_to :controller => "/errors", :action => :bad_request end end + + ## + # test the revoke all blocks page + def test_edit + blocked_user = create(:user) + create(:user_block, :user => blocked_user) + + # Asking for the revoke all blocks page with a bogus user name should fail + get user_received_blocks_path("non_existent_user") + assert_response :not_found + + # Check that the revoke all blocks page requires us to login + get edit_user_received_blocks_path(blocked_user) + assert_redirected_to login_path(:referer => edit_user_received_blocks_path(blocked_user)) + + # Login as a normal user + session_for(create(:user)) + + # Check that normal users can't load the revoke all blocks page + get edit_user_received_blocks_path(blocked_user) + assert_redirected_to :controller => "/errors", :action => "forbidden" + + # Login as a moderator + session_for(create(:moderator_user)) + + # Check that the revoke all blocks page loads for moderators + get edit_user_received_blocks_path(blocked_user) + assert_response :success + assert_select "h1 a[href='#{user_path blocked_user}']", :text => blocked_user.display_name + end + + ## + # test the revoke all action + def test_destroy + blocked_user = create(:user) + active_block1 = create(:user_block, :user => blocked_user) + active_block2 = create(:user_block, :user => blocked_user) + expired_block1 = create(:user_block, :expired, :user => blocked_user) + blocks = [active_block1, active_block2, expired_block1] + moderator_user = create(:moderator_user) + + assert_predicate active_block1, :active? + assert_predicate active_block2, :active? + assert_not_predicate expired_block1, :active? + + # Check that normal users can't revoke all blocks + session_for(create(:user)) + delete user_received_blocks_path(blocked_user, :confirm => true) + assert_redirected_to :controller => "/errors", :action => "forbidden" + + blocks.each(&:reload) + assert_predicate active_block1, :active? + assert_predicate active_block2, :active? + assert_not_predicate expired_block1, :active? + + # Check that confirmation is required + session_for(moderator_user) + delete user_received_blocks_path(blocked_user) + + blocks.each(&:reload) + assert_predicate active_block1, :active? + assert_predicate active_block2, :active? + assert_not_predicate expired_block1, :active? + + # Check that moderators can revoke all blocks + delete user_received_blocks_path(blocked_user, :confirm => true) + assert_redirected_to user_received_blocks_path(blocked_user) + + blocks.each(&:reload) + assert_not_predicate active_block1, :active? + assert_not_predicate active_block2, :active? + assert_not_predicate expired_block1, :active? + assert_equal moderator_user, active_block1.revoker + assert_equal moderator_user, active_block2.revoker + assert_not_equal moderator_user, expired_block1.revoker + end end end diff --git a/test/system/user_blocks_test.rb b/test/system/user_blocks_test.rb index e9fa4e7ec..52fe3bc3d 100644 --- a/test/system/user_blocks_test.rb +++ b/test/system/user_blocks_test.rb @@ -31,7 +31,7 @@ class UserBlocksSystemTest < ApplicationSystemTestCase blocked_user = create(:user) sign_in_as(create(:moderator_user)) - visit revoke_all_user_blocks_path(blocked_user) + visit edit_user_received_blocks_path(blocked_user) assert_title "Revoking all blocks on #{blocked_user.display_name}" assert_text "Revoking all blocks on #{blocked_user.display_name}" assert_no_button "Revoke!"