Skip to content

Conversation

toy
Copy link
Contributor

@toy toy commented Sep 30, 2025

Ticket

https://community.openproject.org/wp/58342

What are you trying to accomplish?

Prevent notifications from being sent sometimes when project is copied with «Send email notifications during the project copy» is unchecked. Why was the bug happening:

  • ActionMailer::Base.perform_deliveries was set around call to Projects::CopyService (using with_deliveries)
  • in most cases send_notifications is used only by BaseContracted to set Journal::NotificationConfiguration.active?, for the duration of perform and attempts to provide send_notifications to wrapped service calls are ignored and logged
  • this is not the case for Members::CreateService, as send_notifications, if set, is also used to override Journal::NotificationConfiguration.active? (Members::Concerns::NotificationSender#send_notifications?)
  • Members::CreateService is called for every member from Projects::Copy::MembersDependentService and was receiving send_notifications explicitly set to ActionMailer::Base.perform_deliveries
  • this was expected to not be a problem, as ActionMailer::Base.perform_deliveries was set around call to Projects::CopyService to the desired value
  • but perform_deliveries is a class attribute, so not thread local and it was set to true before every request and job run (by Setting::MailSettings#reload_mailer_settings!), which was done to ensure email sending configuration is up to date if changed from admin
  • so if any job was started after perform_deliveries was set around Projects::CopyService and before Projects::Copy::MembersDependentService finished, perform_deliveries was reset to true and Members::CreateService was called with send_notifications: true forcing it to send notifications (and perform_deliveries = true was allowing them to be sent)
  • this also should have caused reverse effect if in the middle of a copy project job with «Send email notifications during the project copy» another one started with the preference unchecked forcing no notifications until any other job start reversed the effect, but it should have being rarer and much less obvious, as notifications were not sent when expected to instead of sent when not expected to

What approach did you choose and why?

Stop dynamically setting perform_deliveries in the app (as it is a class variable, so not thread local and is reset on every request and job run) and remove explicitly setting send_notifications in all Projects::CopyService dependencies.

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

@toy toy force-pushed the bug/58342-creating-a-project-via-template-sends-out-mail-notifications-to-all-template-users branch 2 times, most recently from 9ee1619 to d4995c6 Compare October 1, 2025 16:19
@toy toy requested a review from Copilot October 1, 2025 16:54
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug where email notifications were incorrectly sent during project copying even when the "Send email notifications during the project copy" option was unchecked. The issue was caused by thread-safety problems with the ActionMailer::Base.perform_deliveries class attribute being used to control notifications across multiple services.

  • Remove dynamic setting of ActionMailer::Base.perform_deliveries to avoid thread-safety issues
  • Remove explicit send_notifications parameter passing in project copy services
  • Add comprehensive test coverage for the notification behavior

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
app/models/setting/mail_settings.rb Remove automatic setting of perform_deliveries to true during mailer settings reload
app/mailers/application_mailer.rb Remove with_deliveries helper method that manipulated perform_deliveries
app/workers/copy_project_job.rb Remove usage of with_deliveries wrapper and clean up error handling
app/services/projects/copy/members_dependent_service.rb Remove explicit send_notifications parameter from member creation
app/services/projects/copy/wiki_dependent_service.rb Remove explicit send_notifications parameter from wiki page copying
app/services/projects/copy/work_packages_dependent_service.rb Remove explicit send_notifications parameter from work package copying
spec/workers/copy_project_job_spec.rb Add tests to verify notifications are handled correctly without touching perform_deliveries
spec/models/setting_spec.rb Update tests to reflect that perform_deliveries is no longer set during settings reload
spec/mailers/user_mailer_spec.rb Remove tests for the deleted with_deliveries method

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

toy added 4 commits October 2, 2025 14:10
It was setting ActionMailer::Base.perform_deliveries for the duration of
the block, but perform_deliveries is a class attribute, so not thread
local and doesn't work as expected in combination with setting
perform_deliveries to true when calling
Setting::MailSettings#reload_mailer_settings! at the start of every
request and every job run
It was probably needed due to calls of with_deliveries, but it was
removed as perform_deliveries is not thread local and there should be no
reason to reset it
Projects::CopyService wraps everything with service context passing
send_notifications to Journal::NotificationConfiguration, so trying to
set it in dependencies to same value is not needed (and is noop + logs a
warning). Also it is the real reason for
https://community.openproject.org/wp/58342, as Members::CreateService
send_notifications could get set to true and would override
Journal::NotificationConfiguration.active?
@toy toy force-pushed the bug/58342-creating-a-project-via-template-sends-out-mail-notifications-to-all-template-users branch from d4995c6 to 653c7c4 Compare October 2, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant