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.
Created attachment 41498 [details] [review] Bug 14321: Using Koha::Upload in tools/upload-file.pl This patch introduces Koha::Upload. It will replace the modules C4::UploadedFile.pm and the new C4::UploadedFiles.pm (from BZ 6874). This operation will be performed in a few steps. As a first step, we will use Koha::Upload instead of UploadedFile in the following places. (In these three places files are uploaded temporarily.) [1] stage-marc-import.pl [2] upload-cover-image.pl [3] offline_circ/process_koc.pl and enqueue_koc.pl 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. Also we do not need the jquery plugin ajaxfileupload.js anymore. A subsequent patch will remove these files. The js changes now also allow for aborting a file upload. This feature has been added to stage-marc-import only for now. 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. Test plan: [1] Upload a marc file in stage-marc-import. [2] Check new entry in table uploaded_files. Look for the file in the directory defined by upload_path in koha-conf.xml. [3] Upload another (larger) file and abort the upload. Check table and directory again. [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.
Created attachment 41499 [details] [review] Bug 14321: Add some error checking to Upload.pm This patch makes Upload return some error codes if a file cannot be read or cannot be written. Test plan: [1] Upload a file with stage-marc-import.pl. This is a temporary file; it will be saved in your system's temporary dir (e.g. /tmp) within the folder koha_upload. [2] Change the owner of this koha_upload dir and remove write permissions. [3] Upload the file again. You should get: Upload status: Failed. [4] Restore owner and permissions.
Created attachment 41500 [details] [review] Bug 14321: Add public column to uploaded_files This patch adds uploaded_files.public. This column will be used to mark uploaded files as available via opac-retrieve. The db rev sets this flag for current records having a category (as uploaded via the BZ 6874 plugin: so public). A followup patch will adjust the plugin to use Koha::Upload. Test plan: [1] Run the dbrev. [2] Add a record (upload a file) via stage-marc-import. [3] Check that public is null in the new record. (This is a temporary file without category.)
Created attachment 41501 [details] [review] Bug 14321: Redirect upload plugin to general upload script This patch makes the following changes to achieve that: [1] Adds a tools/upload script and template. It allows to upload multiple files at once. [2] Adds additional parameter to js function AjaxUpload (for category and public column). [3] Adds methods to Upload: count, delete, getCategories, httpheaders. [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] Adds decode_utf8 statement for UTF-8 chars in filenames (in the CGI hook). Note that we do not want the -utf8 flag here for binary uploads. [6] The upload plugin is converted to use tools/upload with plugin param. Deleting an upload is now presented via the search results form. NOTE: A unit test is supplied in a follow-up patch. Test plan: [1] Upload three files via tools/upload.pl with a category and marked as public. Check the results in the table. [2] Pick one new file and one of the files of step 1. Upload them in the same category. One upload should succeed. Check for reported error. [3] Upload a file via stage-marc-import. Stage it. [4] Go to Cataloguing editor. Connect upload.pl to field 856$u. 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 step 5. [5] Go back to editor. Click the tag editor on the same 856 field. Choose for Delete. Open the link in your clipboard again. [6] Check the process of upload, search, download and delete of an upload with some diacritical characters in the filename.
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.
Created attachment 41555 [details] [review] Bug 14321: Using Koha::Upload in tools/upload-file.pl This patch introduces Koha::Upload. It will replace the modules C4::UploadedFile.pm and the new C4::UploadedFiles.pm (from BZ 6874). This operation will be performed in a few steps. As a first step, we will use Koha::Upload instead of UploadedFile in the following places. (In these three places files are uploaded temporarily.) [1] stage-marc-import.pl [2] upload-cover-image.pl [3] offline_circ/process_koc.pl and enqueue_koc.pl 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. Also we do not need the jquery plugin ajaxfileupload.js anymore. A subsequent patch will remove these files. The js changes now also allow for aborting a file upload. This feature has been added to stage-marc-import only for now. 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. Test plan: [1] Upload a marc file in stage-marc-import. [2] Check new entry in table uploaded_files. Look for the file in the directory defined by upload_path in koha-conf.xml. [3] Upload another (larger) file and abort the upload. Check table and directory again. [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.
Created attachment 41556 [details] [review] Bug 14321: Add some error checking to Upload.pm This patch makes Upload return some error codes if a file cannot be read or cannot be written. Test plan: [1] Upload a file with stage-marc-import.pl. This is a temporary file; it will be saved in your system's temporary dir (e.g. /tmp) within the folder koha_upload. [2] Change the owner of this koha_upload dir and remove write permissions. [3] Upload the file again. You should get: Upload status: Failed. [4] Restore owner and permissions.
Created attachment 41557 [details] [review] Bug 14321: Add public column to uploaded_files This patch adds uploaded_files.public. This column will be used to mark uploaded files as available via opac-retrieve. The db rev sets this flag for current records having a category (as uploaded via the BZ 6874 plugin: so public). A followup patch will adjust the plugin to use Koha::Upload. Test plan: [1] Run the dbrev. [2] Add a record (upload a file) via stage-marc-import. [3] Check that public is null in the new record. (This is a temporary file without category.)
Created attachment 41558 [details] [review] Bug 14321: Redirect upload plugin to general upload script This patch makes the following changes to achieve that: [1] Adds a tools/upload script and template. It allows to upload multiple files at once. [2] Adds additional parameter to js function AjaxUpload (for category and public column). [3] Adds methods to Upload: count, delete, getCategories, httpheaders. [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] Adds decode_utf8 statement for UTF-8 chars in filenames (in the CGI hook). Note that we do not want the -utf8 flag here for binary uploads. [6] The upload plugin is converted to use tools/upload with plugin param. Deleting an upload is now presented via the search results form. NOTE: A unit test is supplied in a follow-up patch. Test plan: [1] Upload three files via tools/upload.pl with a category and marked as public. Check the results in the table. [2] Pick one new file and one of the files of step 1. Upload them in the same category. One upload should succeed. Check for reported error. [3] Upload a file via stage-marc-import. Stage it. [4] Go to Cataloguing editor. Connect upload.pl to field 856$u. 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 step 5. [5] Go back to editor. Click the tag editor on the same 856 field. Choose for Delete. Open the link in your clipboard again. [6] 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.)
Created attachment 41559 [details] [review] Bug 14321: Switch to Upload in opac-retrieve A last step to obsolete UploadedFiles.pm. Note that we now also check for the public flag. If that flag is not set, the uploaded file cannot be reached via the opac. Test plan: [1] Check downloading a file marked public in OPAC without logging in. [2] Check another file (not marked as public). Should not be possible.
Created attachment 41560 [details] [review] Bug 14321: Unit test for Koha::Upload This unit test replaces the test for UploadedFiles.pm. That one used dependency Test::CGI::Multipart. We are now mocking CGI and its hook to achieve the same result. Some tests are added to cover additional routines in Upload.pm. Test plan: Run the test!
Created attachment 41561 [details] [review] Bug 14321: Fix permissions check in upload-file.pl This fixes an error in 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. At that time we can add the script to the Tools menu too. For now, you need edit_catalogue to start upload.pl and you will additionally need a permission like upload_local_cover_images (see tools/upload-file) to successfully upload a new file. Searching for files and copying URLs to the editor can still be done with edit_catalogue. Test plan: [1] Pick a staff user that only has catalogue and edit_catalogue. [2] Without this patch, you can upload a file on tools/upload.pl. [3] Apply this patch. [4] Retry. Should not work: Upload status is Denied. [5] Login with enough permissions. Upload again.
Created attachment 41562 [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.
At the end, this patch set actually reduces code. Another bug will take care of menu and permission.
Created attachment 41665 [details] [review] Bug 14321: Switch to Upload in opac-retrieve A last step to obsolete UploadedFiles.pm. Note that we now also check for the public flag. If that flag is not set, the uploaded file cannot be reached via the opac. This patch also the encode step to tools/upload for the http header that could include a filename with special chars (just as in opac-retrieve). Test plan: [1] Check downloading a file marked public in OPAC without logging in. If possible, include some special chars in the filename. [2] Check another file (not marked as public). Should not be possible.
Created attachment 41666 [details] [review] Bug 14321: Unit test for Koha::Upload This unit test replaces the test for UploadedFiles.pm. That one used dependency Test::CGI::Multipart. We are now mocking CGI and its hook to achieve the same result. Some tests are added to cover additional routines in Upload.pm. Test plan: Run the test!
Created attachment 41667 [details] [review] Bug 14321: Fix permissions check in upload-file.pl This fixes an error in 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. At that time we can add the script to the Tools menu too. For now, you need edit_catalogue to start upload.pl and you will additionally need a permission like upload_local_cover_images (see tools/upload-file) to successfully upload a new file. Searching for files and copying URLs to the editor can still be done with edit_catalogue. Test plan: [1] Pick a staff user that only has catalogue and edit_catalogue. [2] Without this patch, you can upload a file on tools/upload.pl. [3] Apply this patch. [4] Retry. Should not work: Upload status is Denied. [5] Login with enough permissions. Upload again.
Created attachment 41668 [details] [review] Bug 14321: Just some documentation lines added or fixed Test plan: Actually, you cannot really test this. But you could run the unit test again.
Created attachment 41669 [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.
Small consistency adjustment in patch 5 and new patch 8 for some pod lines in Upload.pm.
Created attachment 41677 [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.
Rebased last patch for change on dependency patches.
Created attachment 41873 [details] [review] Bug 14321: Using Koha::Upload in tools/upload-file.pl This patch introduces Koha::Upload. It will replace the modules C4::UploadedFile.pm and the new C4::UploadedFiles.pm (from BZ 6874). This operation will be performed in a few steps. As a first step, we will use Koha::Upload instead of UploadedFile in the following places. (In these three places files are uploaded temporarily.) [1] stage-marc-import.pl [2] upload-cover-image.pl [3] offline_circ/process_koc.pl and enqueue_koc.pl 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. Also we do not need the jquery plugin ajaxfileupload.js anymore. A subsequent patch will remove these files. The js changes now also allow for aborting a file upload. This feature has been added to stage-marc-import only for now. 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 2: The changes to upload-file and upload-file-progress in respect to replacing the indirect syntax for CGI::Cookie come from bug 14589. Test plan: [1] Upload a marc file in stage-marc-import. [2] Check new entry in table uploaded_files. Look for the file in the directory defined by upload_path in koha-conf.xml. [3] Upload another (larger) file and abort the upload. Check table and directory again. [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.
Created attachment 41874 [details] [review] Bug 14321: Add some error checking to Upload.pm This patch makes Upload return some error codes if a file cannot be read or cannot be written. Test plan: [1] Upload a file with stage-marc-import.pl. This is a temporary file; it will be saved in your system's temporary dir (e.g. /tmp) within the folder koha_upload. [2] Change the owner of this koha_upload dir and remove write permissions. [3] Upload the file again. You should get: Upload status: Failed. [4] Restore owner and permissions.
Created attachment 41875 [details] [review] Bug 14321: Add public column to uploaded_files This patch adds uploaded_files.public. This column will be used to mark uploaded files as available via opac-retrieve. The db rev sets this flag for current records having a category (as uploaded via the BZ 6874 plugin: so public). A followup patch will adjust the plugin to use Koha::Upload. Test plan: [1] Run the dbrev. [2] Add a record (upload a file) via stage-marc-import. [3] Check that public is null in the new record. (This is a temporary file without category.)
Created attachment 41876 [details] [review] Bug 14321: Redirect upload plugin to general upload script This patch makes the following changes to achieve that: [1] Adds a tools/upload script and template. It allows to upload multiple files at once. [2] Adds additional parameter to js function AjaxUpload (for category and public column). [3] Adds methods to Upload: count, delete, getCategories, httpheaders. [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] Adds decode_utf8 statement for UTF-8 chars in filenames (in the CGI hook). Note that we do not want the -utf8 flag here for binary uploads. [6] The upload plugin is converted to use tools/upload with plugin param. Deleting an upload is now presented via the search results form. NOTE: A unit test is supplied in a follow-up patch. Test plan: [1] Upload three files via tools/upload.pl with a category and marked as public. Check the results in the table. [2] Pick one new file and one of the files of step 1. Upload them in the same category. One upload should succeed. Check for reported error. [3] Upload a file via stage-marc-import. Stage it. [4] Go to Cataloguing editor. Connect upload.pl to field 856$u. 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 step 5. [5] Go back to editor. Click the tag editor on the same 856 field. Choose for Delete. Open the link in your clipboard again. [6] 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.)
Created attachment 41877 [details] [review] Bug 14321: Switch to Upload in opac-retrieve A last step to obsolete UploadedFiles.pm. Note that we now also check for the public flag. If that flag is not set, the uploaded file cannot be reached via the opac. This patch also the encode step to tools/upload for the http header that could include a filename with special chars (just as in opac-retrieve). Test plan: [1] Check downloading a file marked public in OPAC without logging in. If possible, include some special chars in the filename. [2] Check another file (not marked as public). Should not be possible.
Created attachment 41878 [details] [review] Bug 14321: Unit test for Koha::Upload This unit test replaces the test for UploadedFiles.pm. That one used dependency Test::CGI::Multipart. We are now mocking CGI and its hook to achieve the same result. Some tests are added to cover additional routines in Upload.pm. Test plan: Run the test!
Created attachment 41879 [details] [review] Bug 14321: Fix permissions check in upload-file.pl This fixes an error in 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. At that time we can add the script to the Tools menu too. For now, you need edit_catalogue to start upload.pl and you will additionally need a permission like upload_local_cover_images (see tools/upload-file) to successfully upload a new file. Searching for files and copying URLs to the editor can still be done with edit_catalogue. Test plan: [1] Pick a staff user that only has catalogue and edit_catalogue. [2] Without this patch, you can upload a file on tools/upload.pl. [3] Apply this patch. [4] Retry. Should not work: Upload status is Denied. [5] Login with enough permissions. Upload again.
Created attachment 41880 [details] [review] Bug 14321: Just some documentation lines added or fixed Test plan: Actually, you cannot really test this. But you could run the unit test again.
Created attachment 41881 [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.
Rebased, included small change from bug 14589 for upload-file and upload-file-progress. No dependencies anymore.
CHECK: No results while files have been uploaded permanently? Test by drojf.
Rearranging..
Created attachment 42411 [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.
Created attachment 42412 [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.
Created attachment 42413 [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.
Created attachment 42414 [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.
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
Created attachment 42558 [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>
Created attachment 42559 [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>
Created attachment 42560 [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>
Created attachment 42561 [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>
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.
(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 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 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.
Julian, could I get your signoff on this one please?
Created attachment 42584 [details] [review] Bug 14321: QA Fix Remove QA errors (keys on reference is experimental).
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.
> ::: 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.
(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 [...]
Created attachment 42587 [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>
Created attachment 42588 [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] 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.
Created attachment 42589 [details] [review] 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>
(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..
Created attachment 42590 [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.
Created attachment 42591 [details] [review] 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>
I have asked Julian to have a look, to confirm there is no regression on bug 6874.
Created attachment 42660 [details] [review] Bug 14321: Add two columns to uploaded_files Signed off
Created attachment 42661 [details] [review] Bug 14321: Introduce Koha::Upload Signed off
Created attachment 42662 [details] [review] Bug 14321: Integrate Upload.pm into Koha Signed off
Created attachment 42663 [details] [review] Bug 14321: Final cleanup, removing obsolete files Signed off
Created attachment 42664 [details] [review] Bug 14321: QA Fix Signed off
Created attachment 42665 [details] [review] Bug 14321: [QA Follow-up] Changes for Upload.pm and upload-file.pl Signed off
Created attachment 42666 [details] [review] Bug 14321: [QA Follow-up] Perltidy upload.pl Signed off
(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.
(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
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.
(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!
(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.
Created attachment 42707 [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.
(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.
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>
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>
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>
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>
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>
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>
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>
I have squashed Bug 14321: Integrate Upload.pm into Koha and Bug 14321: [QA Follow-up] Perltidy upload.pl
(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.
Created attachment 42889 [details] [review] Bug 14321: (RM followup) DBIx update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 42890 [details] [review] Bug 14321: (QA followup) remove Test::CGI::Multipart dependency Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Patches pushed to master. Thanks Marcel!
(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.
(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.
(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.
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?
(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..
Ah, thanks! I checked the blocks/depends on bugs, but managed to overlook the see also ones.
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.
(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.
(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".
Tried to hide the spammer. Don't know if someone else can delete it. Account should be banned.