diff --git a/app/contracts/work_packages/copy_project_contract.rb b/app/contracts/work_packages/copy_project_contract.rb index 7a87dfb673c..5cf56471511 100644 --- a/app/contracts/work_packages/copy_project_contract.rb +++ b/app/contracts/work_packages/copy_project_contract.rb @@ -42,6 +42,12 @@ module WorkPackages 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 diff --git a/app/services/work_packages/create_service.rb b/app/services/work_packages/create_service.rb index a24d7350c5c..9050a940fb3 100644 --- a/app/services/work_packages/create_service.rb +++ b/app/services/work_packages/create_service.rb @@ -55,15 +55,14 @@ class WorkPackages::CreateService < BaseServices::BaseCallable 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 - - set_templated_subject(work_package) - end - 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) + + update_subject_if_automatically_generated(work_package) + # 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 @@ class WorkPackages::CreateService < BaseServices::BaseCallable 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) diff --git a/app/services/work_packages/update_ancestors_service.rb b/app/services/work_packages/update_ancestors_service.rb index 300fa80bf60..7f4ac6c0d67 100644 --- a/app/services/work_packages/update_ancestors_service.rb +++ b/app/services/work_packages/update_ancestors_service.rb @@ -84,6 +84,7 @@ class WorkPackages::UpdateAncestorsService < BaseServices::BaseCallable 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 diff --git a/spec/workers/copy_project_job_spec.rb b/spec/workers/copy_project_job_spec.rb index 5674cfaff24..14b7105b257 100644 --- a/spec/workers/copy_project_job_spec.rb +++ b/spec/workers/copy_project_job_spec.rb @@ -38,12 +38,25 @@ RSpec.describe CopyProjectJob, type: :model, with_good_job_batches: [CopyProject before { allow(mail_double).to receive(:deliver_later) } - describe "copy project succeeds with errors" do - let(:source_project) { create(:project, types: [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]) } - let!(:work_package) { create(:work_package, :skip_validations, done_ratio: 101, project: source_project, 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 @@ RSpec.describe CopyProjectJob, type: :model, with_good_job_batches: [CopyProject 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 @@ RSpec.describe CopyProjectJob, type: :model, with_good_job_batches: [CopyProject 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 @@ RSpec.describe CopyProjectJob, type: :model, with_good_job_batches: [CopyProject 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