[Koha-bugs] [Bug 17501] Koha Objects for uploaded files

[bugzilla-daemon at bugs.koha-community.org]

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.