Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

user notification for file uploads > 4GB in IE11 #27004

Merged
merged 1 commit into from
Jan 24, 2017

Conversation

hurradieweltgehtunter
Copy link
Contributor

Description

Notify users with IE11 to use desktop client for uploading files bigger than 4GB.

Related Issue

https://github.com/owncloud/enterprise/issues/1779

Motivation and Context

IE11 can't handle file uploads bigger than 4GB

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@CLAassistant
Copy link

CLAassistant commented Jan 23, 2017

CLA assistant check
All committers have signed the CLA.

@mention-bot
Copy link

@hurradieweltgehtunter, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic, @PVince81 and @DeepDiver1975 to be potential reviewers.

ieversion = 0;

var ua = window.navigator.userAgent;
var trident = ua.indexOf('Trident/');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix indent - we use tabs

@@ -28,6 +28,7 @@ OC.L10N.register(
"Upload cancelled." : "Upload cancelled.",
"Unable to upload {filename} as it is a directory or has 0 bytes" : "Unable to upload {filename} as it is a directory or has 0 bytes",
"Total file size {size1} exceeds upload limit {size2}" : "Total file size {size1} exceeds upload limit {size2}",
"Total file size {size1} exceeds your browser upload limit. Please use the owncloud desktop client to upload files bigger than 4GB." : "Total file size {size1} exceeds your browser upload limit. Please use the owncloud desktop client to upload files bigger than 4GB.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove - translations are handled via tranisfex

@@ -26,6 +26,7 @@
"Upload cancelled." : "Upload cancelled.",
"Unable to upload {filename} as it is a directory or has 0 bytes" : "Unable to upload {filename} as it is a directory or has 0 bytes",
"Total file size {size1} exceeds upload limit {size2}" : "Total file size {size1} exceeds upload limit {size2}",
"Total file size {size1} exceeds your browser upload limit. Please use the owncloud desktop client to upload files bigger than 4GB." : "Total file size {size1} exceeds your browser upload limit. Please use the owncloud desktop client to upload files bigger than 4GB.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove - translations are handled via tranisfex

if (file.size > 4187593113) {
data.textStatus = 'sizeexceedbrowserlimit';
data.errorThrown = t('files',
'Total file size {size1} exceeds your browser upload limit. Please use the owncloud desktop client to upload files bigger than 4GB.', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

owncloud has to be passed in as argument due to whitelabling

@DeepDiver1975 DeepDiver1975 added this to the 9.1.4 milestone Jan 23, 2017
@michaelstingl
Copy link

@hurradieweltgehtunter Plz check if your size calculation matches the one from ownCloud.

@DeepDiver1975
Copy link
Member

  • please add unit tests
  • generally speaking we implement new features and fixes on master and backport later - please respect this next time - THX
  • please click on the CLA button - THX

'size1': humanFileSize(file.size)
});
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird indents

@@ -336,6 +336,35 @@ OC.Upload = {
data.errorThrown = errorMessage;
}

// detect browser and version to handle IE11 upload file size limit
// $.browser detects "mozilla" for IE11, which in this case we use window.navigator.userAgent
ie = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use var to declare these variables locally, else they'd be global

}

// check browser and version
if (ie && ieversion == 11) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're doing this often, might be worth adding this as utility function to the OC.Util namespace which is currently in core/js/js.js (there is already a isIE() function there)

@michaelstingl
Copy link

@DeepDiver1975 Fix shouldn't be required for oC 10 (#26306)

// detect browser and version to handle IE11 upload file size limit
if (OC.Util.isIE11()) {
// Check for the various File API support.
if (window.File && window.FileReader && window.FileList && window.Blob) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really required or can we assume that IE11 has these ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ blob - http://caniuse.com/#search=Blob

please look up the rest @hurradieweltgehtunter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is completely unneeded because none of the checks below is using any of these APIs.

I'll remove it.

@@ -583,7 +601,7 @@ OC.Upload = {
+ '</span><span class="mobile">'
+ t('files', '...')
+ '</span></em>');
$('#uploadprogressbar').tipsy({gravity:'n', fade:true, live:true});
$('#uploadprogressbar').tipsy({gravity:'n', fade:true, live:true});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hurradieweltgehtunter mind fixing this bad indent as well ?

@PVince81
Copy link
Contributor

I'll take care of the unit tests, it's a bit complicated.

data.errorThrown = t('files',
'Total file size {size1} exceeds your browser upload limit. Please use the {ownCloud} desktop client to upload files bigger than {size2}.', {
'size1': humanFileSize(file.size),
'ownCloud' : 'ownCloud',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to use product name in case of white labeling

@PVince81
Copy link
Contributor

Added missing tests and fix theme name thingy

@PVince81
Copy link
Contributor

@michaelstingl @davitol can you help test this in IE11 and non-IE11 envs with big files ?

@DeepDiver1975
Copy link
Member

@hurradieweltgehtunter any please squash your 4 commits into one - thx

@PVince81 PVince81 force-pushed the stable9.1-IE-large-file-notification branch from 63b3583 to 5e4f2b2 Compare January 24, 2017 14:57
@PVince81
Copy link
Contributor

PVince81 commented Jan 24, 2017

Rebased and squashed.

Please review and test.

@davitol
Copy link
Contributor

davitol commented Jan 24, 2017

👍 "Total file size 3.9 GB exceeds your browser upload limit. Please use the ownCloud desktop client to upload files bigger than 3.9 GB." message appears

@PVince81
Copy link
Contributor

Travis, what's taking you so long ? Even Jenkins was faster...

Anyway, Travis doesn't contain any JS related tests. Merging.

@PVince81 PVince81 merged commit 32b5c6c into stable9.1 Jan 24, 2017
@PVince81 PVince81 deleted the stable9.1-IE-large-file-notification branch January 24, 2017 20:30
@lock
Copy link

lock bot commented Aug 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants