Bug/58342 creating a project via template sends out mail notifications to all template users

app/mailers/application_mailer.rb

@@ -45,15 +45,6 @@ class ApplicationMailer < ActionMailer::Base
   default from: Proc.new { Setting.mail_from }
 
   class << self
-    # Activates/deactivates email deliveries during +block+
-    def with_deliveries(temporary_state = true, &)
-      old_state = ActionMailer::Base.perform_deliveries
-      ActionMailer::Base.perform_deliveries = temporary_state
-      yield
-    ensure
-      ActionMailer::Base.perform_deliveries = old_state
-    end
-
     def host
       if OpenProject::Configuration.rails_relative_url_root.blank?
         Setting.host_name

app/models/setting/mail_settings.rb

@@ -33,7 +33,6 @@ module MailSettings
     ##
     # Reload the currently configured mailer configuration
     def reload_mailer_settings!
-      ActionMailer::Base.perform_deliveries = true
       ActionMailer::Base.delivery_method = Setting.email_delivery_method if Setting.email_delivery_method
 
       case Setting.email_delivery_method

app/services/projects/copy/members_dependent_service.rb

@@ -61,11 +61,7 @@ def create_membership(member)
 
       attributes = member
                      .attributes.dup.except("id", "project_id", "created_at", "updated_at")
-                     .merge(role_ids:,
-                            project: target,
-                            # This is magic for now. The settings has been set before in the Projects::CopyService
-                            # It would be better if this was not sneaked in but rather passed in as a parameter.
-                            send_notifications: ActionMailer::Base.perform_deliveries)
+                     .merge(role_ids:, project: target)
 
       Members::CreateService
         .new(user: User.current, contract_class: EmptyContract)

app/services/projects/copy/wiki_dependent_service.rb

@@ -71,14 +71,9 @@ def copy_wiki_pages(_params)
     end
 
     def copy_wiki_page(source_page, new_parent_id)
-      # Relying on ActionMailer::Base.perform_deliveries is violating cohesion
-      # but the value is currently not otherwise provided
       service_call = WikiPages::CopyService
                      .new(user:, model: source_page, contract_class: WikiPages::CopyContract)
-                     .call(wiki: target.wiki,
-                           parent_id: new_parent_id,
-                           send_notifications: ActionMailer::Base.perform_deliveries,
-                           copy_attachments: copy_attachments?)
+                     .call(wiki: target.wiki, parent_id: new_parent_id, copy_attachments: copy_attachments?)
 
       if service_call.success?
         service_call.result

app/services/projects/copy/work_packages_dependent_service.rb

@@ -138,10 +138,7 @@ def copy_work_package_attribute_overrides(source_work_package, parent_id, user_c
         responsible_id: work_package_responsible_id(source_work_package),
         custom_field_values: custom_value_attributes(source_work_package, user_cf_ids),
         # We don't support copying budgets right now
-        budget_id: nil,
-
-        # We persist the setting in the job which will trigger a delayed job for potentially sending the journal notifications.
-        send_notifications: params.dig(:params, :send_notifications)
+        budget_id: nil
       }
     end
 

app/workers/copy_project_job.rb

@@ -112,20 +112,16 @@ def source_project = batch.properties[:source_project]
   # rubocop:disable Metrics/AbcSize
   # Most of the cost is from handling errors, we need to check what can be moved around / removed
   def create_project_copy(target_project_params, associations_to_copy, send_mails)
-    errors = []
-
-    ProjectMailer.with_deliveries(send_mails) do
-      service_result = copy_project(target_project_params, associations_to_copy, send_mails)
-      target_project = service_result.result
-      errors = service_result.errors.full_messages
-
-      # We assume the copying worked "successfully" if the project was saved
-      if target_project&.persisted?
-        return target_project, errors
-      else
-        logger.error("Copying project fails with validation errors: #{errors.join("\n")}")
-        return nil, errors
-      end
+    service_result = copy_project(target_project_params, associations_to_copy, send_mails)
+    target_project = service_result.result
+    errors = service_result.errors.full_messages
+
+    # We assume the copying worked "successfully" if the project was saved
+    if target_project&.persisted?
+      [target_project, errors]
+    else
+      logger.error("Copying project fails with validation errors: #{errors.join("\n")}")
+      [nil, errors]
     end
   rescue ActiveRecord::RecordNotFound => e
     logger.error("Entity missing: #{e.message} #{e.backtrace.join("\n")}")

spec/mailers/user_mailer_spec.rb

@@ -69,40 +69,6 @@
     allow(Setting).to receive(:default_language).and_return("en")
   end
 
