Merge pull request #21325 from opf/bugfix/69448-send-assignee-notification-email-only-once-after-pir-completion

[69448] Send assignee notification email only once after PIR completion

Author: cbliard

app/contracts/work_packages/copy_project_contract.rb

@@ -42,6 +42,12 @@ class CopyProjectContract < CopyContract
     delegate :to_s,
              to: :model
 
+    def valid?(_context = nil)
+      # For project copying, we want to preserve the exact state
+      # even if copied work packages would normally be invalid
+      true
+    end
+
     def validate_model? = false
 
     private

app/services/journals/create_service.rb

@@ -361,7 +361,7 @@ def insert_data_sql(predecessor, notes, cause)
     # will already have been increased so nothing needs to be done.
     # But if any of the associated data is updated or if only a cause or note is added, the journable would
     # otherwise not have receive an updated timestamp.
-    def touch_journable_sql(predecessor, notes, cause)
+    def touch_journable_sql(predecessor, notes, cause) # rubocop:disable Metrics/AbcSize
       if journable.class.aaj_options[:timestamp].to_sym == :updated_at
         sql = <<~SQL
           UPDATE
@@ -375,8 +375,9 @@ def touch_journable_sql(predecessor, notes, cause)
           RETURNING updated_at
         SQL
 
+        keep_same_updated_at = journable.updated_at_previously_changed? && !journable.previously_new_record?
         sanitize(sql,
-                 update_timestamp: journable.updated_at_previously_changed? ? journable.updated_at : nil,
+                 update_timestamp: keep_same_updated_at ? journable.updated_at : nil,
                  predecessor_timestamp: predecessor&.updated_at,
                  id: journable.id)
       else

app/services/work_packages/create_service.rb

@@ -55,15 +55,14 @@ def perform
   def create(attributes, work_package)
     result = set_attributes(attributes, work_package)
 
-    result.success =
-      if result.success
-        work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements
-        work_package.save
+    if result.success?
+      # Set attributes service passed, meaning the contract is fullfilled.
+      # Avoid running validations again as we might be in a project copy scenario.
+      work_package.attachments = work_package.attachments_replacements if work_package.attachments_replacements
+      work_package.save(validate: false)
 
-        set_templated_subject(work_package)
-      end
+      update_subject_if_automatically_generated(work_package)
 
-    if result.success?
       # update ancestors before rescheduling, as the parent might switch to automatic mode
       multi_update_ancestors(result.all_results).each do |ancestor_result|
         result.merge!(ancestor_result)
@@ -77,11 +76,13 @@ def create(attributes, work_package)
     result
   end
 
-  def set_templated_subject(work_package)
-    return true unless work_package.type&.replacement_pattern_defined_for?(:subject)
-
-    work_package.subject = work_package.type.enabled_patterns[:subject].resolve(work_package)
-    work_package.save
+  def update_subject_if_automatically_generated(work_package)
+    if work_package.type&.replacement_pattern_defined_for?(:subject)
+      Journal::NotificationConfiguration.with(false) do
+        work_package.subject = work_package.type.enabled_patterns[:subject].resolve(work_package)
+        work_package.save(validate: false)
+      end
+    end
   end
 
   def set_attributes(attributes, work_package)
@@ -93,16 +94,22 @@ def reschedule_related(work_package)
     # This is necessary in bulk duplicate scenarios.
     switching_to_automatic_mode = []
     switching_to_automatic_mode << work_package if work_package.schedule_automatically?
-    result = WorkPackages::SetScheduleService.new(user:, work_package:, switching_to_automatic_mode:).call
+    rescheduling_result = WorkPackages::SetScheduleService.new(user:, work_package:, switching_to_automatic_mode:).call
 
-    result.self_and_dependent.each do |r|
+    persist_reschedule_changes(rescheduling_result)
+
+    rescheduling_result
+  end
+
+  def persist_reschedule_changes(rescheduling_result)
+    rescheduling_result.self_and_dependent
+          .filter { it.result.changed? }
+          .each do |r|
       unless r.result.save
-        result.success = false
+        rescheduling_result.success = false
         r.errors = r.result.errors
       end
     end
-
-    result
   end
 
   def set_user_as_watcher(work_package)

app/services/work_packages/update_ancestors_service.rb

@@ -84,6 +84,7 @@ def changes?(work_package)
   def save_updated_work_packages(updated_work_packages)
     updated_initiators, updated_ancestors = updated_work_packages.partition { initiator?(it) }
 
+    # TODO: Can we do better and not save if not changed?
     # Send notifications for initiator updates
     success = updated_initiators.all? { |wp| wp.save(validate: false) }
     # Do not send notifications for parent updates

spec/models/projects/project_acts_as_journalized_spec.rb

@@ -370,10 +370,10 @@
         create(:project_phase, project_id: project.id)
       end
 
-      it "fails when using save_journals" do
+      it "succeeds when using save_journals" do
         expect do
           project.save_journals
-        end.to raise_error(ActiveRecord::StatementInvalid)
+        end.to change { project.journals.count }.from(1).to(2)
       end
 
       it "succeeds when using touch_and_save_journals" do

spec/models/work_package/work_package_acts_as_journalized_spec.rb

@@ -54,6 +54,39 @@
 
     current_user { create(:user) }
 
+    # The below test was failing with the following error:
+    # ERROR:  new row for relation "journals" violates check constraint "journals_validity_period_not_empty" (PG::CheckViolation)
+    # DETAIL:  Failing row contains (1178, WorkPackage, 481, 1252, , 2025-12-04 07:58:21.028586+00, 1,
+    #          2025-12-04 07:58:21.028586+00, Journal::WorkPackageJournal, 833, {}, empty, f).
+    it "can add multiple comments right after creation" do
+      work_package
+      User.execute_as current_user do
+        # create multiple journals after creation
+        work_package.add_journal(user: current_user, notes: "First comment")
+        work_package.save_journals
+        work_package.add_journal(user: current_user, notes: "Second comment")
+        work_package.save_journals
+
+        ##### The fix is incomplete: the part below still fails.
+        ##### There may also be some issues with timestamps inaccuracies: some
+        ##### updated_at not matching the journal's validity periods.
+
+        # # create multiple journals after update
+        # work_package.update(subject: "Updated subject")
+        # work_package.add_journal(user: current_user, notes: "Third comment")
+        # work_package.save_journals
+        # work_package.add_journal(user: current_user, notes: "Fourth comment")
+        # work_package.save_journals
+
+        # # Verify journals were created with aggregation:
+        # # - Journal 1: creation + first comment (aggregated)
+        # # - Journal 2: second comment (can't aggregate - both have notes)
+        # # - Journal 3: update + third comment (aggregated)
+        # # - Journal 4: fourth comment (can't aggregate - both have notes)
+        # expect(work_package.journals.count).to eq(4)
+      end
+    end
+
     context "for work package creation" do
       it { expect(Journal.for_work_package.count).to eq(1) }
 

spec/services/projects/creation_wizard/create_artifact_work_package_service_spec.rb

@@ -161,6 +161,27 @@
       expect(artifact_work_package.description).to include(expected_link)
     end
 
+    it "sends only one notification for the work package comment" do
+      clear_enqueued_jobs
+      result = instance.call
+
+      project = result.result
+      artifact_work_package = WorkPackage.find(project.project_creation_wizard_artifact_work_package_id)
+
+      expect(enqueued_jobs).to include(a_hash_including(job: Notifications::WorkflowJob))
+      workflow_jobs = enqueued_jobs.select { it[:job] == Notifications::WorkflowJob }
+
+      # There should be exactly 2 WorkflowJobs: one for the attachment journal,
+      # one for the work package journal
+      journals = workflow_jobs.pluck(:args)
+                              .map { |_state_arg, journal_arg, _send_notification| journal_arg["_aj_globalid"] }
+                              .map { |journal_gid| GlobalID::Locator.locate(journal_gid) }
+      expect(journals.pluck(:journable_type, :journable_id)).to contain_exactly(
+        [WorkPackage.name, artifact_work_package.id],
+        [Attachment.name, artifact_work_package.attachments.first.id]
+      )
+    end
+
     context "when artifact storage is internal" do
       it "attaches directly to the work package" do
         project.update(project_creation_wizard_artifact_export_type: "attachment")
@@ -406,7 +427,6 @@
         end
       end
     end
-
   end
 
   context "when contract is invalid" do

spec/services/work_packages/create_service_integration_spec.rb

@@ -169,6 +169,22 @@
         .to contain_exactly(user)
     end
 
