From 85b3369032c566316ee37c8150e07fa2f040cd10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20B=C4=82LTEANU?= Date: Tue, 12 May 2026 23:01:50 +0300 Subject: [PATCH 3/3] Implement explicit confirmation when removing user(s) from group. --- app/controllers/groups_controller.rb | 22 ++++++++---- app/views/groups/_users.html.erb | 2 +- app/views/groups/remove_users.erb | 20 +++++++++++ app/views/groups/remove_users.js.erb | 1 - config/locales/en.yml | 1 + test/functional/groups_controller_test.rb | 44 ++++++++++++++++------- 6 files changed, 69 insertions(+), 21 deletions(-) create mode 100644 app/views/groups/remove_users.erb delete mode 100644 app/views/groups/remove_users.js.erb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 81c4ce50b..ef7cc99c0 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -130,12 +130,22 @@ class GroupsController < ApplicationController end def remove_users - @users = User.where(:id => (params[:user_id] || params[:user_ids])).to_a - @group.users.delete(@users) if request.delete? - respond_to do |format| - format.html {redirect_back_or_default edit_group_path(@group, :tab => 'users')} - format.js - format.api {render_api_ok} + @users = @group.users.where(:id => (params[:user_id] || params[:user_ids])).to_a + + if @users.empty? + render_404 + return + end + + if request.delete? && (api_request? || params[:confirm] == I18n.t(:general_text_Yes)) + @group.users.delete(@users) + respond_to do |format| + format.html do + flash[:notice] = l(:notice_successful_delete) + redirect_back_or_default edit_group_path(@group, :tab => 'users') + end + format.api {render_api_ok} + end end end diff --git a/app/views/groups/_users.html.erb b/app/views/groups/_users.html.erb index ad65e5416..19aaab094 100644 --- a/app/views/groups/_users.html.erb +++ b/app/views/groups/_users.html.erb @@ -11,7 +11,7 @@ <%= link_to_user user %> - <%= remove_link group_users_path(@group, :user_id => user), :remote => true %> + <%= link_to sprite_icon('link-break', l(:button_remove)), group_users_path(@group, :user_id => user), :method => :delete, :class => 'icon icon-link-break' %> <% end %> diff --git a/app/views/groups/remove_users.erb b/app/views/groups/remove_users.erb new file mode 100644 index 000000000..3d1497681 --- /dev/null +++ b/app/views/groups/remove_users.erb @@ -0,0 +1,20 @@ +<%= title l(:label_confirmation) %> + +<%= form_tag(group_users_path(@group, :user_ids => @users.map(&:id)), method: :delete) do %> +
+

<%= simple_format l :text_users_remove_from_group_confirmation, group: "#{@group.name}".html_safe %>

+ + <% @users.each do |user| %> +

<%= user.name %> (<%= user.login %>)

+ <% end %> + +

<%= l :text_users_bulk_destroy_confirm, yes: l(:general_text_Yes) %>

+

<%= text_field_tag 'confirm' %>

+ +
+ +

+ <%= submit_tag l(:button_delete) %> + <%= link_to l(:button_cancel), @back_url || users_path %> +

+<% end %> diff --git a/app/views/groups/remove_users.js.erb b/app/views/groups/remove_users.js.erb deleted file mode 100644 index 88daa8f58..000000000 --- a/app/views/groups/remove_users.js.erb +++ /dev/null @@ -1 +0,0 @@ -$('#tab-content-users').html('<%= escape_javascript(render :partial => 'groups/users') %>'); diff --git a/config/locales/en.yml b/config/locales/en.yml index 07fca36b2..29b286cbe 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1478,6 +1478,7 @@ en: twofa_text_group_required: "This setting is only effective when the global two factor authentication setting is set to 'optional'. Currently, two factor authentication is required for all users." twofa_text_group_disabled: "This setting is only effective when the global two factor authentication setting is set to 'optional'. Currently, two factor authentication is disabled." text_user_destroy_confirmation: "Are you sure you want to delete this user and remove all references to them? This cannot be undone. Often, locking a user instead of deleting them is the better solution. To confirm, please enter their login (%{login}) below." + text_users_remove_from_group_confirmation: "You are about to remove the following user(s) from group %{group}." text_project_destroy_enter_identifier: "To confirm, please enter the project's identifier (%{identifier}) below." field_name_or_email_or_login: Name, email or login setting_wiki_tablesort_enabled: JavaScript based table sorting in wiki content diff --git a/test/functional/groups_controller_test.rb b/test/functional/groups_controller_test.rb index f846f9494..c46e4ac0a 100644 --- a/test/functional/groups_controller_test.rb +++ b/test/functional/groups_controller_test.rb @@ -255,41 +255,59 @@ class GroupsControllerTest < Redmine::ControllerTest :remove_users, :params => { :id => 10, - :user_id => '8' + :user_id => '8', + :confirm => I18n.t(:general_text_Yes) } ) end end - def test_remove_users_plural - group = Group.find(10) - group.users << User.find(2) - assert_difference 'group.users.count', -2 do + def test_remove_users_without_confirmation + assert_no_difference 'Group.find(10).users.count' do delete( :remove_users, :params => { :id => 10, - :user_ids => ['2', '8'] + :user_id => '8' } ) end + assert_response :success + assert_select 'input[name=confirm]' end - def test_xhr_remove_users - assert_difference 'Group.find(10).users.count', -1 do + def test_remove_users_plural + group = Group.find(10) + group.users << User.find(2) + assert_difference 'group.users.count', -2 do delete( :remove_users, :params => { :id => 10, - :user_id => '8' - }, - :xhr => true + :user_ids => ['2', '8'], + :confirm => I18n.t(:general_text_Yes) + } ) - assert_response :success - assert_equal 'text/javascript', response.media_type end end + def test_remove_users_should_only_include_group_members + assert !Group.find(10).users.include?(User.find(3)) + + get( + :remove_users, + :params => { + :id => 10, + :user_ids => ['3', '8'] + } + ) + assert_response :success + assert_select 'input[name=confirm]' + # Should show user 8 but not user 3 + assert_select 'p strong', :text => 'User Misc' + assert_select 'p strong', :text => 'Dave Lopper', :count => 0 + end + def test_remove_user_should_be_deprecated Rails.application.deprecators[:redmine].expects(:warn).with(regexp_matches(/GroupsController#remove_user is deprecated/)) delete( -- 2.50.1 (Apple Git-155)