Bug 19318 - Add ability to use custom file upload paths
Summary: Add ability to use custom file upload paths
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
: 20208 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-09-14 15:29 UTC by Kyle M Hall
Modified: 2022-10-02 18:25 UTC (History)
8 users (show)

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


Attachments
Bug 19318: Allow multiple storage spaces for file uploads (68.07 KB, patch)
2018-01-29 09:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19318: Allow multiple storage spaces for file uploads (70.13 KB, patch)
2018-09-13 10:24 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19318: Allow multiple storage spaces for file uploads (70.13 KB, patch)
2019-05-02 10:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19318: Allow multiple storage spaces for file uploads (69.73 KB, patch)
2021-03-17 16:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19318: Allow multiple storage spaces for file uploads (68.82 KB, patch)
2021-04-23 13:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19318: DO NOT PUSH - SCHEMA CHANGES (2.01 KB, patch)
2021-04-23 13:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19318: Allow multiple storage spaces for file uploads (69.24 KB, patch)
2021-11-12 13:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19318: Normalize uploaded file name (1.95 KB, patch)
2021-11-12 13:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19318: DO NOT PUSH - SCHEMA CHANGES (2.06 KB, patch)
2021-12-21 16:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19318: Allow multiple storage spaces for file uploads (69.29 KB, patch)
2021-12-21 16:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19318: Normalize uploaded file name (2.01 KB, patch)
2021-12-21 16:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19318: (follow-up) remove uploadcategory column from tests (1.30 KB, patch)
2021-12-21 16:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19318: DO NOT PUSH - SCHEMA CHANGES (2.06 KB, patch)
2022-04-25 12:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19318: Allow multiple storage spaces for file uploads (69.22 KB, patch)
2022-04-25 12:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19318: Normalize uploaded file name (2.01 KB, patch)
2022-04-25 12:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19318: (follow-up) remove uploadcategory column from tests (1.30 KB, patch)
2022-04-25 12:59 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-09-14 15:29:04 UTC
Some libraries don't like the hard coded way uploaded files are all put in the same directory with the hash prefixing the file.

We should allow uploaded files to retain their original name, and optionally specify a path to upload to

I'm imagining we could accomplish this with something along these lines:
1) Add option on file upload page to not add file checksum to the uploaded filename, modify Koha::Upload
2) Modify Koha::Upload to support this option
3) Create new Authorised Value UPLOAD_PATH to define a set of upload paths
4) Add new 'upload path' option to file upload form as a pulldown of defined upload paths
5) Add new column 'file_path' to uploaded_files table
6) Modify Koha::Upload to support the automatic creation and use of custom file upload paths
Comment 1 Julian Maurice 2018-01-25 09:28:24 UTC
Hi Kyle,

Do you know if someone is working on that ? Because I will be working on something similar, that is a way to choose between several "storage spaces", eventually a subdirectory in that storage space, and other options like keeping the original name and using another URL to access those files.

But I think that the configuration of those storage spaces should not happen in authorised values, but on the server ($KOHA_CONF probably)
Comment 2 Julian Maurice 2018-01-29 09:08:29 UTC Comment hidden (obsolete)
Comment 3 Mirko Tietgen 2018-06-07 06:12:00 UTC
I tried to test, but the patch needs a rebase.
Comment 4 Julian Maurice 2018-09-13 10:24:30 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2018-09-13 10:27:18 UTC
Patch rebased on master
Comment 6 Kyle M Hall 2019-03-01 15:04:50 UTC
Pretty sure this is a duplicate of bug 20208 which is a bit fresher than mine. Closing as a duplicate on that assumption.

