Explicitly remove file attachment if changed after validation failure

[MrSerth]

We recently discovered an issue where an old attachment would be incorrectly linked to an updated file after validation errors occurred. Specifically, the reproduction scenario is as follows:

1. Create a new task with a native file attached that passes all validations.
2. Edit this task and introduce a validation error that is unrelated to the file. For example, remove the task's title or language.
3. Select another native file attachment for the already existing TaskFile.
4. Attempt to save the changes
5. As expected, a validation error regarding the change is shown (title, language). Furthermore, the upload field has been cleared, since the file attachment was changed and the newly selected file was not yet saved on the server.
6. Fix the validation error and (without re-selecting the file attachment) save the task again.
7. Now, the task was saved again, despite the "missing" file attachment. However, it was expected that another validation error regarding the missing file attachment is shown. Unfortunately, the existing TaskFile also gets updated with the name of the previously selected file name (new file name, old content).

During our investigation, we identified that the attachment param of TaskParameters#file_params was simply missing, despite use_attached_file=true. Consequentially, assigning the params as attributes to the task (and the task file respectively) did not modify the existing attachment relationship, so that the previously saved attachment was still "connected". With the commit at hand, we therefore set the attachment param to nil explicitly, when it is not yet present (indicating that no file was sent by the browser) and the parent_blob_id is not present either (indicating that the file attachment got touched in one of the previous attempts to save changes).

---

I didn't add tests on my own, since @florian-str discovered the issue while working on tests for #1103, so that the changes will be covered there.

---

Thanks @nenock for your valuable input on this issue!

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.64%. Comparing base (d4eea0e) to head (c196572).
Report is 1 commits behind head on fs_task_contrib.

Files with missing lines	Patch %	Lines
app/controllers/concerns/task_parameters.rb	80.00%	2 Missing ⚠️

Additional details and impacted files

@@                 Coverage Diff                 @@
##           fs_task_contrib    #1683      +/-   ##
===================================================
- Coverage            94.69%   94.64%   -0.05%     
===================================================
  Files                  130      130              
  Lines                 3278     3288      +10     
===================================================
+ Hits                  3104     3112       +8     
- Misses                 174      176       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nenock]

Looks good to me. I really like the refactoring since our pair programming!

One thought though: I'd add to the commit message and to the comment in the file, that the behaviour we're prevent here is a unintended renaming of the old file.

[MrSerth]

Thanks for your feedback! I've updated the comment and commit message to reflect that fact, too.