-
-
Notifications
You must be signed in to change notification settings - Fork 928
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
SAK-49199 Mailsender check for exceeding attachment size client side #11838
Conversation
9bb161a
to
bfbf26e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see vanilla js used here instead of jquery. I realise jquery is used elsewhere in these files, but it is a dying library. So many of jquery's awesome utilities are now available in vanilla js. Also, try and use const more often in the JS. It makes code a lot easier to understand.
public long getUploadMax() | ||
{ | ||
long uploadMax = (long) (serverConfigurationService.getInt(EmailService.MAIL_SENDFROMSAKAI_MAXSIZE, EmailService.DEFAULT_MAXSIZE)); | ||
return uploadMax; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just return an int here. Why cast to a long when the SCS returns an int? Also, just make it one line, you may as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
* | ||
* @return | ||
*/ | ||
long getUploadMax(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just return int here. Also, I'd remove the comment and just call the method getMaxUploadFileSize. No doc needed then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
@@ -168,3 +168,16 @@ h3.insBorder { | |||
.user-label { | |||
font-weight: normal; | |||
} | |||
.disableAttachMoreIcon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use hyphen separation here. Our more recent css uses that convention. So, .disable-attach-more-icon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
opacity: 0.5; | ||
} | ||
|
||
.disableAttachMoreLink { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same hyphen approach here, imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
|
||
function formatFileSize(bytes) { | ||
if(bytes === 0) { | ||
return '0 Bytes'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be internationalised?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
return parseFloat((bytes / Math.pow(k, i)).toFixed(1)) + ' ' + sizes[i]; | ||
} | ||
|
||
function msg(s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These s and m names could be expanded a little to make this code more understandable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
|
||
function accumulateAndCheckAttachmentSize() { | ||
let wasOverUploadSize = accumulatedFileSize > maxFileUploadSize ; | ||
let isOverUploadSize = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let isOverUploadSize = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
accumulatedFileSize += $(this)[0].files[0].size; | ||
}); | ||
|
||
isOverUploadSize = accumulatedFileSize > maxFileUploadSize ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isOverUploadSize = accumulatedFileSize > maxFileUploadSize ; | |
const isOverUploadSize = accumulatedFileSize > maxFileUploadSize ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
accumulatedFileSize = 0; | ||
|
||
let targetFiles = $('.emailattachment').filter(function(i, ele) { return ele.files.length > 0;}); | ||
targetFiles.each(function (i) { | ||
accumulatedFileSize += $(this)[0].files[0].size; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accumulatedFileSize = 0; | |
let targetFiles = $('.emailattachment').filter(function(i, ele) { return ele.files.length > 0;}); | |
targetFiles.each(function (i) { | |
accumulatedFileSize += $(this)[0].files[0].size; | |
}); | |
accumulatedFileSize = [...document.querySelectorAll(".emailattachment")].filter(el => el.files.length > 0).reduce((acc, el) => acc + el.files[0].size, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
$('#attach-size-error').hide(); | ||
$("input[type=submit]").removeAttr('disabled'); | ||
$('#attachOuter img[alt="attachment_img"]').removeClass("disableAttachMoreIcon"); | ||
$('#attachMoreLink button').removeClass("disableAttachMoreLink"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$('#attach-size-error').hide(); | |
$("input[type=submit]").removeAttr('disabled'); | |
$('#attachOuter img[alt="attachment_img"]').removeClass("disableAttachMoreIcon"); | |
$('#attachMoreLink button').removeClass("disableAttachMoreLink"); | |
document.getElementById("attach-size-error").style.display = "none"; | |
document.querySelectorAll("input[type=submit]").forEach(el => el.disabled = false; | |
document.querySelector("#attachOuter img[alt='attachment_img']").classList.remove("disableAttachMoreIcon"); | |
document.querySelector('#attachMoreLink button').classList.remove("disableAttachMoreLink"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
0a0b0d0
to
e1a2591
Compare
var accumulatedFileSize = 0; | ||
var maxFileUploadSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mylescarey2019 can you use let or const here vs var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move those functions into a mailsender specific scope? If you don't they could clash with other scripts in Sakai.
} | ||
} | ||
|
||
function emailattachmentChange() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should move this function definition into the MailSender function so it is scoped at that level, and not at the global level. Same thing for all these functions - they are all being declared on the window or globalThis object and could be masked by other js loaded by Sakai.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion completed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrianfish - fyi - latest suggestions have been implemented
e1a2591
to
d80c701
Compare
d80c701
to
c217cce
Compare
@ern - fyi - latest suggestions have been implemented |
Adds client side attachment file size testing vs sakai.properties mail.sendfromsakai.maxsize limit (or the default value of 25MB). If sum of attachments exceeds the limit a warning banner is presented instructing user to remove one or more file until below the limit. Along with warning banner, control to add more files is disabled.