Bug 22508 - Add the ability to prefill 856$u with the direct URL of the file
Summary: Add the ability to prefill 856$u with the direct URL of the file
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 22644
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-13 19:22 UTC by Jonathan Druart
Modified: 2019-10-02 09:58 UTC (History)
6 users (show)

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


Attachments
Bug 22508: Add upload_public_path to config (3.46 KB, patch)
2019-03-13 19:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add upload_public_url to config (3.34 KB, patch)
2019-03-13 19:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add the ability to prefill 856$u with the direct URL of the file (11.45 KB, patch)
2019-03-13 19:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add upload_public_path to config (3.50 KB, patch)
2019-04-01 11:42 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 22508: Add upload_public_url to config (3.38 KB, patch)
2019-04-01 11:42 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 22508: Add the ability to prefill 856$u with the direct URL of the file (11.49 KB, patch)
2019-04-01 11:42 UTC, Hugo Agud
Details | Diff | Splinter Review
Bug 22508: Delete the public file on deleting the upload (2.90 KB, patch)
2019-04-05 01:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: add POD (1.96 KB, patch)
2019-04-05 01:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add warnings to about page (5.83 KB, patch)
2019-04-05 01:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add upload_public_path to config (3.50 KB, patch)
2019-05-10 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add upload_public_url to config (3.38 KB, patch)
2019-05-10 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add the ability to prefill 856$u with the direct URL of the file (11.49 KB, patch)
2019-05-10 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Delete the public file on deleting the upload (2.88 KB, patch)
2019-05-10 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: add POD (1.96 KB, patch)
2019-05-10 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add warnings to about page (5.84 KB, patch)
2019-05-10 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22508: Add warnings to about page (5.84 KB, patch)
2019-05-11 17:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-03-13 19:22:59 UTC
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.
Comment 1 Jonathan Druart 2019-03-13 19:25:47 UTC
Created attachment 86583 [details] [review]
Bug 22508: Add upload_public_path to config
Comment 2 Jonathan Druart 2019-03-13 19:26:04 UTC
Created attachment 86584 [details] [review]
Bug 22508: Add upload_public_url to config
Comment 3 Jonathan Druart 2019-03-13 19:26:18 UTC
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.
Comment 4 Marcel de Rooy 2019-03-14 07:22:31 UTC
(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 ?
Comment 5 Jonathan Druart 2019-03-14 19:13:45 UTC
(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.
Comment 6 Marcel de Rooy 2019-03-15 07:00:57 UTC
(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..
Comment 7 Hugo Agud 2019-04-01 11:26:42 UTC
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
Comment 8 Hugo Agud 2019-04-01 11:42:30 UTC
Created attachment 87272 [details] [review]
Bug 22508: Add upload_public_path to config

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 9 Hugo Agud 2019-04-01 11:42:33 UTC
Created attachment 87273 [details] [review]
Bug 22508: Add upload_public_url to config

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 10 Hugo Agud 2019-04-01 11:42:36 UTC
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>
Comment 11 Jonathan Druart 2019-04-05 01:07:53 UTC
Created attachment 87430 [details] [review]
Bug 22508: Delete the public file on deleting the upload
Comment 12 Jonathan Druart 2019-04-05 01:07:58 UTC
Created attachment 87431 [details] [review]
Bug 22508: add POD
Comment 13 Jonathan Druart 2019-04-05 01:08:03 UTC
Created attachment 87432 [details] [review]
Bug 22508: Add warnings to about page
Comment 14 Jonathan Druart 2019-04-05 01:15:53 UTC
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.
Comment 15 Marcel de Rooy 2019-05-10 09:31:12 UTC
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
Comment 16 Jonathan Druart 2019-05-10 12:48:11 UTC
Created attachment 89540 [details] [review]
Bug 22508: Add upload_public_path to config

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 17 Jonathan Druart 2019-05-10 12:48:15 UTC
Created attachment 89541 [details] [review]
Bug 22508: Add upload_public_url to config

Signed-off-by: Hugo Agud <hagud@orex.es>
Comment 18 Jonathan Druart 2019-05-10 12:48:19 UTC
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>
Comment 19 Jonathan Druart 2019-05-10 12:48:23 UTC
Created attachment 89543 [details] [review]
Bug 22508: Delete the public file on deleting the upload
Comment 20 Jonathan Druart 2019-05-10 12:48:26 UTC
Created attachment 89544 [details] [review]
Bug 22508: add POD
Comment 21 Jonathan Druart 2019-05-10 12:48:31 UTC
Created attachment 89545 [details] [review]
Bug 22508: Add warnings to about page
Comment 22 claire.hernandez@biblibre.com 2019-05-10 12:50:34 UTC
(fan of this bz) +1 thanks
Comment 23 Jonathan Druart 2019-05-10 12:51:25 UTC
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.
Comment 24 Katrin Fischer 2019-05-11 17:09:19 UTC
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.
Comment 25 Jonathan Druart 2019-05-11 17:17:29 UTC
(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!
Comment 26 Jonathan Druart 2019-05-11 17:17:51 UTC
Created attachment 89617 [details] [review]
Bug 22508: Add warnings to about page
Comment 27 Marcel de Rooy 2019-05-13 07:21:07 UTC
(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.
Comment 28 Marcel de Rooy 2019-05-13 07:28:14 UTC
> 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.
Comment 29 Marcel de Rooy 2019-05-13 07:42:43 UTC
+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?
Comment 30 Marcel de Rooy 2019-05-13 07:54:33 UTC
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?
Comment 31 Hugo Agud 2019-05-13 08:27:44 UTC
(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?
Comment 32 Fridolin Somers 2019-05-13 12:18:29 UTC
I see there is Bug 22835.
Should we serve also those uploaded file via API ?
In another bug report I mean.
Comment 33 Jonathan Druart 2019-05-13 13:04:48 UTC
(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.
Comment 34 Jonathan Druart 2019-05-13 13:10:36 UTC
(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.
Comment 35 Marcel de Rooy 2019-05-22 13:32:08 UTC
(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.
Comment 36 Marcel de Rooy 2019-05-22 13:33:41 UTC
(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.
Comment 37 Marcel de Rooy 2019-05-22 13:37:40 UTC
(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)..
Comment 38 Hugo Agud 2019-05-22 13:39:53 UTC
the less complexity the better...
Comment 39 Jonathan Druart 2019-05-29 13:10:58 UTC
(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"
Comment 40 Marcel de Rooy 2019-05-29 13:51:36 UTC
(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.
Comment 41 Hugo Agud 2019-05-29 13:58:23 UTC
> 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?
Comment 42 Jonathan Druart 2019-05-29 14:11:54 UTC
(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.
Comment 43 Marcel de Rooy 2019-05-29 14:19:39 UTC
(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
Comment 44 Marcel de Rooy 2019-05-29 14:21:37 UTC
(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..
Comment 45 Nick Clemens 2019-08-09 12:12:20 UTC
(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
Comment 46 Katrin Fischer 2019-10-02 09:58:32 UTC
I've set this to Failed QA since Marcel and Nick agree that a different approach might be better here.