Merge branch '45572-members-invitations-scheduled-before-commit' into 'master'

Resolve "Members invitations scheduled before commit"

Closes #45572

See merge request gitlab-org/gitlab-ce!18538

10 files changed, 142 insertions, 150 deletions

diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 661e668dbf9..5da739f9618 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -37,20 +37,20 @@ class GroupMember < Member
   private
 
   def send_invite
-    notification_service.invite_group_member(self, @raw_invite_token)
+    run_after_commit_or_now { notification_service.invite_group_member(self, @raw_invite_token) }
 
     super
   end
 
   def post_create_hook
-    notification_service.new_group_member(self)
+    run_after_commit_or_now { notification_service.new_group_member(self) }
 
     super
   end
 
   def post_update_hook
     if access_level_changed?
-      notification_service.update_group_member(self)
+      run_after_commit { notification_service.update_group_member(self) }
     end
 
     super
diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb
index 1c7ed4a96df..024106056b4 100644
--- a/app/models/members/project_member.rb
+++ b/app/models/members/project_member.rb
@@ -92,7 +92,7 @@ class ProjectMember < Member
   private
 
   def send_invite
-    notification_service.invite_project_member(self, @raw_invite_token)
+    run_after_commit_or_now { notification_service.invite_project_member(self, @raw_invite_token) }
 
     super
   end
@@ -100,7 +100,7 @@ class ProjectMember < Member
   def post_create_hook
     unless owner?
       event_service.join_project(self.project, self.user)
-      notification_service.new_project_member(self)
+      run_after_commit_or_now { notification_service.new_project_member(self) }
     end
 
     super
@@ -108,7 +108,7 @@ class ProjectMember < Member
 
   def post_update_hook
     if access_level_changed?
-      notification_service.update_project_member(self)
+      run_after_commit { notification_service.update_project_member(self) }
     end
 
     super
diff --git a/changelogs/unreleased/45572-members-invitations-scheduled-before-commit.yml b/changelogs/unreleased/45572-members-invitations-scheduled-before-commit.yml
new file mode 100644
index 00000000000..7cdea436d47
--- /dev/null
+++ b/changelogs/unreleased/45572-members-invitations-scheduled-before-commit.yml
@@ -0,0 +1,5 @@
+---
+title: Ensure member notifications are sent after the member actual creation/update in the DB
+merge_request: 18538
+author:
+type: fixed
diff --git a/spec/features/groups/members/manage_access_requests_spec.rb b/spec/features/groups/members/manage_access_requests_spec.rb
deleted file mode 100644
index b83cd657ef7..00000000000
--- a/spec/features/groups/members/manage_access_requests_spec.rb
+++ /dev/null
@@ -1,47 +0,0 @@
-require 'spec_helper'
-
-feature 'Groups > Members > Manage access requests' do
-  let(:user) { create(:user) }
-  let(:owner) { create(:user) }
-  let(:group) { create(:group, :public, :access_requestable) }
-
-  background do
-    group.request_access(user)
-    group.add_owner(owner)
-    sign_in(owner)
-  end
-
-  scenario 'owner can see access requests' do
-    visit group_group_members_path(group)
-
-    expect_visible_access_request(group, user)
-  end
-
-  scenario 'owner can grant access' do
-    visit group_group_members_path(group)
-
-    expect_visible_access_request(group, user)
-
-    perform_enqueued_jobs { click_on 'Grant access' }
-
-    expect(ActionMailer::Base.deliveries.last.to).to eq [user.notification_email]
-    expect(ActionMailer::Base.deliveries.last.subject).to match "Access to the #{group.name} group was granted"
-  end
-
-  scenario 'owner can deny access' do
-    visit group_group_members_path(group)
-
-    expect_visible_access_request(group, user)
-
-    perform_enqueued_jobs { click_on 'Deny access' }
-
-    expect(ActionMailer::Base.deliveries.last.to).to eq [user.notification_email]
-    expect(ActionMailer::Base.deliveries.last.subject).to match "Access to the #{group.name} group was denied"
-  end
-
-  def expect_visible_access_request(group, user)
-    expect(group.requesters.exists?(user_id: user)).to be_truthy
-    expect(page).to have_content "Users requesting access to #{group.name} 1"
-    expect(page).to have_content user.name
-  end
-end
diff --git a/spec/features/groups/members/master_manages_access_requests_spec.rb b/spec/features/groups/members/master_manages_access_requests_spec.rb
new file mode 100644
index 00000000000..2fd6d1ec599
--- /dev/null
+++ b/spec/features/groups/members/master_manages_access_requests_spec.rb
@@ -0,0 +1,8 @@
+require 'spec_helper'
+
+feature 'Groups > Members > Master manages access requests' do
+  it_behaves_like 'Master manages access requests' do
+    let(:entity) { create(:group, :public, :access_requestable) }
+    let(:members_page_path) { group_group_members_path(entity) }
+  end
+end
diff --git a/spec/features/projects/members/master_manages_access_requests_spec.rb b/spec/features/projects/members/master_manages_access_requests_spec.rb
index 1f4eec0a317..3ac6ca4fc86 100644
--- a/spec/features/projects/members/master_manages_access_requests_spec.rb
+++ b/spec/features/projects/members/master_manages_access_requests_spec.rb
@@ -1,47 +1,8 @@
 require 'spec_helper'
 
 feature 'Projects > Members > Master manages access requests' do
-  let(:user) { create(:user) }
-  let(:master) { create(:user) }
-  let(:project) { create(:project, :public, :access_requestable) }
-
-  background do
-    project.request_access(user)
-    project.add_master(master)
-    sign_in(master)
-  end
-
-  scenario 'master can see access requests' do
-    visit project_project_members_path(project)
-
-    expect_visible_access_request(project, user)
-  end
-
-  scenario 'master can grant access' do
-    visit project_project_members_path(project)
-
-    expect_visible_access_request(project, user)
-
-    perform_enqueued_jobs { click_on 'Grant access' }
-
-    expect(ActionMailer::Base.deliveries.last.to).to eq [user.notification_email]
-    expect(ActionMailer::Base.deliveries.last.subject).to match "Access to the #{project.full_name} project was granted"
-  end
-
-  scenario 'master can deny access' do
-    visit project_project_members_path(project)
-
-    expect_visible_access_request(project, user)
-
-    perform_enqueued_jobs { click_on 'Deny access' }
-
-    expect(ActionMailer::Base.deliveries.last.to).to eq [user.notification_email]
-    expect(ActionMailer::Base.deliveries.last.subject).to match "Access to the #{project.full_name} project was denied"
-  end
-
-  def expect_visible_access_request(project, user)
-    expect(project.requesters.exists?(user_id: user)).to be_truthy
-    expect(page).to have_content "Users requesting access to #{project.name} 1"
-    expect(page).to have_content user.name
+  it_behaves_like 'Master manages access requests' do
+    let(:entity) { create(:project, :public, :access_requestable) }
+    let(:members_page_path) { project_project_members_path(entity) }
   end
 end
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index 5a3b5b1f517..ffc78015f94 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -28,52 +28,12 @@ describe GroupMember do
     end
   end
 
-  describe 'notifications' do
-    describe "#after_create" do
-      it "sends email to user" do
-        membership = build(:group_member)
+  it_behaves_like 'members notifications', :group
 
-        allow(membership).to receive(:notification_service)
-          .and_return(double('NotificationService').as_null_object)
-        expect(membership).to receive(:notification_service)
+  describe '#real_source_type' do
+    subject { create(:group_member).real_source_type }
 
-        membership.save
-      end
-    end
-
-    describe "#after_update" do
-      before do
-        @group_member = create :group_member
-        allow(@group_member).to receive(:notification_service)
-          .and_return(double('NotificationService').as_null_object)
-      end
-
-      it "sends email to user" do
-        expect(@group_member).to receive(:notification_service)
-        @group_member.update_attribute(:access_level, GroupMember::MASTER)
-      end
-
-      it "does not send an email when the access level has not changed" do
-        expect(@group_member).not_to receive(:notification_service)
-        @group_member.update_attribute(:access_level, GroupMember::OWNER)
-      end
-    end
-
-    describe '#after_accept_request' do
-      it 'calls NotificationService.accept_group_access_request' do
-        member = create(:group_member, user: build(:user), requested_at: Time.now)
-
-        expect_any_instance_of(NotificationService).to receive(:new_group_member)
-
-        member.__send__(:after_accept_request)
-      end
-    end
-
-    describe '#real_source_type' do
-      subject { create(:group_member).real_source_type }
-
-      it { is_expected.to eq 'Group' }
-    end
+    it { is_expected.to eq 'Group' }
   end
 
   describe '#update_two_factor_requirement' do
diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb
index b8b0e63f92e..574eb468e4c 100644
--- a/spec/models/members/project_member_spec.rb
+++ b/spec/models/members/project_member_spec.rb
@@ -123,15 +123,5 @@ describe ProjectMember do
     it { expect(@project_2.users).to be_empty }
   end
 
