(maybe) Fix failing tests for BCF XML importer

[cbliard]

There is something going on with validity_period. The new code seems to work.

The tests are failing since the modifications done in #21325.

We paired on it with @ulferts. Can you help further please?

[cbliard]

@ulferts I added a failing test reproducing the issue in spec/models/work_package/work_package_acts_as_journalized_spec.rb:60.

[cbliard]

Another category of tests is also failing, related to project copy: ./spec/workers/copy_project_job_spec.rb:83

Before the modifications done in #21325, there was two saves:

• a first one in the service #perform with work_package.save
• a second one in the same #perform when calling #reschedule_related: the work package would be saved a second time, and errors stored if the save fails

After the modifications done in #21325, there is only one save:

• in the service #perform with work_package.save
• the second one in #reschedule_related is not triggered because the work package was not modified by the rescheduling (that's the modifications from #21325), so no second save

Too bad, that's on the second save that the errors were merged to the result. Without the second save, and ignoring the errors from the first save, the job completes without errors, but the test fails because it expects some errors.

That's why the test was passing, and now is not passing anymore. But there is more!

Actually, the code should not have tried to reschedule at all because the first save did not work. There was a subtle issue introduced in this commit related to automatic subject

• before the result.success was updated with the result of work_package.save
• now the result.success is updated with the result of set_templated_subject(work_package) which always returns true if there is no replacement pattern.

That means that result.success is almost always true regardless of the work package being valid or not.

Now why is it reaching the first #save point?

It's because contract is WorkPackages::CopyProjectContract and this one skips model validation (done in this commit), so the SetAttributesService does not raise any error.

And why is the work package eventually saved?

It's because the WorkPackages::UpdateAncestorsService is being called too, and this one saves with validate: false in #save_updated_work_packages.

That's a mess.

And I'm not sure what the expected behavior is. If a work package has some errors on save, should it be saved anyway when copying a project?

[cbliard]

Ok, so we had the opportunity to explore it more with @EinLama. Here is what we found about the current behavior on latest dev when copying a project:

• a work package with a model error (like done_ratio being 101) will be copied, and the error will be reported in the copy.
• a work package with some contract errors will be copied if the CopyProjectContract did override the validation for it (for instance, work package is automatically scheduled but has no predecessor nor children). The error will not be reported and the work package will be created in the destination project.
• a work package with some contract errors will not be copied if the CopyProjectContract has some validation error (like work / remaining work and % complete being incorrect). The error will be reported and the work package will not be created in the copied project.
  • furthermore, if that work package had children or relation in the parent project, as the parent is not copied in the destination project, the copied children will still reference the parent of the source project. This feels like a bug.
• we did not try with autogenerated subject, but looking at the code, we suspect that the subject will not be updated if the work package has some model errors. It will simply be the same as the source work package (which will be unnoticed most of the time)

The behavior is not consistent.

To make it consistent, one approach could be:

• to always copy the work package in the destination project, despite the validation errors
• and to always report validation errors in the copy result

[oliverguenther]

@cbliard I feel like this is more a fundamental question of how errors are being kept and/or recorded in case of copying. I am leaning towards "if you copy the project, you copy as is and accept if the state was faulty" rather than preventing the copy in the first place. At the same time, I think the decisions that lead to these failures are rather arbitrary, so I could see us fixing it in both ways