Bug 27594 - Add access to public download link for publicly-accessible uploads
Summary: Add access to public download link for publicly-accessible uploads
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords: Manual
: 25475 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-02-02 14:09 UTC by Owen Leonard
Modified: 2021-05-11 14:55 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This patch adds a link to the display of publicly-accessible downloads so that the public link can be copied.
Version(s) released in:
21.05.00,20.11.06


Attachments
Bug 27594: Add access to public download link for publicly-accessible uploads (4.82 KB, patch)
2021-02-02 14:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27594: Add access to public download link for publicly-accessible uploads (4.69 KB, patch)
2021-02-02 14:57 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27594: Add access to public download link for publicly-accessible uploads (4.74 KB, patch)
2021-02-25 03:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 27594: (QA follow-up): Use clipboard API (2.97 KB, patch)
2021-02-25 13:30 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27594: (QA follow-up): Display message only if copy worked (1.37 KB, patch)
2021-04-06 18:13 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 27594: Add access to public download link for publicly-accessible uploads (3.56 KB, patch)
2021-04-07 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27594: (QA follow-up): Display message only if copy worked (1.37 KB, patch)
2021-04-07 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27594: (follow-up) Check for secure context (2.19 KB, patch)
2021-04-09 15:45 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27594: (follow-up) Check for secure context (2.15 KB, patch)
2021-04-20 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 27594: Add access to public download link for publicly-accessible uploads (3.62 KB, patch)
2021-04-22 11:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 27594: (QA follow-up): Display message only if copy worked (1.42 KB, patch)
2021-04-22 11:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 27594: (follow-up) Check for secure context (2.19 KB, patch)
2021-04-22 11:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 27594: (follow-up) don't display "Copy link[...]" when not possible (1.37 KB, patch)
2021-05-03 20:46 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2021-02-02 14:09:56 UTC
When an upload has been marked as public ("Allow public downloads"), we should make it easy to get the publicly-accessible link for that file. I propose to add a link for automatically copying the URL to the clipboard.
Comment 1 Owen Leonard 2021-02-02 14:48:37 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2021-02-02 14:57:14 UTC Comment hidden (obsolete)
Comment 3 David Nind 2021-02-25 03:17:54 UTC
Created attachment 117289 [details] [review]
Bug 27594: Add access to public download link for publicly-accessible uploads

This patch adds a link to the display of publicly-accessible uploads so
that the user can copy the public link. This will hopefully lead to
easier use of sound file uploads in the Audio Alerts system.

To test, apply the patch and go to Tools -> Upload.

- If necessary, upload a file with the "Allow public downloads"
  checkbox checked.
- Upon upload you should be shown the results of your upload in a table
  with the file details.
- In the "Public" column you should see that "Yes" is a link.
  - When you hover your mouse over the link you should see a tooltip,
    "Copy link to this file."
  - When you click the link the tooltip should say "Link copied to the
    clipboard."
  - Verify that the correct link has been copied.
- Test uploading a file which is not publicly-accessible.
  - Confirm that the information in the "Public" column says "No" and is
    not a link.

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-02-25 03:18:10 UTC
Nice!
Comment 5 David Cook 2021-02-25 04:46:37 UTC
I already had a go at this with bug 25475 but eventually ran out of time for it.
Comment 6 David Cook 2021-02-25 04:46:56 UTC
*** Bug 25475 has been marked as a duplicate of this bug. ***
Comment 7 David Cook 2021-02-25 04:58:57 UTC
Oh I guess I could QA this...

I'm not sure what I think of the copToClipboard function. It seems pretty hacky.

What are your thoughts on the clipboard API that I was using at https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=25475&attachment=111296

These days, I don't think we need to worry much about non-modern browsers?

Going to leave as Signed Off as I don't think it warrants a Fail or an In Discussion
Comment 8 Owen Leonard 2021-02-25 13:30:38 UTC Comment hidden (obsolete)
Comment 9 David Nind 2021-02-25 14:02:16 UTC
Can confirm that with the follow up patch, everything continues to work as per the test plan.
Comment 10 Victor Grousset/tuxayo 2021-04-06 18:12:15 UTC
Code looks good, QA script happy. But somehow the link doesn't get copied. Tested on Firefox Dev edition (Beta) and Chromium stable.
Comment 11 Victor Grousset/tuxayo 2021-04-06 18:13:00 UTC Comment hidden (obsolete)
Comment 12 Victor Grousset/tuxayo 2021-04-06 18:26:56 UTC
It works, due to Secure Contexts[1] it wasn't working by default in my dev env.
Dirty workaround: in /etc/host, make localhost point to the ip of the local Koha. Don't forget to undo it, otherwise it will mess things up in the system.

[1] https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts
Comment 13 Jonathan Druart 2021-04-07 14:15:31 UTC
Created attachment 119304 [details] [review]
Bug 27594: Add access to public download link for publicly-accessible uploads

This patch adds a link to the display of publicly-accessible uploads so
that the user can copy the public link. This will hopefully lead to
easier use of sound file uploads in the Audio Alerts system.

To test, apply the patch and go to Tools -> Upload.

- If necessary, upload a file with the "Allow public downloads"
  checkbox checked.
- Upon upload you should be shown the results of your upload in a table
  with the file details.
- In the "Public" column you should see that "Yes" is a link.
  - When you hover your mouse over the link you should see a tooltip,
    "Copy link to this file."
  - When you click the link the tooltip should say "Link copied to the
    clipboard."
  - Verify that the correct link has been copied.
- Test uploading a file which is not publicly-accessible.
  - Confirm that the information in the "Public" column says "No" and is
    not a link.

Signed-off-by: David Nind <david@davidnind.com>

Bug 27594: (QA follow-up): Use clipboard API

While the Clipboard API doesn't
have 100% coverage (https://www.caniuse.com/?search=clipboard), it has
enough coverage for the "copy" operation that it can be counted on for
use in the staff interface.

This patch removes the global copyToClipboard function and replaces it
with a call to ndavigator.clipboard.writeText() in the template.

To test, follow the previous test plan and confirm that the public
download link is still copied correctly to the clipboard.
Comment 14 Jonathan Druart 2021-04-07 14:15:36 UTC
Created attachment 119305 [details] [review]
Bug 27594: (QA follow-up): Display message only if copy worked

And check if writeText is available.
Comment 15 Jonathan Druart 2021-04-07 14:24:34 UTC
It does not work for me, after some debug (console.log(navigator.clipboard.writeText);):
  Uncaught TypeError: navigator.clipboard is undefined

I guess I am facing the same issue than Victor. Shouldn't we display the "Copy link to this file" tooltip only if we can actually copy it?
Comment 16 Victor Grousset/tuxayo 2021-04-08 18:00:19 UTC
> Shouldn't we display the "Copy link to this file" tooltip only if we can actually copy it?

Good point, there might be a fair number of Kohas without HTTPS.

An alternative to not showing the tooltip is to change the message to something like "Copying the link if only available if Koha uses HTTPS".
Comment 17 Owen Leonard 2021-04-09 15:45:10 UTC Comment hidden (obsolete)
Comment 18 Owen Leonard 2021-04-20 12:13:04 UTC
I was hoping to test this last followup on a sandbox, but the PTFS-E sandboxes don't appear to accept uploads, and the ByWater sandboxes have been down for ages. 

I don't know how else to test this in an environment that isn't considered by the browser to be secure for the purposes of copy and paste operations (as local addresses are).

If anyone is able to test this I'd appreciate it.
Comment 19 Owen Leonard 2021-04-20 12:16:42 UTC Comment hidden (obsolete)
Comment 20 Owen Leonard 2021-04-20 12:24:25 UTC
Created attachment 119906 [details] [review]
Bug 27594: (follow-up) Check for secure context
Comment 21 David Nind 2021-04-22 11:35:37 UTC
Created attachment 119995 [details] [review]
Bug 27594: Add access to public download link for publicly-accessible uploads

This patch adds a link to the display of publicly-accessible uploads so
that the user can copy the public link. This will hopefully lead to
easier use of sound file uploads in the Audio Alerts system.

To test, apply the patch and go to Tools -> Upload.

- If necessary, upload a file with the "Allow public downloads"
  checkbox checked.
- Upon upload you should be shown the results of your upload in a table
  with the file details.
- In the "Public" column you should see that "Yes" is a link.
  - When you hover your mouse over the link you should see a tooltip,
    "Copy link to this file."
  - When you click the link the tooltip should say "Link copied to the
    clipboard."
  - Verify that the correct link has been copied.
- Test uploading a file which is not publicly-accessible.
  - Confirm that the information in the "Public" column says "No" and is
    not a link.

Signed-off-by: David Nind <david@davidnind.com>

Bug 27594: (QA follow-up): Use clipboard API

While the Clipboard API doesn't
have 100% coverage (https://www.caniuse.com/?search=clipboard), it has
enough coverage for the "copy" operation that it can be counted on for
use in the staff interface.

This patch removes the global copyToClipboard function and replaces it
with a call to ndavigator.clipboard.writeText() in the template.

To test, follow the previous test plan and confirm that the public
download link is still copied correctly to the clipboard.

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2021-04-22 11:35:43 UTC
Created attachment 119996 [details] [review]
Bug 27594: (QA follow-up): Display message only if copy worked

And check if writeText is available.

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 David Nind 2021-04-22 11:35:48 UTC
Created attachment 119997 [details] [review]
Bug 27594: (follow-up) Check for secure context

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 Victor Grousset/tuxayo 2021-05-03 20:46:19 UTC
Created attachment 120416 [details] [review]
Bug 27594: (follow-up) don't display "Copy link[...]" when not possible

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 25 Victor Grousset/tuxayo 2021-05-03 20:48:15 UTC
> I don't know how else to test this in an environment that isn't considered by the browser to be secure for the purposes of copy and paste operations (as local addresses are).

In koha-testing-docker, running "ip a" allows to get the IP of the container.
And on the host it works to access it. As opposed to localhost, it's not considered a secure context.

Thanks for proposing to test in a non secure context. There was in fact a bug.
Comment 26 Victor Grousset/tuxayo 2021-05-03 21:36:37 UTC
Work, code looks good, QA script happy. Passed :)
Comment 27 Jonathan Druart 2021-05-04 13:16:25 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2021-05-04 13:17:18 UTC
(In reply to Victor Grousset/tuxayo from comment #24)
> Created attachment 120416 [details] [review] [review] [review]
> Bug 27594: (follow-up) don't display "Copy link[...]" when not possible
> 
> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

I don't understand this patch.
It does remove the tooltip, not only when the copy is not possible, but always.

I was expecting to see some JS code to check navigator.clipboard and add the link if defined.
Comment 29 Victor Grousset/tuxayo 2021-05-04 15:50:07 UTC
> It does remove the tooltip, not only when the copy is not possible, but always.

It adds the tooltip when window.isSecureContext true
In my last patch I removed the tooltip in the HTML which said "Copy link to this file" thus saying it even if the window.isSecureContext false. So no tooltip in this case.
Should we have a tooltip for when the copy isn't possible?


> I was expecting to see some JS code to check navigator.clipboard and add the link if defined.

The link is always there. But disabled when window.isSecureContext true. To instead call the clipboard code.


I double checked with:
git diff upstream/master..HEAD
when bug applied to see the overall diff.
Comment 30 Jonathan Druart 2021-05-07 12:41:30 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 31 Fridolin Somers 2021-05-11 14:55:28 UTC
Small useful enhancement, I choose to backport.

Pushed to 20.11.x for 20.11.06