Limit user photo uploads to 512x512 pixels #2004

alicewriteswrongs · 2016-12-06T19:21:53Z

What are the relevant tickets?

What's this PR do?

This implements a function that uses <canvas> to limit the maximum dimensions of user photos to 1024x1024 (right now, we can use whatever number we want).

Where should the reviewer start?

How should this be manually tested?

Probably just ensure that even if you throttle your connection it is possible to upload an image? hopefully...

Any background context you want to provide?

Screenshots (if appropriate)

What GIF best describes this PR or how it makes you feel?

alicewriteswrongs · 2016-12-06T19:38:19Z

With this change I can upload a photo on both the simulated 2G and GPRC throttling in chromium on the PR environment.

codecov-io · 2016-12-06T19:41:07Z

Current coverage is 91.41% (diff: 83.01%)

Merging #2004 into master will decrease coverage by 0.12%

@@             master      #2004   diff @@
==========================================
  Files           287        287          
  Lines         20330      20370    +40   
  Methods        2477       2482     +5   
  Messages          0          0          
  Branches       1802       1805     +3   
==========================================
+ Hits          18611      18622    +11   
- Misses         1690       1715    +25   
- Partials         29         33     +4

Powered by Codecov. Last update 32470cd...949e00c

noisecapella · 2016-12-06T19:54:54Z

Code looks fine, looking into testing on different browsers

noisecapella · 2016-12-06T20:09:08Z

👍 from me. Phone is more responsive

codecov-io · 2016-12-06T20:12:10Z

Current coverage is 91.53% (diff: 71.42%)

Merging #2004 into master will decrease coverage by <.01%

@@             master      #2004   diff @@
==========================================
  Files           289        289          
  Lines         20411      20414     +3   
  Methods        2481       2481          
  Messages          0          0          
  Branches       1806       1806          
==========================================
+ Hits          18684      18686     +2   
+ Misses         1698       1695     -3   
- Partials         29         33     +4

Powered by Codecov. Last update 2204c72...f7491c4

alicewriteswrongs · 2016-12-06T20:38:56Z

not working yet on Safari, seems like it's an encoding issue.

singingwolfboy · 2016-12-06T20:53:16Z

static/js/util/util.js

+    const ctx: any = canvas.getContext("2d");
+    ctx.drawImage(img, 0, 0, width, height);
+
+    return canvas.toBlob(saveCallback, 'image/jpeg');


This code is always using JPEG format. Wouldn't it be better to use whatever format the incoming image had? If the user tries to upload a PNG file, maybe it should stay as a PNG file.

We want to use JPEG because it is a substantial reduction in size over PNG. I merged a change yesterday which switched that over.

alicewriteswrongs · 2016-12-07T20:55:05Z

@noisecapella this is ready for further review, and things are working on iOS. The PNG option you suggested didn't pan out, so what I ended up doing is just not attempting to do any resizing on safari or iOS.

pdpinch · 2016-12-07T21:03:55Z

Ugh. Doesn't that defeat the purpose -- especially on iOS?

alicewriteswrongs · 2016-12-07T21:04:22Z

Yes, but I could not find a way to make it work.

noisecapella · 2016-12-09T16:51:05Z

static/js/components/CropperWrapper.js

+        height: 512,
+      });
    }
+    canvas.toBlob(blob => updatePhotoEdit(blob), 'image/jpeg');


We used to check for toBlob here. Will it always exist?

yes, we have a polyfill for it.

noisecapella · 2016-12-09T17:05:51Z

If the image is small this will increase the size to 512x512. That might be fine but just wanted to bring it up

IE Chrome, Firefox, and Chrome on Android all work. Can someone do a final test for OS X and iOS?

alicewriteswrongs · 2016-12-09T17:08:05Z

It works on my iPhone, not sure about Safari (on macOS I mean). @giocalitri would you mind testing to see if you can upload a profile image on the PR build?

giocalitri · 2016-12-09T17:16:40Z

it works on Safari on Mac OS

noisecapella

👍

This limits the profile image size on the client, by scaling down the canvas used in the `<Cropper />`. This doesn't work on Safari and on iOS, so on those platforms we just upload the image as-is. pr #2004

alicewriteswrongs added the Work in Progress label Dec 6, 2016

odlbot temporarily deployed to micromasters-ci-pr-2004 December 6, 2016 19:21 Inactive

alicewriteswrongs added Needs Review and removed Work in Progress labels Dec 6, 2016

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 6, 2016 19:48 Inactive

alicewriteswrongs force-pushed the ap/max-file-size branch from 6752416 to 2f569a6 Compare December 6, 2016 20:37

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 6, 2016 20:37 Inactive

singingwolfboy reviewed Dec 6, 2016

View reviewed changes

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 6, 2016 20:59 Inactive

alicewriteswrongs force-pushed the ap/max-file-size branch from 025905e to 28c21db Compare December 6, 2016 21:20

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 6, 2016 21:20 Inactive

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 6, 2016 21:39 Inactive

alicewriteswrongs force-pushed the ap/max-file-size branch from 237f684 to 260fcb6 Compare December 6, 2016 21:42

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 6, 2016 21:42 Inactive

alicewriteswrongs force-pushed the ap/max-file-size branch from 260fcb6 to 7887efa Compare December 6, 2016 22:22

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 6, 2016 22:22 Inactive

alicewriteswrongs force-pushed the ap/max-file-size branch from 7887efa to b6043d8 Compare December 7, 2016 15:35

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 15:35 Inactive

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 15:54 Inactive

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 16:24 Inactive

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 16:36 Inactive

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 16:47 Inactive

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 16:54 Inactive

alicewriteswrongs force-pushed the ap/max-file-size branch from b7676c3 to e13a110 Compare December 7, 2016 18:47

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 20:26 Inactive

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 20:27 Inactive

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 20:36 Inactive

alicewriteswrongs force-pushed the ap/max-file-size branch from 532833b to 055fee1 Compare December 7, 2016 20:51

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 20:51 Inactive

alicewriteswrongs force-pushed the ap/max-file-size branch from 055fee1 to 8e5acde Compare December 7, 2016 20:52

alicewriteswrongs temporarily deployed to micromasters-ci-pr-2004 December 7, 2016 20:52 Inactive

alicewriteswrongs deployed to micromasters-ci-pr-2004 December 7, 2016 20:54 View deployment

alicewriteswrongs assigned alicewriteswrongs and unassigned alicewriteswrongs Dec 8, 2016

alicewriteswrongs requested a review from noisecapella December 8, 2016 17:14

alicewriteswrongs added Needs Review and removed Work in Progress labels Dec 8, 2016

noisecapella suggested changes Dec 9, 2016

View reviewed changes

noisecapella approved these changes Dec 9, 2016

View reviewed changes

noisecapella added Waiting on Author and removed Needs Review labels Dec 9, 2016

Limited profile image size on the client to 512x512

152f2f4

This limits the profile image size on the client, by scaling down the canvas used in the `<Cropper />`. This doesn't work on Safari and on iOS, so on those platforms we just upload the image as-is. pr #2004

alicewriteswrongs force-pushed the ap/max-file-size branch from f7491c4 to 152f2f4 Compare December 9, 2016 18:32

alicewriteswrongs merged commit 152f2f4 into master Dec 9, 2016

alicewriteswrongs deleted the ap/max-file-size branch December 9, 2016 18:32

alicewriteswrongs temporarily deployed to micromasters-ci December 9, 2016 18:44 Inactive

blarghmatey mentioned this pull request Dec 12, 2016

Profile image uploads fail for people on slow or unreliable networks #2079

Closed

pdpinch changed the title ~~Limit user photo uploads to 1024x1024 pixels~~ Limit user photo uploads to 512x512 pixels Dec 12, 2016

Limit user photo uploads to 512x512 pixels #2004

Limit user photo uploads to 512x512 pixels #2004

Uh oh!

Conversation

alicewriteswrongs commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the relevant tickets?

What's this PR do?

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

Screenshots (if appropriate)

What GIF best describes this PR or how it makes you feel?

Uh oh!

alicewriteswrongs commented Dec 6, 2016

Uh oh!

codecov-io commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 91.41% (diff: 83.01%)

Uh oh!

noisecapella commented Dec 6, 2016

Uh oh!

noisecapella commented Dec 6, 2016

Uh oh!

codecov-io commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 91.53% (diff: 71.42%)

Uh oh!

alicewriteswrongs commented Dec 6, 2016

Uh oh!

singingwolfboy Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs commented Dec 7, 2016

Uh oh!

pdpinch commented Dec 7, 2016

Uh oh!

alicewriteswrongs commented Dec 7, 2016

Uh oh!

noisecapella Dec 9, 2016

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs Dec 9, 2016

Choose a reason for hiding this comment

Uh oh!

noisecapella commented Dec 9, 2016

Uh oh!

alicewriteswrongs commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giocalitri commented Dec 9, 2016

Uh oh!

noisecapella left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alicewriteswrongs commented Dec 6, 2016 •

edited

Loading

codecov-io commented Dec 6, 2016 •

edited

Loading

codecov-io commented Dec 6, 2016 •

edited

Loading

alicewriteswrongs commented Dec 9, 2016 •

edited

Loading