Fix for memberships on project transfer (#282)

* first attempt at fixing group transfer memberships

* reverting transfer serivce test

* testing job enqueued in transfer service test

* calling the UpdateMembershipJob in the project transfer service

* adding tests to update_memberships_job_test.rb

* adding tests to update_memberships_job_test.rb

* adding missing changes

* fixing test

* fixing project and ancestor member user ids in transfer service

app/services/projects/transfer_service.rb

@@ -39,7 +39,13 @@ def transfer(project)
         raise TransferError, I18n.t('services.projects.transfer.namespace_project_exists')
       end
 
+      project_ancestor_member_user_ids = Member.for_namespace_and_ancestors(project.namespace).select(:user_id)
+      new_namespace_member_ids = Member.for_namespace_and_ancestors(@new_namespace)
+                                       .where(user_id: project_ancestor_member_user_ids).select(&:id)
+
       project.namespace.update(parent_id: @new_namespace.id)
+
+      UpdateMembershipsJob.perform_later(new_namespace_member_ids)
     end
   end
 end

test/jobs/update_memberships_job_test.rb

@@ -8,17 +8,20 @@ def setup
     @group_member = members(:group_one_member_ryan_doe)
     @first_subgroup_member = members(:subgroup1_member_ryan_doe)
     @second_subgroup_member = members(:subgroup2_member_ryan_doe)
+    @project_member = members(:project_one_member_ryan_doe)
   end
 
   test 'parent group access level higher' do
     perform_enqueued_jobs do
       assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.access_level
+      assert_equal Member::AccessLevel::GUEST, @project_member.access_level
       assert_equal Member::AccessLevel::GUEST, @second_subgroup_member.access_level
 
       valid_params = { user: @group_member.user, access_level: Member::AccessLevel::MAINTAINER }
       Members::UpdateService.new(@group_member, @group_member.namespace, @user, valid_params).execute
 
       assert_equal Member::AccessLevel::MAINTAINER, @group_member.reload.access_level
+      assert_equal Member::AccessLevel::MAINTAINER, @project_member.reload.access_level
       assert_equal Member::AccessLevel::MAINTAINER, @first_subgroup_member.reload.access_level
       assert_equal Member::AccessLevel::MAINTAINER, @second_subgroup_member.reload.access_level
     end
@@ -27,12 +30,14 @@ def setup
   test 'subgroup access level higher' do
     perform_enqueued_jobs do
       assert_equal Member::AccessLevel::GUEST, @group_member.access_level
+      assert_equal Member::AccessLevel::GUEST, @project_member.access_level
       assert_equal Member::AccessLevel::GUEST, @second_subgroup_member.access_level
 
       valid_params = { user: @first_subgroup_member.user, access_level: Member::AccessLevel::MAINTAINER }
       Members::UpdateService.new(@first_subgroup_member, @group_member.namespace, @user, valid_params).execute
 
       assert_equal Member::AccessLevel::GUEST, @group_member.reload.access_level
+      assert_equal Member::AccessLevel::GUEST, @project_member.reload.access_level
       assert_equal Member::AccessLevel::MAINTAINER, @first_subgroup_member.reload.access_level
       assert_equal Member::AccessLevel::MAINTAINER, @second_subgroup_member.reload.access_level
     end
@@ -41,37 +46,60 @@ def setup
   test 'nested subgroup access level higher' do
     perform_enqueued_jobs do
       assert_equal Member::AccessLevel::GUEST, @group_member.access_level
+      assert_equal Member::AccessLevel::GUEST, @project_member.access_level
       assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.access_level
 
       valid_params = { user: @second_subgroup_member.user, access_level: Member::AccessLevel::MAINTAINER }
       Members::UpdateService.new(@second_subgroup_member, @group_member.namespace, @user, valid_params).execute
 
       assert_equal Member::AccessLevel::GUEST, @group_member.reload.access_level
+      assert_equal Member::AccessLevel::GUEST, @project_member.reload.access_level
       assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.reload.access_level
       assert_equal Member::AccessLevel::MAINTAINER, @second_subgroup_member.reload.access_level
     end
   end
 
+  test 'project access level higher' do
+    perform_enqueued_jobs do
+      assert_equal Member::AccessLevel::GUEST, @group_member.access_level
+      assert_equal Member::AccessLevel::GUEST, @project_member.access_level
+      assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.access_level
+      assert_equal Member::AccessLevel::GUEST, @second_subgroup_member.access_level
+
+      valid_params = { user: @project_member.user, access_level: Member::AccessLevel::MAINTAINER }
+      Members::UpdateService.new(@project_member, @group_member.namespace, @user, valid_params).execute
+
+      assert_equal Member::AccessLevel::GUEST, @group_member.reload.access_level
+      assert_equal Member::AccessLevel::MAINTAINER, @project_member.reload.access_level
+      assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.reload.access_level
+      assert_equal Member::AccessLevel::GUEST, @second_subgroup_member.reload.access_level
+    end
+  end
+
   test 'empty memberships' do
     assert_equal Member::AccessLevel::GUEST, @group_member.access_level
+    assert_equal Member::AccessLevel::GUEST, @project_member.access_level
     assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.access_level
     assert_equal Member::AccessLevel::GUEST, @second_subgroup_member.access_level
 
     UpdateMembershipsJob.perform_now([])
 
     assert_equal Member::AccessLevel::GUEST, @group_member.reload.access_level
+    assert_equal Member::AccessLevel::GUEST, @project_member.reload.access_level
     assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.reload.access_level
     assert_equal Member::AccessLevel::GUEST, @second_subgroup_member.reload.access_level
   end
 
   test 'memberships do not exist' do
     assert_equal Member::AccessLevel::GUEST, @group_member.access_level
+    assert_equal Member::AccessLevel::GUEST, @project_member.access_level
     assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.access_level
     assert_equal Member::AccessLevel::GUEST, @second_subgroup_member.access_level
 
     UpdateMembershipsJob.perform_now([0])
 
     assert_equal Member::AccessLevel::GUEST, @group_member.reload.access_level
+    assert_equal Member::AccessLevel::GUEST, @project_member.reload.access_level
     assert_equal Member::AccessLevel::GUEST, @first_subgroup_member.reload.access_level
     assert_equal Member::AccessLevel::GUEST, @second_subgroup_member.reload.access_level
   end

test/policies/group_policy_test.rb

@@ -80,7 +80,7 @@ def setup
   test 'scope' do
     scoped_groups = @policy.apply_scope(Group, type: :relation)
 
-    # John Doe has access to 19 groups
+    # John Doe has access to 23 groups
     assert_equal scoped_groups.count, 23
 
     user = users(:david_doe)

test/services/groups/transfer_service_test.rb

@@ -12,7 +12,6 @@ def setup
 
     test 'transfer group with permission' do
       new_namespace = namespaces_user_namespaces(:john_doe_namespace)
-
       assert_changes -> { @group.parent }, to: new_namespace do
         Groups::TransferService.new(@group, @john_doe).execute(new_namespace)
       end
@@ -39,11 +38,9 @@ def setup
 
     test 'transfer group without group permission' do
       new_namespace = namespaces_user_namespaces(:jane_doe_namespace)
-
       exception = assert_raises(ActionPolicy::Unauthorized) do
         Groups::TransferService.new(@group, @jane_doe).execute(new_namespace)
       end
-
       assert_equal GroupPolicy, exception.policy
       assert_equal :transfer?, exception.rule
       assert exception.result.reasons.is_a?(::ActionPolicy::Policy::FailureReasons)
@@ -55,7 +52,6 @@ def setup
 
     test 'transfer group without target namespace permission' do
       new_namespace = namespaces_user_namespaces(:jane_doe_namespace)
-
       assert_raises(ActionPolicy::Unauthorized) do
         Groups::TransferService.new(@group, @john_doe).execute(new_namespace)
       end
@@ -64,7 +60,6 @@ def setup
 
     test 'authorize allowed to transfer group with permission' do
       new_namespace = namespaces_user_namespaces(:john_doe_namespace)
-
       assert_authorized_to(:transfer?, @group,
                            with: GroupPolicy,
                            context: { user: @john_doe }) do
@@ -76,7 +71,6 @@ def setup
 
     test 'authorize allowed to transfer group into namespace' do
       new_namespace = namespaces_user_namespaces(:john_doe_namespace)
-
       assert_authorized_to(:transfer_into_namespace?, new_namespace,
                            with: Namespaces::UserNamespacePolicy,
                            context: { user: @john_doe }) do

test/services/projects/transfer_service_test.rb

@@ -16,16 +16,20 @@ def setup
       assert_changes -> { @project.namespace.parent }, to: new_namespace do
         Projects::TransferService.new(@project, @john_doe).execute(new_namespace)
       end
+
+      assert_enqueued_with(job: UpdateMembershipsJob)
     end
 
     test 'transfer project without specifying new namespace' do
       assert_not Projects::TransferService.new(@project, @john_doe).execute(nil)
+      assert_no_enqueued_jobs
     end
 
     test 'transfer project to namespace containing project' do
       group_one = groups(:group_one)
 
       assert_not Projects::TransferService.new(@project, @john_doe).execute(group_one)
+      assert_no_enqueued_jobs
     end
 
     test 'transfer project without project permission' do
@@ -41,6 +45,7 @@ def setup
       assert_equal I18n.t(:'action_policy.policy.project.transfer?',
                           name: @project.name),
                    exception.result.message
+      assert_no_enqueued_jobs
     end
 
     test 'transfer project without target namespace permission' do
@@ -49,13 +54,16 @@ def setup
       assert_raises(ActionPolicy::Unauthorized) do
         Projects::TransferService.new(@project, @john_doe).execute(new_namespace)
       end
+
+      assert_no_enqueued_jobs
     end
 
     test 'transfer project to namespace containing project with same name' do
       project = projects(:john_doe_project2)
       group_one = groups(:group_one)
 
       assert_not Projects::TransferService.new(project, @john_doe).execute(group_one)
+      assert_no_enqueued_jobs
     end
 
     test 'authorize allowed to transfer project with permission' do
@@ -67,6 +75,7 @@ def setup
         Projects::TransferService.new(@project,
                                       @john_doe).execute(new_namespace)
       end
+      assert_enqueued_with(job: UpdateMembershipsJob)
     end
 
     test 'authorize allowed to transfer to namespace' do
@@ -78,6 +87,7 @@ def setup
         Projects::TransferService.new(@project,
                                       @john_doe).execute(new_namespace)
       end
+      assert_enqueued_with(job: UpdateMembershipsJob)
     end
 
     test 'project transfer changes logged using logidze' do
@@ -101,6 +111,8 @@ def setup
       assert_equal groups(:group_one), project_namespace.at(version: 1).parent
 
       assert_equal new_namespace, project_namespace.at(version: 2).parent
+
+      assert_enqueued_with(job: UpdateMembershipsJob)
     end
 
     test 'project transfer changes logged using logidze switch version' do
@@ -128,6 +140,8 @@ def setup
       project_namespace.switch_to!(1)
 
       assert_equal groups(:group_one), project_namespace.parent
+
+      assert_enqueued_with(job: UpdateMembershipsJob)
     end
   end
 end

test/services/samples/update_service_test.rb

@@ -4,8 +4,6 @@
 
 module Samples
   class UpdateServiceTest < ActiveSupport::TestCase
-    include ActionPolicy::TestHelper
-
     def setup
       @user = users(:john_doe)
       @sample = samples(:sample23)