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

feat: Increase attachment size options #668

Merged
merged 4 commits into from
Nov 19, 2020
Merged

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Nov 18, 2020

Problem

Closes #662

Solution

  • Expand options for attachment size up to 20mb in intervals of 1mb
  • Limit options for email mode forms to 7mb

Before & After Screenshots

BEFORE:

Screenshot 2020-11-18 at 10 46 41 AM
AFTER:
(email mode)
Screenshot 2020-11-18 at 2 02 04 PM

(storage mode)
Screenshot 2020-11-18 at 10 46 28 AM

Tests

(tested on IE)

  • Create storage mode form. Check that you can set max attachment size up to 20mb. Publish form. Upload file of ~15mb. Check that upload succeeds and you can retrieve it from admin data tab.
  • Check that it is not possible to have total attachments across multiple fields exceeding 20mb.
  • Create email mode form. Check that you can only set max attachment size up to 7mb. Upload file of ~6mb. Check that upload succeeds and you can receive it in your inbox.
  • Check that it is not possible to have total attachments across multiple fields exceeding 7mb
    .

@tshuli tshuli force-pushed the increase-attachment-size branch from aef96c7 to 9b22f03 Compare November 18, 2020 06:10
@tshuli tshuli requested review from karrui and mantariksh November 18, 2020 14:26
@tshuli tshuli marked this pull request as ready for review November 18, 2020 14:27
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

not a fan of being so explicit in the allowed array, hardcoding 1 to 20, but I concede that backwards compatibility probably trumps my opinion.

lgtm

@tshuli tshuli merged commit dc86dc4 into develop Nov 19, 2020
@tshuli tshuli deleted the increase-attachment-size branch November 19, 2020 05:58
'17 MB (e.g. Video files)',
'18 MB (e.g. Video files)',
'19 MB (e.g. Video files)',
'20 MB (e.g. Video files)',
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to reword these to stop repeating examples - doesn't make much sense UI-wise.

SeventeenMb = '17',
EighteenMb = '18',
NineteenMb = '19',
TwentyMb = '20',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is an enum still the best representation, if we allow every integer in the range? Should we model this differently?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase attachment size options
3 participants