https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 86583 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86583&action=edit Bug 22508: Add upload_public_path to config -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 86585 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86585&action=edit Bug 22508: Add the ability to prefill 856$u with the direct URL of the file When uploading a file using the cataloguing plugin, the current value used to fill 856$u (or others) in is OPACBaseURL/cgi-bin/koha/opac-retrieve-file.pl?id=a-big-hash This patch add the ability to define a custom and direct URL to this file. To accomplish that we need 2 new entries in our config file: * upload_public_path * upload_public_url The first one will be the local path where the files will be stored, that will be publicly accessible. The second one is the root of the URL which links to ths public dir path. Test plan: 0. Define the 2 new config entries, like: <upload_public_path>/home/vagrant/kohaclone/koha-tmpl/public_path</upload_public_path> <upload_public_url>http://catalogue.kohadev.org/public_path</upload_public_url> Then restart memcached+plack to take into account the changes 1. Link the cataloguing plugin 'upload.pl' to 856$u 2. Edit a record, 856$u > click the plugin link 3. A new "Use a public path:" appears if the config is set, tick it and upload a file 4. Pick "Choose the direct url" 5. 856$u will be prefilled with http://catalogue.kohadev.org/public_path/$hash_value instead of the usual http://catalogue.kohadev.org/cgi-bin/koha/opac-retrieve-file.pl?id=$hash_val... 6. Use the link and confirm that you can access the file. Note: This patch is not ready for inclusion yet and is published to get feedback. There are several things that will block its inclusion: * TODO: "Delete" button must delete the file * See TODO and FIXME in the patch * Tests and POD for new methods are missing * Variable naming is not perfect * In discussion - what about the upload of files from outside of the cataloguing plugin? * TODO: add check to the about page (?) * FIXME: in upload_public_path the files are not suffixed by the filename as it is for upload_path. We certainly want to be consistent here. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #3)

* In discussion - what about the upload of files from outside of the cataloguing plugin?

Never seen Tools: Upload ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #5)

(In reply to Marcel de Rooy from comment #4)

...

(In reply to Jonathan Druart from comment #3)

...

* In discussion - what about the upload of files from outside of the cataloguing plugin?

Never seen Tools: Upload ?

Hello Marcel, This is exactly what I meant with this comment: this patch does not affect the uploads made outside of the cataloguing plugin.

Great. Just read it diagonally.. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86583|0 |1 is obsolete| | --- Comment #8 from Hugo Agud <hagud@orex.es> --- Created attachment 87272 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87272&action=edit Bug 22508: Add upload_public_path to config Signed-off-by: Hugo Agud <hagud@orex.es> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 Hugo Agud <hagud@orex.es> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86585|0 |1 is obsolete| | --- Comment #10 from Hugo Agud <hagud@orex.es> --- Created attachment 87274 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87274&action=edit Bug 22508: Add the ability to prefill 856$u with the direct URL of the file When uploading a file using the cataloguing plugin, the current value used to fill 856$u (or others) in is OPACBaseURL/cgi-bin/koha/opac-retrieve-file.pl?id=a-big-hash This patch add the ability to define a custom and direct URL to this file. To accomplish that we need 2 new entries in our config file: * upload_public_path * upload_public_url The first one will be the local path where the files will be stored, that will be publicly accessible. The second one is the root of the URL which links to ths public dir path. Test plan: 0. Define the 2 new config entries, like: <upload_public_path>/home/vagrant/kohaclone/koha-tmpl/public_path</upload_public_path> <upload_public_url>http://catalogue.kohadev.org/public_path</upload_public_url> Then restart memcached+plack to take into account the changes 1. Link the cataloguing plugin 'upload.pl' to 856$u 2. Edit a record, 856$u > click the plugin link 3. A new "Use a public path:" appears if the config is set, tick it and upload a file 4. Pick "Choose the direct url" 5. 856$u will be prefilled with http://catalogue.kohadev.org/public_path/$hash_value instead of the usual http://catalogue.kohadev.org/cgi-bin/koha/opac-retrieve-file.pl?id=$hash_val... 6. Use the link and confirm that you can access the file. Note: This patch is not ready for inclusion yet and is published to get feedback. There are several things that will block its inclusion: * TODO: "Delete" button must delete the file * See TODO and FIXME in the patch * Tests and POD for new methods are missing * Variable naming is not perfect * In discussion - what about the upload of files from outside of the cataloguing plugin? * TODO: add check to the about page (?) * FIXME: in upload_public_path the files are not suffixed by the filename as it is for upload_path. We certainly want to be consistent here. Signed-off-by: Hugo Agud <hagud@orex.es> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 87431 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87431&action=edit Bug 22508: add POD -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=22644 Status|In Discussion |Signed Off --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Second pass and I think it's ready to get a first QA feedback. Regarding my previous commit message: (In reply to Jonathan Druart from comment #3)

Note: This patch is not ready for inclusion yet and is published to get feedback. There are several things that will block its inclusion: * TODO: "Delete" button must delete the file

Done in a new patch "Delete the public file on deleting the upload"

* See TODO and FIXME in the patch

One FIXME is new and could be fixed later, unless someone has a good idea about how to fix it, the second one is not related to these patches. The TODO (in direct_url) can be considered as an enhancement.

* Tests and POD for new methods are missing

I had trouble providing tests (they do not pass currently), see bug 22644. POD has been added by a new patch "Add POD"

* Variable naming is not perfect

Not blocker (?)

* In discussion - what about the upload of files from outside of the cataloguing plugin?

Could be done later, if it is needed.

* TODO: add check to the about page (?)

Done in a new patch "Add warnings to about page"

* FIXME: in upload_public_path the files are not suffixed by the filename as it is for upload_path. We certainly want to be consistent here.

The original request I got was to have a "clean" URL (read: without spaces, "weird" characters, UTF-8, etc.). The easiest was to only use the hash, I am not sure how we would answer this need matching the current behavior. As Hugo signed those patches off, I am switching the status accordingly. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 89541 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89541&action=edit Bug 22508: Add upload_public_url to config Signed-off-by: Hugo Agud <hagud@orex.es> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 89543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89543&action=edit Bug 22508: Delete the public file on deleting the upload -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 89545 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=89545&action=edit Bug 22508: Add warnings to about page -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |22644 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- This is actually blocked by bug 22644. I am keeping the NQA status as I would like to get feedback from the QA team on this patchset. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22644 [Bug 22644] Some tests in Upload.t are not testing the right thing -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Katrin Fischer from comment #24)

You've asked for feedback from the QA team... so I'll try:

1) QA tools:

FAIL valid_template parse error - /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/about.tt line 186: unexpected token (|) [% IF warnPrefBiblioAddsAuthorities || warnPrefEasyAnalyticalRecords || warnPrefAnonymousPatron || warnPrefAnonymousPatron_PatronDoesNotExist || warnNoActiveCurrency || QueryParserError || warnIsRootUser || xml_config_warnings.size || AutoSelfCheckPatronDoesNotHaveSelfCheckPerm || AutoSelfCheckPatronHasTooManyPerm || warnStatisticsFieldsError || warnNoTemplateCaching || warnILLConfiguration || has_ai_issues || oauth2_missing_deps || bad_yaml_prefs | warnUploadPublic %]

Erk, wrong conflict resolution in the last rebase.

2) Would 'direct' be more fitting than 'public' here?

Not sure, the other link is public as well :)

3) Documentation could be a bit more helpful, explaining what those are used for:

+ --upload-public-path dir Set a user defined upload_public_path. + --upload_public_url url Set a user defined upload_public_url.

I know it's not much better for other options.

I do not think the documentation belongs to this script. Maybe we should add a link to the manual when it has been documented?

4) There are still 2 remaining FIXMEs:

tools/upload-file.pl:

# FIXME The copy must be done in Koha::Uploader, but where? # I (Joubu) did not find a place where the file was completely written

push @$uploads, $rec # FIXME replace find with search and let the DB do that

Sadly I am the wrong person to ask this :(#

Yes, that's why I asked for feedback, also for comment 14.

5) I remember a lot of thought went into the current implementation, so wondering if there are possible risks or security issues with this approach that we tried to avoid by using he hash?

I cannot think of anything, but I will be happy to know if there are any.

I will move this to BLOCKED for the moment, but ask for QA opinions on Monday.

Thanks! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #23)

This is actually blocked by bug 22644. I am keeping the NQA status as I would like to get feedback from the QA team on this patchset.

Bug 22644 is a non-issue. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- +sub local_public_path { + my ($self) = @_; + my $upload_public_path = C4::Context->config('upload_public_path'); + return unless $upload_public_path; + $upload_public_path =~ s|/$||; + my $filepath = "$upload_public_path/" . $self->hashvalue; + return $filepath; +} Compare with full_path sub: my $path = File::Spec->catfile( $self->permanent ? $self->permanent_directory : C4::Context->temporary_directory, $self->dir, $self->hashvalue. '_'. $self->filename, ); You do not use the filename only the hashvalue? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #31 from Hugo Agud <hagud@orex.es> --- (In reply to Marcel de Rooy from comment #30)

In general I have my doubts on making these changes. It does not really fit in with the original design. Having a flag public to control OPAC access and a new public path could be a bit confusing. As you already noted, the public path should be added to the Tools/Upload form too if we continue here. Do we really want to copy the file? Another approach would be to grant access via the Apache configuration. You would only need a URL config var. Using that approach would make all files public. Or create a PUBLIC category etc to keep a distinction between public and private?

The original goal of this patch is to allow discoveries to access directly the files located at koha repository for full text indexing, it is not really a matter of public and private files, the perhaps the checkbox public here is not appropiate... just direct access? To be able to manage public and private files (just the file, not the records) is also a great functionality that koha could have that could be developed in another bug? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #32 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- I see there is Bug 22835. Should we serve also those uploaded file via API ? In another bug report I mean. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Marcel de Rooy from comment #29)

+sub local_public_path { + my ($self) = @_; + my $upload_public_path = C4::Context->config('upload_public_path'); + return unless $upload_public_path; + $upload_public_path =~ s|/$||; + my $filepath = "$upload_public_path/" . $self->hashvalue; + return $filepath; +}

Compare with full_path sub: my $path = File::Spec->catfile( $self->permanent ? $self->permanent_directory : C4::Context->temporary_directory, $self->dir, $self->hashvalue. '_'. $self->filename, );

You do not use the filename only the hashvalue?

Yes, see commit message: * FIXME: in upload_public_path the files are not suffixed by the filename as it is for upload_path. We certainly want to be consistent here. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #36 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Fridolin SOMERS from comment #32)

I see there is Bug 22835. Should we serve also those uploaded file via API ? In another bug report I mean.

Looking at that bug, I think it serves another specific need apart from this functionality request. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #38 from Hugo Agud <hagud@orex.es> --- the less complexity the better... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #40 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #39)

Are you suggesting I should rewrite the whole patch and rethink the approach? More or less, yes. Note that we now have three parallel developments on this topic. Also one from Devinim and Julian.

I am not sure I understand exactly what you meant by "just keep one file and allow (by default) direct access to a *public* subdirectory of uploads" Keep one file == Do not make a copy of the original upload to yet another location. Direct access to public subdir == Define a 'public' subdir where Apache(?) allows direct access. Not to all subdirs of uploads.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #42 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Marcel de Rooy from comment #40)

(In reply to Jonathan Druart from comment #39)

...

Are you suggesting I should rewrite the whole patch and rethink the approach? More or less, yes. Note that we now have three parallel developments on this topic. Also one from Devinim and Julian.

It seems that other developments answer different needs. I am not willing to rewrite this development, sorry.

...

I am not sure I understand exactly what you meant by "just keep one file and allow (by default) direct access to a *public* subdirectory of uploads" Keep one file == Do not make a copy of the original upload to yet another location.

It will complexity the code a bit more, but I can try that if it is enough to get a PQA stamp.

Direct access to public subdir == Define a 'public' subdir where Apache(?) allows direct access. Not to all subdirs of uploads.

I wanted to provide a solution without the need to edit the Apache config. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #44 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #42)

...

Keep one file == Do not make a copy of the original upload to yet another location.

It will complexity the code a bit more, but I can try that if it is enough to get a PQA stamp.

Maybe ask first another QA opinion before spending too much time on that.. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #45 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Marcel de Rooy from comment #44)

(In reply to Jonathan Druart from comment #42)

...

...

Keep one file == Do not make a copy of the original upload to yet another location.

It will complexity the code a bit more, but I can try that if it is enough to get a PQA stamp.

Maybe ask first another QA opinion before spending too much time on that..

I would agree with Marcel here, I don't see the need for copying the file if all we really achieve is a different URL for the user. It seems a rewrite rule could achieve this, though it wouldn't automatically be used when linking file I feel like this needs to merge with the other two bugs as they are all solving very closely related needs -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22508 --- Comment #46 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I've set this to Failed QA since Marcel and Nick agree that a different approach might be better here. -- You are receiving this mail because: You are watching all bug changes.