Image validation and size optimization for Profile image upload from both front-end and back-end #513

jacovkim · 2024-12-05T09:04:22Z

Added validation for extension and file size before reading.
Added validation after reading.
Added file size compression.

Some other considerations:

Crop the image after resizing to preserve the ratio
Placeholder image for missing profiles
Might need to replace the current compression method as it's an iterative approach
(Optional) Change public S3 bucket to private with AWS signing

Things to do:

Should disable the "upload" button when the file exceeds the limit when editing
Also clear form after uploading

cypress · 2024-12-05T09:08:29Z

csm_web Run #395

Run Properties: Failed #395 • b57621271c: Image validation and size optimization for Profile image upload from both front-...

Project	`csm_web`
Branch Review	`feat/profiles2024/images`
Run status	`Failed #395`
Run duration	`02m 04s`
Commit	`b57621271c: Image validation and size optimization for Profile image upload from both front-...`
Committer	`Jacob Taegon Kim`
View all properties for this run ↗︎

Test results
Failures	`10`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`74`
View all changes introduced in this branch ↗︎

Tests for review

student-course.cy.ts • 2 failed tests • Tests on Firefox

View Output

Test		Artifacts
... > should be able to enroll in a section		`Screenshots`
... > should be able to enroll in a section		`Screenshots`

restricted-courses.cy.ts • 3 failed tests • Tests on Firefox

View Output

Test		Artifacts
unwhitelisted courses > should still show unrestricted courses		`Screenshots`
whitelisted courses > should see and enroll in whitelisted courses and sections		`Screenshots`
whitelisted courses > should see whitelisted courses among unrestricted courses		`Screenshots`

student-course.cy.ts • 2 failed tests • Tests on Chrome

View Output

Test		Artifacts
... > should be able to enroll in a section		`Test Replay` `Screenshots`
... > should be able to enroll in a section		`Test Replay` `Screenshots`

restricted-courses.cy.ts • 3 failed tests • Tests on Chrome

View Output

Test		Artifacts
unwhitelisted courses > should still show unrestricted courses		`Test Replay` `Screenshots`
whitelisted courses > should see and enroll in whitelisted courses and sections		`Test Replay` `Screenshots`
whitelisted courses > should see whitelisted courses among unrestricted courses		`Test Replay` `Screenshots`

smartspot2

I just did a cursory skim over the PR, but here's some of the feedback I have so far.

Also, did you mean for the target branch to be master here, or is this supposed to be targeting a main feature branch?

runtime.txt

docker-compose.yml

smartspot2 · 2024-12-05T17:41:18Z

csm_web/scheduler/views/user.py

+    # Check file size
+    image_file.seek(0, os.SEEK_END)
+    file_size = image_file.tell()
+    # reset file pointer
+    image_file.seek(0)


This is not a good way of validating the file size; these lines will read in the entire file, regardless of how big it is. That means that if a user uploads a 100G file (ex. directly through the HTTP endpoint), you'll still end up reading the entire file in.

I'd recommend changing this to read in from a file stream, and manually stopping the stream when it exceeds the file size limit.

smartspot2 · 2024-12-05T17:41:58Z

csm_web/scheduler/views/user.py

+# 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


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.

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?

smartspot2 · 2024-12-05T17:43:40Z

csm_web/scheduler/tests/models/test_user.py

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.

smartspot2 · 2024-12-05T17:53:11Z

csm_web/scheduler/migrations/0035_alter_user_profile_image.py

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.

…dthe update

Co-authored-by: Edward Lee <edwardneo@berkeley.edu>

Added minimal frontend changes

…ries

…ated image upload UI, added hyperlinks to section UI, added tooltip

csm_web/frontend/src/components/UserProfile.tsx

isabellaalps · 2025-02-03T02:17:09Z

think it would be nice to add a short description/heading on the profiles landing page to explain what profiles is/does
unable to add image to profiles without triggering fatal error

smartspot2

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 null for the file (the backend would then check if the file is null; 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 :visited links on the page.

(for example)

smartspot2 · 2025-02-03T02:58:44Z

csm_web/csm_web/settings.py

+    def get_accessed_time(self, name):
+        # Implement logic to get the last accessed time
+        raise NotImplementedError("This backend does not support this method.")
+
+    def get_created_time(self, name):
+        # Implement logic to get the creation time
+        raise NotImplementedError("This backend does not support this method.")


Is this going to be an issue? Not sure when these methods are called.

smartspot2 · 2025-02-03T02:59:05Z

csm_web/frontend/src/components/App.tsx

+          {
+            // TODO: add route for profiles (/profile/:id/* element = {UserProfile})
+            // TODO: add route for your own profile /profile/*
+            // reference Section
+          }


TODOs leftover that should be deleted

smartspot2 · 2025-02-03T03:02:25Z

csm_web/frontend/src/components/UserProfile.tsx

+              <button className="primary-btn" onClick={handleCancelToggle}>
+                Cancel
+              </button>


Cancel button should always be the secondary button (in gray).

smartspot2 · 2025-02-03T03:03:45Z

csm_web/frontend/src/components/course/SectionCard.tsx

    const iconWidth = "8em";
    const iconHeight = "8em";
    if (enrollmentSuccessful) {
+      const inlineIconWidth = "1.3em";
+      const inlineIconHeight = "1.3em";
      return (
        <div className="enroll-confirm-modal-contents">
          <CheckCircle height={iconHeight} width={iconWidth} />
          <h3>Successfully enrolled</h3>
-          <ModalCloser>
-            <button className="primary-btn">OK</button>
-          </ModalCloser>
+          <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>
        </div>
      );


Icon sizes should be defined in the SCSS files, not directly in the HTML; I'd recommend adding a CSS class and setting the size there.

(The icon width/height variables are also referenced below.)

smartspot2 · 2025-02-03T03:08:04Z

csm_web/scheduler/models.py

+    preferred_name = models.TextField(default="", blank=True)
+    pronouns = models.CharField(max_length=20, default="", blank=True)
+    pronunciation = models.CharField(max_length=50, default="", blank=True)
+    # uploaded_at = models.DateTimeField(auto_now_add=True)


If we aren't adding an uploaded_at field, this line should be deleted.

smartspot2 · 2025-02-03T03:25:06Z

csm_web/scheduler/views/user.py

+        if "file" in request.FILES:
+            user.profile_image.save(file.name, file)
+        serializer.save()
+    # print({**serializer.data, "profileImage": user.profile_image})


Commented print statement should be deleted.

smartspot2 · 2025-02-03T03:28:59Z

csm_web/frontend/src/components/UserProfile.tsx

+const MAX_SIZE_MB = 2;
+const MAX_FILE_SIZE_BYTES = MAX_SIZE_MB * 1024 * 1024;


I think this is out of sync with the backend limit; the backend enforces a 5MB limit.

smartspot2 · 2025-02-03T03:50:53Z

csm_web/scheduler/views/user.py

+        if file_size > TARGET_FILE_SIZE_BYTES:
+            file = compress_image(image, TARGET_FILE_SIZE_BYTES, img_format)
+        else:
+            buffer = BytesIO()
+            if img_format in ["JPEG", "JPG"]:
+                image.save(buffer, format=img_format)
+            else:
+                image.save(buffer, format="PNG", optimze=True)
+            buffer.seek(0)
+            file = ContentFile(buffer.read(), name=image_file.name)


This section of code currently throws an error when trying to save the image on line 155.

Error details

csm_web-django-1 | "PUT /api/user/3342/update/ HTTP/1.1" 500 13988 csm_web-django-1 | Internal Server Error: /api/user/3342/update/ csm_web-django-1 | Traceback (most recent call last): csm_web-django-1 | File "/venv/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner csm_web-django-1 | response = get_response(request) csm_web-django-1 | ^^^^^^^^^^^^^^^^^^^^^ csm_web-django-1 | File "/venv/lib/python3.12/site-packages/django/core/handlers/base.py", line 197, in _get_response csm_web-django-1 | response = wrapped_callback(request, *callback_args, **callback_kwargs) csm_web-django-1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ csm_web-django-1 | File "/venv/lib/python3.12/site-packages/django/views/decorators/csrf.py", line 65, in _view_wrapper csm_web-django-1 | return view_func(request, *args, **kwargs) csm_web-django-1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ csm_web-django-1 | File "/venv/lib/python3.12/site-packages/django/views/generic/base.py", line 104, in view csm_web-django-1 | return self.dispatch(request, *args, **kwargs) csm_web-django-1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ csm_web-django-1 | File "/venv/lib/python3.12/site-packages/rest_framework/views.py", line 509, in dispatch csm_web-django-1 | response = self.handle_exception(exc) csm_web-django-1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^ csm_web-django-1 | File "/venv/lib/python3.12/site-packages/rest_framework/views.py", line 469, in handle_exception csm_web-django-1 | self.raise_uncaught_exception(exc) csm_web-django-1 | File "/venv/lib/python3.12/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception csm_web-django-1 | raise exc csm_web-django-1 | File "/venv/lib/python3.12/site-packages/rest_framework/views.py", line 506, in dispatch csm_web-django-1 | response = handler(request, *args, **kwargs) csm_web-django-1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ csm_web-django-1 | File "/venv/lib/python3.12/site-packages/rest_framework/decorators.py", line 50, in handler csm_web-django-1 | return func(*args, **kwargs) csm_web-django-1 | ^^^^^^^^^^^^^^^^^^^^^ csm_web-django-1 | File "/opt/csm_web/csm_web/scheduler/views/user.py", line 155, in user_update csm_web-django-1 | user.profile_image.save(file.name, file) csm_web-django-1 | ^^^^^^^^^ csm_web-django-1 | AttributeError: '_io.BytesIO' object has no attribute 'name'

Putting the ContentFile wrapper around the buffer returned by compress_image would resolve this.

Suggested change

if file_size > TARGET_FILE_SIZE_BYTES:

file = compress_image(image, TARGET_FILE_SIZE_BYTES, img_format)

else:

buffer = BytesIO()

if img_format in ["JPEG", "JPG"]:

image.save(buffer, format=img_format)

else:

image.save(buffer, format="PNG", optimze=True)

buffer.seek(0)

file = ContentFile(buffer.read(), name=image_file.name)

if file_size > TARGET_FILE_SIZE_BYTES:

buffer = compress_image(image, TARGET_FILE_SIZE_BYTES, img_format)

else:

buffer = BytesIO()

if img_format in ["JPEG", "JPG"]:

image.save(buffer, format=img_format)

else:

image.save(buffer, format="PNG", optimze=True)

buffer.seek(0)

file = ContentFile(buffer.read(), name=image_file.name)

smartspot2 · 2025-02-03T03:52:48Z

csm_web/scheduler/storage.py

+
+
+class ProfileImageStorage(S3Boto3Storage):
+    bucket_name = "csm-web-profile-pictures"


This bucket name should be extracted from an environment variable; during development, we generally want to keep the files separate from prod.

I've created a csm-web-profile-pictures-dev bucket for this as well. The csm-web-dev user was also updated to include access to the development bucket.

smartspot2 · 2025-02-03T04:33:05Z

csm_web/frontend/src/components/course/SectionCard.tsx

+          <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>


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.

smartspot2 requested changes Dec 5, 2024

View reviewed changes

ElaineShu0312 and others added 16 commits January 31, 2025 23:23

implemented backend logic for retrieving and updating a user profile

ac8aec2

fixed the permissions so that there is separation of concerns, + fixe…

35c2515

…dthe update

wrote tests for user.py and fixed bugs, passing all 10

33bc020

fixed little bug in test

a48d202

dynamic user routing works now I think

cb4d2aa

finished profile form

242c18c

hopefully resolved one of the merge errors

3e6d7f9

created AWS bucket and tried to link it to backend

bbc335f

configured settings for Image bucket

2c5efa1

clean slate

52f828e

created component and view for testing the upload image pipeline

2b3573d

trying to figure out image upload json error

7ab25be

pillow image validation

c8bd6ff

Fixed image validation with other minor fixes

8bdfc37

Added UI features on section enrollment page (#509)

aa2e655

Co-authored-by: Edward Lee <edwardneo@berkeley.edu>

Added CSS

9a79f9b

Added minimal frontend changes

edwardneo force-pushed the feat/profiles2024/images branch from fdfc29d to 9a79f9b Compare February 1, 2025 07:39

edwardneoprime added 2 commits February 1, 2025 06:49

Added preferred name, overhauled profile upload UI, fixed profile que…

7d03638

…ries

Completed profiles -- (re)connected S3 bucket, added signed URLs, cre…

b576212

…ated image upload UI, added hyperlinks to section UI, added tooltip

isabellaalps self-requested a review February 3, 2025 01:43

isabellaalps reviewed Feb 3, 2025

View reviewed changes

csm_web/frontend/src/components/UserProfile.tsx Show resolved Hide resolved

isabellaalps reviewed Feb 3, 2025

View reviewed changes

csm_web/frontend/src/components/UserProfile.tsx Show resolved Hide resolved

smartspot2 requested changes Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image validation and size optimization for Profile image upload from both front-end and back-end #513

Image validation and size optimization for Profile image upload from both front-end and back-end #513

jacovkim commented Dec 5, 2024

cypress bot commented Dec 5, 2024 •

edited

Loading

smartspot2 left a comment

smartspot2 Dec 5, 2024

smartspot2 Dec 5, 2024

jacovkim Jan 31, 2025

smartspot2 Dec 5, 2024

smartspot2 Dec 5, 2024

isabellaalps commented Feb 3, 2025

smartspot2 left a comment

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

smartspot2 Feb 3, 2025

		const MAX_SIZE_MB = 2;
		const MAX_FILE_SIZE_BYTES = MAX_SIZE_MB * 1024 * 1024;



		class ProfileImageStorage(S3Boto3Storage):
		bucket_name = "csm-web-profile-pictures"

Image validation and size optimization for Profile image upload from both front-end and back-end #513

Are you sure you want to change the base?

Image validation and size optimization for Profile image upload from both front-end and back-end #513

Conversation

jacovkim commented Dec 5, 2024

cypress bot commented Dec 5, 2024 • edited Loading

Tests for review

smartspot2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabellaalps commented Feb 3, 2025

smartspot2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Dec 5, 2024 •

edited

Loading