-
Notifications
You must be signed in to change notification settings - Fork 88
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
fix: Allow JPEG and GIF images as profile photos #2332 #2353
Conversation
- update desktop component to allow jpeg and gif - update userprofilestore to support jpeg and gif - add start:desktop script at root for ease
Fixes #2332 |
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.
Thanks. I would move the changelog changes under unreleased, but I can also do it later. Here are some of my thoughts around validation for this feature: #2173 in case you are interested ... perhaps the image magic byte sequence check is unnecessary, but it doesn't seem like a big deal.
Updated the CHANGELOG files with that formatting. I think for now we can leave the magic byte checks but would be curious if we could do away with it while still maintaining assurance that images are what they say they are. Part of me also wonders if we should merge the type check (e.g. |
Refactored the photo validation so its consolidated and connected the file type check with the magic number check for added assurance/performance. |
Could you add backend unit tests for new file types? |
await menu.uploadJPEGPhoto() | ||
}) | ||
|
||
it('Owner updates their profile photo with GIF', async () => { |
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.
Could you check locally if userProfile test is passing?
- Last of "Owner updates their profile photo with..." tests uploads gif photo so if the upload suppose to be successful then assertion in "Owner's message with profile photo is visible on channel" should be updated from checking png to gif
- Could you maybe add assertion at the end of each of "Owner updates their profile photo with..." cases to make sure that e.g users do not see upload error message?
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 assertions and testing locally now
fac75e8
to
d292fb5
Compare
…ning kills when no process exists Tor was only spawning one process when running on my machine (might be an arm64 thing) so this test was failing since it expected two processes to close but there was just one
UserProfileStore
andUserProfileContextMenu
to allow JPEG and GIF profile photos as well as PNG.npm
scripts at the root for conveniently running the desktop app locally instead of having tocd
into the desktop package folder and linting all packages at once.npm
scripts for local binary compilation and updates E2E tests allow using local binariesContextMenuProps
having typeunknown
for the fieldvisible
GitHub Issue: #2332
Pull Request Checklist
(Optional) Mobile checklist
Please ensure you completed the following checks if you did any changes to the mobile package: