[Koha-bugs] [Bug 17501] Koha Objects for uploaded files
bugzilla-daemon at bugs.koha-community.org
bugzilla-daemon at bugs.koha-community.org
Wed Jan 11 14:15:25 CET 2017
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17501
--- Comment #22 from Marcel de Rooy <m.de.rooy at 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.
More information about the Koha-bugs
mailing list