Bug 6874 - Attach files to bibliographic records
Summary: Attach files to bibliographic records
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P3 new feature
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 5010
Blocks: 14944 6274 14321
  Show dependency treegraph
 
Reported: 2011-09-16 10:29 UTC by Julian Maurice
Modified: 2016-06-21 21:40 UTC (History)
22 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Patch (34.20 KB, patch)
2011-09-16 10:32 UTC, Julian Maurice
Details | Diff | Splinter Review
Patch (34.22 KB, patch)
2011-09-16 10:34 UTC, Julian Maurice
Details | Diff | Splinter Review
Patch (36.00 KB, patch)
2011-10-07 10:20 UTC, Julian Maurice
Details | Diff | Splinter Review
Reviewed patch (38.77 KB, patch)
2012-01-12 17:10 UTC, Julian Maurice
Details | Diff | Splinter Review
File upload in MARC (29.21 KB, patch)
2012-03-02 12:44 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.30 KB, patch)
2012-03-16 15:49 UTC, Julian Maurice
Details | Diff | Splinter Review
File upload in MARC (29.36 KB, patch)
2012-03-22 08:39 UTC, Julian Maurice
Details | Diff | Splinter Review
File upload in MARC (28.77 KB, patch)
2012-07-06 09:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (28.87 KB, patch)
2012-07-06 14:28 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.23 KB, patch)
2012-07-16 04:14 UTC, Mason James
Details | Diff | Splinter Review
Bug 6874: Follow-up: fix filename encoding (794 bytes, patch)
2012-07-18 14:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: Follow-up: fix filename encoding and sanitization (1016 bytes, patch)
2012-07-24 08:19 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: Allow to reupload a file immediately after deletion (1.88 KB, patch)
2012-08-16 09:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (28.88 KB, patch)
2012-09-19 15:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: Follow-up: fix filename encoding and sanitization (1009 bytes, patch)
2012-09-19 15:13 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: Allow to reupload a file immediately after deletion (1.88 KB, patch)
2012-09-19 15:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: Change filename construction method. (2.79 KB, patch)
2012-09-19 15:14 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.17 KB, patch)
2012-12-14 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6874: Follow-up: fix filename encoding and sanitization (1.04 KB, patch)
2012-12-14 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6874: Allow to reupload a file immediately after deletion (1.92 KB, patch)
2012-12-14 15:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6874: Change filename construction method. (2.84 KB, patch)
2012-12-14 15:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6874: Follow-up: fix filename encoding and sanitization (1.09 KB, patch)
2013-03-19 09:49 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6874: Follow-up: fix filename encoding and sanitization (1.09 KB, patch)
2013-03-19 09:56 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6874: Allow to reupload a file immediately after deletion (1.98 KB, patch)
2013-03-19 09:56 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6874: Change filename construction method. (2.90 KB, patch)
2013-03-19 09:56 UTC, Paul Poulain
Details | Diff | Splinter Review
QA follow-up return with explicit undef removed (690 bytes, patch)
2013-03-19 09:56 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6874: Follow-up for PBP fixes (1.40 KB, patch)
2013-03-19 11:26 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: File upload in MARC (28.88 KB, patch)
2013-03-21 22:52 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: Follow-up: fix filename encoding and sanitization (1.16 KB, patch)
2013-03-21 22:52 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: Allow to reupload a file immediately after deletion (2.05 KB, patch)
2013-03-21 22:52 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: Change filename construction method. (2.97 KB, patch)
2013-03-21 22:52 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] QA follow-up return with explicit undef removed (759 bytes, patch)
2013-03-21 22:52 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: Follow-up for PBP fixes (1.47 KB, patch)
2013-03-21 22:52 UTC, Paul Poulain
Details | Diff | Splinter Review
bug 6874 follow-up fix POD syntax (1.35 KB, patch)
2013-03-21 22:52 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: File upload in MARC (29.04 KB, patch)
2013-03-22 08:39 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: Follow-up: fix filename encoding and sanitization (1.22 KB, patch)
2013-03-22 08:39 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: Allow to reupload a file immediately after deletion (2.10 KB, patch)
2013-03-22 08:39 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: Change filename construction method. (3.02 KB, patch)
2013-03-22 08:40 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] QA follow-up return with explicit undef removed (816 bytes, patch)
2013-03-22 08:40 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874: Follow-up for PBP fixes (1.52 KB, patch)
2013-03-22 08:40 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] bug 6874 follow-up fix POD syntax (1.41 KB, patch)
2013-03-22 08:40 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED QA] Bug 6874 follow-up jquery has moved (1.27 KB, patch)
2013-03-22 08:40 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (3.22 KB, patch)
2013-04-11 12:38 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: Add dependency to Test::CGI::Multipart (781 bytes, patch)
2013-04-11 12:43 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (28.90 KB, patch)
2013-06-08 01:43 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: Follow-up: fix filename encoding and sanitization (1.16 KB, patch)
2013-06-08 01:43 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: Allow to reupload a file immediately after deletion (2.04 KB, patch)
2013-06-08 01:43 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: Change filename construction method. (2.96 KB, patch)
2013-06-08 01:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: QA follow-up return with explicit undef removed (712 bytes, patch)
2013-06-08 01:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: Follow-up for PBP fixes (1.52 KB, patch)
2013-06-08 01:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
bug 6874 follow-up fix POD syntax (1.46 KB, patch)
2013-06-08 01:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874 follow-up jquery has moved (1.33 KB, patch)
2013-06-08 01:44 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (3.29 KB, patch)
2013-06-08 01:45 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: Add dependency to Test::CGI::Multipart (825 bytes, patch)
2013-06-08 01:45 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6874: Add dependency to Test::CGI::Multipart (971 bytes, patch)
2013-06-08 01:49 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: File upload in MARC (29.39 KB, patch)
2013-06-12 13:14 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: QA follow-up (3.94 KB, patch)
2013-06-12 13:14 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles (3.94 KB, patch)
2013-06-12 13:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (11.16 KB, patch)
2013-06-12 13:15 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml (11.34 KB, patch)
2013-08-02 15:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.10 KB, patch)
2013-08-02 15:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Reconstructed attempt for the upload plugin feature. (45.09 KB, patch)
2013-09-04 15:33 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: File upload in MARC (29.55 KB, patch)
2013-09-25 07:08 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: QA follow-up (3.95 KB, patch)
2013-09-25 07:09 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles (3.94 KB, patch)
2013-09-25 07:09 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml (10.28 KB, patch)
2013-09-25 07:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.16 KB, patch)
2013-09-25 07:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874 - Patch kohastructure.sql issues follow up (1.16 KB, patch)
2013-09-26 15:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Focus and jquery.js fix (2.12 KB, patch)
2013-09-28 13:31 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql & jquery.js fix with refocus (3.12 KB, patch)
2013-09-28 16:16 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.21 KB, patch)
2013-09-30 07:38 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (7.73 KB, patch)
2013-10-03 20:40 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (7.66 KB, patch)
2013-10-03 20:42 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874 - File upload in MARC - QA Followup (6.15 KB, patch)
2013-10-03 23:21 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874 - File upload in MARC - QA Followup (6.15 KB, patch)
2013-10-03 23:24 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: Force user to select a directory (3.26 KB, patch)
2013-10-03 23:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (7.66 KB, patch)
2013-10-03 23:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.65 KB, patch)
2013-12-16 23:47 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: File upload in MARC (29.67 KB, patch)
2013-12-16 23:49 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: QA follow-up (3.80 KB, patch)
2013-12-16 23:54 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: Add unit tests for C4::UploadedFiles (3.96 KB, patch)
2013-12-16 23:59 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: File upload in MARC (29.72 KB, patch)
2013-12-17 20:32 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: QA follow-up (3.85 KB, patch)
2013-12-17 20:32 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: Add unit tests for C4::UploadedFiles (4.04 KB, patch)
2013-12-17 20:33 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml (10.35 KB, patch)
2013-12-17 20:33 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874 - File upload in MARC - QA Followup (6.15 KB, patch)
2013-12-17 20:34 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: Force user to select a directory (3.26 KB, patch)
2013-12-17 20:34 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.65 KB, patch)
2013-12-17 20:34 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: File upload in MARC (29.70 KB, patch)
2014-03-17 18:24 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: QA follow-up (3.84 KB, patch)
2014-03-17 18:24 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2014-03-17 18:24 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml (10.33 KB, patch)
2014-03-17 18:25 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874 - File upload in MARC - QA Followup (6.14 KB, patch)
2014-03-17 18:25 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 6874: Force user to select a directory (3.25 KB, patch)
2014-03-17 18:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.65 KB, patch)
2014-03-17 18:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.67 KB, patch)
2014-04-07 19:42 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.84 KB, patch)
2014-04-07 19:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2014-04-07 19:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.33 KB, patch)
2014-04-07 19:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.14 KB, patch)
2014-04-07 19:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2014-04-07 19:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.65 KB, patch)
2014-04-07 19:43 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.73 KB, patch)
2014-04-15 03:57 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Fix file URL (1.03 KB, patch)
2014-04-15 11:20 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.51 KB, patch)
2014-04-16 04:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.60 KB, patch)
2014-04-16 04:34 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (958 bytes, patch)
2014-04-17 00:52 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.99 KB, patch)
2014-07-31 04:44 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.84 KB, patch)
2014-07-31 04:44 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2014-07-31 04:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.62 KB, patch)
2014-07-31 04:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.13 KB, patch)
2014-07-31 04:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2014-07-31 04:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.65 KB, patch)
2014-07-31 04:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.73 KB, patch)
2014-07-31 04:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.60 KB, patch)
2014-07-31 04:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (958 bytes, patch)
2014-07-31 04:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.95 KB, patch)
2014-10-16 04:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.65 KB, patch)
2014-10-16 04:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2014-10-16 04:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.63 KB, patch)
2014-10-16 04:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.13 KB, patch)
2014-10-16 04:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2014-10-16 04:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.65 KB, patch)
2014-10-16 04:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.73 KB, patch)
2014-10-16 04:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.32 KB, patch)
2014-10-16 04:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (958 bytes, patch)
2014-10-16 04:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.98 KB, patch)
2015-02-26 20:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-02-26 20:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-02-26 20:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (9.69 KB, patch)
2015-02-26 20:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.98 KB, patch)
2015-02-26 20:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-02-26 20:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-02-26 20:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (9.69 KB, patch)
2015-02-26 20:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.20 KB, patch)
2015-02-26 20:25 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2015-02-26 20:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.45 KB, patch)
2015-02-26 20:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.72 KB, patch)
2015-02-26 20:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.63 KB, patch)
2015-02-26 20:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (953 bytes, patch)
2015-02-26 20:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.98 KB, patch)
2015-02-26 22:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-02-26 22:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-02-26 22:46 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.58 KB, patch)
2015-02-26 22:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.20 KB, patch)
2015-02-26 22:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2015-02-26 22:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.45 KB, patch)
2015-02-26 22:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.72 KB, patch)
2015-02-26 22:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.63 KB, patch)
2015-02-26 22:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (953 bytes, patch)
2015-02-26 22:47 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (29.97 KB, patch)
2015-02-27 00:04 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-02-27 00:04 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-02-27 00:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.57 KB, patch)
2015-02-27 00:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.20 KB, patch)
2015-02-27 00:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2015-02-27 00:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.45 KB, patch)
2015-02-27 00:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.72 KB, patch)
2015-02-27 00:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.34 KB, patch)
2015-02-27 00:05 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (953 bytes, patch)
2015-02-27 00:06 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (30.20 KB, patch)
2015-04-12 01:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-04-12 01:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-04-12 01:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.58 KB, patch)
2015-04-12 01:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (30.20 KB, patch)
2015-04-12 01:34 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-04-12 01:35 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-04-12 01:35 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.58 KB, patch)
2015-04-12 01:35 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.20 KB, patch)
2015-04-12 01:35 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2015-04-12 01:35 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.45 KB, patch)
2015-04-12 01:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.72 KB, patch)
2015-04-12 01:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.34 KB, patch)
2015-04-12 01:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (953 bytes, patch)
2015-04-12 01:36 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (30.09 KB, patch)
2015-04-12 04:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-04-12 04:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-04-12 04:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.58 KB, patch)
2015-04-12 04:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.20 KB, patch)
2015-04-12 04:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2015-04-12 04:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.45 KB, patch)
2015-04-12 04:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.72 KB, patch)
2015-04-12 04:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.34 KB, patch)
2015-04-12 04:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (953 bytes, patch)
2015-04-12 04:04 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Catch warnings for clean tests. (4.73 KB, patch)
2015-04-12 04:04 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (30.09 KB, patch)
2015-05-06 21:12 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-05-06 21:12 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.07 KB, patch)
2015-05-06 21:12 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.58 KB, patch)
2015-05-06 21:13 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.20 KB, patch)
2015-05-06 21:13 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2015-05-06 21:13 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.45 KB, patch)
2015-05-06 21:13 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.72 KB, patch)
2015-05-06 21:13 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.34 KB, patch)
2015-05-06 21:13 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (953 bytes, patch)
2015-05-06 21:14 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Catch warnings for clean tests. (4.73 KB, patch)
2015-05-06 21:14 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (30.09 KB, patch)
2015-05-06 21:19 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-05-06 21:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-05-06 21:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.58 KB, patch)
2015-05-06 21:20 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (30.09 KB, patch)
2015-05-06 21:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.68 KB, patch)
2015-05-06 21:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.03 KB, patch)
2015-05-06 21:21 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.58 KB, patch)
2015-05-06 21:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.20 KB, patch)
2015-05-06 21:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.25 KB, patch)
2015-05-06 21:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.45 KB, patch)
2015-05-06 21:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.72 KB, patch)
2015-05-06 21:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.34 KB, patch)
2015-05-06 21:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (953 bytes, patch)
2015-05-06 21:22 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6874: Catch warnings for clean tests. (4.73 KB, patch)
2015-05-06 21:23 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: File upload in MARC (30.18 KB, patch)
2015-05-13 13:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: QA follow-up (3.73 KB, patch)
2015-05-13 13:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles (4.10 KB, patch)
2015-05-13 13:12 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml (10.67 KB, patch)
2015-05-13 13:13 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874 - File upload in MARC - QA Followup (6.21 KB, patch)
2015-05-13 13:13 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: Force user to select a directory (3.32 KB, patch)
2015-05-13 13:13 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.53 KB, patch)
2015-05-13 13:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874 - License Text Update (5.80 KB, patch)
2015-05-13 13:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874 - Clean up file URL generation (3.39 KB, patch)
2015-05-13 13:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874 - id based on contents, filename, and directory. (1.01 KB, patch)
2015-05-13 13:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6874: Catch warnings for clean tests. (5.09 KB, patch)
2015-05-13 13:15 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (24.25 KB, patch)
2015-05-28 12:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.84 KB, patch)
2015-05-28 12:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.19 KB, patch)
2015-05-28 12:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.75 KB, patch)
2015-05-28 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.36 KB, patch)
2015-05-28 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.41 KB, patch)
2015-05-28 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.62 KB, patch)
2015-05-28 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.88 KB, patch)
2015-05-28 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.50 KB, patch)
2015-05-28 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (1.09 KB, patch)
2015-05-28 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: Catch warnings for clean tests. (5.17 KB, patch)
2015-05-28 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: [QA Follow-up] Adjustments for opac-retrieve (3.12 KB, patch)
2015-05-28 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: [QA Follow-up] File test, plugin style, finddirs (7.63 KB, patch)
2015-05-28 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: File upload in MARC (24.25 KB, patch)
2015-07-31 17:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: QA follow-up (3.15 KB, patch)
2015-07-31 17:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: Add unit tests for C4::UploadedFiles (4.19 KB, patch)
2015-07-31 17:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: Move uploadPath syspref to koha-conf.xml (10.75 KB, patch)
2015-07-31 17:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - File upload in MARC - QA Followup (6.36 KB, patch)
2015-07-31 17:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: Force user to select a directory (3.41 KB, patch)
2015-07-31 17:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more (14.62 KB, patch)
2015-07-31 17:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - License Text Update (5.88 KB, patch)
2015-07-31 17:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - Clean up file URL generation (3.50 KB, patch)
2015-07-31 17:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874 - id based on contents, filename, and directory. (1.09 KB, patch)
2015-07-31 17:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: Catch warnings for clean tests. (5.17 KB, patch)
2015-07-31 17:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: [QA Follow-up] Adjustments for opac-retrieve (3.12 KB, patch)
2015-07-31 17:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: [QA Follow-up] Last adjustments for future developments (18.92 KB, patch)
2015-07-31 17:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: [QA Follow-up] Last adjustments for future developments (18.94 KB, patch)
2015-07-31 17:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6874: (QA followup) Warnings in about page (2.88 KB, patch)
2015-08-11 14:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 6874: (QA followup) upload_path missing in koha-conf.xml.in (990 bytes, patch)
2015-08-11 15:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 6874: (RM followup) DBIx updates (2.59 KB, patch)
2015-08-11 15:00 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 6874: (QA followup) fix templates (1.54 KB, patch)
2015-08-21 12:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2011-09-16 10:29:29 UTC
Add a plugin for uploading a file and link to it in MARC.
Comment 1 Julian Maurice 2011-09-16 10:32:12 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2011-09-16 10:34:46 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2011-09-16 10:42:17 UTC
To test:

- Create the directory where uploaded files will go
    mkdir koha-tmpl/uploads
- Set the 2 sysprefs
    uploadPath: "KOHA_ROOT/koha-tmpl/uploads"
    uploadWebPath: "/uploads"
- Set the MARC framework to use "upload.pl" plugin for 856$u
- Modify an existing biblio and use the plugin on this field
- Save the modified biblio, verify that the url is well displayed at catalogue/detail.pl and try to download the file.
Comment 4 Julian Maurice 2011-09-16 10:43:14 UTC
P.S.: Don't forget to give rights on upload directory
Comment 5 Sébastien Marie 2011-09-16 13:55:29 UTC
Hi,

Just some remarks...

1. Template name
The uploaded file for template are:
 - upload.tt
 - upload_delete_file.tt

'cataloguing/value_builder/upload.pl' use:
  my $template_name = $result || $delete ? "upload_delete_file.tmpl" : "upload.tmpl";

So template extensions are bad (in perl script, I think).



2. A security consideration
in 'cataloguing/value_builder/upload.pl', in the sub "plugin", the template_name could be constructed with untrusted user parameter ($result).



3. A second security consideration (near same as above)
in 'cataloguing/value_builder/upload.pl', in the sub "plugin", when a file is requested for deletion, the code is:

> unlink("$upload_path/$result")

please consider that result come from untrusted input: $input->param('result'). So if $result contains something like "../../../" any file owned by http server could be deleted...


4. And a ore general security consideration (sorry, professional deformation :-) )

About permit uploading arbitrary file... how the filename is treated ? specially the extension ? Be careful with filename like 'test.pl' on a http server that served perl code (like Koha): this could result arbitrary command execution. (same problem for all server-side executable script configured on the server, like PHP, perl, python, cgi, SSI, ...). 

The minimal security is to advertise the administrator who want enable this preference to have special http server configuration for the "upload_path" (in order to disable all script execution, SSI, or others, and don't use user configuration in the directory [like .htaccess])


5. Template remark for modified files (like MARCdetail.tt)

The code added is generally like:

[% IF ( subfiel.is_url ) %]
  <a href="[% subfiel.marc_value %]">[% subfiel.marc_value |html %]</a>
[% ELSIF (subfiel.is_url ) %]
  [% subfiel.marc_value %]
[% ELSE %]
  [% subfiel.marc_value |html %]
[% END %]

Why a second ELSIF with the same condition than before ? (subfiel.is_url)


That's all for now !
Thanks.
Comment 6 Robin Sheat 2011-09-18 23:28:17 UTC
A couple of things:
How will this handle installations that have multiple instances? 

I'm quite wary of allowing files to be uploaded into a webserver accessible directory, it seems there's too much that can go wrong unless you're really careful (whitelisting extensions, etc.) My preferences would be an out-of-server-tree location, and download requests go through a program that does the actual transfer.

The upload.pl has no copyright header, or useful perldoc. "plugin_parameters : other parameters added when the plugin is called by the dopop function" is meaningless.
Comment 7 Julian Maurice 2011-10-07 10:20:25 UTC Comment hidden (obsolete)
Comment 8 Robin Sheat 2011-10-09 22:32:58 UTC
Multiple instances is when you're running more than one koha instance on a server. 

Having the files accessible directly through apache strikes me as problematic. Adding an alias doesn't solve that really, and I don't think chmod 0644 will either. For a simple example, what if someone uploads a PHP file and you have mod_php enabled?

The patch has a lot of 'warn's in it, looks like debug code.

Most of the functions are undocumented, which is bad.

Putting HTML/Javascript in .pl files is something that should really really be avoided, it's violating separation of concerns (I know that other parts of Koha have that, that upsets me too :) It should be pushed into a template.

I note that you search through to find a free filename. I can't tell what the base name will be (or perhaps it's provided by the uploader), but that a) risks a (very unlikely) race condition, and b) could get slow if there are many files. Perhaps include a timestamp as well?

If the OPAC is set so that it requires a valid login to access, then this will still allow those files to be accessible, which is bad.
Comment 9 Paul Poulain 2011-10-13 13:16:11 UTC
(In reply to comment #8)
> Multiple instances is when you're running more than one koha instance on a
> server. 
> 
> Having the files accessible directly through apache strikes me as problematic.
> Adding an alias doesn't solve that really, and I don't think chmod 0644 will
> either. For a simple example, what if someone uploads a PHP file and you have
> mod_php enabled?
can't it be solved just by an apache config (like "noexec" or something like that ?)

> The patch has a lot of 'warn's in it, looks like debug code.

Agreed, Julian, please update unconditional warns to "$debug and warn"

> Most of the functions are undocumented, which is bad.
If you're speaking of upload.pl, I fully agree, Julian, please add some documentation to this script.


> Putting HTML/Javascript in .pl files is something that should really really be
> avoided, it's violating separation of concerns (I know that other parts of Koha
> have that, that upsets me too :) It should be pushed into a template.

If you speak of upload.pl, it's how it's made for all plugins and I don't think this should be changed for this one.
I agree we should improve the cataloguing plugin system (& probably completely rewrite the cataloguing tool in fact), but that's outside the scope of this patch. For consistency reason I think upload.pl should continue to have this structure & embeed code & html & javascript.

> I note that you search through to find a free filename. I can't tell what the
> base name will be (or perhaps it's provided by the uploader), but that a) risks
> a (very unlikely) race condition, and b) could get slow if there are many
> files. Perhaps include a timestamp as well?
I don't understand what you mean here, so won't say anything.

> If the OPAC is set so that it requires a valid login to access, then this will
> still allow those files to be accessible, which is bad.
True, but how could we avoid this ? should we obfuscate all files URL by providing a pass-through perl script? Something like :
<a href="/cgi-bin/koha/opac-retrieve.pl?filename=blabla">Download file</a>
and opac-retrieve.pl:
1- check for Opac requiring login
2- send the file

Note that is would also solve your php uploaded concern right ?

(Julian : if Robin confirms, ask stephane_n for what he did for Electre, it's that kind of thing)
Comment 10 Robin Sheat 2011-10-13 22:06:06 UTC
* I don't think expecting people to configure apache to do that is reasonable, many of them have enough trouble with the basic stuff.

* Yeah, I saw how the other plugins worked after I wrote that, so I think that's OK (but should probably be fixed in the longer term.)

* Having a pass-through script is ideal. I'd be inclined to have it be like:
opac-retrieve.pl?id=abc123
where abc123 is a random string, or perhaps a hash of the file. This will prevent people enumerating ID numbers, and can reduce the chance of collisions compared to using a filename (also, if you use a hash, then uploading the same file multiple times will mean that only one version needs to be stored.) In the longer term, it also allows it to have fancier permissions or what not.

And, it would remove the risk of executable files being uploaded. I think it would also allow multiple Koha instances to share one file store, if it used hashes, as they wouldn't be guessable.
Comment 11 Paul Poulain 2011-10-24 11:38:09 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 12 Paul Poulain 2011-10-25 15:05:42 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 13 claire.hernandez@biblibre.com 2011-12-02 13:41:33 UTC
$ git-bz apply 6874
Bug 6874 - File upload in MARC

Patch
Apply? [yn] y

Applying: Bug 6874: Adds the ability to upload a file for a subfield
/home/claire/dev/community_repo/koha/.git/rebase-apply/patch:329: trailing whitespace.
    return $string; 
/home/claire/dev/community_repo/koha/.git/rebase-apply/patch:555: trailing whitespace.
    <script type="text/javascript" src="[% yuipath %]/utilities/utilities.js"></script> 
/home/claire/dev/community_repo/koha/.git/rebase-apply/patch:564: trailing whitespace.
            var doc   = opener.document; 
/home/claire/dev/community_repo/koha/.git/rebase-apply/patch:611: trailing whitespace.
    <script type="text/javascript" src="[% yuipath %]/utilities/utilities.js"></script> 
/home/claire/dev/community_repo/koha/.git/rebase-apply/patch:620: trailing whitespace.
            var doc   = opener.document; 
error: patch failed: catalogue/detail.pl:104
error: catalogue/detail.pl: patch does not apply
error: patch failed: installer/data/mysql/sysprefs.sql:318
error: installer/data/mysql/sysprefs.sql: patch does not apply
error: patch failed: kohaversion.pl:16
error: kohaversion.pl: patch does not apply
error: patch failed: opac/opac-detail.pl:223
error: opac/opac-detail.pl: patch does not apply
Patch failed at 0001 Bug 6874: Adds the ability to upload a file for a subfield
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/Patch-69WGpm.patch
Comment 14 claire.hernandez@biblibre.com 2011-12-02 14:44:15 UTC
Forget it, this patch have conflicts better message with 3 way merge in git bz.

$ git bz apply 6874
Bug 6874 - File upload in MARC

Patch
Apply? [yn] y

Applying: Bug 6874: Adds the ability to upload a file for a subfield
Using index info to reconstruct a base tree...
<stdin>:329: trailing whitespace.
    return $string; 
<stdin>:555: trailing whitespace.
    <script type="text/javascript" src="[% yuipath %]/utilities/utilities.js"></script> 
<stdin>:564: trailing whitespace.
            var doc   = opener.document; 
<stdin>:611: trailing whitespace.
    <script type="text/javascript" src="[% yuipath %]/utilities/utilities.js"></script> 
<stdin>:620: trailing whitespace.
            var doc   = opener.document; 
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging C4/Biblio.pm
Auto-merging catalogue/detail.pl
CONFLICT (content): Merge conflict in catalogue/detail.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging kohaversion.pl
CONFLICT (content): Merge conflict in kohaversion.pl
Auto-merging opac/opac-detail.pl
CONFLICT (content): Merge conflict in opac/opac-detail.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 6874: Adds the ability to upload a file for a subfield
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/Patch-ExUoch.patch


(In reply to comment #13)
Comment 15 Julian Maurice 2012-01-12 17:10:10 UTC Comment hidden (obsolete)
Comment 16 Robin Sheat 2012-02-01 00:29:08 UTC
That looks good, my only quibble is about the use of MD5. There's no real reason to use it any more, and while it's not a particularly critical situation, it should probably still be avoided. See also: http://marc-stevens.nl/research/md5-1block-collision/
Comment 17 Jared Camins-Esakov 2012-02-07 15:45:13 UTC
I've tested this, and encountered a number of problems.

The following issues cause the patch to fail QA:
1. The syspref does not appear in the syspref interface.
2. There's no explanation of what the syspref should be set to.
3. This patch adds C4::UploadedFiles (notice the 's') with no indication of how it relates to C4::UploadedFile (notice the lack of the 's'). It's possible there is a legitimate reason for this, but there is absolutely no documentation of the new class.

The following issues do not cause the patch to fail QA, but are worth noting:
4. Uploaded files can only be accessed when the display is set to normal mode. They do not work with XSLT mode. Perhaps the uploader could be modified to insert the entire URL so that it would work with XSLT displays too?
5. The patch does not rebase cleanly due to changes in the various installer files. You might as well fix this when you address issues 1-3 above, so that the next sign-off-er doesn't need to resolve the conflicts again.
6. The patch should not modify kohaversion.pl (again, easy to fix at sign off, but you might as well do it while fixing the above).
7. Instructions to activate the upload.pl plugin for 856 (or whatever) are not displayed on update.

In terms of functionality in normal mode, when the 856$u is linked to the catalog.pl plugin, this is *very* cool.
Comment 18 Julian Maurice 2012-03-02 12:44:40 UTC Comment hidden (obsolete)
Comment 19 Sébastien Marie 2012-03-02 16:07:27 UTC
Hi,
I only do a "visual review" of the code, but just some suggestions...

In "C4/UploadedFiles.pm"
 - line 147:
>    if( -f $file_path ) {
>        warn "Id $id not in database, but present in filesystem, do nothing";
>        return;
>    }

Use "-e" instead of "-f": the difference is when $file_path is an existing link. -f say "not a file", but -e "a node exists" (may be a directory, a plain file, a socket, a pipe, a special block device, a symbolic link...)

 - line 153:
>    unless(open $out_fh, '>', $file_path) {
>        warn "Failed to open file '$file_path': $!";
>        return;
>    }

a race condition is possible between the check of file existence and the open of file. You could use sysopen function instead of file-check + open.

> use IO::Handle;
> unless( sysopen($out_fh, $path, O_WRONLY|O_CREAT|O_EXCL) ) {
>   warn "Failed to open file '$file_path': $!";
>   return;
> }

note: the file is created only if don't exist, due to O_CREAT|O_EXCL. There is not time between check and open because all is done by system kernel.


In ".../cataloguing/value_builder/upload.tt" (and others templates):
 please use "html" filter. It is (generally) always to use it, a prevent XSS (here only "uploaded_file" seems to be build with user-input)

> The file [% uploaded_file | html %] has been successfully uploaded.



In the syspref message, perhaps it should be added that the uploadPath MUST NOT be a public directory, accessible from the webserver (because else, the server could be exposed to arbitrary code execution by evil uploader).


Else, the patch seems promising.
Thanks
Comment 20 Jared Camins-Esakov 2012-03-10 18:28:11 UTC
Marking failed QA because of the security issues noted by Frere Marie. Once those have been resolved, I look forward to testing this.
Comment 21 Julian Maurice 2012-03-16 15:49:49 UTC Comment hidden (obsolete)
Comment 22 Jared Camins-Esakov 2012-03-17 01:37:37 UTC
Julian,

I don't think this works right. I have uploadPath set to /tmp, and I get the following result: http://screencast.com/t/x9ihzYSpWC

Uploading doesn't seem to work to any directory. Am I missing something? Last time I tested it, the uploading worked, and there weren't all those radio boxes.

Regards,
Jared
Comment 23 Pierre Angot 2012-03-20 10:58:24 UTC
Hello,

We need more documentation about using syspref and directories (which one ? relative ? absolute ?).
Comment 24 Julian Maurice 2012-03-22 08:15:29 UTC
Jared, this hierarchy of radio buttons allow you to choose in which sub-directory you want to save your file. It normally displays only directories under uploadPath and where it have permission to write. So you should only have subdirectories of /tmp.
I will take a look at this.

Pierre, you must enter the absolute path to directory you want to save files. This can be any directory you want, but for security reasons it should not be reachable by webserver (so a directory outside of Koha source tree should be ok)
Comment 25 Julian Maurice 2012-03-22 08:39:33 UTC Comment hidden (obsolete)
Comment 26 Matthias Meusburger 2012-06-07 10:56:01 UTC
I've tried to reproduce the bug described by Jared as well, but it did work correctly for me. Using /tmp, I had (root) and the only www-data writable directory (and it's subdirectories). Non www-data writable directories were hidden.
Comment 27 Julian Maurice 2012-07-06 09:03:35 UTC Comment hidden (obsolete)
Comment 28 Jared Camins-Esakov 2012-07-06 14:28:27 UTC Comment hidden (obsolete)
Comment 29 Jared Camins-Esakov 2012-07-06 14:31:31 UTC
The problem I reported earlier seemed to have something to do with setting the syspref *prior* to the revised patches. I tested it on an installation which had not had the syspref set previously, and it worked great.
Comment 30 Mason James 2012-07-16 04:14:52 UTC Comment hidden (obsolete)
Comment 31 Mason James 2012-07-16 04:24:52 UTC
(In reply to comment #29)
> The problem I reported earlier seemed to have something to do with setting
> the syspref *prior* to the revised patches. I tested it on an installation
> which had not had the syspref set previously, and it worked great.


QA comment, passing QA...
all perl-critic, perl syntax, and tt-template tests pass OK

$ koha-qa.pl
starting perlcritic-progressive tests... OK
starting 'perl -c' syntax tests... OK
starting xt/tt_valid.t tests... OK
starting xt/author/vaild-template.t tests... OK

rebased patch on 60b6c187c46614431f109512c740a3bcf077f342
Comment 32 Paul Poulain 2012-07-18 09:15:48 UTC
I think i've found a problem with this patch.
When testing, I uploaded a file, then tried to display it, I got:
not found (my URL was: /cgi-bin/koha/opac-retrieve-file.pl?id=796851af7a586fad6a19883c8ad714fa116ee9ed)

Investigating in /tmp/bz6874 I saw what can be the origin of the bug:
-rw-rw-r-- 1 paul paul 41461 juil. 18 11:11 Capture d'écran - 06072012 - 14:04:07.png

I uploaded a file called "Capture d'écran - 06072012 - 14:04:07.png"
=> the encoding on the filesystem is wrong, that's probably why it's failing...

You should either fix the encoding or use Text::Unaccent to remove accents

sorry, failed QA
Comment 33 Julian Maurice 2012-07-18 14:51:58 UTC Comment hidden (obsolete)
Comment 34 Sébastien Marie 2012-07-22 04:41:32 UTC
I mark the bug "Failed QA" for a security problem (or perhaps two).

in C4/UploadedFiles.pm, sub UploadFile, the regex for sanitize "../" is invalid.

> if($filename =~ m#/(^|/)\.\.(/|$)# or $dir =~ m#(^|/)\.\.(/|$)#) {

for $filename, a "/" was inserted at the beginning... so a filename like "../foo" is ok. That line should be corrected like:

> if($filename =~ m#(^|/)\.\.(/|$)# or $dir =~ m#(^|/)\.\.(/|$)#) { 


And a second (possible) problem is using
> $filename = decode_utf8($filename);
*after* the previous check.

I think it may be possible to pass the "../" check and with decode_utf8 obtain a "../". To be sure, it should be investigated, but it should be ok to have the "decode_utf8" *before* the "../" check.
Comment 35 Julian Maurice 2012-07-24 08:19:42 UTC Comment hidden (obsolete)
Comment 36 Jared Camins-Esakov 2012-08-11 11:57:34 UTC
The follow up does not fix filename encoding. When I try to upload a file with a diacritic in its name, the name is not changed. Also, as an aside, if you use the plugin after the field has already been linked to an uploaded file, you are asked if you want to delete the file, which is good, but -- after you delete the file -- you are not redirected to upload a new file. You have to close the window and reopen it by click on the plugin link.
Comment 37 Julian Maurice 2012-08-16 09:22:50 UTC
The follow-up does not intend to remove accents but to fix encoding. If the name of your uploaded file is the same as the original file, and does not contain "é"-like characters, it's ok. To be sure, you may try to download it.

I'm going to submit a patch for the 2nd point.
Comment 38 Julian Maurice 2012-08-16 09:23:28 UTC Comment hidden (obsolete)
Comment 39 Jared Camins-Esakov 2012-08-27 17:21:11 UTC
(In reply to comment #37)
> The follow-up does not intend to remove accents but to fix encoding. If the
> name of your uploaded file is the same as the original file, and does not
> contain "é"-like characters, it's ok. To be sure, you may try to download
> it.

I don't understand this- I thought that the filenames were supposed to be hashes of some sort?
Comment 40 Julian Maurice 2012-09-14 14:09:48 UTC
At the end, filenames are just filenames. I think this is needed if you want to access uploaded files 'manually' (not with Koha).
Comment 41 Owen Leonard 2012-09-14 15:11:12 UTC
This does not apply anymore.
Comment 42 Jared Camins-Esakov 2012-09-14 15:13:15 UTC
(In reply to comment #40)
> At the end, filenames are just filenames. I think this is needed if you want
> to access uploaded files 'manually' (not with Koha).

I thought we switched to using SHA1 as identifier to avoid clobbering files with the same name?
Comment 43 Julian Maurice 2012-09-19 15:13:34 UTC Comment hidden (obsolete)
Comment 44 Julian Maurice 2012-09-19 15:13:50 UTC Comment hidden (obsolete)
Comment 45 Julian Maurice 2012-09-19 15:14:05 UTC Comment hidden (obsolete)
Comment 46 Julian Maurice 2012-09-19 15:14:14 UTC Comment hidden (obsolete)
Comment 47 Julian Maurice 2012-09-19 15:18:14 UTC
Jared, I finally choose to use both SHA-1 and name for filename. So we have the advantages of both sides, I think.
Comment 48 Kyle M Hall 2012-12-14 15:09:24 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2012-12-14 15:09:38 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2012-12-14 15:09:48 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2012-12-14 15:10:01 UTC Comment hidden (obsolete)
Comment 52 Paul Poulain 2013-03-19 09:49:34 UTC Comment hidden (obsolete)
Comment 53 Paul Poulain 2013-03-19 09:56:29 UTC Comment hidden (obsolete)
Comment 54 Paul Poulain 2013-03-19 09:56:34 UTC Comment hidden (obsolete)
Comment 55 Paul Poulain 2013-03-19 09:56:39 UTC Comment hidden (obsolete)
Comment 56 Paul Poulain 2013-03-19 09:56:43 UTC Comment hidden (obsolete)
Comment 57 Paul Poulain 2013-03-19 09:58:53 UTC
QA comment: sorry, another failed QA, because of koha-qa/perlcritic complaining:

 * opac/opac-retrieve-file.pl                                               FAIL
	critic                      FAIL
		Bareword file handle opened at line 35, column 5. See pages 202,204 of PBP.
 * cataloguing/value_builder/upload.pl                                      FAIL
	critic                      FAIL
		Glob written as <...> at line 143, column 17. See page 167 of PBP.


However, i've uploaded rebased patches with a 3rd perlcritic error fixed, I let you fix those 2.

Once follow-up uploaded, switch back to signed off
Comment 58 Julian Maurice 2013-03-19 11:26:03 UTC Comment hidden (obsolete)
Comment 59 Paul Poulain 2013-03-21 22:52:19 UTC Comment hidden (obsolete)
Comment 60 Paul Poulain 2013-03-21 22:52:27 UTC Comment hidden (obsolete)
Comment 61 Paul Poulain 2013-03-21 22:52:33 UTC Comment hidden (obsolete)
Comment 62 Paul Poulain 2013-03-21 22:52:39 UTC Comment hidden (obsolete)
Comment 63 Paul Poulain 2013-03-21 22:52:48 UTC Comment hidden (obsolete)
Comment 64 Paul Poulain 2013-03-21 22:52:54 UTC Comment hidden (obsolete)
Comment 65 Paul Poulain 2013-03-21 22:52:59 UTC Comment hidden (obsolete)
Comment 66 Katrin Fischer 2013-03-21 22:59:57 UTC
Paul, isn't it kinda late where you are? ;)
Comment 67 Paul Poulain 2013-03-21 23:11:21 UTC
(In reply to comment #66)
> Paul, isn't it kinda late where you are? ;)

yep, but I cannot sleep, so I do some QA ;-)

QA comment: I've added a follow-up to fix POD, it now passes koha-qa.pl

QUESTION : trying to use the feature, I think I made something wrong or there's a problem.
Here is what I do:
 * I've defined the UploadPath syspref to /tmp/bz6874 & created the directory
 * I've attached "upload.pl" plugin to 856$u
 * I've opened and edited an existing biblio
 * I click on the ... (on the right of 856)
 * I select a file, and put it in root of UploadPath
 * Koha says "The file logo2.jpg has been successfully uploaded."
 * I click "close"
 * i'm back to cataloguing, but the 856$u is empty

shouldn't it contain the file I just uploaded ?
Comment 68 Paul Poulain 2013-03-22 08:39:32 UTC Comment hidden (obsolete)
Comment 69 Paul Poulain 2013-03-22 08:39:44 UTC Comment hidden (obsolete)
Comment 70 Paul Poulain 2013-03-22 08:39:53 UTC Comment hidden (obsolete)
Comment 71 Paul Poulain 2013-03-22 08:40:00 UTC Comment hidden (obsolete)
Comment 72 Paul Poulain 2013-03-22 08:40:11 UTC Comment hidden (obsolete)
Comment 73 Paul Poulain 2013-03-22 08:40:18 UTC Comment hidden (obsolete)
Comment 74 Paul Poulain 2013-03-22 08:40:26 UTC Comment hidden (obsolete)
Comment 75 Paul Poulain 2013-03-22 08:40:35 UTC Comment hidden (obsolete)
Comment 76 Paul Poulain 2013-03-22 08:41:53 UTC
QA comment (continued)
 * the problem was caused by the move of the jquery library. Fixing the jquery path in http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16719 solves the problem, works well now

passed QA !
Comment 77 Jared Camins-Esakov 2013-03-23 02:48:44 UTC
This feature is lacking unit tests for the new C4::UploadedFiles, and at first glance does not look Plack-safe:

my $upload_path = C4::Context->preference('uploadPath');

is at a file level, but $upload_path is used in subroutines.
Comment 78 Paul Poulain 2013-03-29 09:40:00 UTC
(In reply to comment #77)
> and at
> first glance does not look Plack-safe:
> my $upload_path = C4::Context->preference('uploadPath');
> is at a file level, but $upload_path is used in subroutines.

I QAed it using Plack, and it worked well.
Comment 79 Jared Camins-Esakov 2013-03-29 11:22:01 UTC
(In reply to comment #78)
> (In reply to comment #77)
> > and at
> > first glance does not look Plack-safe:
> > my $upload_path = C4::Context->preference('uploadPath');
> > is at a file level, but $upload_path is used in subroutines.
> 
> I QAed it using Plack, and it worked well.

That's good. The QA failure was due to the missing unit tests.
Comment 80 Julian Maurice 2013-04-11 12:38:39 UTC Comment hidden (obsolete)
Comment 81 Julian Maurice 2013-04-11 12:43:31 UTC Comment hidden (obsolete)
Comment 82 Julian Maurice 2013-04-29 13:40:30 UTC
Oops. Forgot to reset status.
Comment 83 Jared Camins-Esakov 2013-06-08 01:43:33 UTC Comment hidden (obsolete)
Comment 84 Jared Camins-Esakov 2013-06-08 01:43:48 UTC Comment hidden (obsolete)
Comment 85 Jared Camins-Esakov 2013-06-08 01:43:59 UTC Comment hidden (obsolete)
Comment 86 Jared Camins-Esakov 2013-06-08 01:44:08 UTC Comment hidden (obsolete)
Comment 87 Jared Camins-Esakov 2013-06-08 01:44:19 UTC Comment hidden (obsolete)
Comment 88 Jared Camins-Esakov 2013-06-08 01:44:28 UTC Comment hidden (obsolete)
Comment 89 Jared Camins-Esakov 2013-06-08 01:44:43 UTC Comment hidden (obsolete)
Comment 90 Jared Camins-Esakov 2013-06-08 01:44:53 UTC Comment hidden (obsolete)
Comment 91 Jared Camins-Esakov 2013-06-08 01:45:03 UTC Comment hidden (obsolete)
Comment 92 Jared Camins-Esakov 2013-06-08 01:45:13 UTC Comment hidden (obsolete)
Comment 93 Jared Camins-Esakov 2013-06-08 01:49:08 UTC Comment hidden (obsolete)
Comment 94 Robin Sheat 2013-06-09 22:46:48 UTC
(In reply to comment #83)
> New cataloging plugin upload.pl and new system preference 'uploadPath'.
...
> uploadPath is the absolute path on the server where the files will be
> stored. It's possible to have a hierarchy of directories under this path

This should not be a system preference. It should be something that's set in the koha-conf.xml. Library staff should not be modifying it, as they can't ensure that directories are set up with the right permissions. 

If you're running a hosted setup without permission separation, you could upload into someone else's koha space (probably not terrible, but not ideal.)

Finally, it's hard to do an automated configuration of a system preference, and it's easy to do one for koha-conf.xml.
Comment 95 Julian Maurice 2013-06-10 09:43:44 UTC
> This should not be a system preference. It should be something that's set in
> the koha-conf.xml. Library staff should not be modifying it, as they can't
> ensure that directories are set up with the right permissions. 
This is the same for all other system preferences that contain a path. So why
this one is different?
Moreover, I don't think a preference for a cataloguing plugin should be in
koha-conf.xml (its place is not in global system preferences either but there is
no other place)
I'm not opposed to move this pref in koha-conf.xml, I just want to understand :)
Also I would like to have someone else's opinion before writing a patch.
Comment 96 Paul Poulain 2013-06-10 10:00:48 UTC
(In reply to comment #95)
> > This should not be a system preference. It should be something that's set in
> > the koha-conf.xml. Library staff should not be modifying it, as they can't
> > ensure that directories are set up with the right permissions. 
> This is the same for all other system preferences that contain a path. So why
> this one is different?
At 1st glance, I had the same reaction. But there is a difference : writing ! Other systempreferences only require reading (opacusercss for example), this one will write, and there is no other syspref in this case I think.

> Moreover, I don't think a preference for a cataloguing plugin should be in
> koha-conf.xml (its place is not in global system preferences either but
> there is no other place)
I hesitate... what I think is that this preference don't need to be setup during installation, because it's an additional feature that most libraries won't use. 
(like for ldap for example : there are default values, but the koha-conf.xml must be edited manually if you want to enable it)

> I'm not opposed to move this pref in koha-conf.xml, I just want to
> understand :)
> Also I would like to have someone else's opinion before writing a patch.

my conclusion would be:
 * move the uploadPath to koha-conf.xml
 * add <uploadPath><uploadPath> (without any value) to default koha-conf
 * add a disclaimer on the plugin page if uploadPath is empty, saying something like "WARNING : your server is not properly setup, it won't work. uploadPath must be set in koha configuration file"

(wait for another feedback before writing the patch)

PS: will this patch make it's way one day ? :\
Comment 97 Robin Sheat 2013-06-10 22:25:12 UTC
(In reply to comment #95)
> This is the same for all other system preferences that contain a path. So why
> this one is different?

Most of those are URL paths and such like. The only exception I can think of (though there are probably others) is the XSLT, and that is designed to be user editable.

> Moreover, I don't think a preference for a cataloguing plugin should be in
> koha-conf.xml (its place is not in global system preferences either but
> there is
> no other place)

Well, it's a preference for where your things get uploaded on the server, it's not something that's likely to change during normal configuration I think. And it's pointless being exposed to library staff as they're typically not sysadmins.
Comment 98 Julian Maurice 2013-06-12 13:14:30 UTC Comment hidden (obsolete)
Comment 99 Julian Maurice 2013-06-12 13:14:56 UTC Comment hidden (obsolete)
Comment 100 Julian Maurice 2013-06-12 13:15:20 UTC Comment hidden (obsolete)
Comment 101 Julian Maurice 2013-06-12 13:15:29 UTC Comment hidden (obsolete)
Comment 102 Julian Maurice 2013-06-12 13:20:10 UTC
The last patch introduces a subroutine in t/lib/Mocks/Context.pm that is also introduced by Bug 10298. But there are quite similar and resolving the conflicts will be very easy.
Comment 103 Kyle M Hall 2013-08-02 15:28:09 UTC Comment hidden (obsolete)
Comment 104 Kyle M Hall 2013-08-02 15:44:46 UTC Comment hidden (obsolete)
Comment 105 Jared Camins-Esakov 2013-08-22 13:50:55 UTC
Unfortunately the fourth patch, "Move uploadPath syspref to koha-conf.xml" will not apply at all because of missing sha1 information for sysprefs.sql. Could you please rebase this patchset and reupload?
Comment 106 M. Tompsett 2013-08-22 14:04:33 UTC
I'm attempting to backport to 3.12.x
Comment 107 M. Tompsett 2013-08-23 23:38:54 UTC
Okay, I accidentally messed up and got what I think is this working for master. However, I have some concerns.

misc/strip_value_from_tag.pl
The patches containing this were obsoleted (see first 5 ones, I think). Is this module not useful?

t/lib/Mocks/Context.pm
This exists in part in the non-obsoleted patches ([SIGNED-OFF] Bug 6874: Move uploadPath syspref to koha-conf.xml), but I think something substituting for t::lib::Mocks would be sufficient based on my reading of the code. 3-5 lines of code are missing in the patch, so I can't reconstruct what I think is the original file. I hunted through all the obsoleted patches too.

I would very much prefer the original developer/author to rebase their patch, but am willing to provide what I have reconstructed. I know that my colleagues have asked for this patch, and so I'll make sure to back port to 3.12.x too.
Comment 108 M. Tompsett 2013-09-04 15:33:15 UTC Comment hidden (obsolete)
Comment 109 M. Tompsett 2013-09-04 15:36:15 UTC
As far as I can tell:
- configure the koha-conf.xml with the appropriate values
- make sure the directories exist and have the correct permissions
 <pluginsdir>/home/mtompset/koha-dev/var/lib/plugins</pluginsdir>
 <enable_plugins>1</enable_plugins>
 <upload_path>/home/mtompset/kohaclone/koha-tmpl/uploads</upload_path>
- link 856$u to the upload.pl plugin.
- test.
Comment 110 Jared Camins-Esakov 2013-09-04 15:39:36 UTC
(In reply to M. Tompsett from comment #109)
> As far as I can tell:
> - configure the koha-conf.xml with the appropriate values
> - make sure the directories exist and have the correct permissions
>  <pluginsdir>/home/mtompset/koha-dev/var/lib/plugins</pluginsdir>
>  <enable_plugins>1</enable_plugins>

Please provide an updated test plan that reflects your new dependency on Koha's plugin system.
Comment 111 Julian Maurice 2013-09-25 07:08:54 UTC Comment hidden (obsolete)
Comment 112 Julian Maurice 2013-09-25 07:09:18 UTC Comment hidden (obsolete)
Comment 113 Julian Maurice 2013-09-25 07:09:38 UTC Comment hidden (obsolete)
Comment 114 Julian Maurice 2013-09-25 07:09:57 UTC Comment hidden (obsolete)
Comment 115 Julian Maurice 2013-09-25 07:10:14 UTC Comment hidden (obsolete)
Comment 116 Julian Maurice 2013-09-25 07:11:57 UTC
(In reply to M. Tompsett from comment #107)
> I would very much prefer the original developer/author to rebase their
> patch

Done ;-)
Comment 117 M. Tompsett 2013-09-25 18:41:26 UTC
Comment on attachment 20779 [details] [review]
Reconstructed attempt for the upload plugin feature.

Original author rebased patches. No need for my intermediate attempt.
Comment 118 M. Tompsett 2013-09-26 01:56:43 UTC
There is a missing semi-colon in the first attachment for the kohastructure.sql
Comment 119 M. Tompsett 2013-09-26 02:59:23 UTC
YIKES!

" ) ENGINE=InnoDB DEFAULT CHARSET=utf8;"

missing from above the uploaded_files table in kohastructure.sql
Comment 120 M. Tompsett 2013-09-26 03:13:22 UTC
Comment on attachment 21432 [details] [review]
[SIGNED-OFF] Bug 6874: File upload in MARC

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

kohastructure.sql - missing ; on the DROP and the ") ENGINE=..." line was hijacked by the new table, but not replaced. This was obvious when I was attempting to install a UNIMARC installation to hopefully trigger cases 6, 6a, and 6b in C4::Biblio.
Comment 121 M. Tompsett 2013-09-26 04:19:08 UTC
Comment on attachment 21436 [details] [review]
Bug 6874 - File upload in MARC - QA Followup

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt
@@ +4,4 @@
>  <head>
>      <title>Upload plugin</title>
>      <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
> +    <script type="text/javascript" src="[% interface %]/lib/jquery/jquery.js"></script>

This is correct here, but the upload_delete_file.tt needs the same change.
Comment 122 M. Tompsett 2013-09-26 04:19:20 UTC
Comment on attachment 21432 [details] [review]
[SIGNED-OFF] Bug 6874: File upload in MARC

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

::: installer/data/mysql/kohastructure.sql
@@ +3202,5 @@
> +--
> +-- Table structure for table uploaded_files
> +--
> +
> +DROP TABLE IF EXISTS uploaded_files

This line is missing a semi-colon.

@@ +3211,1 @@
>  ) ENGINE=InnoDB DEFAULT CHARSET=utf8;

This line should be copied to 3201.

::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload_delete_file.tt
@@ +3,5 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +<head>
> +    <title>Upload plugin</title>
> +    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
> +    <script type="text/javascript" src="[% themelang %]/lib/jquery/jquery.js"></script>

This should be [% interface %] not, [% themelang %]
Comment 123 M. Tompsett 2013-09-26 04:21:09 UTC
$ giz bz apply 6874

Had to fix up installer/data/mysql/updatedatabase.pl
Had to install new dependency for this.

$ koha_perl_deps.pl -m -u

Listed new requirement (hence revised PerlDependencies.pm checked)

$ sudo apt-get install libtest-cgi-multipart-perl

Did the database upgrade by starting staff client.

$ prove -v t/db_dependent/UploadedFiles.t

everything passed.

$ ~/qa-test-tools/koha-qa.pl -v 2 -c 10

(I forgot how many patches were applied, so I just said 10)

No problems with this set of patches.

Noticed POD updates unrelated to patches (C4::Biblio)
Probably required to get C4::Biblio to pass QA tool tests.

New files include:
C4/UploadedFiles.pm
cataloguing/value_builder/upload.pl
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload_delete_file.tt
opac/opac-retrieve-file.pl
t/db_dependent/UploadedFiles.t

Did a opac-detail view, which means the "use C4::Biblio" isn't broken.

There are 1,2,3,4,5,6,6a,6b,7,8,9 changes in C4::Biblio

$ perldoc C4::Biblio

This triggers 2,7,8,9

Refresh of opac-detail page triggers 3,4

Home -> administration -> Marc frameworks -> BKS framework structure -> tag 856 subfield structure -> edit subfields contraint -> advanced -> plugin = upload.pl

Attempted to use plug in without configuring koha-conf.xml

Correctly gave feedback.

$ mkdir ~/uploads
$ sudo chown www-data.www-data ~/uploads

add in <upload_path>/home/mtompset/uploads</upload_path>

Link a file into some biblio entry.

Go to opac detail for it:
http://demo.library.kohasystem.ca/cgi-bin/koha/opac-detail.pl?biblionumber=14206

Triggers 1,3,4,5

In order to trigger 6, 6a, and 6b, GetMarcUrls needs to be called with something other than MARC21. I couldn't figure out how to do that.

I'm attempting a UNIMARC installation.

I used https://www.dropbox.com/s/idw98d7jiy5e5xd/dump_unimarc.zip
as mentioned by Mathieu Saby in his August 20th email "pending bugs about UNIMARC marc flavor".

Linked upload.pl
uploaded a dummy file for a known biblio record.
went to opac and did an opac detail view of it. (6, 6a)
unlinked upload.pl
refreshed opac detail page (6, 6b)

testing to continue later.
Comment 124 Katrin Fischer 2013-09-26 09:25:12 UTC
Hi Mark, could you describe a bit what you tested on the pages? The numbers make your test plan quite cryptic to read.
Comment 125 M. Tompsett 2013-09-26 12:21:03 UTC
Comment on attachment 21432 [details] [review]
[SIGNED-OFF] Bug 6874: File upload in MARC

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

::: C4/Biblio.pm
@@ +37,4 @@
>  use C4::Charset;
>  use C4::Linker;
>  use C4::OAI::Sets;
> +use C4::UploadedFiles;

This is C4::Biblio Change 1.

@@ +1908,4 @@
>  
>  =head2 GetMarcUrls
>  
> +  $marcurls = GetMarcUrls($record,$marcflavour,$frameworkcode);

This is C4::Biblio Change 2.

@@ +1919,5 @@
> +    my ( $record, $marcflavour, $frameworkcode ) = @_;
> +
> +    my $tagslib = &GetMarcStructure(1, $frameworkcode);
> +    my $urltag = '856';
> +    my $urlsubtag = 'u';

This block is C4::Biblio Change 3.

@@ +1924,3 @@
>  
>      my @marcurls;
> +    for my $field ( $record->field($urltag) ) {

This is C4::Biblio Change 4.

@@ +1927,5 @@
>          my @notes;
>          for my $note ( $field->subfield('z') ) {
>              push @notes, { note => $note };
>          }
> +        my @urls = $field->subfield($urlsubtag);

This is C4::Biblio Change 5.

@@ +1956,3 @@
>                  $marcurl->{'part'} = $s3 if ($link);
>                  $marcurl->{'toc'} = 1 if ( defined($s3) && $s3 =~ /^[Tt]able/ );
>              } else {

This begins C4::Biblio Change 6.

@@ +1956,5 @@
>                  $marcurl->{'part'} = $s3 if ($link);
>                  $marcurl->{'toc'} = 1 if ( defined($s3) && $s3 =~ /^[Tt]able/ );
>              } else {
> +                if ($tagslib->{ $urltag }->{ $urlsubtag }->{value_builder} eq "upload.pl"
> +                  and $url =~ /id=([0-9a-f]+)/) {

Inside this IF is C4::Biblio Change 6a.

@@ +1963,5 @@
> +                    $marcurl->{'linktext'} = $field->subfield('2')
> +                                          || C4::Context->preference('URLLinkText')
> +                                          || $file->{filename};
> +                    $marcurl->{'MARCURL'} = $url;
> +                } else {

Inside this ELSE is C4::Biblio Change 6b.
Comment 126 M. Tompsett 2013-09-26 12:24:52 UTC
Comment on attachment 21433 [details] [review]
[SIGNED-OFF] Bug 6874: QA follow-up

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

::: C4/Biblio.pm
@@ +1446,4 @@
>  =head2 GetMarcPrice
>  
>  return the prices in accordance with the Marc format.
> +

I ignored white space while labeling changes.

@@ +3395,5 @@
>  =head2 CountBiblioInOrders
>  
> +=over
> +
> +=item $count = &CountBiblioInOrders( $biblionumber);

This is C4::Biblio change 7.

@@ +3419,5 @@
>  =head2 GetSubscriptionsId
>  
> +=over
> +
> +=item $subscriptions = &GetSubscriptionsId($biblionumber);

This is C4::Biblio change 8.

@@ +3443,5 @@
>  =head2 GetHolds
>  
> +=over
> +
> +=item $holds = &GetHolds($biblionumber);

This is C4::Biblio change 9.
Comment 127 M. Tompsett 2013-09-26 12:26:08 UTC
I hope that clarifies the complete testing of the C4::Biblio changes that I have done.
Comment 128 Katrin Fischer 2013-09-26 12:49:06 UTC
Hi Mark,
actually I was more thinking about some 'non-programmer' notes about testing the actual functionality :)
Comment 129 M. Tompsett 2013-09-26 15:35:28 UTC
Fear not. :) I'll be getting to less programmer oriented testing instructions. The programmer oriented instructions are a matter of proving that the code changes introduced do not cause any problems by triggering all the code points. Now, back to testing...
Comment 130 M. Tompsett 2013-09-26 15:47:47 UTC Comment hidden (obsolete)
Comment 131 M. Tompsett 2013-09-26 18:33:54 UTC
Set up VM from scratch (include qa-test-tools, git-bz [fishsoup branch], koha origin/master branch)
$ git checkout -b bug_6874 origin/master
$ git bz apply 6874
$ perl Makefile.PL
$ make
$ make test
$ make install
Remember to set up some useful data after you get a working Koha. :)

Configure patch
- edit koha-conf.xml and make sure to fill in <upload_path></upload_path>
  (e.g. <upload_path>/home/mtompset/uploads</upload_path>)
- make sure to give that directory correct permissions
  (e.g. sudo chown www-data.www-data /home/mtompset/uploads)
- attach the plug-in to the 856$u subfield
  (e.g. - log in to staff client
        - click 'Koha administration'
        - click 'MARC bibliographic framework'
        - click 'MARC structure' for a framework
        - Find the 856 tag
        - click 'Subfields'
        - Find the 'u' subfield
        - click 'Edit'
        - click 'Other options (choose one)'
        - select 'upload.pl' in the Plugin drop-down
        - click 'Save changes')

You will want to test in both MARC21 and UNIMARC systems.

$ ./koha_perl_deps -m -u
$ perldoc C4::Biblio
$ perldoc C4::UploadedFiles

This will trigger all the functions in C4::UploadedFiles
$ prove -v t/db_dependent/UploadedFiles.t

Once you have a bilio with 856$u linked up to a file, go to the same biblio on the OPAC. (e.g. http://demo.library.kohasystem.ca/cgi-bin/koha/opac-detail.pl?biblionumber=14206)
Then unlink the upload.pl plugin and refresh the OPAC.
This latter step is necessary to test a secondary code point under UNIMARC.

In the staff client, find a biblio.
Add it to your cart.
Click the cart at the top of the page.

This will test opac-retrieve-file.pl fully:
While at the OPAC detail page for the item added
- Click the 'Online resources:' URL.
1) It should work.
- Delete the file from the filesystem.
  (e.g. sudo rm /home/mtompset/uploads/*)
- Click the 'Online resources:' URL.
2) It should fail.
- Open up MySQL and delete the record from the uploaded_files table.
3) It should fail.
- Make sure to clear the 856$u entry for that biblio manually and save.

opac-detail.pl change is tested just by viewing the details of the biblio with an 856$u link.

opac-basket.pl change is tested by adding the item to your cart and clicking the "Cart" button.

To test the template files, it is a matter of doing a lot of upload, delete, upload, whatever type tests in the staff client. This has not been fully tested, but there are some issues that I want to test to the point of being able to say what is wrong and how I was expecting it to be handled.

I'm beginning to run short of testing time today again. However, I am pretty confident that I have tested the following thoroughly:
C4/Biblio.pm
C4/Installer/PerlDependencies.pm
C4/UploadedFiles.pm
basket/basket.pl
etc/koha-conf.xml
installer/data/mysql/kohastructure.sql
installer/data/mysql/updatedatabase.pl
opac/opac-basket.pl
opac/opac-detail.pl
opac/opac-retrieve-file.pl
t/db_dependent/UploadedFiles.t

Patches with updatedatabase.pl in them need to be rebased.
I have already attached an extra patch to correct issues in kohastructure.sql

I will continue my testing later of:
catalogue/MARCdetail.pl
catalogue/detail.pl
cataloguing/value_builder/upload.pl
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload_delete_file.tt
Comment 132 M. Tompsett 2013-09-27 16:53:29 UTC
initial state: blank, no records in uploaded_files, no files on filesystem.
click plug in icon
- expected window pops up.
click submit query with nothing clicked.
- nothing happens, refreshes as expected.
click submit query with root selected
- nothing happens, refreshes as expected.
click browse, but cancel
- nothing happens as expected
click browse and choose a file
- file name appears as expected
click submit query with root not selected
- "Error: Failed to upload file. See logs for details."
current state: file is uploaded, but database is blank.
checked error logs
jquery.js issue and "Use of uninitialized $dir" error.
click Close.
delete file from file system.
make sure the jquery.js line in upload.tt has interface not themelang
click plug in icon
- expected window pops up
click browse and choose a file
- file name appears as expected
click submit query with root not selected
- "Error: Failed to upload file. See logs for details."

++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
FIX REQUESTED:
I was expecting to have a refresh of the page with perhaps a "You forgot to select a location" in red, just above/beside/below the "Choose where to upload file".
I was also expect to NOT have the file uploaded.
This disconnect between the file existing and the record not existing in the database leads to weird problems.
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

click Close.
delete file from file system.
click plug in icon
- expected window pops up
click browse and choose a file
- file name appears as expected
click root
- NOTE: I was lazy and didn't create subdirectories in this testing.
click submit query
- happy it worked kind of message
click close
- URL is filled in nicely.

Not finished fully testing, but I thought I'd mention this problem since I found it.
Comment 133 M. Tompsett 2013-09-28 02:23:36 UTC
Comment on attachment 21432 [details] [review]
[SIGNED-OFF] Bug 6874: File upload in MARC

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

::: cataloguing/value_builder/upload.pl
@@ +50,5 @@
> +            var id = document.getElementById(index).value;
> +            if(id.match(/id=([0-9a-f]+)/)){
> +                id = RegExp.\$1;
> +            }
> +            window.open(\"../cataloguing/plugin_launcher.pl?plugin_name=upload.pl&index=\"+index+\"&id=\"+id, 'upload', 'width=600,height=400,toolbar=false,scrollbars=no');

var newin=... (this line)
newin.focus();

::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/value_builder/upload.tt
@@ +3,5 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +<head>
> +    <title>Upload plugin</title>
> +    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
> +    <script type="text/javascript" src="[% themelang %]/lib/jquery/jquery.js"></script>

This should be interface, not themelang.
Comment 134 Katrin Fischer 2013-09-28 12:11:12 UTC
Reading through the comments of M.Tompset I think this is failed QA. There are issues and further testing should be postponed until those are fixed. Julian, could you please take a look and provide follow-ups where needed?
Comment 135 M. Tompsett 2013-09-28 13:31:09 UTC Comment hidden (obsolete)
Comment 136 M. Tompsett 2013-09-28 13:35:11 UTC
Seeing as I can't sign off on my own patches. Could you squash these into a single patch, re-test, and re-post, Julian? Thanks. I'll continue testing once reposted. :)
Comment 137 Katrin Fischer 2013-09-28 13:40:18 UTC
As they haven't been signed off you could squash your patches into one - but don't squash them with Julian's.
Comment 138 M. Tompsett 2013-09-28 16:16:12 UTC Comment hidden (obsolete)
Comment 139 M. Tompsett 2013-09-28 16:20:57 UTC
Squashed my patches together as per Katrin Fischer's request.
Comment 140 Julian Maurice 2013-09-30 07:38:19 UTC Comment hidden (obsolete)
Comment 141 M. Tompsett 2013-10-02 19:39:48 UTC
First patch needs to be rebased. uploaddatabase.pl patch issues. Still going ahead with testing.
Comment 142 M. Tompsett 2013-10-03 02:06:41 UTC
Testing with Javascript enabled.

click plugin icon
click Submit Query with nothing filled in.
- Was only told about the file location not filled in. I would expected to be told neither were filled in, if I get told this.

Selected a file
click Submit Query
- expected to be told location not filled it. As expected.

closed pop up window.
click plugin icon
click location, but not select a file.
- Refreshed blank. A valid behaviour, but confusing since now we have the file location validation.

selected a file and chose a location
click Submit Query
- File uploaded as expected.

click close
- URL is filled in nicely, as expected.

click plugin icon
- Would you like to delete window pops up. Okay behaviour.

click cancel
- windows closes no changes as expected.

click plugin icon
click delete
- told it was deleted successfully, as expected. URL is blanked, as expected.

click close
repeat steps to get back to delete popup and click delete
upload a new file

-- after a few cycles of trying to repeat the nothing, browse, root, browse & root selections, I realized I would really like a way to clear the file selected. Though, perhaps this isn't the real need.

I did try testing with Javascript off, but it is nearly impossible to work with different browers and scripting off. I was able to trigger your non-javascript red message under IE 10, but not Firefox 24 since the plugin icon refused to popup a window. The problem with it disabled was that then the 'close' doesn't work with javascript disabled, and the URL isn't filled in, but the file is uploaded.

On a positive note it continues to behave as if there was no file uploaded, which is what is expected from a blank 856$u field. However, there are table entries and files sitting on the server now.

SUGGESTIONS FOR IMPROVEMENTS:
- If we validate the location, could we also validate if the filename is set?
- Could we add a 'Cancel' which just closes the window to this? Not just a 'Submit Query' button.
- Could we change 'Submit Query' to 'Upload File'?
- When nothing was selected, got a refresh, which is okay, but strange in light of validation message logic that was added.
- Javascript off generates bizarre (but not unstable) state of file uploaded and table pointing to it, but textbox empty, under IE 10. I couldn't even open the new popup windows under FireFox 24. I think if we document this requires Javascript, this is passable.

This is getting really close to useable. Thank you for working on this, Julian. :)
Comment 143 M. Tompsett 2013-10-03 20:40:54 UTC Comment hidden (obsolete)
Comment 144 M. Tompsett 2013-10-03 20:42:39 UTC Comment hidden (obsolete)
Comment 145 M. Tompsett 2013-10-03 20:43:29 UTC
Comment on attachment 21546 [details] [review]
Bug 6874 - kohastructure.sql & jquery.js fix with refocus

squashed my patches into one on the end.
Comment 146 M. Tompsett 2013-10-03 23:21:50 UTC Comment hidden (obsolete)
Comment 147 M. Tompsett 2013-10-03 23:24:46 UTC Comment hidden (obsolete)
Comment 148 M. Tompsett 2013-10-03 23:25:00 UTC Comment hidden (obsolete)
Comment 149 M. Tompsett 2013-10-03 23:25:24 UTC Comment hidden (obsolete)
Comment 150 M. Tompsett 2013-10-03 23:33:19 UTC
Test Plan for my patch:
- click plugin icon (after all the necessary configuration and setup).
No red message is given.
- Turn off javascript
- click 'Upload file'
Should get pretty red message about both things not selected.
- go through combinations of one or the other, but not both, and clicking upload
Should get pretty red message each time with appropriate message.
- Turn on javascript
- go through combinations of none, one, and then the other, javascript alerts should warn appropriately.
- after linking the 856$u field to a preset URL, click the plugin icon, and delete the file.
Comment 151 M. Tompsett 2013-10-04 14:02:43 UTC
Comment on attachment 21434 [details] [review]
[SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles

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

::: C4/Installer/PerlDependencies.pm
@@ +692,5 @@
> +    'Test::CGI::Multipart' => {
> +        'usage'    => 'Tests',
> +        'required' => '0',
> +        'min_ver'  => '0.0.3',
> +    },

This needs a rebase now. :(
Comment 152 Dobrica Pavlinusic 2013-10-22 12:47:26 UTC
I also have conflict in sysprefs:

CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Comment 153 Dobrica Pavlinusic 2013-10-22 12:58:13 UTC
I'm applying on 3.2.x branch, but since it's removed in another patch, it would probably be better to drop all changes to sysprefs from this patchset instead of modifying it and than reverting it back.
Comment 154 M. Tompsett 2013-10-24 04:22:49 UTC
Shoot... My librarian colleague came up with a scenario that breaks this.

1) Upload and set everything for a 856$u.
2) Clone the subfield.
3) Click the plugin icon.
This is where the problem comes in, because now the two subfields you can end up deleting the file for the current 856$u, but the other 856$u still has a reference to the deleted file in it.

Any ideas?
Comment 155 M. Tompsett 2013-11-09 15:32:22 UTC
While we're waiting for a design suggestion to solve the test case I posted, could the original author please rebase his first two signed off patches? Thank you. :)
Comment 156 M. Tompsett 2013-11-16 03:40:14 UTC
I have some other related questions:
- should validate URLs, since not all 856$u urls will be uploaded file urls.
- external urls will generate the same apparent error deleting as the cloning scenario I mentioned in comment #154.
- is telling a user to check errors logs reasonable? They may not have shell access to the system.
- If a valid external url is detected, then a different response is needed than if we discover a url to a deleted file.
- How will the user know to delete the text field before clicking the plugin icon in order to continue onto the upload screen to put a different file in place?

The signed off patches need a rebase. Feedback on these questions would be useful. I'm leaving the status at In Discussion, because even if my patch was signed off, the problem with respect to external urls and deleted files, due to a clone, would still exist. And that seems like enough to merit not just running for QA's approval.
Comment 157 Paul Poulain 2013-12-16 09:56:34 UTC
(In reply to M. Tompsett from comment #154)
> Shoot... My librarian colleague came up with a scenario that breaks this.
> 
> 1) Upload and set everything for a 856$u.
> 2) Clone the subfield.
> 3) Click the plugin icon.
> This is where the problem comes in, because now the two subfields you can
> end up deleting the file for the current 856$u, but the other 856$u still
> has a reference to the deleted file in it.
I think it's not a big deal: the user just get an invalid URL. That's exactly the same thing as when the librarian fills an invalid URL.
Also note that the script misc/cronjobs/check-url-quick.pl will detect the problem.
Comment 158 Paul Poulain 2013-12-16 09:59:50 UTC
(In reply to M. Tompsett from comment #156)
> I have some other related questions:
> - should validate URLs, since not all 856$u urls will be uploaded file urls.
Not sure I understand well the question, but isn't misc/cronjobs/check-url-quick.pl what you're asking ?

> - external urls will generate the same apparent error deleting as the
> cloning scenario I mentioned in comment #154.
see my previous answer.

> - is telling a user to check errors logs reasonable? They may not have shell
> access to the system.
No, it isn't

> - If a valid external url is detected, then a different response is needed
> than if we discover a url to a deleted file.
I think check-url-quick.pl achieve the goal, and it's better because it detects when an URL becomes invalid at any time.

> - How will the user know to delete the text field before clicking the plugin
> icon in order to continue onto the upload screen to put a different file in
> place?
I don't understand this one.

> The signed off patches need a rebase. Feedback on these questions would be
> useful. I'm leaving the status at In Discussion, because even if my patch
> was signed off, the problem with respect to external urls and deleted files,
> due to a clone, would still exist. And that seems like enough to merit not
> just running for QA's approval.
Are my answer OK and is it back to go to signed-off ?
(I have not tested the patches still applies cleanly)
Comment 159 M. Tompsett 2013-12-16 16:27:48 UTC
(In reply to Paul Poulain from comment #158)
> (In reply to M. Tompsett from comment #156)
> > I have some other related questions:
> > - should validate URLs, since not all 856$u urls will be uploaded file urls.
> Not sure I understand well the question, but isn't
> misc/cronjobs/check-url-quick.pl what you're asking ?

No.

if ( is this one of our uploaded files ) { continue processing as expected }
else { give big warning, and then go immediately to the upload, not the delete page }


> > - is telling a user to check errors logs reasonable? They may not have shell
> > access to the system.
> No, it isn't

Then we need to fix that. However, I think that is in part of the signed off patches.


> > - If a valid external url is detected, then a different response is needed
> > than if we discover a url to a deleted file.
> I think check-url-quick.pl achieve the goal, and it's better because it
> detects when an URL becomes invalid at any time.

This isn't about an invalid URL. Frankly, I don't care about the validity as much as I care about the process flow. If I have a url in the 856$u field and click the plugin icon, I do not expect to attempt to delete an external url (not our hosted files that were uploaded).


> > - How will the user know to delete the text field before clicking the plugin
> > icon in order to continue onto the upload screen to put a different file in
> > place?
> I don't understand this one.

Test this.
Upload a file.
Clone the field.
Now there are two fields pointing to the same uploaded file.
Click the plug in icon, and delete the file, but then cancel.
Now there are two fields: one blanked (because you just deleted), and the other with a dangling reference.

Now, click the plugin icon for the non-empty 856$u field.
It's going to prompt to delete. The user will click "Delete", because there is no way to go to upload otherwise, and an error will be generated (because the file was already deleted). And there is no meaningful feedback suggesting to clear the field before clicking the plugin icon, so the user will be frustrated because they can't seem to upload a file anymore, because they overlook the obvious idea of clearing the field.


> Are my answer OK and is it back to go to signed-off ?
> (I have not tested the patches still applies cleanly)

If I have clarified what I mean, and you do test and see the process flow, then you'll understand that with a couple more validations the process can be made smoother.
Comment 160 M. Tompsett 2013-12-16 23:47:21 UTC Comment hidden (obsolete)
Comment 161 M. Tompsett 2013-12-16 23:49:22 UTC Comment hidden (obsolete)
Comment 162 M. Tompsett 2013-12-16 23:50:22 UTC
Comment on attachment 21432 [details] [review]
[SIGNED-OFF] Bug 6874: File upload in MARC

I rebased it.
Comment 163 M. Tompsett 2013-12-16 23:54:18 UTC Comment hidden (obsolete)
Comment 164 M. Tompsett 2013-12-16 23:56:18 UTC
Comment on attachment 21433 [details] [review]
[SIGNED-OFF] Bug 6874: QA follow-up

I rebased this.
Comment 165 M. Tompsett 2013-12-16 23:59:31 UTC Comment hidden (obsolete)
Comment 166 M. Tompsett 2013-12-17 00:01:04 UTC
Comment on attachment 21434 [details] [review]
[SIGNED-OFF] Bug 6874: Add unit tests for C4::UploadedFiles

I rebased this.
Comment 167 M. Tompsett 2013-12-17 04:07:46 UTC
I've almost got something nice working. I fixed DelUploadedFile and added DanglingEntry. Hopefully something will be finished in 24 hours. :)
Comment 168 M. Tompsett 2013-12-17 20:32:25 UTC Comment hidden (obsolete)
Comment 169 M. Tompsett 2013-12-17 20:32:49 UTC Comment hidden (obsolete)
Comment 170 M. Tompsett 2013-12-17 20:33:13 UTC Comment hidden (obsolete)
Comment 171 M. Tompsett 2013-12-17 20:33:41 UTC Comment hidden (obsolete)
Comment 172 M. Tompsett 2013-12-17 20:34:07 UTC Comment hidden (obsolete)
Comment 173 M. Tompsett 2013-12-17 20:34:33 UTC Comment hidden (obsolete)
Comment 174 M. Tompsett 2013-12-17 20:34:55 UTC Comment hidden (obsolete)
Comment 175 M. Tompsett 2013-12-18 05:13:00 UTC
MEGA TEST PLAN
--------------
git checkout -b bug_6874 origin/master
git bz apply 6874
-- yes, it should all apply smoothly.
I intentionally restored an old backup from a couple months ago to force the upgrade.
staff client:
-- upgrade
-- login
Koha administration
MARC Bibliographic framework
MARC structure (for BKS)
Search for 856
Subfields
Edit (for u)
Other options
Plugin: upload.pl
Save changes
Search the catalog tab
{choose a word, any word. I used fancy, because I knew we had a cookbook}
click first link
Edit
Edit record
8 (that's the tab name)
click the name for 856
scroll down to u and click the plugin icon
-- Correctly got a configuration error message.

At a command prompt:
vi ~/koha-dev/etc/koha-conf.xml
/enable_plug
-- add in:
<upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path>
:wq
mkdir {appropriate path used}
-- I even added some subdirectories
sudo chown -R -v www-data.www-data {appropriate path used}

Back in staff client:
click the plugin icon again
-- this time an upload screen pops up
click the 'Upload file' button
-- message about no file or destination
browse for a file, select one
click the 'Upload file' button
-- message about no destination
click the 'Cancel' button
click the plugin icon
click a destination radio button
click the 'Upload file' button
-- message about no file
browse for a file, select one
click the 'Upload file' button
-- message about success
click the 'close' button.
-- the text box has been filled in with a nice URL
click the plugin icon
click cancel
click the plugin icon
click delete
-- the test box should be blanked, and a success message given
click Close
-- reupload a file properly
click the plugin icon
click delete
click upload file
browse for a file, select a destination, click upload file
click close on success message
click 'Clone this subfield'
click the second plugin icon
click delete
click close
click the first plugin icon
-- Nice error message about a URL which points to nothing.
click cancel
In the second 856$u type in a URL (eg. www.google.com)
click the second plugin icon
-- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it.
click save (we need to save the bibliographic record)

In OPAC:
search for the same word (I used fancy)
find the entry you just updated with links
click the two links. The dangling entry should give you a 500 error, and the other link should work just fine.
get back to the detail page

In staff client:
edit
edit record
8
change the 856$u to a valid file in the first link.
save

In OPAC:
refresh the detail page, and click the first link again
this time it should get downloaded nicely.

From a command line:
prove -v t/db_dependent/UploadedFiles.t
perldoc C4::Biblio
perldoc C4::UploadedFiles
Comment 176 M. Tompsett 2014-03-17 18:24:21 UTC Comment hidden (obsolete)
Comment 177 M. Tompsett 2014-03-17 18:24:39 UTC Comment hidden (obsolete)
Comment 178 M. Tompsett 2014-03-17 18:24:58 UTC Comment hidden (obsolete)
Comment 179 M. Tompsett 2014-03-17 18:25:18 UTC Comment hidden (obsolete)
Comment 180 M. Tompsett 2014-03-17 18:25:40 UTC Comment hidden (obsolete)
Comment 181 M. Tompsett 2014-03-17 18:26:00 UTC Comment hidden (obsolete)
Comment 182 M. Tompsett 2014-03-17 18:26:26 UTC Comment hidden (obsolete)
Comment 183 M. Tompsett 2014-03-17 18:29:44 UTC
I rebase'd to master. The last patch still needs sign off. :)
Comment 184 M. Tompsett 2014-03-27 19:42:32 UTC
Someone asked about how to test and why I have my name on so many things. I thought perhaps posting this might help people find the test plan and some background details.

> Also, why many commits have only your sign?
> Why is your sign at all?
 
Because I didn’t write all the first part. I tested it, because we wanted the same functionality.
 
 
> It's also difficult to test something without a clear test plan.
 
Yes, yes, it is.
 
I started...
Comment 109.
Comment 123.
 
Katrin makes similar comments regarding the test plan being difficult or lacking in Comment 128.
 
Comment 131.
Comment 142.
 
Which all cumulated into what you are looking for:
Comment 175
 
I only wrote the last patch, which is not signed off. And in my mind, it is the only thing stopping this from reaching QA.
 
 
> Could you add a description of what to test, before and after patch
 
Before, Koha does not host files, and 856$u needs to point to some external source.
After, Koha’s tag editor lets you upload a file and have a URL automatically generated to point to that newly hosted file.
 
I hope this helps someone decide to take the plunge and test this. :)
Comment 185 Marcel de Rooy 2014-03-27 21:02:27 UTC
(In reply to M. Tompsett from comment #184)
> I hope this helps someone decide to take the plunge and test this. :)

Looking back at the history of this patch, I definitely admire the perseverance of Julian and Mark to get this into Koha.
Comment 186 M. Tompsett 2014-04-07 19:42:44 UTC Comment hidden (obsolete)
Comment 187 M. Tompsett 2014-04-07 19:43:04 UTC Comment hidden (obsolete)
Comment 188 M. Tompsett 2014-04-07 19:43:14 UTC Comment hidden (obsolete)
Comment 189 M. Tompsett 2014-04-07 19:43:23 UTC Comment hidden (obsolete)
Comment 190 M. Tompsett 2014-04-07 19:43:34 UTC Comment hidden (obsolete)
Comment 191 M. Tompsett 2014-04-07 19:43:42 UTC Comment hidden (obsolete)
Comment 192 M. Tompsett 2014-04-07 19:43:49 UTC Comment hidden (obsolete)
Comment 193 M. Tompsett 2014-04-07 19:48:38 UTC
Given that master has had things added to it. I rebased against master. The only thing that really needs sign off is the last patch (kohastructure.sql, jquery.js, refocus, and more). You can find a mega test plan in comment 175.
Comment 194 Bernardo Gonzalez Kriegel 2014-04-15 01:33:31 UTC
Hi, testing

Found a problem, at first I have an empty OPACbaseURL, file upload worked.
Then I set base url, now when I upload a file, the original file name is not appended at the end of the 'new' name, I got something like "/cgi-bin/koha/opac-retrieve-file.pl?id=775f48aaf5f7a44fa534dae4e7dc876ea7191115", with a missing "_filename.ext"
But file is created, and I can delete it, but url does not work

Other little things, koha-qa
 FAIL   C4/UploadedFiles.pm
 FAIL   cataloguing/value_builder/upload.pl
...
 FAIL   opac/opac-retrieve-file.pl

with error

   FAIL   forbidden patterns
                forbidden pattern: Koha is now under the GPLv3 license (line 9)

Just an old version of licence on files, update
http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
Comment 195 M. Tompsett 2014-04-15 03:57:01 UTC Comment hidden (obsolete)
Comment 196 M. Tompsett 2014-04-15 03:59:20 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #194)

So just to clarify. You tried to use this without OPACBaseURL set in the first place? And it seemed to upload fine. Then, you set the OPACBaseURL, and after logging out and back in, when you tried to check on the file it busted?

I just patched the license stuff. I'll attempt to recreate your problem tomorrow.
Comment 197 Bernardo Gonzalez Kriegel 2014-04-15 11:20:45 UTC Comment hidden (obsolete)
Comment 198 Bernardo Gonzalez Kriegel 2014-04-15 11:45:42 UTC
(In reply to M. Tompsett from comment #196)
> (In reply to Bernardo Gonzalez Kriegel from comment #194)
> 
> So just to clarify. You tried to use this without OPACBaseURL set in the
> first place? And it seemed to upload fine. Then, you set the OPACBaseURL,
> and after logging out and back in, when you tried to check on the file it
> busted?
> 
> I just patched the license stuff. I'll attempt to recreate your problem
> tomorrow.

Mark, found it!
Your OPACBaseURL must be filled with "http://", that's it
And it's wrong, or at last it's not suggested when you fill the preference
("The OPAC is located at http:// ... ")

Look my followup, use it or not, fix as you like and squash with previous patch
Regards :)
Comment 199 Robin Sheat 2014-04-15 23:31:19 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #197)
> Created attachment 27125 [details] [review] [review]
> Bug 6874 - Fix file URL
> 
> This patch prepends 'http://' to file upload URL,
> because this patch series assumes (wrongly) that
> OAPCBaseURL is filled WITH that :)

/me points to https:// and frowns.
Comment 200 M. Tompsett 2014-04-15 23:35:52 UTC
Perhaps we need a dependency on bug 5010 to solve OPAC http vs. https issues?
Comment 201 Bernardo Gonzalez Kriegel 2014-04-15 23:46:26 UTC
(In reply to Robin Sheat from comment #199)
> 
> /me points to https:// and frowns.

But... is strange to put OPAC under https
It's possible of course, but my followup only inserts it if it's not there.
It can be modified to recongnize ^http only...

Mark, it's just an idea, ta
Comment 202 Bernardo Gonzalez Kriegel 2014-04-15 23:50:48 UTC
Sorry, pressed wrong key.

Mark, It's an idea, don't fully know if http(s):// is the expected beginning of base url. But that explained the wrong behavior I got.
Comment 203 Robin Sheat 2014-04-16 00:04:48 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #201)
> But... is strange to put OPAC under https

No it's not. In fact, these days we pretty much won't deploy anything without it. People's passwords and personal information goes over it, after all. Even more so when it's LDAPified to a corporate authentication server.
Comment 204 Bernardo Gonzalez Kriegel 2014-04-16 01:03:47 UTC
(In reply to Robin Sheat from comment #203)
> No it's not. In fact, these days we pretty much won't deploy anything
> without it. People's passwords and personal information goes over it, after
> all. Even more so when it's LDAPified to a corporate authentication server.

Good point, you are right.

Putting 'https://' on OPACBaseURL brakes something? 
Is it safe to use it that way?
Comment 205 Robin Sheat 2014-04-16 01:44:59 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #204)
> Putting 'https://' on OPACBaseURL brakes something? 
> Is it safe to use it that way?

We tend to not put the https:// part in, and it seems to work, though I bet there are corner cases that don't. I don't know what would happen if you included the protocol.

Really, the whole syspref should be redesigned so that it requires it, so it can be set properly.

I'm not sure what the best solution here is, perhaps make it work so that it'll add http:// unless the syspref =~ m|^\w+://| (after all, you might want to host your Koha on gopher://... :)
Comment 206 M. Tompsett 2014-04-16 04:03:37 UTC
(In reply to Robin Sheat from comment #205)
>(after all, you might
> want to host your Koha on gopher://... :)

YES! Oh man. I haven't heard of that protocol for like decades, since university. 

Actually,  Bernardo Gonzalez Kriegel, putting 'http://' or 'https://' into the OPACBaseURL can break things, because some places would then generate http://http://, because we are totally inconsistent in our appending in code vs. appending in templates. That kind of mistake busts really well in FireFox.
Comment 207 M. Tompsett 2014-04-16 04:26:38 UTC Comment hidden (obsolete)
Comment 208 M. Tompsett 2014-04-16 04:30:23 UTC
I have attached my rework which includes a dependency on bug 5010. I have set the status to ASSIGNED, because I have not tested it yet, and did not feel like asking anyone else to sign off until I have revisited a full testing.
Comment 209 M. Tompsett 2014-04-16 04:34:21 UTC Comment hidden (obsolete)
Comment 210 M. Tompsett 2014-04-17 00:47:13 UTC
Further testing has shown an problem with the id generation. You can't upload a file with identical contents to different directories. I'll add a patch for that.
Comment 211 M. Tompsett 2014-04-17 00:52:00 UTC Comment hidden (obsolete)
Comment 212 M. Tompsett 2014-07-31 04:44:49 UTC Comment hidden (obsolete)
Comment 213 M. Tompsett 2014-07-31 04:44:59 UTC Comment hidden (obsolete)
Comment 214 M. Tompsett 2014-07-31 04:45:07 UTC Comment hidden (obsolete)
Comment 215 M. Tompsett 2014-07-31 04:45:15 UTC Comment hidden (obsolete)
Comment 216 M. Tompsett 2014-07-31 04:45:23 UTC Comment hidden (obsolete)
Comment 217 M. Tompsett 2014-07-31 04:45:30 UTC Comment hidden (obsolete)
Comment 218 M. Tompsett 2014-07-31 04:45:37 UTC Comment hidden (obsolete)
Comment 219 M. Tompsett 2014-07-31 04:45:47 UTC Comment hidden (obsolete)
Comment 220 M. Tompsett 2014-07-31 04:45:54 UTC Comment hidden (obsolete)
Comment 221 M. Tompsett 2014-07-31 04:46:02 UTC Comment hidden (obsolete)
Comment 222 M. Tompsett 2014-07-31 04:47:51 UTC
I rebased this. However, I am feeling, I may have to bite the bullet and just fix all the hardcoded http references in Koha across the board for bug 5010, so that OPACBaseURL can include https:// or http:// nicely.
Comment 223 M. Tompsett 2014-10-16 04:22:02 UTC Comment hidden (obsolete)
Comment 224 M. Tompsett 2014-10-16 04:22:12 UTC Comment hidden (obsolete)
Comment 225 M. Tompsett 2014-10-16 04:22:20 UTC Comment hidden (obsolete)
Comment 226 M. Tompsett 2014-10-16 04:22:29 UTC Comment hidden (obsolete)
Comment 227 M. Tompsett 2014-10-16 04:22:39 UTC Comment hidden (obsolete)
Comment 228 M. Tompsett 2014-10-16 04:22:46 UTC Comment hidden (obsolete)
Comment 229 M. Tompsett 2014-10-16 04:22:54 UTC Comment hidden (obsolete)
Comment 230 M. Tompsett 2014-10-16 04:23:01 UTC Comment hidden (obsolete)
Comment 231 M. Tompsett 2014-10-16 04:23:08 UTC Comment hidden (obsolete)
Comment 232 M. Tompsett 2014-10-16 04:23:16 UTC Comment hidden (obsolete)
Comment 233 Katrin Fischer 2015-01-06 11:46:16 UTC
I recently worked on database documentation for 3.18 and noticed that we now have a table misc_files in the database which currently stores scanned invoices from the acquisition module (bug 3050)

If I saw it correctly, this patch set will add a table 'uploaded_files'.

It looks to me like the new table was thought up to store different kinds of files. Not sure if a merge of both features would make sense, but it might be worth investigating in order to avoid having various ways of achieving similar things. We also choose to keep things separate for reasons.
Comment 234 Katrin Fischer 2015-01-06 11:53:03 UTC
We _might_ also choose... 

I see now that one main difference is that we upload files to the file system here while misc_files seems to store the files directly in the database.
Comment 235 Katrin Fischer 2015-01-06 13:04:53 UTC
Looked at the database some more and we got lots of tables where we store files - but no feature yet where we store the files outside the database as far as I can tell.

So I think the idea of merging with misc_files is irrelevant - please don't let me keep you from signing off!
Comment 236 Jonathan Druart 2015-02-26 14:55:24 UTC
fatal: sha1 information is lacking or useless (catalogue/detail.pl).
Comment 237 M. Tompsett 2015-02-26 19:35:25 UTC
Okay, I'll try to work on this.
Comment 238 M. Tompsett 2015-02-26 20:20:34 UTC Comment hidden (obsolete)
Comment 239 M. Tompsett 2015-02-26 20:20:45 UTC Comment hidden (obsolete)
Comment 240 M. Tompsett 2015-02-26 20:21:12 UTC Comment hidden (obsolete)
Comment 241 M. Tompsett 2015-02-26 20:21:26 UTC Comment hidden (obsolete)
Comment 242 M. Tompsett 2015-02-26 20:25:20 UTC Comment hidden (obsolete)
Comment 243 M. Tompsett 2015-02-26 20:25:30 UTC Comment hidden (obsolete)
Comment 244 M. Tompsett 2015-02-26 20:25:39 UTC Comment hidden (obsolete)
Comment 245 M. Tompsett 2015-02-26 20:25:47 UTC Comment hidden (obsolete)
Comment 246 M. Tompsett 2015-02-26 20:25:57 UTC Comment hidden (obsolete)
Comment 247 M. Tompsett 2015-02-26 20:26:05 UTC Comment hidden (obsolete)
Comment 248 M. Tompsett 2015-02-26 20:26:14 UTC Comment hidden (obsolete)
Comment 249 M. Tompsett 2015-02-26 20:26:22 UTC Comment hidden (obsolete)
Comment 250 M. Tompsett 2015-02-26 20:26:31 UTC Comment hidden (obsolete)
Comment 251 M. Tompsett 2015-02-26 20:26:39 UTC Comment hidden (obsolete)
Comment 252 M. Tompsett 2015-02-26 22:46:40 UTC Comment hidden (obsolete)
Comment 253 M. Tompsett 2015-02-26 22:46:48 UTC Comment hidden (obsolete)
Comment 254 M. Tompsett 2015-02-26 22:46:57 UTC Comment hidden (obsolete)
Comment 255 M. Tompsett 2015-02-26 22:47:05 UTC Comment hidden (obsolete)
Comment 256 M. Tompsett 2015-02-26 22:47:13 UTC Comment hidden (obsolete)
Comment 257 M. Tompsett 2015-02-26 22:47:21 UTC Comment hidden (obsolete)
Comment 258 M. Tompsett 2015-02-26 22:47:30 UTC Comment hidden (obsolete)
Comment 259 M. Tompsett 2015-02-26 22:47:37 UTC Comment hidden (obsolete)
Comment 260 M. Tompsett 2015-02-26 22:47:45 UTC Comment hidden (obsolete)
Comment 261 M. Tompsett 2015-02-26 22:47:53 UTC Comment hidden (obsolete)
Comment 262 M. Tompsett 2015-02-27 00:04:44 UTC Comment hidden (obsolete)
Comment 263 M. Tompsett 2015-02-27 00:04:53 UTC Comment hidden (obsolete)
Comment 264 M. Tompsett 2015-02-27 00:05:01 UTC Comment hidden (obsolete)
Comment 265 M. Tompsett 2015-02-27 00:05:12 UTC Comment hidden (obsolete)
Comment 266 M. Tompsett 2015-02-27 00:05:20 UTC Comment hidden (obsolete)
Comment 267 M. Tompsett 2015-02-27 00:05:28 UTC Comment hidden (obsolete)
Comment 268 M. Tompsett 2015-02-27 00:05:37 UTC Comment hidden (obsolete)
Comment 269 M. Tompsett 2015-02-27 00:05:47 UTC Comment hidden (obsolete)
Comment 270 M. Tompsett 2015-02-27 00:05:58 UTC Comment hidden (obsolete)
Comment 271 M. Tompsett 2015-02-27 00:06:08 UTC Comment hidden (obsolete)
Comment 272 M. Tompsett 2015-02-27 01:09:50 UTC
I have moved this to assigned, like I did in comment #208, because I have yet to retest the rebase, not to mention, I had issues rebasing, so some fixing jumped from one patch into another. I wish to retest as a result. The test plan is in comment #175, with later comments perhaps containing some other things to look at.
Comment 273 M. Tompsett 2015-04-12 01:32:06 UTC Comment hidden (obsolete)
Comment 274 M. Tompsett 2015-04-12 01:32:20 UTC Comment hidden (obsolete)
Comment 275 M. Tompsett 2015-04-12 01:32:30 UTC Comment hidden (obsolete)
Comment 276 M. Tompsett 2015-04-12 01:32:49 UTC Comment hidden (obsolete)
Comment 277 M. Tompsett 2015-04-12 01:34:49 UTC Comment hidden (obsolete)
Comment 278 M. Tompsett 2015-04-12 01:35:00 UTC Comment hidden (obsolete)
Comment 279 M. Tompsett 2015-04-12 01:35:19 UTC Comment hidden (obsolete)
Comment 280 M. Tompsett 2015-04-12 01:35:28 UTC Comment hidden (obsolete)
Comment 281 M. Tompsett 2015-04-12 01:35:36 UTC Comment hidden (obsolete)
Comment 282 M. Tompsett 2015-04-12 01:35:49 UTC Comment hidden (obsolete)
Comment 283 M. Tompsett 2015-04-12 01:36:05 UTC Comment hidden (obsolete)
Comment 284 M. Tompsett 2015-04-12 01:36:16 UTC Comment hidden (obsolete)
Comment 285 M. Tompsett 2015-04-12 01:36:24 UTC Comment hidden (obsolete)
Comment 286 M. Tompsett 2015-04-12 01:36:33 UTC Comment hidden (obsolete)
Comment 287 M. Tompsett 2015-04-12 04:02:40 UTC Comment hidden (obsolete)
Comment 288 M. Tompsett 2015-04-12 04:02:49 UTC Comment hidden (obsolete)
Comment 289 M. Tompsett 2015-04-12 04:02:58 UTC Comment hidden (obsolete)
Comment 290 M. Tompsett 2015-04-12 04:03:08 UTC Comment hidden (obsolete)
Comment 291 M. Tompsett 2015-04-12 04:03:17 UTC Comment hidden (obsolete)
Comment 292 M. Tompsett 2015-04-12 04:03:25 UTC Comment hidden (obsolete)
Comment 293 M. Tompsett 2015-04-12 04:03:34 UTC Comment hidden (obsolete)
Comment 294 M. Tompsett 2015-04-12 04:03:42 UTC Comment hidden (obsolete)
Comment 295 M. Tompsett 2015-04-12 04:03:55 UTC Comment hidden (obsolete)
Comment 296 M. Tompsett 2015-04-12 04:04:07 UTC Comment hidden (obsolete)
Comment 297 M. Tompsett 2015-04-12 04:04:19 UTC Comment hidden (obsolete)
Comment 298 Magnus Enger 2015-04-13 14:54:25 UTC
Should the status be "Needs signoff"?
Comment 299 M. Tompsett 2015-04-13 15:01:16 UTC
(In reply to Magnus Enger from comment #298)
> Should the status be "Needs signoff"?

Oops... Yes, but so does bug 5010, on which this dependent.
Comment 300 M. Tompsett 2015-05-06 21:12:39 UTC Comment hidden (obsolete)
Comment 301 M. Tompsett 2015-05-06 21:12:49 UTC Comment hidden (obsolete)
Comment 302 M. Tompsett 2015-05-06 21:12:58 UTC Comment hidden (obsolete)
Comment 303 M. Tompsett 2015-05-06 21:13:06 UTC Comment hidden (obsolete)
Comment 304 M. Tompsett 2015-05-06 21:13:14 UTC Comment hidden (obsolete)
Comment 305 M. Tompsett 2015-05-06 21:13:22 UTC Comment hidden (obsolete)
Comment 306 M. Tompsett 2015-05-06 21:13:31 UTC Comment hidden (obsolete)
Comment 307 M. Tompsett 2015-05-06 21:13:41 UTC Comment hidden (obsolete)
Comment 308 M. Tompsett 2015-05-06 21:13:54 UTC Comment hidden (obsolete)
Comment 309 M. Tompsett 2015-05-06 21:14:06 UTC Comment hidden (obsolete)
Comment 310 M. Tompsett 2015-05-06 21:14:18 UTC Comment hidden (obsolete)
Comment 311 M. Tompsett 2015-05-06 21:19:57 UTC Comment hidden (obsolete)
Comment 312 M. Tompsett 2015-05-06 21:20:06 UTC Comment hidden (obsolete)
Comment 313 M. Tompsett 2015-05-06 21:20:14 UTC Comment hidden (obsolete)
Comment 314 M. Tompsett 2015-05-06 21:20:22 UTC Comment hidden (obsolete)
Comment 315 M. Tompsett 2015-05-06 21:21:33 UTC Comment hidden (obsolete)
Comment 316 M. Tompsett 2015-05-06 21:21:41 UTC Comment hidden (obsolete)
Comment 317 M. Tompsett 2015-05-06 21:21:56 UTC Comment hidden (obsolete)
Comment 318 M. Tompsett 2015-05-06 21:22:05 UTC Comment hidden (obsolete)
Comment 319 M. Tompsett 2015-05-06 21:22:14 UTC Comment hidden (obsolete)
Comment 320 M. Tompsett 2015-05-06 21:22:22 UTC Comment hidden (obsolete)
Comment 321 M. Tompsett 2015-05-06 21:22:30 UTC Comment hidden (obsolete)
Comment 322 M. Tompsett 2015-05-06 21:22:38 UTC Comment hidden (obsolete)
Comment 323 M. Tompsett 2015-05-06 21:22:47 UTC Comment hidden (obsolete)
Comment 324 M. Tompsett 2015-05-06 21:22:57 UTC Comment hidden (obsolete)
Comment 325 M. Tompsett 2015-05-06 21:23:05 UTC Comment hidden (obsolete)
Comment 326 Bernardo Gonzalez Kriegel 2015-05-13 13:12:18 UTC Comment hidden (obsolete)
Comment 327 Bernardo Gonzalez Kriegel 2015-05-13 13:12:33 UTC Comment hidden (obsolete)
Comment 328 Bernardo Gonzalez Kriegel 2015-05-13 13:12:51 UTC Comment hidden (obsolete)
Comment 329 Bernardo Gonzalez Kriegel 2015-05-13 13:13:06 UTC Comment hidden (obsolete)
Comment 330 Bernardo Gonzalez Kriegel 2015-05-13 13:13:27 UTC Comment hidden (obsolete)
Comment 331 Bernardo Gonzalez Kriegel 2015-05-13 13:13:44 UTC Comment hidden (obsolete)
Comment 332 Bernardo Gonzalez Kriegel 2015-05-13 13:14:00 UTC Comment hidden (obsolete)
Comment 333 Bernardo Gonzalez Kriegel 2015-05-13 13:14:17 UTC Comment hidden (obsolete)
Comment 334 Bernardo Gonzalez Kriegel 2015-05-13 13:14:37 UTC Comment hidden (obsolete)
Comment 335 Bernardo Gonzalez Kriegel 2015-05-13 13:14:54 UTC Comment hidden (obsolete)
Comment 336 Bernardo Gonzalez Kriegel 2015-05-13 13:15:12 UTC Comment hidden (obsolete)
Comment 337 Marcel de Rooy 2015-05-21 12:03:07 UTC
QA: Like to do this one next :) But if someone else is faster..
Comment 338 Marcel de Rooy 2015-05-27 14:53:20 UTC
Please bear with me; still working on this one..
Comment 339 Marcel de Rooy 2015-05-28 12:51:26 UTC Comment hidden (obsolete)
Comment 340 Marcel de Rooy 2015-05-28 12:51:44 UTC Comment hidden (obsolete)
Comment 341 Marcel de Rooy 2015-05-28 12:51:53 UTC Comment hidden (obsolete)
Comment 342 Marcel de Rooy 2015-05-28 12:52:05 UTC Comment hidden (obsolete)
Comment 343 Marcel de Rooy 2015-05-28 12:52:13 UTC Comment hidden (obsolete)
Comment 344 Marcel de Rooy 2015-05-28 12:52:20 UTC Comment hidden (obsolete)
Comment 345 Marcel de Rooy 2015-05-28 12:52:30 UTC Comment hidden (obsolete)
Comment 346 Marcel de Rooy 2015-05-28 12:52:37 UTC Comment hidden (obsolete)
Comment 347 Marcel de Rooy 2015-05-28 12:52:44 UTC Comment hidden (obsolete)
Comment 348 Marcel de Rooy 2015-05-28 12:52:53 UTC Comment hidden (obsolete)
Comment 349 Marcel de Rooy 2015-05-28 12:53:01 UTC Comment hidden (obsolete)
Comment 350 Marcel de Rooy 2015-05-28 12:53:11 UTC Comment hidden (obsolete)
Comment 351 Marcel de Rooy 2015-05-28 12:53:18 UTC Comment hidden (obsolete)
Comment 352 Marcel de Rooy 2015-05-28 12:57:36 UTC
QA Comment Bug 6874:

First, thanks for your work, Julian and Marc (and others testing it). This really had a long time.. And it is a nice feature.
But what should QA be without some comments :)  No blockers for me, especially taking into consideration the long history.

[1] The new dependency Test/CGI/Multipart.pm is marked as not required. But you do unconditionally use it in the unit test. Should it be required? 
[2] Pass a nonexistent id to opac-retrieve-file; this results in Premature end of script headers: opac-retrieve-file.pl (Error 500) We should do some more here (instead of just an exit). I would prefer at least a 404 error here instead of a 500. 
[3] The changes to GetMarcUrls are not really needed. They are only done at UNIMARC side; so we create some inconsistency. And imo the change is questionable. Using tagslib and an additional framework parameter seems overkill. You also do not use $text in your code. An easier and more consistent implementation would be: let the plugin provide a link text in the corresponding subfield. I slightly amended the first patch in this respect.
[4] With reference to Jareds comment (dating from 2012):
[Quote]
This patch adds C4::UploadedFiles (notice the 's') with no indication of how it relates to C4::UploadedFile (notice the lack of the 's'). It's possible there is a legitimate reason for this, but there is absolutely no documentation of the new class.
[End of quote]
I would not want to block this development for this reason now, since we are three years later and much effort went into it already. But it is far from ideal to now have UploadedFile.pm and UploadedFiles.pm. (Too bad that this comment seems to be ignored later on.)  The new modules does have far better test coverage and actually implements a TODO from the old module. The overlap between both modules should be worked on.
[5] The plugin still contains old stuff like plugin_parameters. (While still working on converting plugins, I decided to do this one rightaway.) See last follow-up.
[6] The design of the form is rather poor. This c/should be improved on a new followup report. (Additionally, if I uploaded a file successfully, there is no need to say that it was fine and give me a Close button. Just close the form and put the URL in my field. An error is something else.) 
[7] The directory structure on the upload form shows a *tree* of all writable folders and subfolders within the upload dir. It works now, but I have my doubts if we really want it like that.  It could be a huge tree. It may have security implications too. I would rather opt for a combo box with some categories and leave the exact location to the upload manager. (Perhaps start a new subdir after 1000 uploads or whatever. Note that we leave the filename to the upload script too.) I will open a new report for it.
[8] Some interesting extensions (imo) would be: Move upload to Tools (adjust the plugin to provide an additional interface). Provide a Manage upload too with some additional settings like quota, maximum size, etc. Add upload permissions. Add configurable headers for opac-retrieve (to view files inline etc.) I will start a few reports :-)
[9] Personally, I would have renamed the column dir into subdir or relpath or something to indicate that it is no full path. Also for the root dir I would leave it empty instead of putting / into it. 
[10] Translation issues: I see several alerts in the template that cannot be translated. Please adjust (new report).

Note to RM: I would ask for some additional consideration/clemency when deciding on this patch set. As you can see, the number of points listed could have justified a Failed QA too, but there is a lot of work in here. If you push it, we could also mark the new feature as experimental while some improvements are on the way.
Comment 353 Marcel de Rooy 2015-05-29 06:25:49 UTC
Opened umbrella report (bug 14294) for follow-up reports.
Comment 354 Marcel de Rooy 2015-07-31 06:03:33 UTC
Coming back here in a minute.. Hold on.
Comment 355 Marcel de Rooy 2015-07-31 17:05:16 UTC
Created attachment 41275 [details] [review]
Bug 6874: File upload in MARC

New cataloging plugin upload.pl and new system preference 'uploadPath'.

upload.pl provide a way to upload files on the server and store a link
(url) to it in MARC
uploadPath is the absolute path on the server where the files will be
stored. It's possible to have a hierarchy of directories under this path
and the plugin will allow to choose in which directory to store the
file.
Stored value in MARC subfield looks like this:
<OPACBaseURL>/cgi-bin/koha/opac-retrieve-file.pl?id=<SHA-1 of the file>
So both 'uploadPath' and 'OPACBaseURL' sysprefs have to be set for this
plugin to work correctly

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Slightly amended/simplified the patch during QA: The changes to
GetMarcUrls are not really needed, and would have needed some
attention. Another link text can be supplied by the plugin too.

This also reduces the need for changes in basket.pl,
MARCdetail.pl, detail.pl, opac-basket.pl and opac-detail.pl.
Comment 356 Marcel de Rooy 2015-07-31 17:05:24 UTC
Created attachment 41276 [details] [review]
Bug 6874: QA follow-up

Squash of the following commits:
  return with explicit undef removed
  Follow-up for PBP fixes
  follow-up fix POD syntax
  follow-up jquery has moved

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: removed POD changes in Biblio.pm while rebasing.
Comment 357 Marcel de Rooy 2015-07-31 17:05:32 UTC
Created attachment 41277 [details] [review]
Bug 6874: Add unit tests for C4::UploadedFiles

Add unit tests for C4::UploadedFiles and move a variable
declaration at subroutine level instead of file level.
Add dependency to Test::CGI::Multipart

Still works, and the newly-provided unit tests have good test
coverage:
C4/UploadedFiles.pm 90.7    65.0    66.7    100.0   100.0    0.2    86.4

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 358 Marcel de Rooy 2015-07-31 17:05:39 UTC
Created attachment 41278 [details] [review]
Bug 6874: Move uploadPath syspref to koha-conf.xml

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 359 Marcel de Rooy 2015-07-31 17:05:48 UTC
Created attachment 41279 [details] [review]
Bug 6874 - File upload in MARC - QA Followup

* Renames uploadPath to upload_path to follow the standard naming
  conventions in koha-conf which use underscores rather than camel case
* Remove reference to intranet-tmpl and replace with [% interface %]
  required to pass qa

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 360 Marcel de Rooy 2015-07-31 17:05:55 UTC
Created attachment 41280 [details] [review]
Bug 6874: Force user to select a directory

This patch adds a javascript and a server-side checks

Test plan:
1/ Choose a file, leave the radios not selected and submit
2/ You have a javascript alert which prevents form to be submitted
3/ Disable javascript and repeat step 1
4/ Form is submitted but form is redisplayed with an error message
telling you to choose a directory.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 361 Marcel de Rooy 2015-07-31 17:06:03 UTC
Created attachment 41281 [details] [review]
Bug 6874 - kohastructure.sql, jquery.js, refocus, and more

Two problems were discovered while doing a fresh install
of Koha. These problems in the kohastructure.sql file are
addressed with this patch.

Clicking the plug-in icon should cause the popup window
to refocus.  This adds the refocus code to the upload.pl file.

The path to the jquery.js script was wrong in the
upload_delete_file.tt file. Changed [% themelang %] to
[% interface %].

If a user clones 856$u after uploading a file, deletes the file,
and then clicks the plugin icon on the first 856$u, this will go
immediately to the upload screen with an informative error
message.

After some validation was added, it was extended to include
other cases. This serves to patch 6874 to a state where sign
off should be possible.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 362 Marcel de Rooy 2015-07-31 17:06:11 UTC
Created attachment 41282 [details] [review]
Bug 6874 - License Text Update

This updates the license text for
- C4/UploadedFiles.pm
- cataloguing/value_builder/upload.pl
- opac/opac-retrieve-file.pl
to GPL v3 as per the
http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
text.

TEST PLAN
---------
1) Don't apply this patch as part of 6874.
2) Run the koha qa test tool.
   -- It will FAIL for those three file.
3) Apply this patch
4) Run the koha qa test tool.
   -- License problem is corrected, no FAILs.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 363 Marcel de Rooy 2015-07-31 17:06:20 UTC
Created attachment 41283 [details] [review]
Bug 6874 - Clean up file URL generation

This adds a dependency on bug 5010, in that the protocol is
assumed in the OpacBaseURL system preference. It also adds
improved error handling.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 364 Marcel de Rooy 2015-07-31 17:06:39 UTC
Created attachment 41284 [details] [review]
Bug 6874 - id based on contents, filename, and directory.

This patch corrects the inability to upload the same file to a
different directory, or even a renamed file to the same
directory. By including the filename and directory as part of
the $id SHA generation, only identical files in the same
directory with the same file name will generate the same $id.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 365 Marcel de Rooy 2015-07-31 17:06:47 UTC
Created attachment 41285 [details] [review]
Bug 6874: Catch warnings for clean tests.

I hate noisy tests, so I cleaned them up.

MEGA TEST PLAN
--------------
git checkout -b bug_6874 origin/master
git bz apply 6874
-- yes, it should all apply smoothly.
I intentionally restored an old backup from a couple months ago to force the upgrade.
staff client:
-- upgrade
-- login
Koha administration
MARC Bibliographic framework
MARC structure (for BKS)
Search for 856
Subfields
Edit (for u)
Other options
Plugin: upload.pl
Save changes
Search the catalog tab
{choose a word, any word. I used fancy, because I knew we had a cookbook}
click first link
Edit
Edit record
8 (that's the tab name)
click the name for 856
scroll down to u and click the plugin icon
-- Correctly got a configuration error message.

At a command prompt:
vi ~/koha-dev/etc/koha-conf.xml
/enable_plug
-- add in:
<upload_path>{appropriate path. I used /home/mtompset/uploads}</upload_path>
:wq
mkdir {appropriate path used}
-- I even added some subdirectories
sudo chown -R -v www-data.www-data {appropriate path used}

Back in staff client:
click the plugin icon again
-- this time an upload screen pops up
click the 'Upload file' button
-- message about no file or destination
browse for a file, select one
click the 'Upload file' button
-- message about no destination
click the 'Cancel' button
click the plugin icon
click a destination radio button
click the 'Upload file' button
-- message about no file
browse for a file, select one
click the 'Upload file' button
-- message about success
click the 'close' button.
-- the text box has been filled in with a nice URL
click the plugin icon
click cancel
click the plugin icon
click delete
-- the test box should be blanked, and a success message given
click Close
-- reupload a file properly
click the plugin icon
click delete
click upload file
browse for a file, select a destination, click upload file
click close on success message
click 'Clone this subfield'
click the second plugin icon
click delete
click close
click the first plugin icon
-- Nice error message about a URL which points to nothing.
click cancel
In the second 856$u type in a URL (eg. www.google.com)
click the second plugin icon
-- this jumps immediately to the upload screen, but does not give an error message, because the URL does not have opac-retrieve-file in it.
click save (we need to save the bibliographic record)

In OPAC:
search for the same word (I used fancy)
find the entry you just updated with links
click the two links. The dangling entry should give you a 500 error, and the other link should work just fine.
get back to the detail page

In staff client:
edit
edit record
8
change the 856$u to a valid file in the first link.
save

In OPAC:
refresh the detail page, and click the first link again
this time it should get downloaded nicely.

From a command line:
prove -v t/db_dependent/UploadedFiles.t
perldoc C4::Biblio
perldoc C4::UploadedFiles

One more round of tests tomorrow, and this should be ready. :)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Tested on top of 5010

Followed mostly mega test plan, seems to work :)
Can upload, delete, modify, etc
Test runs Ok
No koha-qa errors

I view this as valuable addition, dangling since 2011!
As with 5010, I consider this can be pushed (for 3.22)
and we can fix anything wrong later.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 366 Marcel de Rooy 2015-07-31 17:06:55 UTC
Created attachment 41286 [details] [review]
Bug 6874: [QA Follow-up] Adjustments for opac-retrieve

Adds httpheaders sub to UploadedFiles; this will be extended on a new
report. (Idea is to add configurable headers for file extensions.)
Trivial unit test added.
Small cosmetic changes to opac-retrieve-file.

Test plan:
Run test UploadedFiles.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 367 Marcel de Rooy 2015-07-31 17:07:04 UTC Comment hidden (obsolete)
Comment 368 Marcel de Rooy 2015-07-31 17:08:27 UTC
Created attachment 41288 [details] [review]
Bug 6874: [QA Follow-up] Last adjustments for future developments

This patch does:

[1] Some trivial template changes. Modified some comments (POD lines).
[2] Converted plugin to new style.
[3] Table updates: renames id to hashvalue, adds a autoincrement id,
    adds filesize, timestamp, owner and category.
    RM: This db rev is in a separate sql file in atomicupdate.
[4] Code references to computed hash renamed to hashvalue instead of id.
[5] Removed some code pertaining to exposing upload dir structure. The user
    now may choose a category; the uploader takes care of storage.
    The list of upload categories is now taken from authorised values; this
    might become a separate table in the future. (If there are none,
    the upload process creates one default fallback.)
    We can add e.g. permissions later, subdir structure, etc. (So dir will
    not necessarily be category anymore.)

Test plan:
[1] Run the db revision.
[2] Upload new file. Check the record in the table. Delete it again; check.
[3] Run t/db../UploadedFiles.t.
[4] Run t/db../FrameworkPlugins.t -incl cataloguing/value_builder/upload.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 369 Marcel de Rooy 2015-07-31 17:19:44 UTC
To get this moving again, I rebased and made some additional changes with reference to the last QA comment:
For point 7 I at last removed the dir structure and provide a category. We can build further improvement on that. See the last commit message. (This QA followup got out of hand :) but I leave that to the RM)

I still favor pushing it (with some special consideration..) Note that bugs 14588 and 14589 are slightly related and can follow this one.

For point 4 (one candidate blocker), bug 14321 (WIP) will merge the two modules into Koha::Upload. I already have a prototype in test. This will also include stripping the progress from the server side to client side ajax (improved progress bar). 
This logic can be used in several uploading scripts like this plugin but also stage-marc-import etc.
Hope to submit that in one or two weeks.

Back to PQA
Comment 370 Tomás Cohen Arazi 2015-08-11 14:59:51 UTC
Created attachment 41448 [details] [review]
Bug 6874: (QA followup) Warnings in about page

This patch introduces checks on the about.pl page so the user is warned
for a deficient configuration.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 371 Tomás Cohen Arazi 2015-08-11 15:00:22 UTC
Created attachment 41449 [details] [review]
Bug 6874: (QA followup) upload_path missing in koha-conf.xml.in

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 372 Tomás Cohen Arazi 2015-08-11 15:00:54 UTC
Created attachment 41450 [details] [review]
Bug 6874: (RM followup) DBIx updates

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 373 Tomás Cohen Arazi 2015-08-11 15:59:19 UTC
Feature pushed to master.

Congratulations to everyone involved!

Note: this is an early push of a working feature. It needs testing
and followup bugs will be filled (at least by me) that will be blockers
for the release.
Comment 374 Marcel de Rooy 2015-08-12 08:05:26 UTC
(In reply to Tomás Cohen Arazi from comment #373)
> Feature pushed to master.
> 
> Congratulations to everyone involved!
> 
> Note: this is an early push of a working feature. It needs testing
> and followup bugs will be filled (at least by me) that will be blockers
> for the release.

Patches for bug 14321 forthcoming !
Comment 375 Tomás Cohen Arazi 2015-08-21 12:47:58 UTC
Created attachment 41783 [details] [review]
Bug 6874: (QA followup) fix templates

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>