Merge pull request #12147 from opf/fix/send-notifications-groups

Restore ability to suppress notifications in groups service

Author: cbliard

app/models/journal/notification_configuration.rb

@@ -61,10 +61,10 @@ def with_first(send_notifications)
     end
 
     def log_warning(send_notifications)
-      return if active == send_notifications
+      return if active? == send_notifications
 
       message = <<~MSG
-        Ignoring setting journal notifications to '#{send_notifications}' as a parent block already set it to #{active}"
+        Ignoring setting journal notifications to '#{send_notifications}' as a parent block already set it to #{active?}"
       MSG
       Rails.logger.debug message
     end

app/services/base_services/write.rb

@@ -77,7 +77,7 @@ def instance_class
     end
 
     def set_attributes_params(params)
-      params
+      params.except(:send_notifications)
     end
   end
 end

app/services/groups/add_users_service.rb

@@ -52,7 +52,11 @@ def persist(call)
     def after_perform(call)
       Groups::CreateInheritedRolesService
         .new(model, current_user: user, contract_class:)
-        .call(user_ids: params[:ids], message: params[:message])
+        .call(
+          user_ids: params[:ids],
+          message: params[:message],
+          send_notifications: params.fetch(:send_notifications, true)
+        )
 
       call
     end

app/services/groups/update_service.rb

@@ -49,7 +49,7 @@ def after_perform(call)
     if new_user_ids.any?
       db_call = ::Groups::AddUsersService
                   .new(call.result, current_user: user)
-                  .call(ids: new_user_ids)
+                  .call(ids: new_user_ids, send_notifications: params.fetch(:send_notifications, true))
 
       call.add_dependent!(db_call)
     end

spec/factories/group_factory.rb

@@ -42,7 +42,7 @@
       User.system.run_given do |system_user|
         Groups::AddUsersService
           .new(group, current_user: system_user)
-          .call(ids: members.map(&:id))
+          .call(ids: members.map(&:id), send_notifications: false)
           .on_failure { |call| raise call.message }
       end
     end

spec/services/groups/update_roles_service_integration_spec.rb

@@ -29,7 +29,7 @@
 require 'spec_helper'
 
 describe Groups::UpdateRolesService, 'integration', type: :model do
-  subject(:service_call) { instance.call(member:, message:) }
+  subject(:service_call) { instance.call(member:, message:, send_notifications:) }
 
   let(:project) { create :project }
   let(:role) { create :role }
@@ -46,12 +46,13 @@
 
       ::Groups::CreateInheritedRolesService
         .new(group, current_user: User.system, contract_class: EmptyContract)
-        .call(user_ids: users.map(&:id))
+        .call(user_ids: users.map(&:id), send_notifications: false)
     end
   end
   let(:users) { create_list :user, 2 }
   let(:member) { Member.find_by(principal: group) }
   let(:message) { "Some message" }
+  let(:send_notifications) { true }
 
   let(:instance) do
     described_class.new(group, current_user:)
@@ -89,6 +90,15 @@
     end
   end
 
+  shared_examples_for 'sends no notification' do
+    it 'on the updated membership' do
+      service_call
+
+      expect(Notifications::GroupMemberAlteredJob)
+        .not_to have_received(:perform_later)
+    end
+  end
+
   context 'when adding a role' do
     let(:added_role) { create(:role) }
 
@@ -113,6 +123,12 @@
     it_behaves_like 'sends notification' do
       let(:user) { users }
     end
+
+    context 'when notifications are suppressed' do
+      let(:send_notifications) { false }
+
+      it_behaves_like 'sends no notification'
+    end
   end
 
   context 'with global membership' do
@@ -154,6 +170,12 @@
       it_behaves_like 'sends notification' do
         let(:user) { users }
       end
+
+      context 'when notifications are suppressed' do
+        let(:send_notifications) { false }
+
+        it_behaves_like 'sends no notification'
+      end
     end
 
     context 'when removing a global role' do
@@ -180,6 +202,12 @@
       it_behaves_like 'sends notification' do
         let(:user) { users }
       end
+
+      context 'when notifications are suppressed' do
+        let(:send_notifications) { false }
+
+        it_behaves_like 'sends no notification'
+      end
     end
   end
 
@@ -222,6 +250,12 @@
     it_behaves_like 'sends notification' do
       let(:user) { users.last }
     end
+
+    context 'when notifications are suppressed' do
+      let(:send_notifications) { false }
+
+      it_behaves_like 'sends no notification'
+    end
   end
 
   context 'when removing a role' do
@@ -248,6 +282,12 @@
     it_behaves_like 'sends notification' do
       let(:user) { users }
     end
+
+    context 'when notifications are suppressed' do
+      let(:send_notifications) { false }
+
+      it_behaves_like 'sends no notification'
+    end
   end
 
   context 'when removing a role but with a user having had the role before (no inherited_from)' do
@@ -290,6 +330,12 @@
     it_behaves_like 'sends notification' do
       let(:user) { users.last }
     end
+
+    context 'when notifications are suppressed' do
+      let(:send_notifications) { false }
+
+      it_behaves_like 'sends no notification'
+    end
   end
 
   context 'when replacing roles' do
@@ -316,6 +362,12 @@
     it_behaves_like 'sends notification' do
       let(:user) { users }
     end
+
+    context 'when notifications are suppressed' do
+      let(:send_notifications) { false }
+
+      it_behaves_like 'sends no notification'
+    end
   end
 
   context 'when replacing a role but with a user having had the replaced role before (no inherited_from)' do
@@ -358,6 +410,12 @@
     it_behaves_like 'sends notification' do
       let(:user) { users }
     end
+
+    context 'when notifications are suppressed' do
+      let(:send_notifications) { false }
+
+      it_behaves_like 'sends no notification'
+    end
   end
 
   context 'when adding a role and the user has a role already granted by a different group' do
@@ -373,7 +431,7 @@
 
         ::Groups::CreateInheritedRolesService
           .new(group, current_user: User.system, contract_class: EmptyContract)
-          .call(user_ids: users.map(&:id))
+          .call(user_ids: users.map(&:id), send_notifications: false)
       end
     end
 
@@ -399,6 +457,12 @@
     it_behaves_like 'sends notification' do
       let(:user) { users }
     end
+
+    context 'when notifications are suppressed' do
+      let(:send_notifications) { false }
+
+      it_behaves_like 'sends no notification'
+    end
   end
 
   context 'when not allowed' do

spec/services/groups/update_service_spec.rb

@@ -76,7 +76,7 @@
 
           expect(add_users_service)
             .to have_received(:call)
-            .with(ids: [new_group_user.user_id])
+            .with(ids: [new_group_user.user_id], send_notifications: true)
         end
       end
 
@@ -94,7 +94,7 @@
 
           expect(add_users_service)
             .to have_received(:call)
-            .with(ids: [new_group_user.user_id])
+            .with(ids: [new_group_user.user_id], send_notifications: true)
         end
       end