Set send_notifications only when not nil

When calling a service with `send_notifications: false`, the
`Journal::NotificationConfiguration.active?` will be set to `false` and
subsequent calls to set it to `true` will have no effect and log a
warning.

For this reason, it's better to use `nil` as default value for
`send_notifications` so that
`Journal::NotificationConfiguration.active?` is changed only when the
value is explicitly `true` or `false`, and ignored when the value is
`nil`.

app/services/add_work_package_note_service.rb

@@ -40,7 +40,7 @@ class AddWorkPackageNoteService
     self.contract_class = WorkPackages::CreateNoteContract
   end
 
-  def call(notes, send_notifications: true)
+  def call(notes, send_notifications: nil)
     Journal::NotificationConfiguration.with send_notifications do
       work_package.add_journal(user, notes)
 

app/services/attachments/delete_service.rb

@@ -29,7 +29,7 @@
 class Attachments::DeleteService < ::BaseServices::Delete
   include Attachments::TouchContainer
 
-  def call(params = nil)
+  def call(params = {})
     in_context(model.container || model) do
       perform(params)
     end

app/services/base_services/base_contracted.rb

@@ -50,12 +50,13 @@ module BaseServices
     # Determine the type of context
     # this service is running in
     # e.g., within a resource lock or just executing as the given user
-    def service_context(send_notifications: true, &block)
+    def service_context(send_notifications:, &block)
       in_context(model, send_notifications:, &block)
     end
 
     def perform(params = {})
-      service_context(send_notifications: (params || {}).fetch(:send_notifications, true)) do
+      params, send_notifications = extract(params, :send_notifications)
+      service_context(send_notifications:) do
         service_call = validate_params(params)
         service_call = before_perform(params, service_call) if service_call.success?
         service_call = validate_contract(service_call) if service_call.success?
@@ -67,6 +68,11 @@ module BaseServices
       end
     end
 
+    def extract(params, attribute)
+      params = params ? params.dup : {}
+      [params, params.delete(attribute)]
+    end
+
     def validate_params(_params)
       ServiceResult.success(result: model)
     end

app/services/base_services/create.rb

@@ -30,7 +30,7 @@ module BaseServices
   class Create < Write
     protected
 
-    def service_context(send_notifications: true, &block)
+    def service_context(send_notifications:, &block)
       in_user_context(send_notifications:, &block)
     end
 

app/services/base_services/set_attributes.rb

@@ -40,7 +40,7 @@ module BaseServices
       self.contract_options = contract_options
     end
 
-    def perform(params = nil)
+    def perform(params = {})
       set_attributes(params || {})
 
       validate_and_result

app/services/groups/add_users_service.rb

@@ -54,8 +54,7 @@ module Groups
         .new(model, current_user: user, contract_class:)
         .call(
           user_ids: params[:ids],
-          message: params[:message],
-          send_notifications: params.fetch(:send_notifications, true)
+          message: params[:message]
         )
 
       call

app/services/groups/concerns/membership_manipulation.rb

@@ -35,7 +35,8 @@ module Groups::Concerns
 
       with_error_handled do
         ::Group.transaction do
-          exec_query!(params, params.fetch(:send_notifications, true), params[:message])
+          send_notifications = params.fetch(:send_notifications, Journal::NotificationConfiguration.active?)
+          exec_query!(params, send_notifications, params[:message])
         end
       end
     end
@@ -46,7 +47,10 @@ module Groups::Concerns
       yield
       ServiceResult.success result: model
     rescue StandardError => e
-      Rails.logger.error { "Failed to modify members and associated roles of group #{model.id}: #{e} #{e.message}" }
+      Rails.logger.error do
+        "Failed to modify members and associated roles of group #{model.id}: " \
+          "#{e}\n#{e.backtrace.first(5).join("\n")}"
+      end
       ServiceResult.failure(message: I18n.t(:notice_internal_server_error, app_title: Setting.app_title))
     end
 
@@ -55,7 +59,9 @@ module Groups::Concerns
 
       touch_updated(affected_member_ids)
 
-      send_notifications(affected_member_ids, message, send_notifications) if affected_member_ids.any? && send_notifications
+      if affected_member_ids.any? && send_notifications && Journal::NotificationConfiguration.active?
+        send_notifications(affected_member_ids, message)
+      end
     end
 
     def modify_members_and_roles(_params)
@@ -76,12 +82,12 @@ module Groups::Concerns
         .touch_all
     end
 
-    def send_notifications(member_ids, message, send_notifications)
+    def send_notifications(member_ids, message)
       Notifications::GroupMemberAlteredJob.perform_later(
         User.current,
         member_ids,
         message,
-        send_notifications
+        true
       )
     end
   end

app/services/groups/update_service.rb

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

app/services/members/cleanup_service.rb

@@ -37,7 +37,7 @@ module Members
 
     protected
 
-    def perform(_params = {})
+    def perform(*)
       prune_watchers
       unassign_categories
 

app/services/members/concerns/notification_sender.rb

