From a06e519ed789984cdfb2ef80a1d760ffe23e3cb4 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Mon, 27 Feb 2023 16:21:02 +0100 Subject: [PATCH] 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 | 2 +- app/services/attachments/delete_service.rb | 2 +- app/services/base_services/base_contracted.rb | 10 ++++++-- app/services/base_services/create.rb | 2 +- app/services/base_services/set_attributes.rb | 2 +- app/services/groups/add_users_service.rb | 3 +-- .../concerns/membership_manipulation.rb | 16 +++++++++---- app/services/groups/update_service.rb | 2 +- app/services/members/cleanup_service.rb | 2 +- .../members/concerns/notification_sender.rb | 5 +++- app/services/relations/create_service.rb | 2 +- app/services/shared/service_context.rb | 6 ++--- app/services/wiki_pages/copy_service.rb | 2 +- app/services/work_packages/copy_service.rb | 2 +- app/services/work_packages/create_service.rb | 2 +- lib/services/create_watcher.rb | 3 ++- spec/services/groups/update_service_spec.rb | 4 ++-- spec/services/members/create_service_spec.rb | 1 - spec/services/members/update_service_spec.rb | 24 +++++++++---------- 19 files changed, 53 insertions(+), 39 deletions(-) diff --git a/app/services/add_work_package_note_service.rb b/app/services/add_work_package_note_service.rb index e80b1941186..81b70d98a66 100644 --- a/app/services/add_work_package_note_service.rb +++ b/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) diff --git a/app/services/attachments/delete_service.rb b/app/services/attachments/delete_service.rb index 0dda26e0f3c..e19385d6927 100644 --- a/app/services/attachments/delete_service.rb +++ b/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 diff --git a/app/services/base_services/base_contracted.rb b/app/services/base_services/base_contracted.rb index 59e61d991c4..96bbbc7d74e 100644 --- a/app/services/base_services/base_contracted.rb +++ b/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 diff --git a/app/services/base_services/create.rb b/app/services/base_services/create.rb index b306e51d1b6..a32b200ce81 100644 --- a/app/services/base_services/create.rb +++ b/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 diff --git a/app/services/base_services/set_attributes.rb b/app/services/base_services/set_attributes.rb index 2e92e3e3fe4..44c50a1b640 100644 --- a/app/services/base_services/set_attributes.rb +++ b/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 diff --git a/app/services/groups/add_users_service.rb b/app/services/groups/add_users_service.rb index e8964f6e705..3e6e17ca1f8 100644 --- a/app/services/groups/add_users_service.rb +++ b/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 diff --git a/app/services/groups/concerns/membership_manipulation.rb b/app/services/groups/concerns/membership_manipulation.rb index 3925c1cfb7e..3ce9a89a02e 100644 --- a/app/services/groups/concerns/membership_manipulation.rb +++ b/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 diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index bbd6c8e26e5..3f0a7597db1 100644 --- a/app/services/groups/update_service.rb +++ b/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 diff --git a/app/services/members/cleanup_service.rb b/app/services/members/cleanup_service.rb index 9e6a966838a..d909d6c6263 100644 --- a/app/services/members/cleanup_service.rb +++ b/app/services/members/cleanup_service.rb @@ -37,7 +37,7 @@ module Members protected - def perform(_params = {}) + def perform(*) prune_watchers unassign_categories diff --git a/app/services/members/concerns/notification_sender.rb b/app/services/members/concerns/notification_sender.rb index 65f4d6823e6..11aba97a33f 100644 --- a/app/services/members/concerns/notification_sender.rb +++ b/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 diff --git a/app/services/relations/create_service.rb b/app/services/relations/create_service.rb index 9c9b5fe0c23..f8ea836e10a 100644 --- a/app/services/relations/create_service.rb +++ b/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 diff --git a/app/services/shared/service_context.rb b/app/services/shared/service_context.rb index a68325fd901..ccda00e26f7 100644 --- a/app/services/shared/service_context.rb +++ b/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 diff --git a/app/services/wiki_pages/copy_service.rb b/app/services/wiki_pages/copy_service.rb index 53f35130962..65c60513596 100644 --- a/app/services/wiki_pages/copy_service.rb +++ b/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 diff --git a/app/services/work_packages/copy_service.rb b/app/services/work_packages/copy_service.rb index 8fe83319a1d..6a5d0b39d2d 100644 --- a/app/services/work_packages/copy_service.rb +++ b/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 diff --git a/app/services/work_packages/create_service.rb b/app/services/work_packages/create_service.rb index 2658710d8fd..f0b600b7c45 100644 --- a/app/services/work_packages/create_service.rb +++ b/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) diff --git a/lib/services/create_watcher.rb b/lib/services/create_watcher.rb index 97b230a90aa..83b875afe7f 100644 --- a/lib/services/create_watcher.rb +++ b/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? diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 37becd5b7a6..03380f496c1 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/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 diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 5076fe570fc..338e7781dd0 100644 --- a/spec/services/members/create_service_spec.rb +++ b/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 diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 29e698793ab..26a8916284f 100644 --- a/spec/services/members/update_service_spec.rb +++ b/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