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

[HOLD for payment 1/12][$8000] Profile - Create a limit for Avatar file types or size #13006

Closed
kavimuru opened this issue Nov 24, 2022 · 99 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Nov 24, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Save the below image to your local drive
  2. Open staging.new.expensify.com.
  3. Open Settings, click on Avatar to open Profile Page.
  4. Click on Edit photo and upload the below image
  5. Try to scroll the slider to zoom in or zoom out the photo.
  6. Notice that the slider can't not be scrolled for a while and it's delayed.

Expected Result:

  • Slider need be scrolled smoothly without any delay
  • We need to create an error message if a user is uploading a file/size too large to scroll smoothly

Actual Result:

Slider can't not be scrolled for a while and it's delayed after that.

  • Based on our code we have:

  • Max size: 6291456

  • Min Width: 80

  • Min Height: 80

  • Max Width: XX

  • Max Height: XX

We need to figure out what is causing this issue (is it pixels, weight, file type) and create an error message for the user

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.2.31-5
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/203830021-84c9a597-6507-490b-881e-fb08fe57c900.mov
https://user-images.githubusercontent.com/43996225/203830045-659ad4c2-2fc4-4d86-870c-c3cb6338284f.mp4
Used this image : kinnd-02 (1)

Expensify/Expensify Issue URL:
Issue reported by: @hungvu193
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1669089889220939

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~017185b708f6ccd9fb
  • Upwork Job ID: 1597776558088036352
  • Last Price Increase: 2022-12-26
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 24, 2022

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@s77rt
Copy link
Contributor

s77rt commented Nov 25, 2022

The image is 9001px by 9000px
That's too much, I won't say this is a bug

@melvin-bot melvin-bot bot added the Overdue label Nov 28, 2022
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Nov 28, 2022

I am not able to replicate this.

Staging v1.2.32-1

  1. Open staging.new.expensify.com.
  2. Open Settings, click on Avatar to open Profile Page.
  3. Click on Edit photo and choose an image from local.
  4. Scroll the slider to zoom
  5. Notice that the slider can't not be scrolled for a while and it's delayed.

Image used 80kb:

2022-11-28_13-54-44 (1)

Based on the chat in the Slack link and as @s77rt mentioned, this seems to be an issue with large images. Do we want to change the focus of this to compress images before uploading or send a popup asking for a lighter image?

@melvin-bot melvin-bot bot removed the Overdue label Nov 28, 2022
@Beamanator
Copy link
Contributor

Do we want to change the focus of this to compress images before uploading or send a popup asking for a lighter image?

I like these ideas (prefer asking for a smaller image) - added my comments in Slack!

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Nov 29, 2022

I think we need to figure out what is causing the issue b/c it sounds like it might not be file size based on this comment.

The avatar limit code is here:

AVATAR_MAX_ATTACHMENT_SIZE: 6291456
AVATAR_MIN_WIDTH_PX: 80
AVATAR_MIN_HEIGHT_PX: 80

The image that @hungvu193 has very high pixelation - maybe that's the issue? @hungvu193 or @kavimuru do you have another image with a heavy pixelation/DPI that we can test?

image

@Christinadobrzyn
Copy link
Contributor

Kinda came to a resolution in the Slack convo but love to get your thoughts on what to do next with this GH @Beamanator

@Beamanator Beamanator self-assigned this Nov 29, 2022
@Beamanator
Copy link
Contributor

Beamanator commented Nov 29, 2022

Adding myself here too b/c I'm involved in lots of avatar-related issues, and this is very intriguing 😅

Also noting (as is noted in Slack) this is the same as #13005, which was closed w/out reproduction but it seems there's a few images we can use to reproduce at the moment (in Slack):

(Chose download free => Original size): https://unsplash.com/photos/SiwI2DVMZHI

@Christinadobrzyn
Copy link
Contributor

Thanks @Beamanator and @hungvu193! I updated the title and the OP to hopefully match what we're looking for. Going to all the external label.