*** This bug has been marked as a duplicate of bug 20208 ***
Comment 7 Julian Maurice 2019-05-02 10:40:38 UTC
(In reply to Kyle M Hall from comment #6)
> Pretty sure this is a duplicate of bug 20208 which is a bit fresher than
> mine. Closing as a duplicate on that assumption.
> 
> *** This bug has been marked as a duplicate of bug 20208 ***

Allow me to reopen it. They are both duplicates, that is true, but both have patches that solve the problem in different ways. I think at least someone from the QA team must look at both solutions and pick one.
Comment 8 Julian Maurice 2019-05-02 10:47:28 UTC Comment hidden (obsolete)
Comment 9 Andrew Fuerste-Henry 2021-01-21 15:12:49 UTC
Doesn't apply, to start. But, after clearing conflicts we still saw a few issues:
- when opening the Upload plugin popup, we always get a "no results found" message to start, even though we haven't yet done a search
- we're always given the option of [root] as a directory
- setting hash filename to 0 in the config did not prevent the file name from being hashed in the url
Comment 10 Nick Clemens 2021-03-17 16:35:41 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2021-03-19 07:31:28 UTC
Processing files before patches
Constant subroutine Koha::Storage::KOHA_UPLOAD redefined at /usr/share/perl/5.24/constant.pm line 171.


An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::UploadedFiles':
        current merge results [
                Koha::UploadedFiles,
        ]
        merging failed on 'Koha::Objects' at /app/qatools/koha-qa.pl line 105.
Comment 12 Nick Clemens 2021-04-23 13:40:30 UTC Comment hidden (obsolete)
Comment 13 Nick Clemens 2021-04-23 13:40:34 UTC Comment hidden (obsolete)
Comment 14 Julian Maurice 2021-11-12 13:39:22 UTC Comment hidden (obsolete)
Comment 15 Julian Maurice 2021-11-12 13:39:26 UTC Comment hidden (obsolete)
Comment 16 Nick Clemens 2021-12-21 16:11:10 UTC Comment hidden (obsolete)
Comment 17 Nick Clemens 2021-12-21 16:11:14 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2021-12-21 16:11:18 UTC Comment hidden (obsolete)
Comment 19 Nick Clemens 2021-12-21 16:11:22 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2022-01-07 18:35:08 UTC
Is there room for different storage backends here?
Comment 21 Nick Clemens 2022-01-14 14:24:47 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> Is there room for different storage backends here?

Yes, this patch provides only 'Directory' but see Koha::Storage for documentation on defining in koha conf and provide another Adapter
Comment 22 Nick Clemens 2022-02-22 14:35:53 UTC
*** Bug 20208 has been marked as a duplicate of this bug. ***
Comment 23 Tomás Cohen Arazi 2022-04-25 12:09:16 UTC
Can I have a quick rebase please?
Comment 24 Julian Maurice 2022-04-25 12:59:13 UTC
Created attachment 133772 [details] [review]
Bug 19318: DO NOT PUSH - SCHEMA CHANGES

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Julian Maurice 2022-04-25 12:59:20 UTC
Created attachment 133773 [details] [review]
Bug 19318: Allow multiple storage spaces for file uploads

This patch allow to configure multiple storage spaces to be used when
uploading a file.

Configuration is done in $KOHA_CONF. Element 'upload_path' is replaced
by 'storage', which is repeatable and can contain the following
elements:
- name: The storage's identifier (also used for display)
- adapter: The storage's adapter is a Perl module who is responsible for
  actually writing in the storage (creating/deleting files/directories).
  There is actually only one adapter, 'directory', which writes directly
  on the filesystem.
- adapter_params: Additional parameters required for the storage's
  adapter
- hash_filename: Whether to prepend hashvalue to the filename or not
- temporary: Whether the storage is temporary or not
- baseurl: Base URL of the storage, if one doesn't want to (or can't)
  use opac-retrieve-file.pl
This is all documented in Koha::Storage

There is two built-in storages:
- 'TMP', the default temporary storage (not configurable)
- 'DEFAULT', the default persistent storage (configurable in $KOHA_CONF)

Note that if $KOHA_CONF is not updated, uploads should continues to work
(the 'DEFAULT' storage will use 'upload_path', if set)

This patch affects the following pages:
- Tools › Upload
- Tools › Upload local cover image (ZIP file upload)
- Tools › Stage MARC records for import
- Tools › Upload patron images
- Circulation › Offline circulation file upload
- Cataloguing plugin upload.pl

Test plan:
0. Before applying the patch, be sure to use the pages mentioned above
   and upload some files, those files should continue to be accessible
   after the patch
1. Apply the patch, run updatedatabase and update_dbix_class_files
2. Without modifying $KOHA_CONF, verify that files uploaded in step 0
   are still accessible (you can find them in tools/upload.pl)
3. Verify that you can still upload files using the pages mentioned
   above
4. Edit your $KOHA_CONF, copy the <storage> element from
   etc/koha-conf.xml in the source, set the <path> using the value of
   <upload_path> and remove <upload_path>
5. Verify that you can still upload files
6. Add another storage in $KOHA_CONF, set <hash_filename> to 0,
   and a <baseurl>. Now try to use the cataloguing plugin and see how
   the generated URL change. Also note that the files keep their
   original filename (without the hashvalue prepended)
7. Create some subdirectories under the main storage's path (using a
   shell). Try to upload a new files and see that these directories
   appear in a dropdown list. Select one, finish the upload and verify
   that the file has been uploaded to the selected subdirectory.
