Skip to content
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

Merged
merged 22 commits into from
Mar 14, 2024

Conversation

ikoenigsknecht
Copy link
Collaborator

@ikoenigsknecht ikoenigsknecht commented Mar 11, 2024

  • Fixes UserProfileStore and UserProfileContextMenu to allow JPEG and GIF profile photos as well as PNG.
  • Updates E2E tests to include JPEG and GIF cases on profile photo tests
  • Adds new backend unit tests for img validation
  • Adds new npm scripts at the root for conveniently running the desktop app locally instead of having to cd into the desktop package folder and linting all packages at once.
  • Adds new npm scripts for local binary compilation and updates E2E tests allow using local binaries
  • Adds documentation for new local binary compilation and testing scripts
  • Fixes issues I had while running tor tests locally
  • Fix the issue with ContextMenuProps having type unknown for the field visible

GitHub Issue: #2332

Pull Request Checklist

  • I have linked this PR to a related GitHub issue.
  • I have added a description of the change (and Github issue number, if any) to the root CHANGELOG.md.

(Optional) Mobile checklist

Please ensure you completed the following checks if you did any changes to the mobile package:

  • I have run e2e tests for mobile
  • I have updated base screenshots for visual regression tests

- update desktop component to allow jpeg and gif
- update userprofilestore to support jpeg and gif
- add start:desktop script at root for ease
@ikoenigsknecht
Copy link
Collaborator Author

Fixes #2332

Copy link
Collaborator

@leblowl leblowl left a 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.

@ikoenigsknecht
Copy link
Collaborator Author

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. data:image/png;base64) with the magic byte check for performance (since we'll only have to run one header check per image) and some extra assurance (that way we are validating that not only is the type correct on the uploaded image but it matches the magic byte).

@ikoenigsknecht
Copy link
Collaborator Author

Refactored the photo validation so its consolidated and connected the file type check with the magic number check for added assurance/performance.

@EmiM
Copy link
Collaborator

EmiM commented Mar 12, 2024

Could you add backend unit tests for new file types?

await menu.uploadJPEGPhoto()
})

it('Owner updates their profile photo with GIF', async () => {
Copy link
Collaborator

@EmiM EmiM Mar 12, 2024

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?

Copy link
Collaborator Author

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

@ikoenigsknecht ikoenigsknecht force-pushed the bug/2332-allow-jpeg-profile-photos branch from fac75e8 to d292fb5 Compare March 13, 2024 03:54
@ikoenigsknecht ikoenigsknecht merged commit 233725f into develop Mar 14, 2024
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working desktop images and files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants