Use contracted member services and show only manageable projects in user administration (#9076)

* Reuse contracted services in memberships controller

* Show only manageable projects

* Show only manageable projects in select box

* Reject empty values in role_ids

* Add spec for when user has no manage_members permission

* Also handle delete through service

* Extend spec for when user has no manage_members permission

* Extend Members SetAttributesSpec

* Make manageable_Members an instance method and add spec

* hide buttons if user is not allowed to use them

* Rename manageable_projects to assignable_projects

* Build a membership for the contract

* Fix url for global_roles to work with update and create methods

Co-authored-by: ulferts <jens.ulferts@googlemail.com>

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

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

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

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) %>
 
 <div class="grid-block">
@@ -111,14 +114,16 @@ See docs/COPYRIGHT.rdoc for more details.
                                                                  roles: roles,
                                                                  projects: projects) %>
                   <td class="buttons">
-                    <%= 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 %>
                   </td>
                 </tr>
               <% end %>

app/views/users/_available_global_roles.html.erb

@@ -38,9 +38,16 @@ See docs/COPYRIGHT.rdoc for more details.
       </span>
     <% else %>
       <span id="additional_principal_roles">
-        <%= 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 ) %>

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

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

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 }
 

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 }
 

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