-  describe "#with_deliveries" do
-    context "with false" do
-      before do
-        described_class.with_deliveries(false) do
-          described_class.test_mail(recipient).deliver_now
-        end
-      end
-
-      it_behaves_like "mail is not sent"
-    end
-
-    context "with true" do
-      before do
-        described_class.with_deliveries(true) do
-          described_class.test_mail(recipient).deliver_now
-        end
-      end
-
-      it_behaves_like "mail is sent"
-    end
-
-    context "with true but user is locked" do
-      let(:recipient) { build_stubbed(:user, status: Principal.statuses[:locked]) }
-
-      before do
-        described_class.with_deliveries(true) do
-          described_class.test_mail(recipient).deliver_now
-        end
-      end
-
-      it_behaves_like "mail is not sent"
-    end
-  end
-
   describe "#test_mail" do
     let(:test_email) { "[email protected]" }
     let(:recipient) { build_stubbed(:user, firstname: "Bob", lastname: "Bobbi", mail: test_email) }

spec/models/setting_spec.rb

@@ -501,7 +501,7 @@
            smtp_timeout: 1234
          } do
         described_class.reload_mailer_settings!
-        expect(ActionMailer::Base).to have_received(:perform_deliveries=).with(true)
+        expect(ActionMailer::Base).not_to have_received(:perform_deliveries=).with(true)
         expect(ActionMailer::Base).to have_received(:delivery_method=).with(:smtp)
         expect(ActionMailer::Base.smtp_settings[:smtp_authentication]).to be_nil
         expect(ActionMailer::Base.smtp_settings).to eq(address: "smtp.example.com",
@@ -529,7 +529,7 @@
            smtp_ssl: 1
          } do
         described_class.reload_mailer_settings!
-        expect(ActionMailer::Base).to have_received(:perform_deliveries=).with(true)
+        expect(ActionMailer::Base).not_to have_received(:perform_deliveries=).with(true)
         expect(ActionMailer::Base).to have_received(:delivery_method=).with(:smtp)
         expect(ActionMailer::Base.smtp_settings[:smtp_authentication]).to be_nil
         expect(ActionMailer::Base.smtp_settings).to eq(address: "smtp.example.com",
@@ -556,7 +556,7 @@
            smtp_ssl: 0
          } do
         described_class.reload_mailer_settings!
-        expect(ActionMailer::Base).to have_received(:perform_deliveries=).with(true)
+        expect(ActionMailer::Base).not_to have_received(:perform_deliveries=).with(true)
         expect(ActionMailer::Base).to have_received(:delivery_method=).with(:smtp)
         expect(ActionMailer::Base.smtp_settings[:smtp_authentication]).to be_nil
         expect(ActionMailer::Base.smtp_settings).to eq(address: "smtp.example.com",
@@ -586,7 +586,7 @@
            smtp_ssl: 1
          } do
         described_class.reload_mailer_settings!
-        expect(ActionMailer::Base).to have_received(:perform_deliveries=).with(true)
+        expect(ActionMailer::Base).not_to have_received(:perform_deliveries=).with(true)
         expect(ActionMailer::Base).to have_received(:delivery_method=).with(:smtp)
         expect(ActionMailer::Base.smtp_settings[:smtp_authentication]).to be_nil
         expect(ActionMailer::Base.smtp_settings).to eq(address: "smtp.example.com",

spec/workers/copy_project_job_spec.rb

@@ -355,4 +355,46 @@
       TABLE
     end
   end
+
+  # Bug #58342: combination of class attribute ActionMailer::Base.perform_deliveries that was not thread local, so was
+  # reset to true before every request and job run, and using it to override Journal::NotificationConfiguration.active?
+  # to decide to send notifications could cause notifications to be sent for adding as member despite selecting not to
+  # do it.
+  describe "sending notifications" do
+    shared_let(:project) { create(:project) }
+    shared_let(:admin) { create(:admin) }
+    shared_let(:user) { create(:user) }
+    shared_let(:roles) { [create(:project_role)] }
+    shared_let(:member) { create(:member, user:, project:, roles:) }
+
+    let(:params) { { name: "Copy", identifier: "copy" } }
+
+    def perform_the_job
+      batch = GoodJob::Batch.enqueue(user: admin, source_project: project) do
+        described_class.perform_later(target_project_params: params, associations_to_copy: [:members])
+      end
+      GoodJob.perform_inline
+      batch.reload
+
+      expect(batch).to be_succeeded
+    end
+
+    it "doesn't touch the perform_deliveries" do
+      allow(ActionMailer::Base).to receive(:perform_deliveries=)
+
+      perform_the_job
+
+      expect(ActionMailer::Base).not_to have_received(:perform_deliveries=)
+    end
+
+    it "calls Members::CreateService without send_notifications override" do
+      members_create_service = instance_double(Members::CreateService)
+      allow(Members::CreateService).to receive(:new).and_return(members_create_service)
+      allow(members_create_service).to receive(:call)
+
+      perform_the_job
+
+      expect(members_create_service).to have_received(:call).with(hash_excluding(:send_notifications))
+    end
+  end
 end