-  describe 'notifications' do
-    describe '#after_accept_request' do
-      it 'calls NotificationService.new_project_member' do
-        member = create(:project_member, user: create(:user), requested_at: Time.now)
-
-        expect_any_instance_of(NotificationService).to receive(:new_project_member)
-
-        member.__send__(:after_accept_request)
-      end
-    end
-  end
+  it_behaves_like 'members notifications', :project
 end
diff --git a/spec/support/shared_examples/features/master_manages_access_requests_shared_example.rb b/spec/support/shared_examples/features/master_manages_access_requests_shared_example.rb
new file mode 100644
index 00000000000..b29bb3c2fc0
--- /dev/null
+++ b/spec/support/shared_examples/features/master_manages_access_requests_shared_example.rb
@@ -0,0 +1,52 @@
+RSpec.shared_examples 'Master manages access requests' do
+  let(:user) { create(:user) }
+  let(:master) { create(:user) }
+
+  before do
+    entity.request_access(user)
+    entity.respond_to?(:add_owner) ? entity.add_owner(master) : entity.add_master(master)
+    sign_in(master)
+  end
+
+  it 'master can see access requests' do
+    visit members_page_path
+
+    expect_visible_access_request(entity, user)
+  end
+
+  it 'master can grant access', :js do
+    visit members_page_path
+
+    expect_visible_access_request(entity, user)
+
+    accept_confirm { click_on 'Grant access' }
+
+    expect_no_visible_access_request(entity, user)
+
+    page.within('.members-list') do
+      expect(page).to have_content user.name
+    end
+  end
+
+  it 'master can deny access', :js do
+    visit members_page_path
+
+    expect_visible_access_request(entity, user)
+
+    accept_confirm { click_on 'Deny access' }
+
+    expect_no_visible_access_request(entity, user)
+    expect(page).not_to have_content user.name
+  end
+
+  def expect_visible_access_request(entity, user)
+    expect(entity.requesters.exists?(user_id: user)).to be_truthy
+    expect(page).to have_content "Users requesting access to #{entity.name} 1"
+    expect(page).to have_content user.name
+  end
+
+  def expect_no_visible_access_request(entity, user)
+    expect(entity.requesters.exists?(user_id: user)).to be_falsy
+    expect(page).not_to have_content "Users requesting access to #{entity.name}"
+  end
+end
diff --git a/spec/support/shared_examples/models/members_notifications_shared_example.rb b/spec/support/shared_examples/models/members_notifications_shared_example.rb
new file mode 100644
index 00000000000..76611e54306
--- /dev/null
+++ b/spec/support/shared_examples/models/members_notifications_shared_example.rb
@@ -0,0 +1,63 @@
+RSpec.shared_examples 'members notifications' do |entity_type|
+  let(:notification_service) { double('NotificationService').as_null_object }
+
+  before do
+    allow(member).to receive(:notification_service).and_return(notification_service)
+  end
+
+  describe "#after_create" do
+    let(:member) { build(:"#{entity_type}_member") }
+
+    it "sends email to user" do
+      expect(notification_service).to receive(:"new_#{entity_type}_member").with(member)
+
+      member.save
+    end
+  end
+
+  describe "#after_update" do
+    let(:member) { create(:"#{entity_type}_member", :developer) }
+
+    it "calls NotificationService.update_#{entity_type}_member" do
+      expect(notification_service).to receive(:"update_#{entity_type}_member").with(member)
+
+      member.update_attribute(:access_level, Member::MASTER)
+    end
+
+    it "does not send an email when the access level has not changed" do
+      expect(notification_service).not_to receive(:"update_#{entity_type}_member")
+
+      member.touch
+    end
+  end
+
+  describe '#accept_request' do
+    let(:member) { create(:"#{entity_type}_member", :access_request) }
+
+    it "calls NotificationService.new_#{entity_type}_member" do
+      expect(notification_service).to receive(:"new_#{entity_type}_member").with(member)
+
+      member.accept_request
+    end
+  end
+
+  describe "#accept_invite!" do
+    let(:member) { create(:"#{entity_type}_member", :invited) }
+
+    it "calls NotificationService.accept_#{entity_type}_invite" do
+      expect(notification_service).to receive(:"accept_#{entity_type}_invite").with(member)
+
+      member.accept_invite!(build(:user))
+    end
+  end
+
+  describe "#decline_invite!" do
+    let(:member) { create(:"#{entity_type}_member", :invited) }
+
+    it "calls NotificationService.decline_#{entity_type}_invite" do
+      expect(notification_service).to receive(:"decline_#{entity_type}_invite").with(member)
+
+      member.decline_invite!
+    end
+  end
+end