https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17501 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #21)
1/ Koha::UploadedFile a. Koha::UploadedFile->delete is weird, I'd suggest to call and return $self->SUPER::delete in any cases and warn only `unless -e $file`. As Koha::Object->delete returns 1 on success, I don't think you should return anything else. Calling SUPER::delete always now. Leave two warns. Return false if deleting the file fails.
b. getCategories - use C4::Koha; is missing - The methods in itself should be moved to Koha::UploadedFiles. Agreed
I'd even suggest to remove it, and call GetAuthorisedValues only once in tools/upload.pl. It does not make sense to call it 3 times in the same script. These three times made sense, but nevertheless moved them up into one call. I want to keep getCategories, since it hides the implementation from the script and can be changed easier (say moving it from auth values to separate table).
2/ Koha::UploadedFiles a. ->delete should not be called after ->new, but directly (Koha::UploadedFiles->delete). I am wondering if what you are doing in this method should not be moved to Koha::Objects->delete because actually it is the expected behaviour I think. Changed new->delete into delete. Removed delete_errors as a consequence for now. I suggest to keep delete here now, but we could open a new report to move it to Koha::Objects.
3/ in tools/upload.pl, it's certainly better to use ->find($id) and then check if the public method. Adjusted three calls.
All adjustments in the last follow-up. Thanks for reviewing. -- You are receiving this mail because: You are watching all bug changes.