Bug 17501

Summary: Koha Objects for uploaded files
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, kyle, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14294
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15449, 22644, 17669, 17673    
Attachments: Bug 17501: Introduce Koha::Object[s] classes for UploadedFile(s)
Bug 17501: Use Koha::Object in Koha::Upload::_register
Bug 17501: Use Koha::Object in Koha::Upload::_lookup
Bug 17501: Use Koha::Object in Koha::Upload::_delete
Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]
Bug 17501: Remove Koha::Upload::get from Koha::Upload
Bug 17501: Move getCategories and httpheaders from Upload.pm
Bug 17501: Rename Upload to Uploader
Bug 17501: Additional polishing (POD, unit tests)
Bug 17501: Introduce Koha::Object[s] classes for UploadedFile(s)
Bug 17501: Use Koha::Object in Koha::Upload::_register
Bug 17501: Use Koha::Object in Koha::Upload::_lookup
Bug 17501: Use Koha::Object in Koha::Upload::_delete
Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]
Bug 17501: Remove Koha::Upload::get from Koha::Upload
Bug 17501: Move getCategories and httpheaders from Upload.pm
Bug 17501: Rename Upload to Uploader
Bug 17501: Additional polishing (POD, unit tests)
Bug 17501: [Follow-up] QA Requests
Bug 17501: Introduce Koha::Object[s] classes for UploadedFile(s)
Bug 17501: Use Koha::Object in Koha::Upload::_register
Bug 17501: Use Koha::Object in Koha::Upload::_lookup
Bug 17501: Use Koha::Object in Koha::Upload::_delete
Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]
Bug 17501: Remove Koha::Upload::get from Koha::Upload
Bug 17501: Move getCategories and httpheaders from Upload.pm
Bug 17501: Rename Upload to Uploader
Bug 17501: Additional polishing (POD, unit tests)
Bug 17501: [Follow-up] QA Requests

Description Marcel de Rooy 2016-10-26 13:58:01 UTC
Some code in Koha::Upload can be moved to Koha::Object(s).
Provides groundwork for further work.
Comment 1 Marcel de Rooy 2016-11-24 14:09:28 UTC
Created attachment 57751 [details] [review]
Bug 17501: Introduce Koha::Object[s] classes for UploadedFile(s)

In the next set of patches we will start using these new classes in
Koha::Upload, and scripts using it.
This is just the starting point of that migration.

Test plan:
[1] Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2016-11-24 14:09:34 UTC
Created attachment 57752 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_register

The _register routine basically inserts a new record in uploaded_files.
It should use Koha::UploadedFile now.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file via Tools/Upload.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2016-11-24 14:09:39 UTC
Created attachment 57753 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_lookup

The _lookup routine performs a few select statements on uploaded_files.
In this patch the SQL statements are replaced with Koha::Object calls.
One call of _lookup is replaced directly by Koha::UploadedFiles.

Note: _lookup can be removed in a later stage.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file in some upload category
[3] Try to upload the same file into the same category. Error?
[4] Try to upload the same file in another category. Should work.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2016-11-24 14:09:47 UTC
Created attachment 57754 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_delete

Note: This is the last occurrence where we use DBI to perform a CRUD
operation. In this case a delete from uploaded_files.

We now call Koha::UploadedFile[s]->delete to only delete the record
from the table. A next step will be moving the additional functionality
of removing the file(s) too.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Delete an upload from tools/upload.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2016-11-24 14:09:52 UTC
Created attachment 57755 [details] [review]
Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]

Since delete is not part of the upload process, we will move it now
to Koha::UploadedFile[s].
Deleting the file will be done in UploadedFile.
The (multiple) delete method in UploadedFiles refers to the single delete.

Test plan:
[1] Run t/db_dependent/Upload.t
    The warning ("but file was missing") in the last subtest is fine;
    the file did not exist. Will be addressed in a follow-up.
[2] Search for uploads on Tools/Upload. Clone this tab (repeat search on
    a new tab in your browser).
[3] Delete an existing upload on the first tab.
[4] Try to delete it again on the second tab. Error message?
[5] Bonus points:
    Add an upload. Mark the file immutable with chattr +i. Try to delete
    the file. You should see a "Could not be deleted"-message.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2016-11-24 14:09:57 UTC
Created attachment 57756 [details] [review]
Bug 17501: Remove Koha::Upload::get from Koha::Upload

The get routine actually returns records from uploaded_files. It should be
possible to replace its calls by direct calls of Koha::UploadedFiles.

This patch is the crux of this patch set. It deals with all scripts that
use Koha::Upload.

In the process we do:
[1] Add a file_handle method to Koha::UploadedFile. This was previously
    arranged via the fh parameter of get.
[2] Add a full_path method to UploadedFile. Previously returned in the
    path hash key of get. (Name is replaced by filename.)
[3] Add a search_term method too (implementing get({ term => .. }).
    This logic came from _lookup.
[4] Add a keep_file parameter to delete method. Only used in test now.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Go to Tools/Upload. Add an upload, download and delete.
[3] Add another public upload , search for it.
    Use the hashvalue to download via opac with URL:
        cgi-bin/koha/opac-retrieve-file.pl?id=[hashvalue]
[4] Go to Tools/Stage MARC for import. Import a marc file.
[5] Go to Tools/Upload local cover image. Import an image file.
    Enable OPACLocalCoverImages to see result.
[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: 2016-11-23 16: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 Circulation/Offline circulation file upload.
    Upload and click Apply directly.
    Checkout again. Repeat Offline circulation file upload.
    Now click Add to offline circulation queue.
[7] Connect the upload plugin to field 856$u.
    Enable HTML5MediaEnabled.
    Upload a webm file via the plugin. Click Choose to save the URL,
    and put 'video/webm' into 856$q. Save the biblio record.
    Check if you see the media tab with player on staff detail.
    (See also: Bug 17673 about empty OPACBaseURL.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2016-11-24 14:10:02 UTC
Created attachment 57757 [details] [review]
Bug 17501: Move getCategories and httpheaders from Upload.pm

Class method getCategories has no strict binding to Upload.pm. While
Upload.pm is now restricted to the actual uploading process with CGI
hook, this routine fits better in the UploadedFile package.

Class method httpheaders can be moved as well for the same reason. Note
that it actually is an instance method. The parameter $name is dropped.

Test plan:
[1] Run t/db_dependent/Upload.t.
[2] Check the categories in the combo box of tools/upload.
[3] Check a download via tools/upload and opac-retrieve-file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2016-11-24 14:10:07 UTC
Created attachment 57758 [details] [review]
Bug 17501: Rename Upload to Uploader

Why? Koha::Uploader now only contains the actual CGI upload. The new name
better reflects its handler status.
Pragmatically, the difference between Uploaded and Uploader makes it
easier to specifically search for them in the codebase.

Test plan:
[1] Run t/db_dependent/Upload.t.
[2] Add an upload via the interface.
[3] Check the code:
    git grep "Koha::Upload;"
    git grep "Koha::Upload\->"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2016-11-24 14:10:13 UTC
Created attachment 57759 [details] [review]
Bug 17501: Additional polishing (POD, unit tests)

This patch adds some documentation lines.
And mainly rearrangs the tests in Upload.t. The 'basic CRUD testing' is
not needed separately any more. A new test catches the "file missing"
warn.

Test plan:
[1] Run perldoc on UploadedFile[s].pm
[2] Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2016-11-24 14:15:50 UTC
Note for testers:
When you just apply all patches at once, especially follow the test plan in the sixth patch named Remove Koha::Upload::get from Koha::Upload.
Comment 11 Tomás Cohen Arazi 2017-01-05 18:39:25 UTC
Created attachment 58613 [details] [review]
Bug 17501: Introduce Koha::Object[s] classes for UploadedFile(s)

In the next set of patches we will start using these new classes in
Koha::Upload, and scripts using it.
This is just the starting point of that migration.

Test plan:
[1] Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2017-01-05 18:39:30 UTC
Created attachment 58614 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_register

The _register routine basically inserts a new record in uploaded_files.
It should use Koha::UploadedFile now.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file via Tools/Upload.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2017-01-05 18:39:36 UTC
Created attachment 58615 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_lookup

The _lookup routine performs a few select statements on uploaded_files.
In this patch the SQL statements are replaced with Koha::Object calls.
One call of _lookup is replaced directly by Koha::UploadedFiles.

Note: _lookup can be removed in a later stage.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file in some upload category
[3] Try to upload the same file into the same category. Error?
[4] Try to upload the same file in another category. Should work.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2017-01-05 18:39:41 UTC
Created attachment 58616 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_delete

Note: This is the last occurrence where we use DBI to perform a CRUD
operation. In this case a delete from uploaded_files.

We now call Koha::UploadedFile[s]->delete to only delete the record
from the table. A next step will be moving the additional functionality
of removing the file(s) too.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Delete an upload from tools/upload.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2017-01-05 18:39:47 UTC
Created attachment 58617 [details] [review]
Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]

Since delete is not part of the upload process, we will move it now
to Koha::UploadedFile[s].
Deleting the file will be done in UploadedFile.
The (multiple) delete method in UploadedFiles refers to the single delete.

Test plan:
[1] Run t/db_dependent/Upload.t
    The warning ("but file was missing") in the last subtest is fine;
    the file did not exist. Will be addressed in a follow-up.
[2] Search for uploads on Tools/Upload. Clone this tab (repeat search on
    a new tab in your browser).
[3] Delete an existing upload on the first tab.
[4] Try to delete it again on the second tab. Error message?
[5] Bonus points:
    Add an upload. Mark the file immutable with chattr +i. Try to delete
    the file. You should see a "Could not be deleted"-message.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2017-01-05 18:39:57 UTC
Created attachment 58618 [details] [review]
Bug 17501: Remove Koha::Upload::get from Koha::Upload

The get routine actually returns records from uploaded_files. It should be
possible to replace its calls by direct calls of Koha::UploadedFiles.

This patch is the crux of this patch set. It deals with all scripts that
use Koha::Upload.

In the process we do:
[1] Add a file_handle method to Koha::UploadedFile. This was previously
    arranged via the fh parameter of get.
[2] Add a full_path method to UploadedFile. Previously returned in the
    path hash key of get. (Name is replaced by filename.)
[3] Add a search_term method too (implementing get({ term => .. }).
    This logic came from _lookup.
[4] Add a keep_file parameter to delete method. Only used in test now.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Go to Tools/Upload. Add an upload, download and delete.
[3] Add another public upload , search for it.
    Use the hashvalue to download via opac with URL:
        cgi-bin/koha/opac-retrieve-file.pl?id=[hashvalue]
[4] Go to Tools/Stage MARC for import. Import a marc file.
[5] Go to Tools/Upload local cover image. Import an image file.
    Enable OPACLocalCoverImages to see result.
[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: 2016-11-23 16: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 Circulation/Offline circulation file upload.
    Upload and click Apply directly.
    Checkout again. Repeat Offline circulation file upload.
    Now click Add to offline circulation queue.
[7] Connect the upload plugin to field 856$u.
    Enable HTML5MediaEnabled.
    Upload a webm file via the plugin. Click Choose to save the URL,
    and put 'video/webm' into 856$q. Save the biblio record.
    Check if you see the media tab with player on staff detail.
    (See also: Bug 17673 about empty OPACBaseURL.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Tomás Cohen Arazi 2017-01-05 18:40:02 UTC
Created attachment 58619 [details] [review]
Bug 17501: Move getCategories and httpheaders from Upload.pm

Class method getCategories has no strict binding to Upload.pm. While
Upload.pm is now restricted to the actual uploading process with CGI
hook, this routine fits better in the UploadedFile package.

Class method httpheaders can be moved as well for the same reason. Note
that it actually is an instance method. The parameter $name is dropped.

Test plan:
[1] Run t/db_dependent/Upload.t.
[2] Check the categories in the combo box of tools/upload.
[3] Check a download via tools/upload and opac-retrieve-file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2017-01-05 18:40:11 UTC
Created attachment 58620 [details] [review]
Bug 17501: Rename Upload to Uploader

Why? Koha::Uploader now only contains the actual CGI upload. The new name
better reflects its handler status.
Pragmatically, the difference between Uploaded and Uploader makes it
easier to specifically search for them in the codebase.

Test plan:
[1] Run t/db_dependent/Upload.t.
[2] Add an upload via the interface.
[3] Check the code:
    git grep "Koha::Upload;"
    git grep "Koha::Upload\->"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2017-01-05 18:40:20 UTC
Created attachment 58621 [details] [review]
Bug 17501: Additional polishing (POD, unit tests)

This patch adds some documentation lines.
And mainly rearrangs the tests in Upload.t. The 'basic CRUD testing' is
not needed separately any more. A new test catches the "file missing"
warn.

Test plan:
[1] Run perldoc on UploadedFile[s].pm
[2] Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Marcel de Rooy 2017-01-06 07:11:50 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Great, Tomas. Thanks.
Comment 21 Jonathan Druart 2017-01-10 10:57:37 UTC
Code review:
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.
b. getCategories
 - use C4::Koha; is missing
 - The methods in itself should be moved to Koha::UploadedFiles.
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.
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.
3/ in tools/upload.pl, it's certainly better to use ->find($id) and then check if the public method.
Comment 22 Marcel de Rooy 2017-01-11 13:15:25 UTC
(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.
Comment 23 Marcel de Rooy 2017-01-11 13:15:49 UTC
Created attachment 58809 [details] [review]
Bug 17501: [Follow-up] QA Requests

This patch makes the following changes, as requested by QA:

[1] UploadedFile->delete always calls SUPER::delete.
    The return value normally comes from SUPER::delete; if removing the
    file failed, we return false. Two warns are kept.
    Since delete does no longer return the filename, a few changes were
    needed in tools/upload.pl.
[2] Method getCategories is moved to UploadedFiles. Script tools/upload.pl
    now only contains one call. Added a use C4::Koha.
[3] Calls UploadedFiles->delete as class method. As a result I removed
    method delete_errors for now; may be reconsidered on a new report.
[4] Adjusted three ->search calls for id and public to ->find calls.
[5] If you pass no id to upload.pl when deleting, you don't get an alert.

All by all, we got rid of 15 lines !

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Jonathan Druart 2017-01-20 11:40:17 UTC
Created attachment 59342 [details] [review]
Bug 17501: Introduce Koha::Object[s] classes for UploadedFile(s)

In the next set of patches we will start using these new classes in
Koha::Upload, and scripts using it.
This is just the starting point of that migration.

Test plan:
[1] Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2017-01-20 11:40:22 UTC
Created attachment 59343 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_register

The _register routine basically inserts a new record in uploaded_files.
It should use Koha::UploadedFile now.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file via Tools/Upload.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2017-01-20 11:40:27 UTC
Created attachment 59344 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_lookup

The _lookup routine performs a few select statements on uploaded_files.
In this patch the SQL statements are replaced with Koha::Object calls.
One call of _lookup is replaced directly by Koha::UploadedFiles.

Note: _lookup can be removed in a later stage.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Upload a file in some upload category
[3] Try to upload the same file into the same category. Error?
[4] Try to upload the same file in another category. Should work.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2017-01-20 11:40:31 UTC
Created attachment 59345 [details] [review]
Bug 17501: Use Koha::Object in Koha::Upload::_delete

Note: This is the last occurrence where we use DBI to perform a CRUD
operation. In this case a delete from uploaded_files.

We now call Koha::UploadedFile[s]->delete to only delete the record
from the table. A next step will be moving the additional functionality
of removing the file(s) too.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Delete an upload from tools/upload.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2017-01-20 11:40:36 UTC
Created attachment 59346 [details] [review]
Bug 17501: Move Koha::Upload::delete to Koha::UploadedFile[s]

Since delete is not part of the upload process, we will move it now
to Koha::UploadedFile[s].
Deleting the file will be done in UploadedFile.
The (multiple) delete method in UploadedFiles refers to the single delete.

Test plan:
[1] Run t/db_dependent/Upload.t
    The warning ("but file was missing") in the last subtest is fine;
    the file did not exist. Will be addressed in a follow-up.
[2] Search for uploads on Tools/Upload. Clone this tab (repeat search on
    a new tab in your browser).
[3] Delete an existing upload on the first tab.
[4] Try to delete it again on the second tab. Error message?
[5] Bonus points:
    Add an upload. Mark the file immutable with chattr +i. Try to delete
    the file. You should see a "Could not be deleted"-message.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2017-01-20 11:40:41 UTC
Created attachment 59347 [details] [review]
Bug 17501: Remove Koha::Upload::get from Koha::Upload

The get routine actually returns records from uploaded_files. It should be
possible to replace its calls by direct calls of Koha::UploadedFiles.

This patch is the crux of this patch set. It deals with all scripts that
use Koha::Upload.

In the process we do:
[1] Add a file_handle method to Koha::UploadedFile. This was previously
    arranged via the fh parameter of get.
[2] Add a full_path method to UploadedFile. Previously returned in the
    path hash key of get. (Name is replaced by filename.)
[3] Add a search_term method too (implementing get({ term => .. }).
    This logic came from _lookup.
[4] Add a keep_file parameter to delete method. Only used in test now.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Go to Tools/Upload. Add an upload, download and delete.
[3] Add another public upload , search for it.
    Use the hashvalue to download via opac with URL:
        cgi-bin/koha/opac-retrieve-file.pl?id=[hashvalue]
[4] Go to Tools/Stage MARC for import. Import a marc file.
[5] Go to Tools/Upload local cover image. Import an image file.
    Enable OPACLocalCoverImages to see result.
[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: 2016-11-23 16: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 Circulation/Offline circulation file upload.
    Upload and click Apply directly.
    Checkout again. Repeat Offline circulation file upload.
    Now click Add to offline circulation queue.
[7] Connect the upload plugin to field 856$u.
    Enable HTML5MediaEnabled.
    Upload a webm file via the plugin. Click Choose to save the URL,
    and put 'video/webm' into 856$q. Save the biblio record.
    Check if you see the media tab with player on staff detail.
    (See also: Bug 17673 about empty OPACBaseURL.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2017-01-20 11:40:45 UTC
Created attachment 59348 [details] [review]
Bug 17501: Move getCategories and httpheaders from Upload.pm

Class method getCategories has no strict binding to Upload.pm. While
Upload.pm is now restricted to the actual uploading process with CGI
hook, this routine fits better in the UploadedFile package.

Class method httpheaders can be moved as well for the same reason. Note
that it actually is an instance method. The parameter $name is dropped.

Test plan:
[1] Run t/db_dependent/Upload.t.
[2] Check the categories in the combo box of tools/upload.
[3] Check a download via tools/upload and opac-retrieve-file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2017-01-20 11:40:54 UTC
Created attachment 59349 [details] [review]
Bug 17501: Rename Upload to Uploader

Why? Koha::Uploader now only contains the actual CGI upload. The new name
better reflects its handler status.
Pragmatically, the difference between Uploaded and Uploader makes it
easier to specifically search for them in the codebase.

Test plan:
[1] Run t/db_dependent/Upload.t.
[2] Add an upload via the interface.
[3] Check the code:
    git grep "Koha::Upload;"
    git grep "Koha::Upload\->"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Jonathan Druart 2017-01-20 11:41:00 UTC
Created attachment 59350 [details] [review]
Bug 17501: Additional polishing (POD, unit tests)

This patch adds some documentation lines.
And mainly rearrangs the tests in Upload.t. The 'basic CRUD testing' is
not needed separately any more. A new test catches the "file missing"
warn.

Test plan:
[1] Run perldoc on UploadedFile[s].pm
[2] Run t/db_dependent/Upload.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 33 Jonathan Druart 2017-01-20 11:41:06 UTC
Created attachment 59351 [details] [review]
Bug 17501: [Follow-up] QA Requests

This patch makes the following changes, as requested by QA:

[1] UploadedFile->delete always calls SUPER::delete.
    The return value normally comes from SUPER::delete; if removing the
    file failed, we return false. Two warns are kept.
    Since delete does no longer return the filename, a few changes were
    needed in tools/upload.pl.
[2] Method getCategories is moved to UploadedFiles. Script tools/upload.pl
    now only contains one call. Added a use C4::Koha.
[3] Calls UploadedFiles->delete as class method. As a result I removed
    method delete_errors for now; may be reconsidered on a new report.
[4] Adjusted three ->search calls for id and public to ->find calls.
[5] If you pass no id to upload.pl when deleting, you don't get an alert.

All by all, we got rid of 15 lines !

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 34 Marcel de Rooy 2017-01-20 11:50:32 UTC
(In reply to Jonathan Druart from comment #33)
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Thanks, Jonathan
Comment 35 Kyle M Hall 2017-01-20 14:22:18 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 36 Katrin Fischer 2017-01-22 20:32:15 UTC
This won't get ported back to 16.11.x as it is an enhancement.