@Christinadobrzyn Christinadobrzyn added the External Added to denote the issue can be worked on by a contributor label Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Profile - Not able to scroll the slider to zoom in /out after uploading an image [$1000] Profile - Not able to scroll the slider to zoom in /out after uploading an image Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

Job added to Upwork: https://www.upwork.com/jobs/~017185b708f6ccd9fb

@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

Current assignee @Beamanator is eligible for the External assigner, not assigning anyone new.

@Christinadobrzyn Christinadobrzyn changed the title [$1000] Profile - Not able to scroll the slider to zoom in /out after uploading an image [$1000] Profile - Create a limit for Avatar file types or size Nov 30, 2022
@hungvu193
Copy link
Contributor

hungvu193 commented Nov 30, 2022

Proposal
This issue happened with large image size, in order to prevent that we need to limit the size of it.

App/src/CONST.js

Lines 31 to 34 in e82bcf6

// Minimum width and height size in px for a selected image
AVATAR_MIN_WIDTH_PX: 80,
AVATAR_MIN_HEIGHT_PX: 80,

Solution
Add these values inside CONST next to AVATAR_MIN_WIDTH_PX:

+     AVATAR_MAX_WIDTH_PX: 4000,
+     AVATAR_MAX_HEIGHT_PX: 4000,

Also need to update the isValidResolution function inside AvatarWithImagePicker.js to:

    /**
     * Check if the attachment resolution is bigger than required.
     *
     * @param {String} imageUri
     * @returns {Promise}
     */
 isValidResolution(imageUri) {
        return new Promise((resolve) => {
            Image.getSize(imageUri, (width, height) => {
                resolve((height >= CONST.AVATAR_MIN_HEIGHT_PX && height <= CONST.AVATAR_MAX_HEIGHT_PX) && (width >= CONST.AVATAR_MIN_WIDTH_PX && width <= CONST.AVATAR_MAX_WIDTH_PX));
            });
        });
    }

Since we change the validation we need to update error message in here too, can some one provide me it. For now I leave it like this

        this.isValidResolution(image.uri)
            .then((isValidResolution) => {
                if (!isValidResolution) {
                    this.showErrorModal(
                        this.props.translate('avatarWithImagePicker.imageUploadFailed'),
                        this.props.translate('avatarWithImagePicker.tooSmallResolution', {
                            minHeightInPx: CONST.AVATAR_MIN_HEIGHT_PX,
                            minWidthInPx: CONST.AVATAR_MIN_WIDTH_PX,
                            maxHeightInPx: CONST.AVATAR_MAX_HEIGHT_PX,
                            maxWidthInPx: CONST.AVATAR_MAX_WIDTH_PX,
                        }),
                    );
                    return;
                }
 tooSmallResolution: ({
            minHeightInPx, minWidthInPx, maxHeightInPx, maxWidthInPx,
        }) => `Please upload an image larger than ${minHeightInPx}x${minWidthInPx} pixels and smaller than ${maxHeightInPx}x${maxWidthInPx} pixels`,

Result:

Untitled.mov

@Christinadobrzyn
Copy link
Contributor

Here are some file size limits for other social media sites. Maybe we should use these?

FB Avatar size:
Max Size: 2048px X 2048px
Min Size: 168px X 168px
Aspect ratio: 1:1

WhatsApp Profile Image Size – 500 x 500 px

@hungvu193
Copy link
Contributor

@Christinadobrzyn That's fine I think, but should we post this in slack for discussion as well, just to make sure everyone agree with it.

@sobitneupane
Copy link
Contributor

sobitneupane commented Dec 1, 2022

I believe next step here is to determine file size limit and the error message to display when file size is outside the scope.

Having said that the proposal from @hungvu193 looks good to me.

@Christinadobrzyn
Copy link
Contributor

Hey @Beamanator can you take a peek at #13006 (comment) and let me know if you agree? Thanks!

@Beamanator
Copy link
Contributor

Interesting, here's what I'm seeing in different platforms:

  • Twitter: Error: One or more images exceed the size limit and cannot be resized.
  • Facebook: Took a while to load, no issues after that
  • WhatsApp: Maybe a bit slow to load, no issues moving around / zooming after
  • Discord: Took a while to load, lags a bit when zooming & moving around

Personally I'd love to not see the error message, and instead maybe warn the user that we'll decrease the photo quality before uploading (or do this automatically if possible).

@azimgd
Copy link
Contributor

azimgd commented Dec 29, 2022

@tgolen addressed your feedback by extracting getImageResolution into .js / .native.js.

https://github.com/azimgd/expensify-app/blob/365d10a444f8776f6e3e9d3d3767f57c8557d5dd/src/libs/fileDownload/getImageResolution.native.js

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 29, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 29, 2022

📣 @azimgd You have been assigned to this job by @mallenexpensify!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@mallenexpensify
Copy link
Contributor

Assigned issue to and hired @azimgd can you please accept the job and reply here once you have? @sobitneupane, can you do the same plz
https://www.upwork.com/jobs/~017185b708f6ccd9fb

@azimgd
Copy link
Contributor

azimgd commented Dec 30, 2022

applied

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Dec 30, 2022
@azimgd
Copy link
Contributor

azimgd commented Dec 30, 2022

ready for review 👆

@azimgd
Copy link
Contributor

azimgd commented Dec 30, 2022

@tgolen tgolen assigned tgolen and unassigned Beamanator Jan 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 9, 2023

@tgolen, @azimgd, @mallenexpensify, @sobitneupane, @kavimuru, @Christinadobrzyn Whoops! This issue is 2 days overdue. Let's get this updated quick!

@mallenexpensify mallenexpensify changed the title [$8000] Profile - Create a limit for Avatar file types or size [HOLD for payment 1/12][$8000] Profile - Create a limit for Avatar file types or size Jan 9, 2023
@mallenexpensify
Copy link
Contributor

Deployed to Production on Jan 5, updating title to pay on 1/12
#13899 (comment)

I wonder why auto-bots didn't work for updating, maybe cuz there was additional data here in the PR?

Fixed Issues
$ #13006
PROPOSAL: #13006 (comment)

@mallenexpensify
Copy link
Contributor

Hired Dec 29th, Merged Jan 3, eligible for the 50% bonus (weekend and holiday in between)

@azimgd and @sobitneupane paid $12,000
@hungvu193 , can you please accept the job and reply here once you have so I can pay the reporting bonus?
https://www.upwork.com/jobs/~017185b708f6ccd9fb

@hungvu193
Copy link
Contributor

@mallenexpensify I've just applied, thank you

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Jan 13, 2023

sorry, @mallenexpensify, would you be able to pay this? I'm really sick today and going to take ooo. thank you for taking it

@Christinadobrzyn Christinadobrzyn removed their assignment Jan 13, 2023
@mallenexpensify
Copy link
Contributor

mallenexpensify commented Jan 13, 2023

@hungvu193 paid, thanks!

For regression test steps, proposing
Add to TestRail here - Profile - Modify User Avatar

  1. Save an image that is between 80px and 2048px
  2. Open staging.new.expensify.com.
  3. Open Settings, click on Avatar to open Profile Page.
  4. Click on Edit photo and upload the image
  5. Verify the slides to zoom in and out scrolls smoothly.

@tgolen @sobitneupane can you finish the top three items on the checklist?

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • The PR that introduced the bug has been identified. Link to the PR:
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://github.com/Expensify/Expensify/issues/256180

@mallenexpensify
Copy link
Contributor

Should have feedback on the regression test steps by Monday

@sobitneupane
Copy link
Contributor

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • The PR that introduced the bug has been identified. Link to the PR:

I think this bug was present from the very beginning. This feature was implemented in #8511.

@mallenexpensify
Copy link
Contributor

@melvin-bot
Copy link

melvin-bot bot commented Jan 19, 2023

@tgolen @mallenexpensify Be sure to fill out the Contact List!

@mallenexpensify
Copy link
Contributor

Regression test added https://github.com/Expensify/Expensify/issues/256180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests