[Koha-bugs] [Bug 14321] Merge UploadedFile and UploadedFiles into Koha::Upload

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

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14321

--- Comment #46 from Jonathan Druart <jonathan.druart at bugs.koha-community.org> ---
Comment on attachment 42559
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42559
Bug 14321: Introduce Koha::Upload

Review of attachment 42559:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14321&attachment=42559)
-----------------------------------------------------------------

::: Koha/Upload.pm
@@ +211,5 @@
> +    return @res;
> +}
> +
> +sub DESTROY {
> +}

This is not really useful.

@@ +213,5 @@
> +
> +sub DESTROY {
> +}
> +
> +# **************  HELPER ROUTINES / CLASS METHODS ******************************

Should be part of POD.

@@ +260,5 @@
> +    $self->{uid} = C4::Context->userenv->{number} if C4::Context->userenv;
> +    $self->{public} = $params->{public}? 1: undef;
> +}
> +
> +sub _fh {

I don't understand the usefulness of this method.

@@ +274,5 @@
> +    if( $self->{files}->{$filename} &&
> +            $self->{files}->{$filename}->{errcode} ) {
> +        #skip
> +    } elsif( !$self->{temporary} && !$self->{rootdir} ) {
> +        $self->{files}->{$filename}->{errcode} = 3; #no rootdir

Please avoid that and use readable error code. "no_rootdir" would be perfect.

@@ +311,5 @@
> +    my $p;
> +    if( ref $rec ) {
> +        $p= $rec->{permanent}? $self->{rootdir}: $self->{tmpdir};
> +        $p.= '/';
> +        $p.= $rec->{dir}. '/'. $rec->{hashvalue}. '_'. $rec->{filename};

It would be better to use File::Spec::catfile

@@ +351,5 @@
> +        $self->{public},
> +        $self->{temporary}? 0: 1,
> +    );
> +    $dbh->do( $sql, undef, @pars );
> +    my $i = $dbh->last_insert_id(undef, undef, 'uploaded_files', undef);

last_insert_id is mysql specific.
It would have been better to create the Koha::UploadFile[s] based on
Koha::Object[s] to avoid and write sql queries.

@@ +358,5 @@
> +
> +sub _lookup {
> +    my ( $self, $params ) = @_;
> +    my $dbh = C4::Context->dbh;
> +    my $sql = qq|

Variable interpolations should not be allowed in SQL queries.

-- 
You are receiving this mail because:
You are watching all bug changes.