Created attachment 116218 [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. Note: I believe the "wrong filter" warning from the QA is a false positive because the variable in question should not be URI-encoded. Created attachment 116219 [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. 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> Nice! I already had a go at this with bug 25475 but eventually ran out of time for it. *** Bug 25475 has been marked as a duplicate of this bug. *** 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 Created attachment 117310 [details] [review] 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. Can confirm that with the follow up patch, everything continues to work as per the test plan. Code looks good, QA script happy. But somehow the link doesn't get copied. Tested on Firefox Dev edition (Beta) and Chromium stable. Created attachment 119241 [details] [review] Bug 27594: (QA follow-up): Display message only if copy worked And check if writeText is available. 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 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. Created attachment 119305 [details] [review] Bug 27594: (QA follow-up): Display message only if copy worked And check if writeText is available. 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? > 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".
Created attachment 119398 [details] [review] Bug 27594: (follow-up) Check for secure context 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. (In reply to Owen Leonard from comment #18) > If anyone is able to test this I'd appreciate it. Never mind, something is still broken... Created attachment 119906 [details] [review] Bug 27594: (follow-up) Check for secure context 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> 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> Created attachment 119997 [details] [review] Bug 27594: (follow-up) Check for secure context Signed-off-by: David Nind <david@davidnind.com> 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> > 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.
Work, code looks good, QA script happy. Passed :) (In reply to Victor Grousset/tuxayo from comment #24) > Created attachment 120416 [details] [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 hide the link, 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. (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. > 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. Pushed to master for 21.05, thanks to everybody involved! Small useful enhancement, I choose to backport. Pushed to 20.11.x for 20.11.06 Small enhancement, applied cleanly, backported to 20.05.x for 20.05.12 Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed. |
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.