Wondering if there is not something wrong or missing in t/db_dependent/Upload.t, the keep_file flag is not testing if the file is really kept. Also we are not testing if the files have been correctly created.
Created attachment 87429 [details] [review] Bug 22644: Highlight the problem
There is not a problem here, just a misunderstanding. As noted in the commment, the file does not exist here since a record is added via TestBuilder. In the preceding code you see a test for a warning about the missing file. Deleting a record twice while keeping the missing file is kind of exaggerated testing ;) So the patch here does not make sense.
162 my $delete = Koha::UploadedFiles->search({ id => $id })->delete; 163 isnt( $delete, "0E0", 'Delete successful' ); 164 isnt( -e $path, 1, 'File no longer found after delete' ); We are testing that the file has been deleted, but it never existed. Those tests do not test the whole purpose of the uploaded files. Moreover the new tests I have tried to write for bug 22508 are failing, because of this behavior. So, IMO, there is a real need here.
Marcel, what do you think? How could I provide tests for bug 22508 without having this being fixed? Any ideas?
(In reply to Jonathan Druart from comment #4) > Marcel, what do you think? How could I provide tests for bug 22508 without > having this being fixed? Any ideas? Will have a look very soon.
The test reflects the history of Upload to Uploader and UploadedFile(s) with Koha::Object(s). I think we should put the Uploader stuff into its own test Uploader.t. And put all Koha objects related tests to UploadedFile.t. What UploadedFile::delete may need, is a return code: deleting the record may have failed, but deleting the file as well. The test could be improved by adding some temporary files. And we need some decisions on 22508 as well. See my older comment there. Especially, do we copy the file or keep just one and allow direct access to selected subdirs of the clone upload folder?