Merge branch 'mmj-remove-project-member-owner' into 'master'

Remove ProjectMember#owner? method

See merge request gitlab-org/gitlab!83506

app/models/members/project_member.rb

@@ -82,10 +82,6 @@ class ProjectMember < Member
     source
   end
 
-  def owner?
-    project.owner == user
-  end
-
   def notifiable_options
     { project: project }
   end
@@ -132,7 +128,10 @@ class ProjectMember < Member
   end
 
   def post_create_hook
-    unless owner?
+    # The creator of a personal project gets added as a `ProjectMember`
+    # with `OWNER` access during creation of a personal project,
+    # but we do not want to trigger notifications to the same person who created the personal project.
+    unless project.personal_namespace_holder?(user)
       event_service.join_project(self.project, self.user)
       run_after_commit_or_now { notification_service.new_project_member(self) }
     end

app/models/project.rb

@@ -899,6 +899,18 @@ class Project < ApplicationRecord
     association(:namespace).loaded?
   end
 
+  def personal_namespace_holder?(user)
+    return false unless personal?
+    return false unless user
+
+    # We do not want to use a check like `project.team.owner?(user)`
+    # here because that would depend upon the state of the `project_authorizations` cache,
+    # and also perform the check across multiple `owners` of the project, but our intention
+    # is to check if the user is the "holder" of the personal namespace, so need to make this
+    # check against only a single user (ie, namespace.owner).
+    namespace.owner == user
+  end
+
   def project_setting
     super.presence || build_project_setting
   end

app/services/members/projects/creator_service.rb

@@ -10,12 +10,12 @@ module Members
       private
 
       def can_update_member?
-        super || current_user.can?(:update_project_member, member) || adding_a_new_owner?
+        super || current_user.can?(:update_project_member, member) || adding_the_creator_as_owner_in_a_personal_project?
       end
 
-      def adding_a_new_owner?
+      def adding_the_creator_as_owner_in_a_personal_project?
         # this condition is reached during testing setup a lot due to use of `.add_user`
-        member.owner? && member.new_record?
+        member.project.personal_namespace_holder?(member.user) && member.new_record?
       end
     end
   end

ee/spec/models/project_member_spec.rb

@@ -167,4 +167,32 @@ RSpec.describe ProjectMember do
       end
     end
   end
+
+  describe 'post create hooks' do
+    context 'when a new personal project is created' do
+      it 'does not send notifications or create events for the creator of the project' do
+        expect(NotificationService).not_to receive(:new)
+        expect(EventCreateService).not_to receive(:new)
+
+        create(:project, namespace: create(:user).namespace)
+      end
+    end
+
+    context 'when a different user is added to a personal project as OWNER' do
+      let_it_be(:project) { create(:project, namespace: create(:user).namespace) }
+      let_it_be(:another_user) { create(:user) }
+
+      it 'sends notifications and creates events for the newly added OWNER' do
+        expect_next_instance_of(NotificationService) do |service|
+          expect(service).to receive(:new_project_member).with(project.member(another_user))
+        end
+
+        expect_next_instance_of(EventCreateService) do |service|
+          expect(service).to receive(:join_project).with(project, another_user)
+        end
+
+        project.add_owner(another_user)
+      end
+    end
+  end
 end

spec/models/project_spec.rb

@@ -726,6 +726,33 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
+  describe '#personal_namespace_holder?' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:namespace_user) { create(:user) }
+    let_it_be(:admin_user) { create(:user, :admin) }
+    let_it_be(:personal_project) { create(:project, namespace: namespace_user.namespace) }
+    let_it_be(:group_project) { create(:project, group: group) }
+    let_it_be(:another_user) { create(:user) }
+    let_it_be(:group_owner_user) { create(:user).tap { |user| group.add_owner(user) } }
+
+    where(:project, :user, :result) do
+      ref(:personal_project)      | ref(:namespace_user)   | true
+      ref(:personal_project)      | ref(:admin_user)       | false
+      ref(:personal_project)      | ref(:another_user)     | false
+      ref(:personal_project)      | nil                    | false
+      ref(:group_project)         | ref(:namespace_user)   | false
+      ref(:group_project)         | ref(:group_owner_user) | false
+      ref(:group_project)         | ref(:another_user)     | false
+      ref(:group_project)         | nil                    | false
+      ref(:group_project)         | nil                    | false
+      ref(:group_project)         | ref(:admin_user)       | false
+    end
+
+    with_them do
+      it { expect(project.personal_namespace_holder?(user)).to eq(result) }
+    end
+  end
+
   describe '#default_pipeline_lock' do
     let(:project) { build_stubbed(:project) }