SAK-49199 Mailsender check for exceeding attachment size client side #11838
Conversation
mylescarey2019
force-pushed
the
SAK-49199
branch
2 times, most recently
from
9bb161a
to
bfbf26e
Compare
There was a problem hiding this comment.
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.
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.
suggestion completed
| * | ||
| * @return | ||
| */ | ||
| long getUploadMax(); |
There was a problem hiding this comment.
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.
suggestion completed
| @@ -168,3 +168,16 @@ h3.insBorder { | |||
| .user-label { | |||
| font-weight: normal; | |||
| } | |||
| .disableAttachMoreIcon { | |||
There was a problem hiding this comment.
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.
suggestion completed
| opacity: 0.5; | ||
| } | ||
|
|
||
| .disableAttachMoreLink { |
There was a problem hiding this comment.
Same hyphen approach here, imo.
There was a problem hiding this comment.
suggestion completed
|
|
||
| function formatFileSize(bytes) { | ||
| if(bytes === 0) { | ||
| return '0 Bytes'; |
There was a problem hiding this comment.
Should this be internationalised?
There was a problem hiding this comment.
suggestion completed
| return parseFloat((bytes / Math.pow(k, i)).toFixed(1)) + ' ' + sizes[i]; | ||
| } | ||
|
|
||
| function msg(s) { |
There was a problem hiding this comment.
These s and m names could be expanded a little to make this code more understandable.
There was a problem hiding this comment.
suggestion completed
|
|
||
| function accumulateAndCheckAttachmentSize() { | ||
| let wasOverUploadSize = accumulatedFileSize > maxFileUploadSize ; | ||
| let isOverUploadSize = false; |
There was a problem hiding this comment.
| let isOverUploadSize = false; |
There was a problem hiding this comment.
suggestion completed
| accumulatedFileSize += $(this)[0].files[0].size; | ||
| }); | ||
|
|
||
| isOverUploadSize = accumulatedFileSize > maxFileUploadSize ; |
There was a problem hiding this comment.
| isOverUploadSize = accumulatedFileSize > maxFileUploadSize ; | |
| const isOverUploadSize = accumulatedFileSize > maxFileUploadSize ; |
There was a problem hiding this comment.
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.
| 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.
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.
| $('#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.
suggestion completed
mylescarey2019
force-pushed
the
SAK-49199
branch
2 times, most recently
from
0a0b0d0
to
e1a2591
Compare
| var accumulatedFileSize = 0; | ||
| var maxFileUploadSize; |
There was a problem hiding this comment.
Hi @mylescarey2019 can you use let or const here vs var?
There was a problem hiding this comment.
suggestion completed
| } | ||
| } | ||
|
|
||
| function emailattachmentChange() { |
There was a problem hiding this comment.
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.
suggestion completed
There was a problem hiding this comment.
@adrianfish - fyi - latest suggestions have been implemented
mylescarey2019
force-pushed
the
SAK-49199
branch
from
e1a2591
to
d80c701
Compare
mylescarey2019
force-pushed
the
SAK-49199
branch
from
d80c701
to
c217cce
Compare
|
@ern - fyi - latest suggestions have been implemented |
ern
changed the title
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.