From c442d430a24bfa4666d32780eb312d49043097f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20B=C4=82LTEANU?= Date: Thu, 1 Oct 2026 01:45:53 +0300 Subject: [PATCH 1/3] Make project members page available to all users with the *View members* permission. --- app/controllers/members_controller.rb | 17 +++- app/helpers/members_helper.rb | 13 ++- app/helpers/projects_helper.rb | 2 +- app/helpers/routes_helper.rb | 9 ++ app/views/members/_members.html.erb | 98 +++++++++++++++++++ app/views/members/index.html.erb | 5 + app/views/projects/_members_box.html.erb | 3 + app/views/projects/settings/_members.html.erb | 88 +---------------- config/locales/en.yml | 1 + config/routes.rb | 2 + test/functional/members_controller_test.rb | 95 ++++++++++++++++++ test/functional/projects_controller_test.rb | 2 + test/helpers/members_helper_test.rb | 11 +++ test/helpers/routes_helper_test.rb | 20 ++++ 14 files changed, 269 insertions(+), 97 deletions(-) create mode 100644 app/views/members/_members.html.erb create mode 100644 app/views/members/index.html.erb diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb index a94cd1773..9ea8b72d4 100644 --- a/app/controllers/members_controller.rb +++ b/app/controllers/members_controller.rb @@ -20,6 +20,8 @@ class MembersController < ApplicationController self.model_object = Member + menu_item :overview + before_action :find_model_object, :except => [:index, :new, :create, :autocomplete] before_action :find_project_from_association, :except => [:index, :new, :create, :autocomplete] before_action :find_project_by_project_id, :only => [:index, :new, :create, :autocomplete] @@ -31,12 +33,11 @@ class MembersController < ApplicationController include MembersHelper def index - scope = @project.memberships - @members = scope.includes(:principal, :roles).order(:id) - respond_to do |format| - format.html {head :not_acceptable} + format.html format.api do + scope = @project.memberships + @members = scope.includes(:principal, :roles).order(:id) @offset, @limit = api_offset_and_limit @member_count = scope.count @member_pages = Paginator.new @member_count, @limit, params['page'] @@ -44,7 +45,13 @@ class MembersController < ApplicationController @members = @members.limit(@limit).offset(@offset).to_a end format.csv do - send_data(members_to_csv(@members), type: 'text/csv; header=present', filename: "#{@project.identifier}-members.csv") + if User.current.allowed_to?(:manage_members, @project) + scope = @project.memberships + @members = scope.includes(:principal, :roles).order(:id) + send_data(members_to_csv(@members), type: 'text/csv; header=present', filename: "#{@project.identifier}-members.csv") + else + head :forbidden + end end end end diff --git a/app/helpers/members_helper.rb b/app/helpers/members_helper.rb index bed939635..65fb51beb 100644 --- a/app/helpers/members_helper.rb +++ b/app/helpers/members_helper.rb @@ -59,10 +59,15 @@ module MembersHelper # 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]). - status(params[:member_status]) + scope = + project.memberships. + like(params[:member_name]). + with_role(params[:member_role_id]) + if User.current.allowed_to?(:manage_members, project) + scope.status(params[:member_status]) + else + scope.active + end end # limit/offset on Member.sorted would paginate role join rows, not members diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 635d90f92..9ec6a76fa 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -24,7 +24,7 @@ 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 => 'members/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}, diff --git a/app/helpers/routes_helper.rb b/app/helpers/routes_helper.rb index a27ea783e..7df9e93d1 100644 --- a/app/helpers/routes_helper.rb +++ b/app/helpers/routes_helper.rb @@ -99,4 +99,13 @@ module RoutesHelper def board_path(board, *) project_board_path(board.project, board, *) end + + def _project_members_path(project, parameters = {}) + params = parameters.is_a?(Hash) ? parameters.except('tab', :tab) : parameters + if controller_name == 'members' && action_name == 'index' + project_members_path(project, params) + else + settings_project_path(project, 'members', params) + end + end end diff --git a/app/views/members/_members.html.erb b/app/views/members/_members.html.erb new file mode 100644 index 000000000..e2f433a15 --- /dev/null +++ b/app/views/members/_members.html.erb @@ -0,0 +1,98 @@ +<% 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 %> + +<% if User.current.allowed_to?(:manage_members, @project) %> +

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

+<% end %> + +<%= form_tag(_project_members_path(@project), :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;" %> +<% if User.current.allowed_to?(:manage_members, @project) %> + +<%= select_tag 'member_status', + options_for_select(member_status_options, params[:member_status]), + :onchange => "this.form.submit(); return false;" %> +<% end %> +<%= submit_tag l(:button_apply), :class => "small", :name => nil %> +<%= link_to sprite_icon('reload', l(:button_clear)), _project_members_path(@project), :class => 'icon icon-reload' %> +
+<% end %> +  + +<% if member_count > 0 %> +
+ + + + + + <% if User.current.allowed_to?(:manage_members, @project) %> + + <% end %> + <%= call_hook(:view_projects_settings_members_table_header, :project => @project) %> + + + + <% members.each do |member| %> + <% next if member.new_record? %> + + + + <% if User.current.allowed_to?(:manage_members, @project) %> + + <% end %> + <%= call_hook(:view_projects_settings_members_table_row, { :project => @project, :member => member}) if User.current.allowed_to?(:manage_members, @project) %> + +<% end %> + +
<%= l(:label_user) %> / <%= l(:label_group) %><%= l(:label_role_plural) %>
+ + <% if member.principal %> + <%= link_to_principal member.principal %> + <% end %> + + + <%= member.roles.sort.collect(&:to_s).join(', ') %> +
+ 		+ <%= link_to sprite_icon('edit', l(:button_edit)), + edit_membership_path(member, members_list_params), + :remote => true, + :class => 'icon icon-edit' %> + <%= 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? %> +
+
+ + <%= pagination_links_full(member_pages, member_count) do |text, parameters, options| + link_to text, _project_members_path(@project, request.query_parameters.merge(parameters)), options + end %> + +<% if User.current.allowed_to?(:manage_members, @project) %> + <% other_formats_links do |f| %> + <%= f.link_to_with_query_parameters "CSV", {}, :onclick => "showModal('csv-export-options', '330px'); return false;" %> + <% end %> + +<% end %> +<% else %> +

<%= l(:label_no_data) %>

+<% end %> diff --git a/app/views/members/index.html.erb b/app/views/members/index.html.erb new file mode 100644 index 000000000..0dd606ffe --- /dev/null +++ b/app/views/members/index.html.erb @@ -0,0 +1,5 @@ +

<%= l(:label_member_plural) %>

+ +<%= render :partial => 'members' %> + +<% html_title(l(:label_member_plural)) -%> \ No newline at end of file diff --git a/app/views/projects/_members_box.html.erb b/app/views/projects/_members_box.html.erb index 94a037c25..b6a388114 100644 --- a/app/views/projects/_members_box.html.erb +++ b/app/views/projects/_members_box.html.erb @@ -4,5 +4,8 @@ <% @principals_by_role.keys.sort.each do |role| %>

<%= role %>: <%= @principals_by_role[role].sort.collect{|p| link_to_principal(p, :class => p.is_a?(Group) ? 'icon icon-group' : nil)}.join(", ").html_safe %>

<% end %> + <% if User.current.allowed_to?(:view_members, @project) %> +

<%= link_to l(:label_member_view_all), project_members_path(@project) %>

+ <% end %> <% end %> diff --git a/app/views/projects/settings/_members.html.erb b/app/views/projects/settings/_members.html.erb index 02da44003..ea91c16b3 100644 --- a/app/views/projects/settings/_members.html.erb +++ b/app/views/projects/settings/_members.html.erb @@ -1,87 +1 @@ -<% 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_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;" %> - - <%= select_tag 'member_status', - options_for_select(member_status_options, params[:member_status]), - :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 %> -
- - - - - - - <%= call_hook(:view_projects_settings_members_table_header, :project => @project) %> - - - - <% members.each do |member| %> - <% next if member.new_record? %> - - - - - <%= call_hook(:view_projects_settings_members_table_row, { :project => @project, :member => member}) %> - -<% end %> - -
<%= l(:label_user) %> / <%= l(:label_group) %><%= l(:label_role_plural) %>
- - <% if member.principal %> - <%= link_to_principal member.principal %> - <% end %> - - - <%= member.roles.sort.collect(&:to_s).join(', ') %> -
- 		- <%= link_to sprite_icon('edit', l(:button_edit)), - edit_membership_path(member, members_list_params), - :remote => true, - :class => 'icon icon-edit' %> - <%= 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? %> -
-
- - <%= pagination_links_full(member_pages, member_count) do |text, parameters, options| - link_to text, settings_project_path(@project, 'members', request.query_parameters.except('tab', :tab).merge(parameters)), options - end %> - -<% other_formats_links do |f| %> - <%= f.link_to_with_query_parameters "CSV", {}, :onclick => "showModal('csv-export-options', '330px'); return false;" %> -<% end %> - -<% else %> -

<%= l(:label_no_data) %>

-<% end %> +<%= render :partial => 'members/members' %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 41ba8645e..77e7de2db 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -674,6 +674,7 @@ en: label_member: Member label_member_new: New member label_member_plural: Members + label_member_view_all: View all members label_tracker: Tracker label_tracker_plural: Trackers label_tracker_all: All trackers diff --git a/config/routes.rb b/config/routes.rb index 34bae573c..7a55db15a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -164,6 +164,8 @@ Rails.application.routes.draw do end end + get 'members', :to => 'members#index', :as => 'members' + resource :enumerations, :controller => 'project_enumerations', :only => [:update, :destroy] get 'issues/:copy_from/copy', :to => 'issues#new', :as => 'copy_issue' diff --git a/test/functional/members_controller_test.rb b/test/functional/members_controller_test.rb index 327e87877..c6ae33917 100644 --- a/test/functional/members_controller_test.rb +++ b/test/functional/members_controller_test.rb @@ -461,4 +461,99 @@ class MembersControllerTest < Redmine::ControllerTest assert_response :success assert_include 'User Misc', response.body end + + def test_index_html_should_render_members_page + get(:index, :params => {:project_id => 1}) + assert_response :success + assert_select 'h2', :text => 'Members' + assert_select 'div#main-menu a.overview.selected' + assert_select 'title', :text => 'Members - eCookbook - Redmine' + assert_select 'form#members-filter-form[action=?]', '/projects/ecookbook/members' do + assert_select 'input[name=member_name]' + assert_select 'select[name=member_role_id]' + assert_select 'input[type=submit]' + assert_select 'a[href=?]', '/projects/ecookbook/members' + end + assert_select 'table.list.members' + end + + def test_index_html_filter_by_status + @request.session[:user_id] = 2 + get(:index, :params => {:project_id => 1, :member_status => 3}) + assert_response :success + assert_select 'table.list.members tbody tr.member', :count => 1 + assert_select 'tr#member-4' + assert_select 'tr#member-1', :count => 0 + assert_select 'tr#member-2', :count => 0 + end + + def test_index_html_for_user_with_manage_members_permission_should_display_actions + @request.session[:user_id] = 2 # user 2 has manage_members + get(:index, :params => {:project_id => 1}) + assert_response :success + assert_select 'p a.icon-add' + assert_select 'td.buttons a.icon-edit' + assert_select 'select[name=member_status]' + end + + def test_index_html_for_user_without_manage_members_permission_should_not_display_actions + role = Role.create!(:name => 'Viewer', :permissions => [:view_project, :view_members]) + user = User.generate! + User.add_to_project(user, Project.find(1), role) + @request.session[:user_id] = user.id + + get(:index, :params => {:project_id => 1}) + assert_response :success + assert_select 'p a.icon-add', :count => 0 + assert_select 'td.buttons', :count => 0 + assert_select 'select[name=member_status]', :count => 0 + assert_select 'tr#member-4', :count => 0 + end + + def test_index_html_for_user_without_manage_members_permission_should_only_show_active_members + role = Role.create!(:name => 'Viewer', :permissions => [:view_project, :view_members]) + user = User.generate! + User.add_to_project(user, Project.find(1), role) + @request.session[:user_id] = user.id + + get(:index, :params => {:project_id => 1, :member_status => 3}) + assert_response :success + assert_select 'select[name=member_status]', :count => 0 + assert_select 'tr#member-4', :count => 0 + assert_select 'tr#member-1' + assert_select 'tr#member-2' + end + + def test_index_html_filter_by_name + get(:index, :params => {:project_id => 1, :member_name => 'John'}) + assert_response :success + assert_select 'table.list.members tbody tr.member', :count => 1 + assert_select 'tr#member-1' + end + + def test_index_html_filter_by_role + get(:index, :params => {:project_id => 1, :member_role_id => 2}) + assert_response :success + assert_select 'table.list.members tbody tr.member' + assert_select 'tr#member-2' + assert_select 'tr#member-1', :count => 0 + end + + def test_index_html_with_pagination + project = Project.find(1) + 26.times { User.add_to_project(User.generate!, project, Role.find(2)) } + with_settings :per_page_options => '25,50,100' do + get(:index, :params => {:project_id => 1, :members_page => 2}) + assert_response :success + assert_select 'span.pagination' + assert_select 'span.pagination a[href*=?]', '/projects/ecookbook/members?members_page=' + assert_select 'span.pagination a[href*=?]', 'settings/members', :count => 0 + end + end + + def test_index_html_unauthorized_user_should_be_denied + @request.session[:user_id] = nil + get(:index, :params => {:project_id => 2}) + assert_response :redirect + end end diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb index 899e46438..1f38350dc 100644 --- a/test/functional/projects_controller_test.rb +++ b/test/functional/projects_controller_test.rb @@ -784,6 +784,7 @@ class ProjectsControllerTest < Redmine::ControllerTest get(:show, :params => {:id => 1}) assert_response :success assert_select '#header h1', :text => "eCookbook" + assert_select 'div.members.box a[href=?]', '/projects/ecookbook/members', :text => 'View all members' end def test_show_by_identifier @@ -1037,6 +1038,7 @@ class ProjectsControllerTest < Redmine::ControllerTest } ) assert_response :success + assert_select 'div#main-menu a.settings.selected' assert_select "tr#member-#{user_member.id} td.name a[href=?]", "/users/#{user.id}", :text => user.name assert_select "tr#member-#{group_member.id} td.name a[href=?]", '/groups/10', :text => 'A Team' end diff --git a/test/helpers/members_helper_test.rb b/test/helpers/members_helper_test.rb index ec3f3a54a..775276c96 100644 --- a/test/helpers/members_helper_test.rb +++ b/test/helpers/members_helper_test.rb @@ -87,6 +87,7 @@ class MembersHelperTest < Redmine::HelperTest end def test_members_scope_default_should_return_all_members + User.current = User.find(2) project = Project.find(1) stubs(:params).returns({}) @@ -94,6 +95,7 @@ class MembersHelperTest < Redmine::HelperTest end def test_members_scope_with_status_active_should_filter_by_active_status + User.current = User.find(2) project = Project.find(1) stubs(:params).returns({:member_status => '1'}) @@ -101,9 +103,18 @@ class MembersHelperTest < Redmine::HelperTest end def test_members_scope_with_status_locked_should_return_locked_members + User.current = User.find(2) project = Project.find(1) stubs(:params).returns({:member_status => '3'}) assert_equal [4], members_scope(project).ids.sort end + + def test_members_scope_for_user_without_manage_members_permission_should_return_active_members_only + User.current = User.find(4) + project = Project.find(1) + stubs(:params).returns({:member_status => '3'}) + + assert_equal [1, 2], members_scope(project).ids.sort + end end diff --git a/test/helpers/routes_helper_test.rb b/test/helpers/routes_helper_test.rb index 7c9c2b349..167ffe820 100644 --- a/test/helpers/routes_helper_test.rb +++ b/test/helpers/routes_helper_test.rb @@ -43,4 +43,24 @@ class RoutesHelperTest < Redmine::HelperTest assert_equal 'http://test.host/projects/ecookbook/issues?set_filter=1', _project_issues_url(Project.find(1), set_filter: 1) assert_equal 'http://test.host/issues?set_filter=1', _project_issues_url(nil, set_filter: 1) end + + def test_project_members_path_in_settings_context + project = Project.find('ecookbook') + stubs(:controller_name).returns('projects') + stubs(:action_name).returns('settings') + + assert_equal '/projects/ecookbook/settings/members', _project_members_path(project) + assert_equal '/projects/ecookbook/settings/members?members_page=2', _project_members_path(project, :members_page => 2) + assert_equal '/projects/ecookbook/settings/members?members_page=2', _project_members_path(project, :tab => 'members', :members_page => 2) + end + + def test_project_members_path_in_project_members_context + project = Project.find('ecookbook') + stubs(:controller_name).returns('members') + stubs(:action_name).returns('index') + + assert_equal '/projects/ecookbook/members', _project_members_path(project) + assert_equal '/projects/ecookbook/members?members_page=2', _project_members_path(project, :members_page => 2) + assert_equal '/projects/ecookbook/members?members_page=2', _project_members_path(project, :tab => 'members', :members_page => 2) + end end -- 2.54.0 (Apple Git-157)