Currently the cataloguing plugin upload.pl prefill 856$u (or any other fields linked with the plugin) with something like: OPACBaseURL/cgi-bin/koha/opac-retrieve-file.pl?id=$hash In some cases we would like to have the link of the uploaded file directly.
Created attachment 86583 [details] [review] Bug 22508: Add upload_public_path to config
Created attachment 86584 [details] [review] Bug 22508: Add upload_public_url to config
Created attachment 86585 [details] [review] 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_value 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.
(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 ?
(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.
(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..
It works fine, this is very useful for example if we share records via oai-pmh to a discovery and it has the functionality of full text extraction, it needs direct access to full text, now with this developement this is achievable
Created attachment 87272 [details] [review] Bug 22508: Add upload_public_path to config Signed-off-by: Hugo Agud <hagud@orex.es>
Created attachment 87273 [details] [review] Bug 22508: Add upload_public_url to config Signed-off-by: Hugo Agud <hagud@orex.es>
Created attachment 87274 [details] [review] 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_value 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>
Created attachment 87430 [details] [review] Bug 22508: Delete the public file on deleting the upload
Created attachment 87431 [details] [review] Bug 22508: add POD
Created attachment 87432 [details] [review] Bug 22508: Add warnings to about page
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.
Applying: Bug 22508: Delete the public file on deleting the upload error: sha1 information is lacking or useless (t/db_dependent/Upload.t). error: could not build fake ancestor
Created attachment 89540 [details] [review] Bug 22508: Add upload_public_path to config Signed-off-by: Hugo Agud <hagud@orex.es>
Created attachment 89541 [details] [review] Bug 22508: Add upload_public_url to config Signed-off-by: Hugo Agud <hagud@orex.es>
Created attachment 89542 [details] [review] 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_value 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>
Created attachment 89543 [details] [review] Bug 22508: Delete the public file on deleting the upload
Created attachment 89544 [details] [review] Bug 22508: add POD
Created attachment 89545 [details] [review] Bug 22508: Add warnings to about page
(fan of this bz) +1 thanks
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.
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 %] 2) Would 'direct' be more fitting than 'public' here? 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. 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 :(# 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 will move this to BLOCKED for the moment, but ask for QA opinions on Monday.
(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!
Created attachment 89617 [details] [review] Bug 22508: Add warnings to about page
(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.
> 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 Look at Uploader sub cgi when it calls sub _done. The file is written in _hook. So you should be in _done.
+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?
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?
(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?
I see there is Bug 22835. Should we serve also those uploaded file via API ? In another bug report I mean.
(In reply to Marcel de Rooy from comment #28) > > 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 > > Look at Uploader sub cgi when it calls sub _done. The file is written in > _hook. So you should be in _done. I have tried _done, but it did not work. I cannot remember exactly why but IIRC it was not called when needed. I will have another look later.
(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.
(In reply to Hugo Agud from comment #31) > 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? We might be misunderstanding each other here. But what we do here, is making a change in a feature that already supports the distinction between private and public.
(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.
(In reply to Jonathan Druart from comment #33) > (In reply to Marcel de Rooy from comment #28) > > > 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 > > > > Look at Uploader sub cgi when it calls sub _done. The file is written in > > _hook. So you should be in _done. > > I have tried _done, but it did not work. I cannot remember exactly why but > IIRC it was not called when needed. I will have another look later. I feel that we should not touch Uploader after all. Maybe we should just keep one file and allow (by default) direct access to a *public* subdirectory of uploads? Probably allowing subdirs as well there. Maybe pref controlled (nothing better than adding yet another one)..
the less complexity the better...
(In reply to Marcel de Rooy from comment #37) > (In reply to Jonathan Druart from comment #33) > > (In reply to Marcel de Rooy from comment #28) > > > > 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 > > > > > > Look at Uploader sub cgi when it calls sub _done. The file is written in > > > _hook. So you should be in _done. > > > > I have tried _done, but it did not work. I cannot remember exactly why but > > IIRC it was not called when needed. I will have another look later. > > I feel that we should not touch Uploader after all. Maybe we should just > keep one file and allow (by default) direct access to a *public* > subdirectory of uploads? Probably allowing subdirs as well there. Maybe pref > controlled (nothing better than adding yet another one).. Are you suggesting I should rewrite the whole patch and rethink the approach? 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"
(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.
> More or less, yes. Note that we now have three parallel developments on this > topic. Also one from Devinim and Julian. please may you provide de bugnumber, please?
(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.
(In reply to Hugo Agud from comment #41) > > More or less, yes. Note that we now have three parallel developments on this > > topic. Also one from Devinim and Julian. > > please may you provide de bugnumber, please? Added in see also
(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..
(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
I've set this to Failed QA since Marcel and Nick agree that a different approach might be better here.