-
Notifications
You must be signed in to change notification settings - Fork 68
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: Allow users to set custom profile pictures #2405
feat: Allow users to set custom profile pictures #2405
Conversation
Please take a look at the CI test failures. You have introduced a new required positional argument to a constructor that is not supplied everywhere it is used, so it crashes a lot of tests. I think the profile image should be optional and the default behavior should match what we had before this PR. |
I made the profile image optional but still, the tests are failing. There is something more to it that I am not able to grasp. |
This is now a new test failure because some data files in skyfield-data have expired yesterday. This is unrelated to your code. Coincidentally, I've already noticed this last week and opened an issue: brunobord/skyfield-data#36. @ReimarBauer it looks like you fixed this the last time it came up, judging by the commit history in skyfield-data. Do you mind taking a look again? I am not quite sure what the process is, over there. |
You could xfail the test with a good hint to our issue, so it can be later enabled again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments
mslib/msui/mscolab.py
Outdated
file_format = mime_type.split('/')[1].upper() | ||
|
||
# Resize the image and set profile image pixmap | ||
image = Image.open(file_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs an try: except PIL.UnidentifiedImageError: because one can have wrong extensions used, e.g. renamed a mp4 to a jpg
currently this gives a traceback:
Fatal error in MSS 9.0.0 on Linux-6.5.0-10043-tuxedo-x86_64-with-glibc2.35
Python 3.11.6 | packaged by conda-forge | (main, Oct 3 2023, 10:40:35) [GCC 12.3.0]
Please report bugs in MSS to https://github.com/Open-MSS/MSS
Information about the fatal error:
Traceback (most recent call last):
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 850, in upload_image
image = Image.open(file_name)
^^^^^^^^^^^^^^^^^^^^^
File "/home/user/Miniforge/envs/mssdev/lib/python3.11/site-packages/PIL/Image.py", line 3283, in open
raise UnidentifiedImageError(msg)
PIL.UnidentifiedImageError: cannot identify image file '/home/user/tmp/1.jpg'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming a gif to a png and uploading works, so gif can maybe added to the list of extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming a gif to a jpg and uploading gives
Please report bugs in MSS to https://github.com/Open-MSS/MSS
Information about the fatal error:
Traceback (most recent call last):
File "/home/user/Miniforge/envs/mssdev/lib/python3.11/site-packages/PIL/JpegImagePlugin.py", line 643, in _save
rawmode = RAWMODE[im.mode]
~~~~~~~^^^^^^^^^
KeyError: 'P'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 853, in upload_image
image.save(img_byte_arr, format=file_format)
File "/home/user/Miniforge/envs/mssdev/lib/python3.11/site-packages/PIL/Image.py", line 2431, in save
save_handler(self, fp, filename)
File "/home/user/Miniforge/envs/mssdev/lib/python3.11/site-packages/PIL/JpegImagePlugin.py", line 646, in _save
raise OSError(msg) from e
OSError: cannot write mode P as JPEG
So also catch OSError
Becasue this is not server code it is not critical, we help here the user and maybe get no issues when someone has files like this.
mslib/mscolab/server.py
Outdated
# base_path = os.path.join(base_directory, 'profile') | ||
base_path = mscolab_settings.PROFILE_IMG_FOLDER | ||
filename = os.path.basename(user.profile_image_path) | ||
return send_from_directory(base_path, filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this to #2103
mslib/mscolab/server.py
Outdated
# base_path = os.path.join(base_directory, 'profile') | ||
base_path = mscolab_settings.PROFILE_IMG_FOLDER | ||
filename = os.path.basename(user.profile_image_path) | ||
return send_from_directory(base_path, filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@workaryangupta
Here a ToDo can be added, e.g. see open Discussion about PyFilesystem2 on ticket
or it can be solved as @matrss wrote.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I have a new user created add an image and logout and create the next user and logout and then login thee first user I get
Traceback (most recent call last):
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 353, in login_handler
self.mscolab.after_login(data["email"], self.mscolab_server_url, r)
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 682, in after_login
self.fetch_profile_image()
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 720, in fetch_profile_image
self.set_profile_pixmap(response.content)
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 705, in set_profile_pixmap
self.profile_dialog.gravatarLabel.setPixmap(resized_pixmap)
RuntimeError: wrapped C/C++ object of type QLabel has been deleted
CRITICAL: MSS Version: 9.0.0
CRITICAL: Python Version: 3.11.6 | packaged by conda-forge | (main, Oct 3 2023, 10:40:35) [GCC 12.3.0]
CRITICAL: Platform: Linux-6.5.0-10043-tuxedo-x86_64-with-glibc2.35 (('64bit', 'ELF'))
CRITICAL: Fatal error: Traceback (most recent call last):
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 353, in login_handler
self.mscolab.after_login(data["email"], self.mscolab_server_url, r)
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 682, in after_login
self.fetch_profile_image()
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 720, in fetch_profile_image
self.set_profile_pixmap(response.content)
File "/home/user/PycharmProjects/2024/workaryangupta/MSS/mslib/msui/mscolab.py", line 705, in set_profile_pixmap
self.profile_dialog.gravatarLabel.setPixmap(resized_pixmap)
RuntimeError: wrapped C/C++ object of type QLabel has been deleted
failure.mp4
you can also when you want to populate the db with some test users by
mscolab.py db --seed
The data is described:
https://github.com/Open-MSS/MSS/blob/develop/mslib/mscolab/seed.py#L228
only users which don't have a profile image can login without this failure
on the first two users from the seed example I added the profile image, same problem. seed_example.mp4 |
I am looking into why this is happening. Could be somewhat related to how UI elements are set after the logout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost all done, a last ToDo should be added to the file_manager that we ack to add a namespace for the chat, see comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, for the pretty nice feature
mslib/mscolab/server.py
Outdated
# base_path = os.path.join(base_directory, 'profile') | ||
base_path = mscolab_settings.PROFILE_IMG_FOLDER | ||
filename = os.path.basename(user.profile_image_path) | ||
return send_from_directory(base_path, filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, this shouldn't be a todo, this is a bug that has already bitten us in other places. The fix is trivial, so it can be done here:
base_path = mscolab_settings.UPLOAD_FOLDER
if sys.platform.startswith('win'):
base_path = base_path.replace('\\', '/')
filename = user.profile_image_path
with fs.open_fs(base_path) as _fs:
return send_from_directory(_fs.getsyspath(""), filename)
I'll do a thorough review tomorrow, but I think this is looking good now. |
To be noted I looked that also on the send_from_directory before. Itselfs it has a builtin secureness by safe_join. But I also prefer the way it is done now. https://github.com/pallets/werkzeug/blob/main/src/werkzeug/utils.py#L538 https://github.com/pallets/werkzeug/blob/main/src/werkzeug/security.py#L131 Edit: not readed correctly it is that windows backslash. Thx. |
@ReimarBauer I think you are replying to #2405 (comment), right? If that is the case: the change was neither for path sanitization nor for the backslash/slash conversion. It was to properly interpret UPLOAD_FOLDER as an fs URL and then use the only supported option to convert those to a path-like value (getsyspath). It would have errored previously as soon as someone actually tried to use something other than a plain path (e.g. an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks!
AFAICT there is currently no way to unset the profile image after one was set (it can just be replaced with a new one, not brought back into a state in which there is none set for the account). That could be a future improvement.
@@ -250,6 +258,74 @@ def modify_user(self, user, attribute=None, value=None, action=None): | |||
db.session.commit() | |||
return True | |||
|
|||
def delete_user_profile_image(self, image_to_be_deleted): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a sidenote, nothing to do now: since this method is named delete_user_profile_image
I think it would semantically make more sense to pass a user to this method, instead of a path.
It works as-is though, so this is something that could be refactored in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
cfdaf14
into
Open-MSS:GSOC2024-AryanGupta
Enhancing the functionality of the MSColab server to allow users to upload small images which would replace generic initials as identifiers during login. This feature personalizes user accounts and significantly enhances the visual aspect of user identification and interaction within the MSColab platform. Co-authored-by: Aryan Gupta <work.aryangupta@gmail.com>
This pull request aims to introduce a new feature to mscolab, enabling users to upload and set personalized profile pictures.