8. Run prove t/ImportBatch.t t/Koha/Storage.t
   t/db_dependent/ImportBatch.t t/db_dependent/Upload.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Julian Maurice 2022-04-25 12:59:26 UTC
Created attachment 133774 [details] [review]
Bug 19318: Normalize uploaded file name

The uploaded file name can appear in the MARC record (if using the
upload plugin), and all the strings in the MARC record are normalized
using Unicode::Normalize::NFC.
So the file name must be normalized everywhere (in the filesystem and in
the uploaded_files table too).

Test plan:
1. Create a file whose name contains combining characters. For instance:
   echo 'contents' > $(perl -e 'print "cine\x{cc}\x{81}ma"')

   0xcc 0x81 is utf8 for "Combining acute accent" (́ )
   https://unicode-table.com/fr/0301/
   Once normalized it should be 0xc3 0xa9 (é)

2. Use the upload tool to upload this file
3. Verify on your filesystem that the filename is normalized
   % ls cin* | xxd
   00000000: 6369 6ec3 a96d 610a                      cin..ma.
                    ^^ ^^
4. If you have configured the storage with a `baseurl`, use the upload
   cataloguing plugin, and verify that the generated URL works correctly

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Julian Maurice 2022-04-25 12:59:31 UTC
Created attachment 133775 [details] [review]
Bug 19318: (follow-up) remove uploadcategory column from tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Julian Maurice 2022-04-25 13:00:24 UTC
(In reply to Tomás Cohen Arazi from comment #23)
> Can I have a quick rebase please?

Here you go ;)
Comment 29 Martin Renvoize 2022-06-13 09:22:10 UTC
uploadcategorycode still exists in the browse block in tools/upload.pl

Must admit, I'm not all that close to this feature.. why do we remove the category browser as part of this development?
Comment 30 Julian Maurice 2022-06-13 09:39:53 UTC
(In reply to Martin Renvoize from comment #29)
> why do we remove the category browser as part of this development?
I thought it was not useful anymore, since the main purpose of the category was to indicate a sub-directory where to put uploaded files, and with this patch it's possible without it.
Can you describe a use case that is possible today with the category that is not possible anymore with these patches ?
Comment 31 Marcel de Rooy 2022-06-13 09:43:49 UTC
Would be glad to have a look later this week
Comment 32 Martin Renvoize 2022-06-13 09:45:26 UTC
Stepping out of this one, I'm not close enough to the feature set to properly review. Thanks for offering to take it on later this week Marcel :)
Comment 33 Katrin Fischer 2022-06-13 09:46:13 UTC
How do we transition from categories to the new way of handling things? We can assume that a lot of libraries are using the upload file feature.
Comment 34 Julian Maurice 2022-06-13 11:26:50 UTC
(In reply to Katrin Fischer from comment #33)
> How do we transition from categories to the new way of handling things? We
> can assume that a lot of libraries are using the upload file feature.

There may be something missing here. I'd say we should prepend the category name to the file path or something like that (patches don't do that yet).
But first we must decide if we need to keep upload category or not.
Comment 35 Marcel de Rooy 2022-06-13 13:22:36 UTC
Just at first glance, I do not understand why we need so much changes to accomplish what you can do already.
You can add categories or subdirectories via an AV (which could be moved away to its own config).
Probably you dont want to put uploads everywere on your server. So one paraplu directory just as upload_path should be fine?
Comment 36 Katrin Fischer 2022-10-02 18:25:27 UTC
I have decided to move this to 'In discussion' for now. My reasoning:

The patch set does appear to do much more than is written on the 'tin': It claims to allow for custom upload paths, but it does a lot more than that. There are unresolved questions about some of the design decisions and how we can deal with the consequences:

* While Kyle had originally proposed using an AV in the initial description, this patch set moves everything away from the GUI into the koha-conf.xml. In doing so, it removes the UPLOAD AV. 
If this is proven to be the right path, it would require more work to remove the AV category properly and ensure a clean upgrade path (see comment#34, comment#29).
But we also should discuss if this is a required step as it takes away control from libraries that often don't have easy access to server side configuration. 
Personally this feels contrary to what I see in other work (like making a GUI for SMTP servers).

* Adds other extra features, like: 
> hash_filename: Whether to prepend hashvalue to the filename or not.
I wonder if some of these features would be better suited as follow-ups to keep the initial bug smaller. (comment#35)

My feeling is that these conceptual questions cannot be resolved in QA, but need to be resolved before it reaches the QA step with a clear test plan/upgrade path/reasoning of changes.