Merge branch 'ldap-sync-notifications' into 'master'

Don't send "Added to group" notifications when group is LDAP synched

Addresses gitlab/gitlab-ee#313.

See merge request !463

CHANGELOG-EE

+v 7.14
+  - Don't send "Added to group" notifications when group is LDAP synched
+
 v 7.13.2
   - Fix group web hook
 
@@ -180,4 +183,4 @@ v 6.2.0
   - Use omniauth-ldap nickname attribute as GitLab username
   - Improve group sharing UI for installation with many groups
   - Fix empty LDAP group raises exception
-  - Respect LDAP user filter for git access
\ No newline at end of file
+  - Respect LDAP user filter for git access

app/models/group.rb

@@ -64,18 +64,18 @@ class Group < Namespace
     @owners ||= group_members.owners.map(&:user)
   end
 
-  def add_users(user_ids, access_level, current_user = nil)
+  def add_users(user_ids, access_level, current_user = nil, skip_notification: false)
     user_ids.each do |user_id|
-      Member.add_user(self.group_members, user_id, access_level, current_user)
+      Member.add_user(self.group_members, user_id, access_level, current_user, skip_notification: skip_notification)
     end
   end
 
-  def add_user(user, access_level, current_user = nil)
-    add_users([user], access_level, current_user)
+  def add_user(user, access_level, current_user = nil, skip_notification: false)
+    add_users([user], access_level, current_user, skip_notification: skip_notification)
   end
 
-  def add_owner(user, current_user = nil)
-    self.add_user(user, Gitlab::Access::OWNER, current_user)
+  def add_owner(user, current_user = nil, skip_notification: false)
+    self.add_user(user, Gitlab::Access::OWNER, current_user, skip_notification: skip_notification)
   end
 
   def has_owner?(user)

app/models/member.rb

@@ -23,6 +23,7 @@ class Member < ActiveRecord::Base
   include Gitlab::Access
 
   attr_accessor :raw_invite_token
+  attr_accessor :skip_notification
 
   belongs_to :created_by, class_name: "User"
   belongs_to :user
@@ -71,7 +72,7 @@ class Member < ActiveRecord::Base
       user
     end
 
-    def add_user(members, user_id, access_level, current_user = nil)
+    def add_user(members, user_id, access_level, current_user = nil, skip_notification: false)
       user = user_for_id(user_id)
       
       # `user` can be either a User object or an email to be invited
@@ -85,6 +86,8 @@ class Member < ActiveRecord::Base
       member.created_by ||= current_user
       member.access_level = access_level
 
+      member.skip_notification = skip_notification
+
       member.save
     end
   end

app/models/members/group_member.rb

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

app/models/members/project_member.rb

@@ -123,7 +123,7 @@ class ProjectMember < Member
   private
 
   def send_invite
-    notification_service.invite_project_member(self, @raw_invite_token)
+    notification_service.invite_project_member(self, @raw_invite_token) unless @skip_notification
 
     super
   end
@@ -131,7 +131,7 @@ class ProjectMember < Member
   def post_create_hook
     unless owner?
       event_service.join_project(self.project, self.user)
-      notification_service.new_project_member(self)
+      notification_service.new_project_member(self) unless @skip_notification
     end
     
     super
@@ -139,7 +139,7 @@ class ProjectMember < Member
 
   def post_update_hook
     if access_level_changed?
-      notification_service.update_project_member(self) 
+      notification_service.update_project_member(self)  unless @skip_notification
     end
 
     super
@@ -152,13 +152,13 @@ class ProjectMember < Member
   end
 
   def after_accept_invite
-    notification_service.accept_project_invite(self)
+    notification_service.accept_project_invite(self) unless @skip_notification
 
     super
   end
 
   def after_decline_invite
-    notification_service.decline_project_invite(self)
+    notification_service.decline_project_invite(self) unless @skip_notification
 
     super
   end

lib/gitlab/ldap/access.rb

@@ -140,7 +140,7 @@ module Gitlab
           active_group_links = group.ldap_group_links.where(cn: cns_with_access)
 
           if active_group_links.any?
-            group.add_users([user.id], fetch_group_access(group, user, active_group_links))
+            group.add_users([user.id], fetch_group_access(group, user, active_group_links), skip_notification: true)
           elsif group.last_owner?(user)
             Rails.logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
           else

spec/lib/gitlab/ldap/access_spec.rb

@@ -258,6 +258,11 @@ objectclass: posixGroup
         access.update_ldap_group_links
         expect( gitlab_group_1.has_master?(user) ).to be_truthy
       end
+
+      it "doesn't send a notification email" do
+        expect { access.update_ldap_group_links }.not_to \
+          change { ActionMailer::Base.deliveries }
+      end
     end
 
     context "existing access as guest for group-1, allowed via ldap-group1 as DEVELOPER" do
@@ -271,6 +276,11 @@ objectclass: posixGroup
         expect { access.update_ldap_group_links }.to \
           change{ gitlab_group_1.has_master?(user) }.from(false).to(true)
       end
+
+      it "doesn't send a notification email" do
+        expect { access.update_ldap_group_links }.not_to \
+          change { ActionMailer::Base.deliveries }
+      end
     end
 
     context "existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER" do
@@ -284,6 +294,11 @@ objectclass: posixGroup
         expect { access.update_ldap_group_links }.not_to \
           change{ gitlab_group_1.has_master?(user) }
       end
+
+      it "doesn't send a notification email" do
+        expect { access.update_ldap_group_links }.not_to \
+          change { ActionMailer::Base.deliveries }
+      end
     end
 
     context "existing access as master for group-1, not allowed" do