Upload check #997
Upload check #997
Conversation
change header code to 204 to avoid spurious error messages
Looks like the original website and repo for this went away. Were you able to find another copy somewhere that we can update |
The one I looked at has gone away now. this one seems to have the same information. |
scripts/file_resume.js
Outdated
| }); | ||
| } | ||
|
|
||
| // polyfill for String.padStart(2, "0") |
There was a problem hiding this comment.
do we need a polyfil? The support looks greater than our supported browser matrix:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padStart
https://www.pgdp.net/wiki/DP_Official_Documentation:General/Supported_Browsers
There was a problem hiding this comment.
Looks like we would have to support Chrome 57 but we only do 54
There was a problem hiding this comment.
Ah, missed that. I'm fine with keeping this, but do we know why we support chrome 54 specifically? Was it the last versino supported on xp or something like that? It was released in 2016.
There was a problem hiding this comment.
I don't know. According to browserstack xp only supports chrome 49. xp supports firefox 52 which is the reason for that and firefox 52 does PadStart.
There was a problem hiding this comment.
DP's supported browser list was last updated two-ish years ago in October of 2021. See Supported Browsers, and note the comment about Safari, Chrome and Opera on macOS: I couldn't find an official end-of-life schedule for macOS, but it looks like a three-year cycle. Big Sur (macOS 11) is no longer supported now.
How rigorous do we want to be in updating our supported browser list?
There was a problem hiding this comment.
To not boil the ocean, for this PR I think we should bump our minimum Chrome version to 57 and drop the polyfill. Based on the platform support numbers for Chrome 52 -> 57 doesn't cross any threshold and seems very sensible.
There was a problem hiding this comment.
I'm happy to revisit all of our minimum browser versions if we need to, but it doesn't sound like this warrants that extra effort.
| @@ -23,6 +23,42 @@ window.addEventListener("DOMContentLoaded", function() { | |||
| return true; | |||
| } | |||
|
|
|||
| // polyfill for file.arrayBuffer() | |||
| function fileToArrayBuffer(file) { | |||
There was a problem hiding this comment.
same here, do we need a polyfil? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer
There was a problem hiding this comment.
It's File:arrayBuffer() we need which seems to be same as Blob: arrayBuffer() which is not supported by any of the browsers we can use.
scripts/file_resume.js
Outdated
| showProgress(uploadMessages.working); | ||
| ev.preventDefault(); | ||
| fileHash(file) |
There was a problem hiding this comment.
Good point. We should. Perhaps just upload an empty string in this case and let the server say there was no checksum provided.
As luck would have it, I actually have a checkout of the old repo on TEST. And I found https://github.com/QuanTemplate/resumable.js/commits/master/ which appears to be a newer fork of the original repo (last commit is by original repo owner). I'll get a fork of that made in DistributedProofreaders so we have it. |
|
We now have a fork in https://github.com/DistributedProofreaders/resumable.js with an updated |
and let host report no checksum provided
|
Sandbox updated. |
|
We should also bump minimum Safari version from 10 to 11 and Opera from 41 to 44. |
Minimum version for Safari is already at 11. And confirmed the other two. I was already checking out browserstack, because I was thinking I still needed to be checking out the broken non-resumable issue (only to discover that I had to go to XP on a browser no longer supported to be able to find a browser version that didn't support resumables (of the browsers that we support). And I didn't get an error message from it. |
|
https://www.pgdp.net/wiki/DP_Official_Documentation:General/Supported_Browsers has been updated with the new minimum versions. |
This makes a checksum for the uploaded file, uploads it and checks it with the file received by the server.
The javascript function is based on the example in this mozilla document reworked for the browsers we support which cannot use async, await, File.arrayBuffer() or String.padStart().
This checking should not really be necessary because lower levels of the internet protocol should already validate data but two users have encountered errors given by the zip validator so this is intended to discover if the problem is caused by data corruption or something else.
The change to upload_resumable_file.php is recommended in the resumable docs to avoid unnecessary error messages in the console.
Sandbox at: https://www.pgdp.org/~rp31/c.branch/upload_check