Skip to content

fix: Profile - No error after uploading corrupted jpg for avatar, console error shows up. #49203

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.

Sign up for GitHub

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

Merged
srikarparsi merged 10 commits into from
Oct 1, 2024
Merged

fix: Profile - No error after uploading corrupted jpg for avatar, console error shows up. #49203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
34a4d8c
fix: Profile - No error after uploading corrupted jpg for avatar, con…
Krishna2323 Sep 14, 2024
5651e49
fix error message styling for native.
Krishna2323 Sep 14, 2024
fa74969
updated error message.
Krishna2323 Sep 14, 2024
512555d
fix: corrupt image validation on native.
Krishna2323 Sep 14, 2024
fa96a9c
minor fix.
Krishna2323 Sep 14, 2024
f988185
skip image validation in AttachmentPicker.
Krishna2323 Sep 14, 2024
387c896
Merge branch 'main' into krishna2323/issue/48707
Krishna2323 Sep 23, 2024
c6b7bb8
minor styling fix.
Krishna2323 Sep 23, 2024
3939e4c
Merge branch 'Expensify:main' into krishna2323/issue/48707
Krishna2323 Sep 30, 2024
72cf668
add comment.
Krishna2323 Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions src/components/AttachmentPicker/index.native.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ const getDataForUpload = (fileData: FileResponse): Promise<FileObject> => {
* a callback. This is the ios/android implementation
* opening a modal with attachment options
*/
function AttachmentPicker({type = CONST.ATTACHMENT_PICKER_TYPE.FILE, children, shouldHideCameraOption = false}: AttachmentPickerProps) {
function AttachmentPicker({type = CONST.ATTACHMENT_PICKER_TYPE.FILE, children, shouldHideCameraOption = false, shouldValidateImage = true}: AttachmentPickerProps) {
const styles = useThemeStyles();
const [isVisible, setIsVisible] = useState(false);

Expand Down Expand Up @@ -318,6 +318,26 @@ function AttachmentPicker({type = CONST.ATTACHMENT_PICKER_TYPE.FILE, children, s
width: ('width' in fileData && fileData.width) || undefined,
height: ('height' in fileData && fileData.height) || undefined,
};

if (!shouldValidateImage && fileDataName && Str.isImage(fileDataName)) {
ImageSize.getSize(fileDataUri)
.then(({width, height}) => {
fileDataObject.width = width;
fileDataObject.height = height;
return fileDataObject;
})
.then((file) => {
getDataForUpload(file)
.then((result) => {
completeAttachmentSelection.current(result);
})
.catch((error: Error) => {
showGeneralAlert(error.message);
throw error;
});
});
return;
}

@Krishna2323 Krishna2323 Sep 14, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ollyws, we have to skip corrupt image validation in AttachmentPicker because, on native devices, we were showing an alert modal if the image was corrupt, which we don't need when using AttachmentPicker inside AvatarWithImagePicker because we already perform validation in the AvatarWithImagePicker component.

@Ollyws Ollyws Sep 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...would it make more sense to do the image validation in AttachmentPicker on both platforms?

@Krishna2323 Krishna2323 Sep 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but I wasn't able to get it working. I'll try again today or tomorrow.

@Krishna2323 Krishna2323 Sep 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ollyws, I tried and we can show the validation modal in AttachmentPicker for web, but I don't think it's worth it since the native validation shows only the default alert and in this case we should show the our custom modal. We can simply skip the validation in native file. WDYT?

@Krishna2323 Krishna2323 Sep 24, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ollyws, friendly bump to check the comment above.

@Ollyws Ollyws Sep 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simply skip the validation in native file

You mean just leave the changes as they are?

@Krishna2323 Krishna2323 Sep 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep.

@Ollyws Ollyws Sep 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it's not a huge issue so I think it's fine as is.

/* eslint-enable @typescript-eslint/prefer-nullish-coalescing */
if (fileDataName && Str.isImage(fileDataName)) {
ImageSize.getSize(fileDataUri)
Expand All @@ -331,7 +351,7 @@ function AttachmentPicker({type = CONST.ATTACHMENT_PICKER_TYPE.FILE, children, s
return validateAndCompleteAttachmentSelection(fileDataObject);
}
},
[validateAndCompleteAttachmentSelection, showImageCorruptionAlert],
[validateAndCompleteAttachmentSelection, showImageCorruptionAlert, shouldValidateImage, showGeneralAlert],
);

/**
Expand Down
3 changes: 3 additions & 0 deletions src/components/AttachmentPicker/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ type AttachmentPickerProps = {
type?: ValueOf<typeof CONST.ATTACHMENT_PICKER_TYPE>;

acceptedFileTypes?: Array<ValueOf<typeof CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS>>;

/** Whether to validate the image and show the alert or not. */
shouldValidateImage?: boolean;
};

export default AttachmentPickerProps;
49 changes: 29 additions & 20 deletions src/components/AvatarWithImagePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,26 +232,31 @@ function AvatarWithImagePicker({
return;
}

isValidResolution(image).then((isValid) => {
if (!isValid) {
setError('avatarWithImagePicker.resolutionConstraints', {
minHeightInPx: CONST.AVATAR_MIN_HEIGHT_PX,
minWidthInPx: CONST.AVATAR_MIN_WIDTH_PX,
maxHeightInPx: CONST.AVATAR_MAX_HEIGHT_PX,
maxWidthInPx: CONST.AVATAR_MAX_WIDTH_PX,
FileUtils.validateImageForCorruption(image)
.then(() => isValidResolution(image))
.then((isValid) => {
if (!isValid) {
setError('avatarWithImagePicker.resolutionConstraints', {
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;
}

setIsAvatarCropModalOpen(true);
setError(null, {});
setIsMenuVisible(false);
setImageData({
uri: image.uri ?? '',
name: image.name ?? '',
type: image.type ?? '',
});
return;
}

setIsAvatarCropModalOpen(true);
setError(null, {});
setIsMenuVisible(false);
setImageData({
uri: image.uri ?? '',
name: image.name ?? '',
type: image.type ?? '',
})
.catch(() => {
setError('attachmentPicker.errorWhileSelectingCorruptedAttachment', {});
});
});
},
[isValidExtension, isValidSize],
);
Expand Down Expand Up @@ -339,7 +344,11 @@ function AvatarWithImagePicker({
maybeIcon={isUsingDefaultAvatar}
>
{({show}) => (
<AttachmentPicker type={CONST.ATTACHMENT_PICKER_TYPE.IMAGE}>
<AttachmentPicker
type={CONST.ATTACHMENT_PICKER_TYPE.IMAGE}
// We need to skip the validation in AttachmentPicker because it is handled in this component itself
shouldValidateImage={false}

@srikarparsi srikarparsi Sep 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a comment explaining why we don't need to validate here

@Krishna2323 Krishna2323 Sep 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment added.

>
{({openPicker}) => {
const menuItems = createMenuItems(openPicker);

Expand Down Expand Up @@ -383,7 +392,7 @@ function AvatarWithImagePicker({
{source ? (
<Avatar
containerStyles={avatarStyle}
imageStyles={[avatarStyle, styles.alignSelfCenter]}
imageStyles={[styles.alignSelfCenter, avatarStyle]}
source={source}
avatarID={avatarID}
fallbackIcon={fallbackIcon}
Expand Down
11 changes: 9 additions & 2 deletions src/libs/fileDownload/FileUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,15 @@ function validateImageForCorruption(file: FileObject): Promise<{width: number; h
}
return new Promise((resolve, reject) => {
ImageSize.getSize(file.uri ?? '')
.then(() => resolve())
.catch(() => reject(new Error('Error reading file: The file is corrupted')));
.then((size) => {
if (size.height <= 0 || size.width <= 0) {

@Ollyws Ollyws Sep 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why's this change necessary?

@Krishna2323 Krishna2323 Sep 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On native, when an image is corrupted, ImageSize.getSize does not throw an error but returns an image with a width and height of -1.

The same method is used here:

App/src/components/AttachmentPicker/index.native.tsx

Lines 272 to 280 in 37bf6b2

const validateAndCompleteAttachmentSelection = useCallback(
(fileData: FileResponse) => {
// Check if the file dimensions indicate corruption
// The width/height for a corrupted file is -1 on android native and 0 on ios native
// We must check only numeric values because the width/height can be undefined for non-image files
if ((typeof fileData.width === 'number' && fileData.width <= 0) || (typeof fileData.height === 'number' && fileData.height <= 0)) {
showImageCorruptionAlert();
return Promise.resolve();
}

return reject(new Error('Error reading file: The file is corrupted'));
}
resolve();
})
.catch(() => {
return reject(new Error('Error reading file: The file is corrupted'));
});
});
}

Expand Down
2 changes: 1 addition & 1 deletion src/pages/settings/Profile/ProfilePage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ function ProfilePage() {
childrenStyles={styles.pt5}
titleStyles={styles.accountSettingsSectionTitle}
>
<View style={[styles.pt3, styles.pb6, styles.alignSelfStart]}>
<View style={[styles.pt3, styles.pb6, styles.alignSelfStart, styles.w100]}>
{isEmptyObject(currentUserPersonalDetails) || accountID === -1 || !avatarURL ? (
<AvatarSkeleton size={CONST.AVATAR_SIZE.XLARGE} />
) : (
Expand Down