Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 17 commits
aa9a93a
b7ceb1e
0742c39
f2cb29a
0fbad83
51f0ccb
254e47e
e805e4c
89af2d0
cefba82
9c15bf8
9d49c6b
f4321d4
ab514c4
0b68218
69a9ecc
57bbd5a
b56ab25
e0a038a
d27b876
3141566
ce0a390
a646774
67a28da
0bfe0ec
8ca40f5
e532435
3b3744c
4e49277
07b06b6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 again mixes
fs
andos
. This could be simplified towhen the subfolder is opened as
_fs
.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.
@matrss I refactored the code according to this, now the static path is returned is relative to the uploads directory, which works fine for profile images,
BUT, as this function also handles chat attachment functionality, the chat attachments now return a path like
6/{filename}
, which breaks the code for adding the messages to the chat as it expects a path likeuploads/6/{filename}
Please suggest me what I must do.
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.
Hmm, without seeing the exact error I can only guess, but if the chat attachment stuff breaks then that code has the same bug and also needs to be fixed.
But that is a bit more work: it would require a database migration to fix the wrong paths that are already saved and the migration infrastructure to do that properly doesn't exist yet. Also, that would be well out of scope for your project.
I think you could just introduce a keyword argument to specify if the method returns the path with or without the prefix to allow the chat attachment code to get the old wrongly-prefixed path, while using the proper path for profile images. The default should be to not add the prefix.
That can then be fixed and refactored later.
@ReimarBauer does that sound reasonable or do you have a different idea?
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'm fine with the suggested solution.
for the chat window this is the operation id, there can be many images. So that needs better a subdir.
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 please add also a ToDo here
ToDo: add a namespace for the chat, similar as for profile
https://github.com/workaryangupta/MSS/blob/feat/upload-profile-img/mslib/mscolab/file_manager.py#L300
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.
with a manually migration the existing chat message attachment can be seen. so that needs no additional migration, but when we implement the namespace it definitly does.
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.
At this point the whole
fs
abstraction breaks down, becausesend_from_directory
expectsos.path
semantics and we shouldn't really use anything else. Until here, everything should be treated asfs
, and then can be somewhat cleanly converted to somethingsend_from_directory
understands withsend_from_directory(_fs.getsyspath(""), user.profile_image_path)
(_fs
would need to be opened first).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
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.
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:
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
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
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.