Show Warning, Cancel and Redirect on size > 2GB ; fixes #578 #695
Conversation
There was a problem hiding this comment.
A better location to check the file size is in welcome.js. We can simply show an alert return early from that function.
Yeah, even I realized that right now. I'll have a look at it and make the necessary changes here itself. |
|
@dannycoates Although However displaying the relevant warning to the user in such a case is what is causing the problem. Though I've managed to do that using some script statements in the |
Let's have the check in |
|
@dannycoates Ok, I'll put that in place. Should I also include one for a 0 byte file ? |
That one can stay how it is |
|
OK no problem 👍 |
|
@dannycoates I've now finally introduced alert warnings. I guess this might be it. Here's a preview The demo has been performed on a file size of 200MB (though the final commit contains the limit for 2GB) , reasons for which have been stated above.
Please review |
app/templates/upload.js
Outdated
| `; | ||
| return fileSizeExceededDiv; | ||
| } | ||
| */ |
There was a problem hiding this comment.
please delete the commented code
app/templates/welcome.js
Outdated
| @@ -64,6 +64,14 @@ module.exports = function(state, emit) { | |||
| if (file.size === 0) { | |||
| return; | |||
| } | |||
| if (file.size > 2000000000) { | |||
There was a problem hiding this comment.
please use the MAXFILESIZE global variable instead of 2000000000
app/templates/welcome.js
Outdated
| @@ -64,6 +64,14 @@ module.exports = function(state, emit) { | |||
| if (file.size === 0) { | |||
| return; | |||
| } | |||
| if (file.size > 2000000000) { | |||
| window.alert( | |||
There was a problem hiding this comment.
please use state.translate('fileTooBig', { size: bytes(MAXFILESIZE) }) instead of the hardcoded string. Where bytes is from app/utils.js
|
I've made the required changes @dannycoates. Please review. P.S. why is the build test failing ? |
app/templates/welcome.js
Outdated
| @@ -1,5 +1,7 @@ | |||
| const html = require('choo/html'); | |||
| const assets = require('../../common/assets'); | |||
| const bytes = require('../utils'); | |||
| const config = require('../../server/config'); | |||
| const fileList = require('./fileList'); | |||
| const { fadeOut } = require('../utils'); | |||
There was a problem hiding this comment.
add bytes here const { bytes, fadeOut } = require('../utils'); instead of a separate require
app/templates/welcome.js
Outdated
| @@ -59,11 +61,17 @@ module.exports = function(state, emit) { | |||
|
|
|||
| async function upload(event) { | |||
| event.preventDefault(); | |||
| const MAXFILESIZE = config.max_file_size; | |||
There was a problem hiding this comment.
not quite. The server path in the require is a clue that we shouldn't use that file here. MAXFILESIZE gets set globally in the browser window by https://send.firefox.com/jsconfig.js so we can use it directly here.
There was a problem hiding this comment.
Using the variable directly gives the following error
67:21 error 'MAXFILESIZE' is not defined no-undef
68:7 warning Unexpected alert no-alert
68:64 error 'MAXFILESIZE' is not defined no-undef
And this prevents any commits.
So, we need to do something different I guess
However, If this is the correct approach maybe then I shall edit the file directly on github itself
There was a problem hiding this comment.
This is the linter complaining. Try adding /* global MAXFILESIZE */ as the first line of the file.
There was a problem hiding this comment.
Correct, Its working now. I've pushed the commit please have a look.
There was a problem hiding this comment.
Also, I would squash my commits (if you want) once the PR is ready to be merged.
included global MAXFILESIZE
shikhar-scs
force-pushed
the
warning-for-high-file-size
branch
from
ffb7bb9 to
9501c1c
Compare
|
@dannycoates All the commits have been squashed, this PR is ready to be merged. |
|
Thanks @shikhar-scs 😄 |
As mentioned by @SoftVision-CosminMuntean and @johngruen at #578, this PR resolves issues related to file uploads of sizes > 2GB. It displays the warning message, cancels the upload and further redirects back to the home page.
The following preview has been generated for file sizes > 200MB as my laptop crashes for any file having size > 1 GB. However, this limit can be very easily altered as per our needs.
Here is the preview (its a gif file so please be patient while it loads)
@dannycoates @SoftVision-CosminMuntean @johngruen Please review and suggest changes.