From 113a3b6f0a382b6b6c3e70f90b203e6105b8afaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20B=C4=82LTEANU?= Date: Wed, 16 Sep 2026 22:56:22 +0300 Subject: [PATCH 1/4] Add filter form to project members settings tab Allow filtering project members by user/group name and role. Preserve filter parameters across pagination, tab switching, and member CRUD actions. --- app/controllers/members_controller.rb | 3 +- app/helpers/members_helper.rb | 23 ++++- app/helpers/projects_helper.rb | 3 +- app/models/member.rb | 12 +++ app/views/members/_edit.html.erb | 2 +- app/views/members/_new_modal.html.erb | 2 +- app/views/projects/settings/_members.html.erb | 22 ++++- test/functional/members_controller_test.rb | 50 ++++++++++ test/functional/projects_controller_test.rb | 91 +++++++++++++++++++ test/helpers/members_helper_test.rb | 14 ++- test/unit/member_test.rb | 24 +++++ 11 files changed, 232 insertions(+), 14 deletions(-) diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb index 8508fdb40..a94cd1773 100644 --- a/app/controllers/members_controller.rb +++ b/app/controllers/members_controller.rb @@ -143,8 +143,7 @@ class MembersController < ApplicationController def members_settings_url_params query = {:tab => 'members'} - query[:members_page] = params[:members_page] if params[:members_page].present? query[:per_page] = params[:per_page] if params[:per_page].present? - query + query.merge(members_list_params) end end diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb index b561efa6f..74fdb74ba 100644 --- a/app/helpers/members_helper.rb +++ b/app/helpers/members_helper.rb @@ -48,17 +48,34 @@ module MembersHelper s + content_tag('span', links, :class => 'pagination') end + # Returns the scope of the members of project matching the filters + # set in the request params + def members_scope(project) + project.memberships.like(params[:member_name]).with_role(params[:member_role_id]) + end + # limit/offset on Member.sorted would paginate role join rows, not members - def paginate_members(project) - ordered_ids = project.memberships.sorted.pluck("#{Member.table_name}.id").uniq + def paginate_members(scope) + ordered_ids = scope.sorted.pluck("#{Member.table_name}.id").uniq member_count = ordered_ids.size member_pages = Redmine::Pagination::Paginator.new(member_count, per_page_option, params['members_page'], 'members_page') page_ids = ordered_ids[member_pages.offset, member_pages.per_page] || [] - members_by_id = project.memberships.where(:id => page_ids).preload(:project, :principal, :roles).index_by(&:id) + members_by_id = scope.where(:id => page_ids).preload(:project, :principal, :roles).index_by(&:id) members = page_ids.filter_map {|id| members_by_id[id]} [members, member_pages, member_count] end + # Returns the params that define the members list currently displayed + # (pagination and filters), so that they can be preserved across the member + # creation, edition and deletion requests + def members_list_params + { + :members_page => params[:members_page], + :member_name => params[:member_name], + :member_role_id => params[:member_role_id] + }.select {|_, value| value.present?} + end + # Returns inheritance information for an inherited member role def render_role_inheritance(member, role) content = member.role_inheritance(role).filter_map do |h| diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 528fe3089..635d90f92 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -24,7 +24,8 @@ module ProjectsHelper {:name => 'info', :action => :edit_project, :partial => 'projects/edit', :label => :label_project}, {:name => 'members', :action => :manage_members, - :partial => 'projects/settings/members', :label => :label_member_plural}, + :partial => 'projects/settings/members', :label => :label_member_plural, + :url => {:tab => 'members'}.merge(members_list_params)}, {:name => 'issues', :action => :edit_project, :module => :issue_tracking, :partial => 'projects/settings/issues', :label => :label_issue_tracking}, {:name => 'versions', :action => :manage_versions, diff --git a/app/models/member.rb b/app/models/member.rb index 1f597c96c..0d562e14c 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -33,6 +33,18 @@ class Member < ApplicationRecord scope :active, (lambda do joins(:principal).where(:users => {:status => Principal::STATUS_ACTIVE}) end) + # Members whose principal matches the given string + scope :like, (lambda do |arg| + if arg.present? + where(:user_id => Principal.like(arg).select(:id)) + end + end) + # Members having the given role, inherited or not + scope :with_role, (lambda do |arg| + if arg.present? + where(:id => MemberRole.where(:role_id => arg.to_i).select(:member_id)) + end + end) # Sort by first role and principal scope :sorted, (lambda do includes(:member_roles, :roles, :principal). diff --git a/app/views/members/_edit.html.erb b/app/views/members/_edit.html.erb index 09369dc76..aa6cc7b6f 100644 --- a/app/views/members/_edit.html.erb +++ b/app/views/members/_edit.html.erb @@ -1,4 +1,4 @@ -<%= form_for(@member, :url => membership_path(@member, :members_page => params[:members_page]), +<%= form_for(@member, :url => membership_path(@member, members_list_params), :as => :membership, :remote => request.xhr?, :method => :put) do |f| %> diff --git a/app/views/members/_new_modal.html.erb b/app/views/members/_new_modal.html.erb index 9719a254d..7d54d4b62 100644 --- a/app/views/members/_new_modal.html.erb +++ b/app/views/members/_new_modal.html.erb @@ -1,6 +1,6 @@

<%= l(:label_member_new) %>

-<%= form_for @member, :as => :membership, :url => project_memberships_path(@project, :members_page => params[:members_page]), :remote => true, :method => :post do |f| %> +<%= form_for @member, :as => :membership, :url => project_memberships_path(@project, members_list_params), :remote => true, :method => :post do |f| %> <%= render :partial => 'new_form' %>

<%= submit_tag l(:button_add), :id => 'member-add-submit' %> diff --git a/app/views/projects/settings/_members.html.erb b/app/views/projects/settings/_members.html.erb index ddb9751c2..942e173a2 100644 --- a/app/views/projects/settings/_members.html.erb +++ b/app/views/projects/settings/_members.html.erb @@ -1,9 +1,23 @@ -<% members, member_pages, member_count = paginate_members(@project) %> +<% members, member_pages, member_count = paginate_members(members_scope(@project)) %> <% if User.current.admin? %>

<%= link_to sprite_icon('settings', l(:label_administration)), users_path, :class => "icon icon-settings" %>
<% end %> -

<%= link_to sprite_icon('add', l(:label_member_new)), new_project_membership_path(@project, :members_page => params[:members_page]), :remote => true, :class => "icon icon-add" %>

+

<%= link_to sprite_icon('add', l(:label_member_new)), new_project_membership_path(@project, members_list_params), :remote => true, :class => "icon icon-add" %>

+ +<%= form_tag(settings_project_path(@project, :tab => 'members'), :method => :get, :id => 'members-filter-form') do %> +
<%= l(:label_filter_plural) %> + +<%= text_field_tag 'member_name', params[:member_name], :size => 20 %> + +<%= select_tag 'member_role_id', + options_for_select([[l(:label_all), '']] + Role.givable.pluck(:name, :id), params[:member_role_id]), + :onchange => "this.form.submit(); return false;" %> +<%= submit_tag l(:button_apply), :class => "small", :name => nil %> +<%= link_to sprite_icon('reload', l(:button_clear)), settings_project_path(@project, :tab => 'members'), :class => 'icon icon-reload' %> +
+<% end %> +  <% if member_count > 0 %>
@@ -33,10 +47,10 @@ <%= link_to sprite_icon('edit', l(:button_edit)), - edit_membership_path(member, :members_page => params[:members_page]), + edit_membership_path(member, members_list_params), :remote => true, :class => 'icon icon-edit' %> - <%= remove_link membership_path(member, :members_page => params[:members_page]), + <%= remove_link membership_path(member, members_list_params), :remote => true, :data => (!User.current.admin? && member.include?(User.current) ? {:confirm => l(:text_own_membership_delete_confirmation)} : {}) if member.deletable? %> diff --git a/test/functional/members_controller_test.rb b/test/functional/members_controller_test.rb index 73d41a578..327e87877 100644 --- a/test/functional/members_controller_test.rb +++ b/test/functional/members_controller_test.rb @@ -274,6 +274,56 @@ class MembersControllerTest < Redmine::ControllerTest assert_match %r{settings/members\?members_page=}, response.body end + def test_edit_xhr_should_keep_current_filters_in_form_action + @request.session[:user_id] = 2 + get(:edit, :params => {:id => 2, :member_name => 'Smith', :member_role_id => 1}, :xhr => true) + assert_response :success + assert_match %r{/memberships/2\?member_name=Smith&member_role_id=1}, response.body + end + + def test_new_xhr_should_keep_current_filters_in_form_action + @request.session[:user_id] = 2 + get(:new, :params => {:project_id => 1, :member_name => 'Smith', :member_role_id => 1}, :xhr => true) + assert_response :success + assert_match %r{/projects/ecookbook/memberships\?member_name=Smith&member_role_id=1}, response.body + end + + def test_update_xhr_should_keep_the_members_list_filtered + @request.session[:user_id] = 2 + put( + :update, + :params => { + :id => 2, + :membership => {:role_ids => [2]}, + :member_role_id => 1 + }, + :xhr => true + ) + assert_response :success + # The updated member does not match the filter anymore, but the list + # must still be filtered by role 1 + assert_match %r{/memberships/1\?member_role_id=1}, response.body + assert_no_match %r{member-4}, response.body + end + + def test_destroy_xhr_should_keep_the_members_list_filtered + @request.session[:user_id] = 2 + assert_difference 'Member.count', -1 do + delete(:destroy, :params => {:id => 2, :member_name => 'Smith'}, :xhr => true) + end + assert_response :success + assert_match %r{/memberships/1\?member_name=Smith}, response.body + assert_no_match %r{member-4}, response.body + end + + def test_destroy_should_redirect_to_the_filtered_members_list + @request.session[:user_id] = 2 + assert_difference 'Member.count', -1 do + delete(:destroy, :params => {:id => 2, :member_name => 'Smith', :member_role_id => 1}) + end + assert_redirected_to '/projects/ecookbook/settings/members?member_name=Smith&member_role_id=1' + end + def test_update_locked_member_should_be_allowed User.find(3).lock! diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb index 9a06652a7..806da57cf 100644 --- a/test/functional/projects_controller_test.rb +++ b/test/functional/projects_controller_test.rb @@ -1112,10 +1112,101 @@ class ProjectsControllerTest < Redmine::ControllerTest @request.session[:user_id] = 1 get(:settings, :params => {:id => project.id, :tab => 'members'}) assert_response :success + assert_select 'div#tab-content-members form#members-filter-form' assert_select 'div#tab-content-members p.nodata' assert_select 'div#tab-content-members table.list.members', :count => 0 end + def test_settings_members_should_display_filter_form + @request.session[:user_id] = 2 + get(:settings, :params => {:id => 'ecookbook', :tab => 'members'}) + assert_response :success + assert_select 'div#tab-content-members form#members-filter-form[action=?]', '/projects/ecookbook/settings/members' do + assert_select 'input[name=member_name]' + assert_select 'select[name=member_role_id][onchange=?]', 'this.form.submit(); return false;' + assert_select 'input[type=submit]' + assert_select 'a[href=?]', '/projects/ecookbook/settings/members' + end + end + + def test_settings_members_should_filter_by_name + @request.session[:user_id] = 2 + get( + :settings, + :params => { + :id => 'ecookbook', + :tab => 'members', + :member_name => 'John' + } + ) + assert_response :success + assert_select 'div#tab-content-members form#members-filter-form' do + assert_select 'input[name=member_name][value=?]', 'John' + end + assert_select 'div#tab-content-members tr#member-1' + assert_select 'div#tab-content-members tr#member-2', :count => 0 + assert_select 'a#tab-members[href*=?]', 'member_name=John' + end + + def test_settings_members_should_filter_by_role + @request.session[:user_id] = 2 + get( + :settings, + :params => { + :id => 'ecookbook', + :tab => 'members', + :member_role_id => '2' + } + ) + assert_response :success + assert_select 'div#tab-content-members form#members-filter-form' do + assert_select 'select[name=member_role_id]' do + assert_select 'option[value="2"][selected=selected]' + end + end + assert_select 'div#tab-content-members tr#member-2' + assert_select 'div#tab-content-members tr#member-1', :count => 0 + assert_select 'a#tab-members[href*=?]', 'member_role_id=2' + end + + def test_settings_members_filter_with_no_matches_should_show_no_data + @request.session[:user_id] = 2 + get( + :settings, + :params => { + :id => 'ecookbook', + :tab => 'members', + :member_name => 'NonexistentPrincipal' + } + ) + assert_response :success + assert_select 'div#tab-content-members form#members-filter-form' + assert_select 'div#tab-content-members p.nodata' + assert_select 'div#tab-content-members table.list.members', :count => 0 + end + + def test_settings_members_pagination_should_preserve_filters + project = Project.find(1) + per_page = 25 + (per_page + 5).times do + user = User.generate! + Member.create!(:project => project, :principal => user, :role_ids => [2]) + end + @request.session[:user_id] = 2 + with_settings :per_page_options => '25,50,100' do + get( + :settings, + :params => { + :id => 'ecookbook', + :tab => 'members', + :member_role_id => '2' + } + ) + end + assert_response :success + assert_select 'div#tab-content-members span.pagination a[href*=?]', 'member_role_id=2' + end + def test_settings_should_show_tabs_depending_on_permission @request.session[:user_id] = 3 project = Project.find(1) diff --git a/test/helpers/members_helper_test.rb b/test/helpers/members_helper_test.rb index 76b94e3bd..a816678fa 100644 --- a/test/helpers/members_helper_test.rb +++ b/test/helpers/members_helper_test.rb @@ -46,7 +46,7 @@ class MembersHelperTest < Redmine::HelperTest project = Project.generate! 5.times { User.add_to_project(User.generate!, project) } - members, member_pages, member_count = paginate_members(project) + members, member_pages, member_count = paginate_members(project.memberships) assert_equal 3, members.size assert_equal 3, member_pages.per_page @@ -59,11 +59,21 @@ class MembersHelperTest < Redmine::HelperTest 3.times { User.add_to_project(User.generate!, project) } Member.where(:project_id => project.id).first.update!(:role_ids => [1, 2]) - members, _member_pages, member_count = paginate_members(project) + members, _member_pages, member_count = paginate_members(project.memberships) member_ids = members.map(&:id) assert_equal member_ids.uniq, member_ids assert_equal member_count, members.size assert_equal project.memberships.count, member_count end + + def test_paginate_members_should_paginate_the_given_scope + stubs(:per_page_option).returns(25) + project = Project.find(1) + + members, _member_pages, member_count = paginate_members(project.memberships.with_role(2)) + + assert_equal [2, 4], members.map(&:id).sort + assert_equal 2, member_count + end end diff --git a/test/unit/member_test.rb b/test/unit/member_test.rb index df9088027..c01c0287b 100644 --- a/test/unit/member_test.rb +++ b/test/unit/member_test.rb @@ -33,6 +33,30 @@ class MemberTest < ActiveSupport::TestCase assert_equal roles, roles.sort end + def test_like_scope_should_match_users_and_groups + assert_equal [1], Project.find(1).memberships.like('Smith').ids + assert_equal [9], Project.find(2).memberships.like('B Team').ids + end + + def test_like_scope_with_blank_value_should_return_all_the_members + project = Project.find(1) + assert_equal project.memberships.ids.sort, project.memberships.like('').ids.sort + end + + def test_with_role_scope_should_return_the_members_having_the_role + assert_equal [2, 4], Project.find(1).memberships.with_role(2).ids.sort + end + + def test_with_role_scope_should_return_the_members_having_the_role_inherited + # Member 7 has the role 1 inherited from the member 6 + assert_include 7, Project.find(5).memberships.with_role(1).ids + end + + def test_with_role_scope_with_blank_value_should_return_all_the_members + project = Project.find(1) + assert_equal project.memberships.ids.sort, project.memberships.with_role(nil).ids.sort + end + def test_create member = Member.new(:project_id => 1, :user_id => 4, :role_ids => [1, 2]) assert member.save -- 2.50.1 (Apple Git-155)