@@ -46,7 +46,10 @@ module Members::Concerns::NotificationSender
     end
 
     def send_notifications?
-      params.fetch(:send_notifications, true)
+      # Because this class is mixed in in a service using around_call hook, it
+      # can not rely on Service#perform method setting the send_notifications
+      # configuration. It would be nice to unify both.
+      params.fetch(:send_notifications, Journal::NotificationConfiguration.active?)
     end
 
     def event_type

app/services/relations/create_service.rb

@@ -32,7 +32,7 @@ class Relations::CreateService < Relations::BaseService
     self.contract_class = Relations::CreateContract
   end
 
-  def perform(send_notifications: true, **attributes)
+  def perform(send_notifications: nil, **attributes)
     in_user_context(send_notifications:) do
       update_relation ::Relation.new, attributes
     end

app/services/shared/service_context.rb

@@ -30,7 +30,7 @@ module Shared
   module ServiceContext
     private
 
-    def in_context(model, send_notifications: true, &)
+    def in_context(model, send_notifications: nil, &)
       if model
         in_mutex_context(model, send_notifications:, &)
       else
@@ -38,7 +38,7 @@ module Shared
       end
     end
 
-    def in_mutex_context(model, send_notifications: true, &)
+    def in_mutex_context(model, send_notifications: nil, &)
       result = nil
 
       OpenProject::Mutex.with_advisory_lock_transaction(model) do
@@ -50,7 +50,7 @@ module Shared
       result
     end
 
-    def in_user_context(send_notifications: true, &)
+    def in_user_context(send_notifications: nil, &)
       result = nil
 
       ActiveRecord::Base.transaction do

app/services/wiki_pages/copy_service.rb

@@ -41,7 +41,7 @@ class WikiPages::CopyService
     self.contract_class = contract_class
   end
 
-  def call(send_notifications: true, copy_attachments: true, **attributes)
+  def call(send_notifications: nil, copy_attachments: true, **attributes)
     in_context(model, send_notifications:) do
       copy(attributes, copy_attachments)
     end

app/services/work_packages/copy_service.rb

@@ -41,7 +41,7 @@ class WorkPackages::CopyService
     self.contract_class = contract_class
   end
 
-  def call(send_notifications: true, copy_attachments: true, **attributes)
+  def call(send_notifications: nil, copy_attachments: true, **attributes)
     in_context(work_package, send_notifications:) do
       copy(attributes, copy_attachments, send_notifications)
     end

app/services/work_packages/create_service.rb

@@ -39,7 +39,7 @@ class WorkPackages::CreateService < BaseServices::BaseCallable
   end
 
   def perform(work_package: WorkPackage.new,
-              send_notifications: true,
+              send_notifications: nil,
               **attributes)
     in_user_context(send_notifications:) do
       create(attributes, work_package)

lib/services/create_watcher.rb

@@ -34,7 +34,8 @@ class Services::CreateWatcher
     @watcher = Watcher.new(user:, watchable: work_package)
   end
 
-  def run(send_notifications: true, success: ->(*) {}, failure: ->(*) {})
+  def run(send_notifications: nil, success: ->(*) {}, failure: ->(*) {})
+    send_notifications = Journal::NotificationConfiguration.active? if send_notifications.nil?
     if @work_package.watcher_users.include?(@user)
       success.(created: false)
     elsif @watcher.valid?

spec/services/groups/update_service_spec.rb

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

spec/services/members/create_service_spec.rb

@@ -75,7 +75,6 @@ describe Members::CreateService, type: :model do
                       member: model_instance,
                       message: call_attributes[:notification_message],
                       send_notifications: true)
-
       end
 
       describe 'for a group' do

spec/services/members/update_service_spec.rb

@@ -39,43 +39,43 @@ describe Members::UpdateService, type: :model do
       }
     end
 
-    let!(:allow_notification_call) do
+    before do
       allow(OpenProject::Notifications)
         .to receive(:send)
     end
 
     describe 'if successful' do
       it 'sends a notification' do
+        subject
+
         expect(OpenProject::Notifications)
-          .to receive(:send)
+          .to have_received(:send)
           .with(OpenProject::Events::MEMBER_UPDATED,
                 member: model_instance,
                 message: call_attributes[:notification_message],
                 send_notifications: call_attributes[:send_notifications])
-
-        subject
       end
     end
 
     context 'if the SetAttributeService is unsuccessful' do
       let(:set_attributes_success) { false }
 
-      it 'sends no notification' do
-        expect(OpenProject::Notifications)
-          .not_to receive(:send)
-
+      it 'sends no notifications' do
         subject
+
+        expect(OpenProject::Notifications)
+          .not_to have_received(:send)
       end
     end
 
     context 'when the member is invalid' do
       let(:model_save_result) { false }
 
-      it 'sends no notification' do
-        expect(OpenProject::Notifications)
-          .not_to receive(:send)
-
+      it 'sends no notifications' do
         subject
+
+        expect(OpenProject::Notifications)
+          .not_to have_received(:send)
       end
     end
   end