Bug 14321 - Merge UploadedFile and UploadedFiles into Koha::Upload
Summary: Merge UploadedFile and UploadedFiles into Koha::Upload
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 6874
Blocks: 14904 14905
  Show dependency treegraph
 
Reported: 2015-06-03 11:38 UTC by Marcel de Rooy
Modified: 2017-12-15 08:00 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14321: Using Koha::Upload in tools/upload-file.pl (29.17 KB, patch)
2015-08-14 12:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add some error checking to Upload.pm (4.63 KB, patch)
2015-08-14 12:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add public column to uploaded_files (3.14 KB, patch)
2015-08-14 12:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Redirect upload plugin to general upload script (32.54 KB, patch)
2015-08-14 12:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Using Koha::Upload in tools/upload-file.pl (29.37 KB, patch)
2015-08-17 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add some error checking to Upload.pm (4.63 KB, patch)
2015-08-17 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add public column to uploaded_files (3.17 KB, patch)
2015-08-17 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Redirect upload plugin to general upload script (33.70 KB, patch)
2015-08-17 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Switch to Upload in opac-retrieve (2.74 KB, patch)
2015-08-17 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Unit test for Koha::Upload (5.67 KB, patch)
2015-08-17 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Fix permissions check in upload-file.pl (1.93 KB, patch)
2015-08-17 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Final cleanup, removing obsolete files (46.30 KB, patch)
2015-08-17 13:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Switch to Upload in opac-retrieve (3.43 KB, patch)
2015-08-19 13:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Unit test for Koha::Upload (5.67 KB, patch)
2015-08-19 13:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Fix permissions check in upload-file.pl (1.93 KB, patch)
2015-08-19 13:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Just some documentation lines added or fixed (2.97 KB, patch)
2015-08-19 13:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Final cleanup, removing obsolete files (46.30 KB, patch)
2015-08-19 13:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Final cleanup, removing obsolete files (46.30 KB, patch)
2015-08-19 14:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Using Koha::Upload in tools/upload-file.pl (30.58 KB, patch)
2015-08-25 07:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add some error checking to Upload.pm (4.63 KB, patch)
2015-08-25 07:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add public column to uploaded_files (3.17 KB, patch)
2015-08-25 07:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Redirect upload plugin to general upload script (33.70 KB, patch)
2015-08-25 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Switch to Upload in opac-retrieve (3.43 KB, patch)
2015-08-25 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Unit test for Koha::Upload (5.67 KB, patch)
2015-08-25 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Fix permissions check in upload-file.pl (1.93 KB, patch)
2015-08-25 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Just some documentation lines added or fixed (2.97 KB, patch)
2015-08-25 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Final cleanup, removing obsolete files (46.36 KB, patch)
2015-08-25 07:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add two columns to uploaded_files (1.86 KB, patch)
2015-09-04 15:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Introduce Koha::Upload (19.21 KB, patch)
2015-09-04 15:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Integrate Upload.pm into Koha (47.16 KB, patch)
2015-09-04 15:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Final cleanup, removing obsolete files (46.40 KB, patch)
2015-09-04 15:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add two columns to uploaded_files (1.87 KB, patch)
2015-09-15 13:03 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14321: Introduce Koha::Upload (19.23 KB, patch)
2015-09-15 13:05 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14321: Integrate Upload.pm into Koha (47.18 KB, patch)
2015-09-15 13:08 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14321: Final cleanup, removing obsolete files (46.41 KB, patch)
2015-09-15 13:09 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14321: QA Fix (1.58 KB, patch)
2015-09-16 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14321: QA Fix (1.68 KB, patch)
2015-09-16 12:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Changes for Upload.pm and upload-file.pl (2.21 KB, patch)
2015-09-16 12:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Perltidy upload.pl (4.01 KB, patch)
2015-09-16 12:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Changes for Upload.pm and upload-file.pl (2.52 KB, patch)
2015-09-16 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Perltidy upload.pl (4.01 KB, patch)
2015-09-16 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add two columns to uploaded_files (1.92 KB, patch)
2015-09-17 13:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14321: Introduce Koha::Upload (19.29 KB, patch)
2015-09-17 13:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14321: Integrate Upload.pm into Koha (47.22 KB, patch)
2015-09-17 13:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14321: Final cleanup, removing obsolete files (46.46 KB, patch)
2015-09-17 13:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14321: QA Fix (1.69 KB, patch)
2015-09-17 13:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Changes for Upload.pm and upload-file.pl (2.53 KB, patch)
2015-09-17 13:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Perltidy upload.pl (4.03 KB, patch)
2015-09-17 13:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Additional QA requests (8.47 KB, patch)
2015-09-18 14:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14321: Add two columns to uploaded_files (2.00 KB, patch)
2015-09-21 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14321: Introduce Koha::Upload (19.36 KB, patch)
2015-09-21 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14321: Integrate Upload.pm into Koha (47.64 KB, patch)
2015-09-21 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14321: Final cleanup, removing obsolete files (46.53 KB, patch)
2015-09-21 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14321: QA Fix (1.76 KB, patch)
2015-09-21 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Changes for Upload.pm and upload-file.pl (2.60 KB, patch)
2015-09-21 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14321: [QA Follow-up] Additional QA requests (8.49 KB, patch)
2015-09-21 15:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14321: (RM followup) DBIx update (1.92 KB, patch)
2015-09-25 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14321: (QA followup) remove Test::CGI::Multipart dependency (911 bytes, patch)
2015-09-25 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2015-06-03 11:38:38 UTC
One of the main QA concerns on bug 6874 was the introduction of UploadedFiles.pm while we already have UploadedFile.pm.
Recognizing the amount of work on that report, it would not be very productive to just park the development in FQA.
The older module UploadedFile contains functionality for temporary uploads and has minimum test coverage; the newer module contains functionality for permanent uploads and access via hash id, and with much more unit tests.

Koha::Uploader should contain the best of both: allow temporary and permanent uploads, also keeping the progress and ajax upload of the old one. Providing an object that allows for some future extensions.
Comment 1 Marcel de Rooy 2015-08-14 12:33:08 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2015-08-14 12:33:12 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2015-08-14 12:33:15 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2015-08-14 12:33:19 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2015-08-14 12:36:35 UTC
This shows my progress until now. Almost done :)
I will still add a test file and remove some obsolete files.
After that there may be some smaller style issues, adding upload to the menu and perhaps adding a specific permission or so.
Comment 6 Marcel de Rooy 2015-08-17 13:18:28 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2015-08-17 13:18:31 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2015-08-17 13:18:35 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2015-08-17 13:18:39 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2015-08-17 13:18:42 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2015-08-17 13:18:46 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2015-08-17 13:18:49 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2015-08-17 13:18:53 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2015-08-17 13:23:19 UTC
At the end, this patch set actually reduces code. 
Another bug will take care of menu and permission.
Comment 15 Marcel de Rooy 2015-08-19 13:50:57 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2015-08-19 13:51:00 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2015-08-19 13:51:04 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2015-08-19 13:51:07 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2015-08-19 13:51:11 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2015-08-19 13:56:17 UTC
Small consistency adjustment in patch 5 and new patch 8 for some pod lines in Upload.pm.
Comment 21 Marcel de Rooy 2015-08-19 14:25:27 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2015-08-19 14:26:00 UTC
Rebased last patch for change on dependency patches.
Comment 23 Marcel de Rooy 2015-08-25 07:02:51 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2015-08-25 07:02:55 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2015-08-25 07:02:58 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2015-08-25 07:03:02 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2015-08-25 07:03:06 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2015-08-25 07:03:09 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2015-08-25 07:03:13 UTC Comment hidden (obsolete)
Comment 30 Marcel de Rooy 2015-08-25 07:03:17 UTC Comment hidden (obsolete)
Comment 31 Marcel de Rooy 2015-08-25 07:03:21 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2015-08-25 07:04:15 UTC
Rebased, included small change from bug 14589 for upload-file and upload-file-progress. No dependencies anymore.
Comment 33 Marcel de Rooy 2015-09-03 17:00:21 UTC
CHECK: No results while files have been uploaded permanently? Test by drojf.
Comment 34 Marcel de Rooy 2015-09-04 07:41:30 UTC
Rearranging..
Comment 35 Marcel de Rooy 2015-09-04 15:43:23 UTC Comment hidden (obsolete)
Comment 36 Marcel de Rooy 2015-09-04 15:43:30 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2015-09-04 15:43:34 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2015-09-04 15:43:38 UTC Comment hidden (obsolete)
Comment 39 Marcel de Rooy 2015-09-04 16:04:42 UTC
Mirko: I rearranged the patches and as you will see, the test plan on the third patch is the crux now (you will recognize it).
Apart from reorganizing, I added a permanent column to make life easier in relation to public and category. Also added an extra test for search results (get).
HTH
Comment 40 Mirko Tietgen 2015-09-15 13:03:35 UTC Comment hidden (obsolete)
Comment 41 Mirko Tietgen 2015-09-15 13:05:08 UTC Comment hidden (obsolete)
Comment 42 Mirko Tietgen 2015-09-15 13:08:55 UTC Comment hidden (obsolete)
Comment 43 Mirko Tietgen 2015-09-15 13:09:12 UTC Comment hidden (obsolete)
Comment 44 Mirko Tietgen 2015-09-15 13:20:07 UTC
Works for me. One thing I noticed is that in the cataloguing editor, having the plugin and add/remove field options next to each other looks a little off.
Comment 45 Marcel de Rooy 2015-09-15 15:47:26 UTC
(In reply to Mirko Tietgen from comment #44)
> Works for me. One thing I noticed is that in the cataloguing editor, having
> the plugin and add/remove field options next to each other looks a little
> off.

Great. Thanks for signing off. Yes, the popup form could still be further improved in terms of styling. We can do that on a new report..
Comment 46 Jonathan Druart 2015-09-16 10:38:20 UTC
Comment on attachment 42559 [details] [review]
Bug 14321: Introduce Koha::Upload

Review of attachment 42559 [details] [review]:
-----------------------------------------------------------------

::: 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.
Comment 47 Jonathan Druart 2015-09-16 10:43:27 UTC
Comment on attachment 42560 [details] [review]
Bug 14321: Integrate Upload.pm into Koha

Review of attachment 42560 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt
@@ +1,1 @@
> +[% INCLUDE 'doc-head-open.inc' %]

I don't really like the structure of this file. You have to jump in the file to know what is next.
Is it a new proposal for template files?

::: tools/upload-file.pl
@@ +82,3 @@
>  }
>  
> +sub upload_pars {

Maybe a quick comment would be useful here.

::: tools/upload.pl
@@ +1,1 @@
> +#!/usr/bin/perl

Please execute perltidy on new file.
Comment 48 Jonathan Druart 2015-09-16 10:43:58 UTC
Julian, could I get your signoff on this one please?
Comment 49 Jonathan Druart 2015-09-16 10:45:23 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2015-09-16 11:41:31 UTC
Jonathan: Thanks for looking into this one!

> +sub DESTROY {
> +}
> This is not really useful.
Agreed. Removing it.

> +# **************  HELPER ROUTINES / CLASS METHODS
> Should be part of POD.
Right!

> +sub _fh {
> I don't understand the usefulness of this method.
Hm. I am not sure anymore, but this routine may have had some more lines in development. But it still contains a test if (a) then a->b, so I want to keep it in order to not repeat this test a few times. Keeps the code more readable at the calling spots.

> +        $self->{files}->{$filename}->{errcode} = 3; #no rootdir
> Please avoid that and use readable error code. "no_rootdir" would be perfect.
I hope that is not a blocker for you :) and prefer to leave it as-is now..

> +        $p= $rec->{permanent}? $self->{rootdir}: $self->{tmpdir};
> +        $p.= '/';
> +        $p.= $rec->{dir}. '/'. $rec->{hashvalue}. '_'. $rec->{filename};
> It would be better to use File::Spec::catfile
OK. Adjusting it.

> +    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.
Could be true, but that is a too large rewrite operation at this moment.
We could do that on a new report at some point in time. Some day we hope to not have anything mysql specific anymore?

> +sub _lookup {
> +    my ( $self, $params ) = @_;
> +    my $dbh = C4::Context->dbh;
> +    my $sql = qq|
> Variable interpolations should not be allowed in SQL queries.
Which line do you exactly refer to now?
Probably the WHERE id IN ($params->{id}) line later on.
Note that the id variable is only allowed to be of the form 1,2,3,4 by the regex directly preceding it. If you persist here, I could rebuild it in the form ?,?,? here and first count the number of id's..

Will continue with your second comment now.
Comment 51 Marcel de Rooy 2015-09-16 11:53:02 UTC
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt
> I don't really like the structure of this file. You have to jump in the file to know what is next.
> Is it a new proposal for template files?
No, it is not a new proposal or so. This could be a matter of taste, but this template reads much easier to me than the long blocks of text with horrible indentation that we already have in Koha. Note that most of us prefer to use subroutines in code instead of one large block. This is more or less the same.
It may be quite easy to copy the blocks to their corresponding places, but for me it will not enhance readability.
Since this is just using normal TT directives, it should not be considered imo in terms of new format, guidelines, etc.
If you still insist, I will remove the blocks.. 

::: tools/upload-file.pl
> +sub upload_pars {
> Maybe a quick comment would be useful here.
Added a small comment.

> ::: tools/upload.pl
> Please execute perltidy on new file.
OK. Will do that.
Comment 52 Jonathan Druart 2015-09-16 11:58:23 UTC
(In reply to Marcel de Rooy from comment #50)
> > +sub _lookup {
> > +    my ( $self, $params ) = @_;
> > +    my $dbh = C4::Context->dbh;
> > +    my $sql = qq|
> > Variable interpolations should not be allowed in SQL queries.
> Which line do you exactly refer to now?

It's general:
  my $query = "SELECT [...]"
should be
  my $query = 'SELECT [...]'
or
  my $query = qq|SELECT [...]
should be
  my $query = q|SELECT [...]
Comment 53 Marcel de Rooy 2015-09-16 12:09:25 UTC Comment hidden (obsolete)
Comment 54 Marcel de Rooy 2015-09-16 12:09:30 UTC Comment hidden (obsolete)
Comment 55 Marcel de Rooy 2015-09-16 12:09:35 UTC Comment hidden (obsolete)
Comment 56 Marcel de Rooy 2015-09-16 12:10:47 UTC
(In reply to Jonathan Druart from comment #52)
> (In reply to Marcel de Rooy from comment #50)
> > > +sub _lookup {
> > > +    my ( $self, $params ) = @_;
> > > +    my $dbh = C4::Context->dbh;
> > > +    my $sql = qq|
> > > Variable interpolations should not be allowed in SQL queries.
> > Which line do you exactly refer to now?
> 
> It's general:
>   my $query = "SELECT [...]"
> should be
>   my $query = 'SELECT [...]'
> or
>   my $query = qq|SELECT [...]
> should be
>   my $query = q|SELECT [...]

OK. Clear. Will add that. Note that there were no variables..
Comment 57 Marcel de Rooy 2015-09-16 12:12:41 UTC Comment hidden (obsolete)
Comment 58 Marcel de Rooy 2015-09-16 12:12:45 UTC Comment hidden (obsolete)
Comment 59 Jonathan Druart 2015-09-17 09:36:54 UTC
I have asked Julian to have a look, to confirm there is no regression on bug 6874.
Comment 60 Julian Maurice 2015-09-17 13:07:25 UTC Comment hidden (obsolete)
Comment 61 Julian Maurice 2015-09-17 13:07:39 UTC Comment hidden (obsolete)
Comment 62 Julian Maurice 2015-09-17 13:07:54 UTC Comment hidden (obsolete)
Comment 63 Julian Maurice 2015-09-17 13:08:12 UTC Comment hidden (obsolete)
Comment 64 Julian Maurice 2015-09-17 13:08:24 UTC Comment hidden (obsolete)
Comment 65 Julian Maurice 2015-09-17 13:08:35 UTC Comment hidden (obsolete)
Comment 66 Julian Maurice 2015-09-17 13:08:46 UTC Comment hidden (obsolete)
Comment 67 Julian Maurice 2015-09-17 13:10:08 UTC
(In reply to Jonathan Druart from comment #59)
> I have asked Julian to have a look, to confirm there is no regression on bug
> 6874.

Confirmed ;)
I also tested the MARC import tool and all works well.
Comment 68 Marcel de Rooy 2015-09-17 13:11:55 UTC
(In reply to Julian Maurice from comment #67)
> (In reply to Jonathan Druart from comment #59)
> > I have asked Julian to have a look, to confirm there is no regression on bug
> > 6874.
> 
> Confirmed ;)
> I also tested the MARC import tool and all works well.

Thx
Comment 69 Jonathan Druart 2015-09-17 15:51:31 UTC
1/ DB structure:
a. It would have been better not to choose "categorycode", already used everywhere into Koha for another use.
b. owner should be a fk.
c. public and permanent are tinyint(4) but actually are boolean, so tinyint(1) is more appropriated.

2/ How can I access tools/upload.pl? I don't find any links.

3/ There is no confirmation message on deleting a file "Are you sure you want to delete?"

4/ The navmenu from the tools module is not displayed on this page.

5/ click on "Allow public downloads" should enable/disable the checkbox (label)

6/ How can I see all files? I have to fill something in the search box.

7/ Would be great to have DT on the table (with sort, pagination and filters)

8/ From the result list, I cannot go back to search for something else.

9/ I cannot choose "Allow public downloads" from the upload plugin, does it mean it's always|never public?

10/
    [3] Use File::Spec->catfile in sub _full_fname.
    [4] Remove variable interpolation from a sql query.
Well, it was a general comment, for everywhere :)
Anyway, we can live with them.

Conclusion: Either I missed something and I did not understand at all the goal of this feature (I have not really looked into bug 6874), or there are several stuffs to fix to improve the ergonomic.
Comment 70 Jonathan Druart 2015-09-17 15:56:09 UTC
(In reply to Jonathan Druart from comment #69)
> 1/ DB structure:
> a. It would have been better not to choose "categorycode", already used
> everywhere into Koha for another use.
> b. owner should be a fk.

Does not come from your patches!
Comment 71 Marcel de Rooy 2015-09-18 13:57:56 UTC
(In reply to Jonathan Druart from comment #69)
Thanks for looking further !

> 1/ DB structure:
> a. It would have been better not to choose "categorycode", already used
> everywhere into Koha for another use.
OK I am renaming it ;)

> b. owner should be a fk.
I will move this to a new report: Bug 14849

> c. public and permanent are tinyint(4) but actually are boolean, so
> tinyint(1) is more appropriated.
tinyint is always one byte: the 4 has to do with display just like int(11)

> 2/ How can I access tools/upload.pl? I don't find any links.
That is correct. As mentioned in the commit message:
NOTE: Bug 14686 will add a specific permission for tools/upload.pl, and
will add the tools/upload script to the Tools menu.

> 3/ There is no confirmation message on deleting a file "Are you sure you
> want to delete?"
Will add that too.

> 4/ The navmenu from the tools module is not displayed on this page.
I was thinking for some future extension when Patron images, Local covers, etc. would come together on this page to have some room there.
Note also that not all tools include that menu.

> 5/ click on "Allow public downloads" should enable/disable the checkbox
> (label)
Will adjust.

> 6/ How can I see all files? I have to fill something in the search box.
By design :) If you would have many files, you may not want that.

> 7/ Would be great to have DT on the table (with sort, pagination and filters)
I agree. But want to postpone that to a new report now.

> 8/ From the result list, I cannot go back to search for something else.
Breadcrumbs? Added a back button (if not called as plugin). 

> 9/ I cannot choose "Allow public downloads" from the upload plugin, does it
> mean it's always|never public?
The thought here was: If I add a link in a biblio record, it [normally] displays in the OPAC and you should make it public to make sense.

> 10/
>     [3] Use File::Spec->catfile in sub _full_fname.
>     [4] Remove variable interpolation from a sql query.
> Well, it was a general comment, for everywhere :)
> Anyway, we can live with them.
Adjusted a few other double quotes around queries.
Did not find any other useful candidates for catfile.

> Conclusion: Either I missed something and I did not understand at all the
> goal of this feature (I have not really looked into bug 6874), or there are
> several stuffs to fix to improve the ergonomic.
Main goal was merge two modules.. Hope to have improved this again somewhat with your feedback. You probably don't miss that much :)

Patch forthcoming.
Comment 72 Marcel de Rooy 2015-09-18 14:21:44 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2015-09-21 15:27:20 UTC
(In reply to Marcel de Rooy from comment #71)
> > c. public and permanent are tinyint(4) but actually are boolean, so
> > tinyint(1) is more appropriated.
> tinyint is always one byte: the 4 has to do with display just like int(11)

tinyint is still more appropriated, in my opinion :)
Not blocker anyway.

> > 6/ How can I see all files? I have to fill something in the search box.
> By design :) If you would have many files, you may not want that.

I don't think it's a useful limitation, not blocker.

> > 7/ Would be great to have DT on the table (with sort, pagination and filters)
> I agree. But want to postpone that to a new report now.

Agreed.

Marked as Passed QA.
Comment 74 Jonathan Druart 2015-09-21 15:29:07 UTC
Created attachment 42726 [details] [review]
Bug 14321: Add two columns to uploaded_files

This patch adds uploaded_files.public and permanent.
Public will be used later to mark uploaded files as available via OPAC.
Permanent will be used to make the distinction between temporary and
permanent storage.

The db rev sets both flags for all current records (uploaded via the
plugin from BZ 6874).

Test plan:
[1] If you tested this before, drop the column public first.
[1] Run the dbrev.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 75 Jonathan Druart 2015-09-21 15:29:13 UTC
Created attachment 42727 [details] [review]
Bug 14321: Introduce Koha::Upload

This patch introduces Koha::Upload. It will replace the modules
C4::UploadedFile.pm and the new C4::UploadedFiles.pm (from BZ 6874).
It also includes a new unit test.

NOTE: This unit test will replace the test for UploadedFiles.pm. It will no
longer use dependency Test::CGI::Multipart. We are now mocking CGI and its
hook to achieve the same result.

Test plan:
[1] Run t/db_dependent/Upload.t.
    Note that if you see a WARNING, you will still need to add an entry
    upload_path to your koha-conf.xml. Or you need to give write
    permission to your Koha instance user for that folder.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 76 Jonathan Druart 2015-09-21 15:29:18 UTC
Created attachment 42728 [details] [review]
Bug 14321: Integrate Upload.pm into Koha

This patch makes the following changes to achieve that:
[1] Use Upload.pm in stage-marc-import.pl, upload-cover-image.pl,
    offline_circ/process_koc.pl and enqueue_koc.pl.
[2] A new file-upload.js replaces file-upload.inc in the associated template.
    We now use ajax to get progress figures instead of launching perl script
    upload-file-progress.
    The js changes now also allow for aborting a file upload.
[3] Adds a tools/upload script and template. It allows to upload multiple
    files at once.
[4] Makes upload-file return error messages in JSON. For a multiple upload,
    we could have some files with errors and others without errors.
    The upload is now marked as Failed only if there was no upload at all.
[5] The upload plugin is converted to use tools/upload with plugin param.
    Deleting an upload is now presented via the search results form.

NOTE: In editing the process_koc.tt I noticed that the form enqueuefile was
hidden and no longer used (with associated code in process_koc.pl). When a
file has been uploaded, I display the form again (with the Apply directly
button). The code still works.

NOTE: We fix an error in upload-file from one of the patches of bug 6874.
The userid of the Koha admin user is passed to haspermission, but we
should pick the userid from the session.

NOTE: Bug 14686 will add a specific permission for tools/upload.pl, and
will add the tools/upload script to the Tools menu.
For now, you need edit_catalogue to start upload.pl and you will
additionally need a permission like upload_local_cover_images
to successfully upload a new file.

Test plan:
[1] Upload a marc file in stage-marc-import. (This is temp storage.)
[2] Check new entry in table uploaded_files. Look for the file in your
    temporary directory (/tmp ?), subfolder koha_upload.
    Bonus: Remove permissions on this subfolder. Retry, check error and
    restore permissions again.
[3] Upload another (larger) file and abort the upload. Check table and
    directory again. You should have a partial file, but no record.
[4] Verify that Stage for import still works as expected.
[5] Test Upload local cover image. (Enable OPACLocalCoverImages.) You can
    test an individual image or a zip file including images and a file
    called datalink.txt (with lines biblionumber,filename).
[6] Test uploading a offline circulation file:
    Enable AllowOfflineCirculation, and create a koc file (plain text):
    Line1: Version=1.0\tA=1\tB=2
    Line2: 2015-08-06 08:00:00 345\treturn\t[barcode]
    Note: Replace tabs and barcode. The number of tabs is essential!
    Checkout the item with your barcode.
    Go to Offline circulation file upload. Upload and click Apply directly.
    Checkout again. Upload again, click Add to offline circulation queue.
[7] Upload three files via tools/upload.pl with a category and marked as
    public. Check the results in the table.
    Verify that you can download the file in OPAC without being logged in.
[8] Pick one new file and one of the files of step 7. Upload them in the
    same category. One upload should succeed. Check for reported error.
[9] Connect upload.pl to field 856$u.
    Goto Cataloguing editor.
    In an empty 856$u, click the tag editor. Upload a file and click Choose.
    Save the record. Open the record in the OPAC and click the link.
    Copy this link to your clipboard for next step.
[A] Go back to editor. Click the tag editor on the same 856 field.
    Choose for Delete.
    Open the link in your clipboard again. Error message?
[B] Check the process of upload, search, download and delete of an upload
    with some diacritical characters in the filename. (Bonus points for
    adding special chars in the category code.)
    Note: You can add categories via authorized values, UPLOAD key.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Bug 14321: [QA Follow-up] Perltidy upload.pl

Run perltidy -pro=xt/perltidyrc on tools/upload.pl.
No other changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 77 Jonathan Druart 2015-09-21 15:29:25 UTC
Created attachment 42729 [details] [review]
Bug 14321: Final cleanup, removing obsolete files

The new Upload.pm, unit test and file-upload.js obsolete a number of
files, including an older jQuery plugin.
The test files progressbar.pl and progressbarsubmit.pl are outdated and
do not serve any purpose in this form. (Actually, we could argue if they
should be here or just be part of a debugging phase.)

Test plan:
[1] Git grep on file-progress, file-upload.inc, UploadedFile,
    ajaxfileupload, ajaxFileUpload
    UploadedFile: Only a reference to DBIx file is found
    ajaxfileupload: Only release notes
[2] Upload a file with tools/upload and stage-marc-import.

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 78 Jonathan Druart 2015-09-21 15:29:30 UTC
Created attachment 42730 [details] [review]
Bug 14321: QA Fix

Remove QA errors (keys on reference is experimental).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 79 Jonathan Druart 2015-09-21 15:29:35 UTC
Created attachment 42731 [details] [review]
Bug 14321: [QA Follow-up] Changes for Upload.pm and upload-file.pl

Based on QA comments, this patch does the following:
[1] Destroy an empty DESTROY.
[2] Promote some comment lines to POD.
[3] Use File::Spec->catfile in sub _full_fname.
[4] Remove variable interpolation from a sql query.
[5] Add a comment in upload-file.pl.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Run the unit test again, and uploaded a file.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 80 Jonathan Druart 2015-09-21 15:29:39 UTC
Created attachment 42732 [details] [review]
Bug 14321: [QA Follow-up] Additional QA requests

This patch makes following requested changes:

[1] Renamed categorycode to uploadcategorycode (with atomicupdate file)
[2] Confirmation before deleting an uploaded file
[3] Label for Public checkbox can be clicked
[4] Back button on result list (not in plugin mode)
[5] Correct additional variable interpolation from sql queries

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested the db revision, unit test.
Uploaded temporary and permanent file, incl. plugin mode.
Deleted an upload.
Ran stage-marc-import.pl.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 81 Jonathan Druart 2015-09-21 15:31:01 UTC
I have squashed
  Bug 14321: Integrate Upload.pm into Koha
and
  Bug 14321: [QA Follow-up] Perltidy upload.pl
Comment 82 Marcel de Rooy 2015-09-23 11:27:13 UTC
(In reply to Jonathan Druart from comment #73)
> (In reply to Marcel de Rooy from comment #71)
> > > c. public and permanent are tinyint(4) but actually are boolean, so
> > > tinyint(1) is more appropriated.
> > tinyint is always one byte: the 4 has to do with display just like int(11)
> 
> tinyint is still more appropriated, in my opinion :)
> Not blocker anyway.

Thanks for QA.
You say that tinyint would be more appropriate, and I agree. The good news is that they also are tinyint's already! The 4 is just some display stuff in mysql. Tinyint on MySQL goes from -128 to 127. The -128 takes 4 display positions. A value like 3 will be left-padded with 4-1=3 spaces.
Comment 83 Tomás Cohen Arazi 2015-09-25 18:35:18 UTC
Created attachment 42889 [details] [review]
Bug 14321: (RM followup) DBIx update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Tomás Cohen Arazi 2015-09-25 18:35:38 UTC
Created attachment 42890 [details] [review]
Bug 14321: (QA followup) remove Test::CGI::Multipart dependency

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 85 Tomás Cohen Arazi 2015-09-25 18:43:09 UTC
Patches pushed to master.

Thanks Marcel!
Comment 86 Tomás Cohen Arazi 2015-09-27 11:54:44 UTC
(In reply to Tomás Cohen Arazi from comment #85)
> Patches pushed to master.
> 
> Thanks Marcel!

This patches broke the search_utf8.t tests. Please take a look.
Comment 87 Marcel de Rooy 2015-09-28 08:28:47 UTC
(In reply to Tomás Cohen Arazi from comment #86)
> This patches broke the search_utf8.t tests. Please take a look.

I fixed the warning from tt_valid.t. See bug 14904.
search_utf8.t is not on jenkins.
Which tests exactly fail?
If I am running this test now without the patches pushed yet, I also have some errors. But this may be a configuration thing..
Still looking into this.
Comment 88 Marcel de Rooy 2015-09-28 09:03:43 UTC
(In reply to Marcel de Rooy from comment #87)
> Which tests exactly fail?
> If I am running this test now without the patches pushed yet, I also have
> some errors. But this may be a configuration thing..
> Still looking into this.

See bug 14905.
Comment 89 Mirko Tietgen 2016-01-05 11:29:21 UTC
Now that this is in 3.22, I wonder how the nonpublic file upload is supposed to work? There is no direct link to tools/upload.pl AFAICT and with the cataloguing plugin, there is no option to choose public/nonpublic. Am I missing something?
Comment 90 Marcel de Rooy 2016-01-05 12:06:58 UTC
(In reply to Mirko Tietgen from comment #89)
> Now that this is in 3.22, I wonder how the nonpublic file upload is supposed
> to work? There is no direct link to tools/upload.pl AFAICT and with the
> cataloguing plugin, there is no option to choose public/nonpublic. Am I
> missing something?

Yes, you miss bug 14686. Still in the pipeline :)
Note that I am still waiting too for any progress on bug 14893. At the time I hoped to still get that into 3.22, but unfortunately..
Comment 91 Mirko Tietgen 2016-01-05 12:14:05 UTC
Ah, thanks! I checked the blocks/depends on bugs, but managed to overlook the see also ones.
Comment 92 David Cook 2016-01-27 23:11:53 UTC
Just a FYI for people testing out permanent uploads.

If you're using "/tmp" as the "upload_path", you might want to check if you're using "PrivateTmp". I'm using openSUSE 13.2 and when I look at /usr/lib/systemd/system/apache2.service, I see "PrivateTmp=true". I think that file is located elsewhere on Debian, but I'll leave that up to others to find that.

In any case, instead of creating "/tmp/koha_upload/36411ad1c1adeb6d0d774f5282fe8790_test1" when permanently uploading a file, it creates "/tmp/systemd-private-bf25b4b0e5994604b54422ea98e4b8c8-apache2.service-PJS1uF/tmp/koha_upload/36411ad1c1adeb6d0d774f5282fe8790_test1".

Fortunately, as far as I can tell, this doesn't create a problem for Koha, at least in the short run. While Koha might still think it's looking in "/tmp/koha_upload", it's actually looking in "/tmp/systemd-private-bf25b4b0e5994604b54422ea98e4b8c8-apache2.service-PJS1uF/tmp/koha_upload".

But of course if you go looking for a file in "/tmp/koha_upload", you're going to be looking for a long time.

Of course, you'd never actually use "/tmp" for storing a file permanently, as "/tmp" should be deleted when your server next reboots (or if some other clean-up job is running). 

If you do something like /home/dcook/koha/uploads, you'll be fine. The file will be stored where you expect it to be stored.

Not a problem, but just a FYI to developers and testers.
Comment 93 Marcel de Rooy 2016-01-28 07:21:16 UTC
(In reply to David Cook from comment #92)
> Just a FYI for people testing out permanent uploads.
> 
> If you're using "/tmp" as the "upload_path", you might want to check if
> you're using "PrivateTmp". 

David: I would not recommend to user /tmp for permanent uploads. Note that File::Spec->tmpdir is also used for *temporary* uploads.
The default for permanent uploads in Debian packaging is /var/lib/koha/instance/uploads.
Comment 94 David Cook 2016-02-01 02:44:15 UTC
(In reply to Marcel de Rooy from comment #93)
> (In reply to David Cook from comment #92)
> > Just a FYI for people testing out permanent uploads.
> > 
> > If you're using "/tmp" as the "upload_path", you might want to check if
> > you're using "PrivateTmp". 
> 
> David: I would not recommend to user /tmp for permanent uploads. Note that
> File::Spec->tmpdir is also used for *temporary* uploads.
> The default for permanent uploads in Debian packaging is
> /var/lib/koha/instance/uploads.

That's why I said:

'Of course, you'd never actually use "/tmp" for storing a file permanently, as "/tmp" should be deleted when your server next reboots (or if some other clean-up job is running).'

Yeah, I don't use Debian packages, as I run Koha on openSUSE, and I don't think there's any default path. By default, it's just empty, so up to the user to decide where to put it. In my case, it would probably be, "instance/var/uploads".
Comment 96 Mirko Tietgen 2016-02-11 05:31:04 UTC
Tried to hide the spammer. Don't know if someone else can delete it. Account should be banned.