diff --git a/app/contracts/members/base_contract.rb b/app/contracts/members/base_contract.rb index 7a4c7158ac..10563185df 100644 --- a/app/contracts/members/base_contract.rb +++ b/app/contracts/members/base_contract.rb @@ -40,6 +40,12 @@ module Members validate :project_set validate :project_manageable + def assignable_projects + Project + .active + .where(id: Project.allowed_to(user, :manage_members)) + end + private def user_allowed_to_manage diff --git a/app/contracts/members/delete_contract.rb b/app/contracts/members/delete_contract.rb index fed3425291..11007e070d 100644 --- a/app/contracts/members/delete_contract.rb +++ b/app/contracts/members/delete_contract.rb @@ -29,5 +29,13 @@ module Members class DeleteContract < ::DeleteContract delete_permission :manage_members + + validate :member_is_deletable + + private + + def member_is_deletable + errors.add(:base, :not_deletable) unless model.deletable? + end end end diff --git a/app/services/members/set_attributes_service.rb b/app/services/members/set_attributes_service.rb index aa6e2afe99..8c65e048c3 100644 --- a/app/services/members/set_attributes_service.rb +++ b/app/services/members/set_attributes_service.rb @@ -33,9 +33,19 @@ module Members private def set_attributes(params) - model.assign_roles(params.delete(:role_ids)) if params[:role_ids] + assign_roles(params) super end + + def assign_roles(params) + return unless params[:role_ids] + + role_ids = params + .delete(:role_ids) + .select(&:present?) + + model.assign_roles(role_ids) + end end end diff --git a/app/views/individual_principals/_memberships.html.erb b/app/views/individual_principals/_memberships.html.erb index 6de259238b..b16a08d441 100644 --- a/app/views/individual_principals/_memberships.html.erb +++ b/app/views/individual_principals/_memberships.html.erb @@ -27,7 +27,10 @@ See docs/COPYRIGHT.rdoc for more details. ++#%> <% roles = Role.givable %> -<% projects = Project.visible.active.order(Arel.sql('lft')) %> +<% projects = ::Members::CreateContract + .new(@individual_principal.memberships.build, current_user) + .assignable_projects + .order(Arel.sql('lft')) %> <% memberships = @individual_principal.memberships.visible(current_user) %>
@@ -111,14 +114,16 @@ See docs/COPYRIGHT.rdoc for more details. roles: roles, projects: projects) %> - <%= link_to_function icon_wrapper('icon icon-edit', t(:button_edit)), - "jQuery('.member-#{membership.id}--edit-toggle-item').toggle();", - class: "member-#{membership.id}--edit-toggle-item memberships--edit-button", - title: t(:button_edit) %> - <%= link_to(icon_wrapper('icon icon-remove', t(:button_remove)), - polymorphic_path([@individual_principal, :membership], id: membership), - method: :delete, - title: t(:button_remove)) if membership.deletable? %> + <% if User.current.allowed_to?(:manage_members, membership.project) %> + <%= link_to_function icon_wrapper('icon icon-edit', t(:button_edit)), + "jQuery('.member-#{membership.id}--edit-toggle-item').toggle();", + class: "member-#{membership.id}--edit-toggle-item memberships--edit-button", + title: t(:button_edit) %> + <%= link_to(icon_wrapper('icon icon-remove', t(:button_remove)), + polymorphic_path([@individual_principal, :membership], id: membership), + method: :delete, + title: t(:button_remove)) if membership.deletable? %> + <% end %> <% end %> diff --git a/app/views/users/_available_global_roles.html.erb b/app/views/users/_available_global_roles.html.erb index bdf766f336..7098bd7536 100644 --- a/app/views/users/_available_global_roles.html.erb +++ b/app/views/users/_available_global_roles.html.erb @@ -38,9 +38,16 @@ See docs/COPYRIGHT.rdoc for more details. <% else %> - <%= form_for(:principal_roles, :url => user_memberships_path(user_id: user), :method => :post) do %> + <% args = + if global_member + { url: user_membership_path(id: global_member.id, user_id: user.id), method: :patch } + else + { url: user_memberships_path(user_id: user.id), method: :post } + end + %> + <%= form_for(:principal_roles, **args) do %> <% if global_member %> - <%= hidden_field_tag(:id, global_member.id ) %> + <%= hidden_field_tag('membership[id]', global_member.id ) %> <% global_member.roles.each do |role| %> <%= hidden_field_tag('membership[role_ids][]', role.id ) %> diff --git a/lib/individual_principals/membership_controller_methods.rb b/lib/individual_principals/membership_controller_methods.rb index f4dd042982..363214626d 100644 --- a/lib/individual_principals/membership_controller_methods.rb +++ b/lib/individual_principals/membership_controller_methods.rb @@ -1,43 +1,52 @@ module IndividualPrincipals module MembershipControllerMethods - def update - update_or_create(request.patch?, :notice_successful_update) + extend ActiveSupport::Concern + + included do + before_action :find_membership, only: %i[update destroy] end def create - update_or_create(request.post?, :notice_successful_create) + membership_params = permitted_params.membership.merge(principal: @individual_principal) + call = ::Members::CreateService + .new(user: current_user) + .call(membership_params) + + respond_with_service_call call, message: :notice_successful_create end - def destroy - @membership = @individual_principal.memberships.find(params[:id]) - tab = redirected_to_tab(@membership) + def update + call = ::Members::UpdateService + .new(model: @membership, user: current_user) + .call(permitted_params.membership) - if @membership.deletable? && request.delete? - @membership.destroy - @membership = nil + respond_with_service_call call, message: :notice_successful_update + end - flash[:notice] = I18n.t(:notice_successful_delete) - end + def destroy + call = ::Members::DeleteService + .new(model: @membership, user: current_user) + .call - redirect_to edit_polymorphic_path(@individual_principal, tab: tab) + respond_with_service_call call, message: :notice_successful_delete end private - def update_or_create(save_record, message) - @membership = params[:id].present? ? Member.find(params[:id]) : Member.new(principal: @individual_principal, project: nil) - - result = ::Members::EditMembershipService - .new(@membership, save: save_record, current_user: current_user) - .call(attributes: permitted_params.membership) + def find_membership + @membership = Member.visible(current_user).find(params[:id]) + rescue ActiveRecord::RecordNotFound + render_404 + end - if result.success? + def respond_with_service_call(call, message:) + if call.success? flash[:notice] = I18n.t(message) else - flash[:error] = result.errors.full_messages.join("\n") + flash[:error] = call.errors.full_messages.join("\n") end - redirect_to edit_polymorphic_path(@individual_principal, tab: redirected_to_tab(@membership)) + redirect_to edit_polymorphic_path(@individual_principal, tab: redirected_to_tab(call.result)) end end end diff --git a/spec/contracts/members/create_contract_spec.rb b/spec/contracts/members/create_contract_spec.rb index d3fceceffb..fb1ea5b73e 100644 --- a/spec/contracts/members/create_contract_spec.rb +++ b/spec/contracts/members/create_contract_spec.rb @@ -61,5 +61,29 @@ describe Members::CreateContract do it_behaves_like 'contract is invalid', principal: :unassignable end end + + describe '#assignable_projects' do + context 'as a user without permission' do + let(:current_user) { FactoryBot.build_stubbed :user } + + it 'is empty' do + expect(contract.assignable_projects).to be_empty + end + end + + context 'as a user with permission in one project' do + let!(:project1) { FactoryBot.create :project } + let!(:project2) { FactoryBot.create :project } + let(:current_user) do + FactoryBot.create :user, + member_in_project: project1, + member_with_permissions: %i[manage_members] + end + + it 'returns the one project' do + expect(contract.assignable_projects.to_a).to eq [project1] + end + end + end end end diff --git a/spec/controllers/placeholder_users/memberships_controller_spec.rb b/spec/controllers/placeholder_users/memberships_controller_spec.rb index deb01079c9..b3c15e778d 100644 --- a/spec/controllers/placeholder_users/memberships_controller_spec.rb +++ b/spec/controllers/placeholder_users/memberships_controller_spec.rb @@ -81,7 +81,7 @@ describe PlaceholderUsers::MembershipsController, type: :controller do id: 1234 } - expect(response.status).to eq 403 + expect(response.status).to eq 404 end end @@ -92,7 +92,7 @@ describe PlaceholderUsers::MembershipsController, type: :controller do id: 1234 } - expect(response.status).to eq 403 + expect(response.status).to eq 404 end end end @@ -103,12 +103,63 @@ describe PlaceholderUsers::MembershipsController, type: :controller do it_behaves_like 'update memberships flow' end - context 'as user with global permission' do - current_user { FactoryBot.create :user, global_permission: %i[manage_placeholder_user] } + context 'as user with global permission and manage_members' do + current_user do + FactoryBot.create :user, + member_in_project: project, + member_with_permissions: %i[manage_members], + global_permission: %i[manage_placeholder_user] + end it_behaves_like 'update memberships flow' end + context 'as user with global permission but not project permission' do + current_user { FactoryBot.create :user, global_permission: %i[manage_placeholder_user] } + + describe 'POST create' do + it 'redirects but fails to create' do + post :create, params: { + placeholder_user_id: placeholder_user.id, + membership: { + project_id: project.id, + role_ids: [role.id] + } + } + + expect(response.status).to eq 302 + expect(placeholder_user.reload.memberships).to be_empty + end + end + + context 'with a membership in another project that is invisible' do + shared_let(:project2) { FactoryBot.create :project } + shared_let(:membership) { FactoryBot.create :member, principal: placeholder_user, project: project2, roles: [role] } + + describe 'PUT update' do + it 'returns an error' do + put :update, params: { + placeholder_user_id: placeholder_user.id, + id: membership.id + } + + expect(response.status).to eq 404 + end + end + + describe 'DELETE destroy' do + it 'returns an error' do + delete :destroy, params: { + placeholder_user_id: placeholder_user.id, + id: membership.id + } + + expect(response.status).to eq 404 + end + end + end + end + context 'as user without global permission' do current_user { FactoryBot.create :user } diff --git a/spec/features/principals/shared_memberships_examples.rb b/spec/features/principals/shared_memberships_examples.rb index f06e97d329..9c57c8e680 100644 --- a/spec/features/principals/shared_memberships_examples.rb +++ b/spec/features/principals/shared_memberships_examples.rb @@ -81,6 +81,25 @@ shared_examples 'global user principal membership management flows' do |permissi end end + context 'as user with global and project permissions, but not manage_members' do + current_user do + FactoryBot.create :user, + global_permission: permission, + member_in_project: project, + member_with_permissions: %i[view_work_packages] + end + + it 'does not allow to select that project' do + principal_page.visit! + principal_page.open_projects_tab! + + expect(page).to have_no_selector('tr.member') + expect(page).to have_text 'There is currently nothing to display.' + expect(page).to have_no_text project.name + expect(page).to have_no_text project2.name + end + end + context 'as user without global permission' do current_user { FactoryBot.create :user } diff --git a/spec/services/members/set_attributes_service_spec.rb b/spec/services/members/set_attributes_service_spec.rb index 8485176233..aeb6768d15 100644 --- a/spec/services/members/set_attributes_service_spec.rb +++ b/spec/services/members/set_attributes_service_spec.rb @@ -130,6 +130,18 @@ describe Members::SetAttributesService, type: :model do it 'adds the new role' do expect(subject.result.roles = [second_role, third_role]) end + + context 'with role_ids not all being present' do + let(:call_attributes) do + { + role_ids: [nil, '', second_role.id, third_role.id] + } + end + + it 'ignores the empty values' do + expect(subject.result.roles = [second_role, third_role]) + end + end end end end