Story 2448 - Webpage Integration Edit User Profile Avatar#2523
Story 2448 - Webpage Integration Edit User Profile Avatar#2523jlchilders11 wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jlchilders11
changed the title
herzog0
left a comment
herzog0
left a comment
There was a problem hiding this comment.
Looking pretty good!!
I just have a couple of notes/findings we could take a look at.
One general note: the backend is already returning the correct error message. Would be nice if we could use it to display in the form instead of a generic message. Btw, backend says the user shouldn't exceed 1MB, while FE says 5MB.
| permission_classes = [IsAuthenticated] | ||
|
|
||
| def perform_update(self, serializer): | ||
| instance = serializer.save() |
There was a problem hiding this comment.
This is bypassing a previous block path that existed in the legacy version.
I don't know exactly why this exists, but in the admin panel you're able to disallow a user to update their profile picture, and this logic exists in users/forms.py line 133.
There was a problem hiding this comment.
Actually, I think we should maybe re-evaluate this new section entirely and take a deeper look into the pre-existing save() function in the model I just mentioned, as it includes some other important stuff like preventing automatic avatar overrides when refresh_users_github_photos runs, for example.
There was a problem hiding this comment.
Could you clarify what you mean? I'm not entirely clear what the logic in the form save is doing, other than the thumbnail cleanup myself.
| permission_classes = [IsAuthenticated] | ||
|
|
||
| def perform_update(self, serializer): | ||
| instance = serializer.save() |
There was a problem hiding this comment.
Just creating a new comment here in the same place, as I don't have much else where to comment.
I accidentally stumbled into this error, and I truly believe we should either rename the file ourselves to some random uuid, or at least tell the user exactly what went wrong.
| if self.request.POST.get("delete_profile_image", "").lower() == "true": | ||
| instance.profile_image.delete() |
There was a problem hiding this comment.
Something interesting that happens here: the user can add/replace their avatar AND send a "delete" request at the same time, causing the image to be first uploaded, saved to the users record, then deleted right after that. You can see how the FE actually sends both in the payload as well:
There was a problem hiding this comment.
This is true, but I'm not sure if this is a problem?
| if request.GET.get("edit", "").lower() == "true" and isinstance( | ||
| request.user, AnonymousUser | ||
| ): |
There was a problem hiding this comment.
Here you're only covering for ?edit=True, but if a logged out user navigates to /users/me they get an error:
2 participants
Issue: #2448
Summary & Context
Performs the necessary hook up for allowing for the saving of a new avatar image for the user, or deleting that same image.
Changes
Please list any potential risks or areas that need extra attention during review/testing
The states in the form are somewhat fragile, as they rely on invisible checkboxes and srcs on images.
Javascriptless functionality will be implemented in a future ticket.
Self-review Checklist
Frontend