Make "Project copy" copy all work packages as-is

When doing a project copy, copy as-is and accept that the state can be
faulty.

Before, it was disabling parts of the validations while keeping some
other parts, which made the experience confusing: the
SetAttributesService would be ok, but then saving the work package would
fail, and later the work package would be saved again without validation
in the `UpdateAncestorsService`.

Actually it was working previously only due to side-effects of
`UpdateAncestorsService` which does save and also due to changes
introduced in 0700299 which refactored the part that detect `save`
returned `false`, leading to believe that `save` always returned `true`.

Now the validation are done solely by the `SetAttributesService`, and
when saving the work package validations are ignored.

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/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)

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/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