Image validation and size optimization for Profile image upload from both front-end and back-end #513
Conversation
csm_web/scheduler/views/user.py
Outdated
| # not very efficient but way to compress image until it meets the file target size. | ||
| def compress_image(image, target_size_bytes, img_format): | ||
| """Compress the image until it's smaller than target_size_bytes.""" | ||
| buffer = BytesIO() | ||
| quality = 95 # start with high quality | ||
| while True: | ||
| buffer.seek(0) | ||
| if img_format == "JPEG" or img_format == "JPG": | ||
| image.save(buffer, format=img_format, quality=quality) | ||
| else: | ||
| image.save(buffer, format="PNG", optimze=True) | ||
| if buffer.tell() <= target_size_bytes or quality <= 50: | ||
| break | ||
| quality -= 5 # decrease quality to reduce file size | ||
| buffer.seek(0) | ||
| return buffer |
There was a problem hiding this comment.
Is there any particular reason why we're compressing the file? We have a file size limit anyways, which should be sufficient (and can be lowered) if this is for storage concerns.
There was a problem hiding this comment.
usually, selfies or phone images tend to be larger than 5MB so allowing 10MB input size and compressing it to 5MB was what I was initially thinking but just realized I didn't change the target size and input limit. But should allowing 10MB then compressing to 5MB to save space make sense?
There was a problem hiding this comment.
There should be tests added for uploading images; in particular one should be added where a request is sent using a file stream that never ends, and ensuring that the request is rejected after only reading in the file size limit.
There was a problem hiding this comment.
These migration files should be merged together at the end prior to merging to master, to reduce on the number of migration files in the repo.
Co-authored-by: Edward Lee <[email protected]>
edwardneo
force-pushed
the
feat/profiles2024/images
branch
from
fdfc29d to
9a79f9b
Compare
…ated image upload UI, added hyperlinks to section UI, added tooltip
|
There was a problem hiding this comment.
Made a bunch of comments in the thread. Overall, looks pretty good!
Some other miscellaneous things as I tested manually:
-
Coordinators are able to edit anybody else's profile, which shouldn't be happening; I mentioned this as a review comment as well pointing to the section of code responsible. In general, there should be additional testing to ensure that negative constraints are satisfied. (To test, I had to remove the superuser exception in the code.)
-
Putting a blank name does not default to the user's full name. Interestingly, this only occurs when there is no image uploaded. Otherwise, the name defaults as usual. (This is probably because of some query invalidation to display the image once it's uploaded.)
-
I think there should be a button to clear the image if the user wants to delete their profile picture; currently, there is no way to do that, and if a user wants to change their profile picture to the default, they'd need to upload the CSM logo. For initial release, this can probably be deprioritized, but this can probably be relatively easily implemented by adding an "X" icon overlaid with the uplaod icon, and the PUT request can specify
nullfor the file (the backend would then check if the file isnull; a file that is not updated would not have a key in the request). -
The upload icon is a little bit faint when there's an image uploaded; I think either (a) make the icon less transparent, or (b) put a white background on the icon so that the image itself is a little more faint, bringing out the icon more.
(for example)
-
The links look a little bit out of place; styling the links to fit in a little more with the color scheme could be good? The links on the resources page for worksheets could be good to model off of here. If you'd like, you can use a lighter blue (kinda like the csmentors.org website links) instead of the green to make it more clearly link-like. One big thing is that there shouldn't be a difference between normal links and
:visitedlinks on the page.(for example)
| <h4>To view and update your profile, click the button below</h4> | ||
| <Link className="primary-btn" to="/profile"> | ||
| <UserIcon width={inlineIconWidth} height={inlineIconHeight} /> | ||
| Profile | ||
| </Link> |
There was a problem hiding this comment.
This change is the cause of the Cypress test failures; the Cypress tests should change to look for Profile instead, and should make sure that the user is redirected as desired.
|
Ok I addressed most of y'all's comments. There is a working delete button, hyperlink colors are fixed, editing should be fixed. Theoretically this feature is fully functional, but if/when I have time tonight, I'll add a tooltip on the profile landing page to explain what profiles does (like Isabella mentioned) and maybe try adding a page to list all of the mentors for a certain course. Also, the S3 bucket is currently |
edwardneo
requested review from
isabellaalps,
smartspot2,
gabrielhan23 and
heidi-chann
There was a problem hiding this comment.
Honestly overall this looks pretty good. I'll give a stamp of approval for now, but see my comments for various things I've noticed while testing.
Make sure that all conflicts are resolved and all CI tests pass, and additionally make sure that this is tested on staging before merging into master; check all permissions and do spot checks on functionality to ensure that nothing weird happens on staging. (It's also a good way to check that dependencies are all fine, since you've updated some dependencies in this PR.)
| <div className="profile-section-times"> | ||
| {sectionTimes.map((spacetimes, index) => ( | ||
| <div key={index} className="profile-section-time"> | ||
| <ClockIcon /> <span>{spacetimes.map(formatSpacetimeInterval).join(" + ")}</span> |
There was a problem hiding this comment.
Also I think it'd be pretty cool if clicking on the section time (or a link adjacent) would bring you directly to the section card on the sections page, for ease of enrollment.
Or honestly some way of combining the bios page and sections page in the future would probably good too, ex. incorporating the enroll button on the bios page for reduced friction.
These are all nice-to-have things as extra additions for the future though (feel free to separate this into a separate issue to tackle later).
There was a problem hiding this comment.
Something for the future, but some kind of back button to return the user to their previous page could be nice here (there should be some utilities to just pop from the navigation history); having to navigate back using the browser back button is a little bit annoying. After looking at a profile, I naturally look for a back button to go back to what I was looking at before, ex. the sections page, or the bios page.
Not a priority, so this can also be punted to a separate issue.
…her small fixes from PR comments
Added validation for extension and file size before reading.
Added validation after reading.
Added file size compression.
Some other considerations:
Things to do: