-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Explicitly remove file attachment if changed after validation failure #1683
Conversation
Codecov ReportAttention: Patch coverage is
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. |
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. |
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).
c64a85f
to
c196572
Compare
Thanks for your feedback! I've updated the comment and commit message to reflect that fact, too. |
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:
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 existingattachment
relationship, so that the previously saved attachment was still "connected". With the commit at hand, we therefore set theattachment
param tonil
explicitly, when it is not yet present (indicating that no file was sent by the browser) and theparent_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!