+    it "creates only one WorkflowJob and one JournalsCompletedJob for the created work package" do
+      # avoid setting the parent to avoid creating unrelated jobs
+      attributes[:parent] = nil
+
+      # create objects before clearing jobs to make sure unrelated jobs are cleared
+      project
+      clear_enqueued_jobs
+      expect(service_result).to be_success
+
+      got_enqueued_jobs = enqueued_jobs.pluck(:job)
+      expect(got_enqueued_jobs)
+        .to contain_exactly(WorkPackages::WorkflowJob,
+                            Notifications::WorkflowJob,
+                            Journals::CompletedJob)
+    end
+
     describe "setting the attachments" do
       let!(:other_users_attachment) do
         create(:attachment, container: nil, author: create(:user))

spec/workers/copy_project_job_spec.rb

@@ -38,12 +38,25 @@
 
   before { allow(mail_double).to receive(:deliver_later) }
 
-  describe "copy project succeeds with errors" do
-    let(:source_project) { create(:project, types: [type]) }
-
-    let!(:work_package) { create(:work_package, :skip_validations, done_ratio: 101, project: source_project, type:) }
+  describe "with a source project having invalid work packages" do
+    shared_let(:type) { create(:type_bug) }
+    shared_let(:source_project) { create(:project, types: [type]) }
+
+    shared_let(:work_package_with_model_errors) do
+      # done ratio must be between 0 and 100, this is a model validation
+      create(:work_package, :skip_validations,
+             project: source_project, type:,
+             subject: "wp with invalid done_ratio",
+             done_ratio: 101)
+    end
+    shared_let(:work_package_with_contract_errors) do
+      # remaining work must be less than or equal to work, this is a WorkPackages::BaseContract validation
+      create(:work_package, :skip_validations,
+             project: source_project, type:,
+             subject: "wp with invalid work and remaining work",
+             remaining_hours: 10, estimated_hours: 2)
+    end
 
-    let(:type) { create(:type_bug) }
     let(:job_args) do
       {
         target_project_params: params,
@@ -52,13 +65,8 @@
     end
 
     let(:params) { { name: "Copy", identifier: "copy", type_ids: [type.id] } }
-    let(:expected_error_message) do
-      "#{WorkPackage.model_name.human} '#{work_package.type.name} ##{work_package.id}: " \
-        "#{work_package.subject}': % Complete " \
-        "#{I18n.t('activerecord.errors.models.work_package.attributes.done_ratio.inclusion')}"
-    end
 
-    it "copies the project", :aggregate_failures do
+    it "copies the project and invalid work packages without reporting any errors", :aggregate_failures do
       copy_job = nil
       batch = GoodJob::Batch.enqueue(user: admin, source_project:) do
         copy_job = described_class.perform_later(**job_args)
@@ -67,9 +75,12 @@
       batch.reload
 
       copied_project = Project.find_by(identifier: params[:identifier])
-
       expect(copied_project).to eq(batch.properties[:target_project])
-      expect(batch.properties[:errors].first).to eq(expected_error_message)
+
+      # all work packages were copied, even if they are invalid
+      expect(copied_project.work_packages.count).to eq(source_project.work_packages.count)
+      # no errors reported
+      expect(batch.properties[:errors]).to be_empty
 
       # expect to create a status
       expect(copy_job.job_status).to be_present
@@ -80,15 +91,30 @@
       expect(copy_job.job_status[:payload]["_links"]["project"]).to eq(expected_link)
     end
 
-    it "ensures that error messages are correctly localized" do
-      batch = GoodJob::Batch.enqueue(user: user_de, source_project:) do
-        described_class.perform_later(**job_args)
+    context "when the source project has automatically generated subjects" do
+      before do
+        type.update(patterns: { subject: { blueprint: "wp {{id}} in project {{project_id}}", enabled: true } })
       end
-      GoodJob.perform_inline
-      batch.reload
 
-      msg = /Arbeitspaket 'Bug #\d+: WorkPackage No. \d+': % abgeschlossen muss zwischen 0 und 100 liegen\./
-      expect(batch.properties[:errors].first).to match(msg)
+      it "copies the project without reporting any errors and generates subjects different from source project" do
+        batch = GoodJob::Batch.enqueue(user: admin, source_project:) do
+          described_class.perform_later(**job_args)
+        end
+        GoodJob.perform_inline
+        batch.reload
+
+        copied_project = Project.find_by(identifier: params[:identifier])
+
+        # all work packages were copied, even if they are invalid
+        expect(copied_project.work_packages.count).to eq(source_project.work_packages.count)
+        # no errors reported
+        expect(batch.properties[:errors]).to be_empty
+
+        # generated subjects are different from source project
+        copied_project.work_packages.each do |work_package|
+          expect(work_package.subject).to eq("wp #{work_package.id} in project #{copied_project.id}")
+        end
+      end
     end
   end