Merge branch 'rs-null-notification-service' into 'master'

Add a NullNotificationService

See merge request gitlab-org/gitlab-ee!8743

app/models/members/group_member.rb

@@ -44,33 +44,33 @@ class GroupMember < Member
   private
 
   def send_invite
-    run_after_commit_or_now { notification_service.invite_group_member(self, @raw_invite_token) } unless @skip_notification
+    run_after_commit_or_now { notification_service.invite_group_member(self, @raw_invite_token) }
 
     super
   end
 
   def post_create_hook
-    run_after_commit_or_now { notification_service.new_group_member(self) } unless @skip_notification
+    run_after_commit_or_now { notification_service.new_group_member(self) }
 
     super
   end
 
   def post_update_hook
     if access_level_changed?
-      run_after_commit { notification_service.update_group_member(self) } unless @skip_notification
+      run_after_commit { notification_service.update_group_member(self) }
     end
 
     super
   end
 
   def after_accept_invite
-    notification_service.accept_group_invite(self) unless @skip_notification
+    notification_service.accept_group_invite(self)
 
     super
   end
 
   def after_decline_invite
-    notification_service.decline_group_invite(self) unless @skip_notification
+    notification_service.decline_group_invite(self)
 
     super
   end

app/models/members/project_member.rb

@@ -101,7 +101,7 @@ class ProjectMember < Member
   end
 
   def send_invite
-    run_after_commit_or_now { notification_service.invite_project_member(self, @raw_invite_token) } unless @skip_notification
+    run_after_commit_or_now { notification_service.invite_project_member(self, @raw_invite_token) }
 
     super
   end
@@ -109,7 +109,7 @@ class ProjectMember < Member
   def post_create_hook
     unless owner?
       event_service.join_project(self.project, self.user)
-      run_after_commit_or_now { notification_service.new_project_member(self) } unless @skip_notification
+      run_after_commit_or_now { notification_service.new_project_member(self) }
     end
 
     super
@@ -117,7 +117,7 @@ class ProjectMember < Member
 
   def post_update_hook
     if access_level_changed?
-      run_after_commit { notification_service.update_project_member(self) } unless @skip_notification
+      run_after_commit { notification_service.update_project_member(self) }
     end
 
     super
@@ -134,13 +134,13 @@ class ProjectMember < Member
   end
 
   def after_accept_invite
-    notification_service.accept_project_invite(self) unless @skip_notification
+    notification_service.accept_project_invite(self)
 
     super
   end
 
   def after_decline_invite
-    notification_service.decline_project_invite(self) unless @skip_notification
+    notification_service.decline_project_invite(self)
 
     super
   end

ee/app/models/ee/member.rb

@@ -3,8 +3,7 @@
 module EE
   module Member
     extend ActiveSupport::Concern
-
-    attr_accessor :skip_notification
+    extend ::Gitlab::Utils::Override
 
     class_methods do
       extend ::Gitlab::Utils::Override
@@ -14,10 +13,19 @@ module EE
         super
 
         member.attributes = {
-          skip_notification: ldap,
           ldap: ldap
         }
       end
     end
+
+    override :notification_service
+    def notification_service
+      if ldap
+        # LDAP users shouldn't receive notifications about membership changes
+        ::EE::NullNotificationService.new
+      else
+        super
+      end
+    end
   end
 end

ee/app/services/ee/null_notification_service.rb

+# frozen_string_literal: true
+
+# This class can be used as a drop-in for NotificationService in order to
+# prevent sending notifications.
+#
+# It will respond to any message that NotificationService responds to with
+# itself in order to support method chaining.
+module EE
+  class NullNotificationService
+    def method_missing(name, *)
+      if respond_to_missing?(name)
+        self
+      else
+        super
+      end
+    end
+
+    def respond_to_missing?(name, *)
+      ::NotificationService.method_defined?(name)
+    end
+  end
+end

ee/spec/models/member_spec.rb

+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Member, type: :model do
+  describe '#notification_service' do
+    it 'returns a NullNotificationService instance for LDAP users' do
+      member = described_class.new
+
+      allow(member).to receive(:ldap).and_return(true)
+
+      expect(member.__send__(:notification_service))
+        .to be_instance_of(::EE::NullNotificationService)
+    end
+  end
+end

ee/spec/services/ee/null_notification_service_spec.rb

+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe ::EE::NullNotificationService do
+  it 'responds to methods implemented by NotificationService' do
+    method = NotificationService.instance_methods(false).sample
+
+    expect(subject.public_send(method)).to be_instance_of(described_class)
+  end
+
+  it 'raises NoMethodError for methods not implemented by NotificationService' do
+    expect { subject.not_a_real_method }.to raise_error(NoMethodError)
+  end
+end