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.

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

Scan - No error modal shown after selecting corrupted image. #40162

Merged
merged 6 commits into from
Apr 15, 2024
Merged

Scan - No error modal shown after selecting corrupted image. #40162

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7cf12f5
Scan - No error shown after selecting corrupted image.
Krishna2323 Apr 12, 2024
3f819b8
fix: no error alert for corrupt image in native devices.
Krishna2323 Apr 12, 2024
a7a7729
Merge branch 'Expensify:main' into krishna2323/issue/37435
Krishna2323 Apr 12, 2024
5e41d85
update image validator function name.
Krishna2323 Apr 12, 2024
dc28844
minor fix.
Krishna2323 Apr 13, 2024
028748b
update validateAndCompleteAttachmentSelection and add comments.
Krishna2323 Apr 15, 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
18 changes: 11 additions & 7 deletions src/components/AttachmentPicker/index.native.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ function AttachmentPicker({type = CONST.ATTACHMENT_PICKER_TYPE.FILE, children, s

const validateAndCompleteAttachmentSelection = useCallback(
(fileData: FileResponse) => {
if (fileData.width === -1 || fileData.height === -1) {
if (fileData.width === -1 || fileData.height === -1 || (fileData.height === 0 && fileData.width === 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this some edge case when fileData.height === 0 && fileData.width === 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, on ios native the image width/height for corrupted image is 0 and on android native it is -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paultsimura, you mean you are unable to find corrupted image on ios attachment picker or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I've removed the previous comment – finally figured out how to upload the corrupted image into my simulator

showImageCorruptionAlert();
return Promise.resolve();
}
Expand Down Expand Up @@ -283,16 +283,20 @@ function AttachmentPicker({type = CONST.ATTACHMENT_PICKER_TYPE.FILE, children, s
};
/* eslint-enable @typescript-eslint/prefer-nullish-coalescing */
if (fileDataName && Str.isImage(fileDataName)) {
ImageSize.getSize(fileDataUri).then(({width, height}) => {
fileDataObject.width = width;
fileDataObject.height = height;
validateAndCompleteAttachmentSelection(fileDataObject);
});
ImageSize.getSize(fileDataUri)
.then(({width, height}) => {
fileDataObject.width = width;
fileDataObject.height = height;
validateAndCompleteAttachmentSelection(fileDataObject);
})
.catch(() => {
showImageCorruptionAlert();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: could you please update this?

Suggested change
.catch(() => {
showImageCorruptionAlert();
});
.catch(() => showImageCorruptionAlert());

} else {
return validateAndCompleteAttachmentSelection(fileDataObject);
}
},
[validateAndCompleteAttachmentSelection],
[validateAndCompleteAttachmentSelection, showImageCorruptionAlert],
);

/**
Expand Down
19 changes: 19 additions & 0 deletions src/libs/fileDownload/FileUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import Str from 'expensify-common/lib/str';
import {Alert, Linking, Platform} from 'react-native';
import ImageSize from 'react-native-image-size';
import type {FileObject} from '@components/AttachmentModal';
import DateUtils from '@libs/DateUtils';
import * as Localize from '@libs/Localize';
import Log from '@libs/Log';
Expand Down Expand Up @@ -238,6 +241,21 @@ function base64ToFile(base64: string, filename: string): File {
return file;
}

function checkIfImageIsCorrupted(file: FileObject) {
return new Promise((resolve, reject) => {
if (!Str.isImage(file.name ?? '')) {
resolve(true);
}
ImageSize.getSize(file.uri ?? '')
.then(() => {
resolve(true);
})
.catch(() => {
reject(new Error(`'Error reading file: The file is corrupted`));
});
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's better go this way:

  1. Rename the function;
  2. If the file is not an image – return the resolve instead of simply calling it;
Suggested change
function checkIfImageIsCorrupted(file: FileObject) {
return new Promise((resolve, reject) => {
if (!Str.isImage(file.name ?? '')) {
resolve(true);
}
ImageSize.getSize(file.uri ?? '')
.then(() => {
resolve(true);
})
.catch(() => {
reject(new Error(`'Error reading file: The file is corrupted`));
});
});
}
function validateImageForCorruption(file: FileObject): Promise<void> {
if (!Str.isImage(file.name ?? '')) {
return Promise.resolve();
}
return new Promise((resolve, reject) => {
ImageSize.getSize(file.uri ?? '')
.then(() => resolve())
.catch(() => reject(new Error('Error reading file: The file is corrupted')));
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a friendly piece of advice: for a better review process, please click the "refresh" button here once the changes are ready for re-review. This way, the reviewer will get a proper notification.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will keep this in mind from next time. I appreciate the tip :)


export {
showGeneralErrorAlert,
showSuccessAlert,
Expand All @@ -250,4 +268,5 @@ export {
appendTimeToFileName,
readFileAsync,
base64ToFile,
checkIfImageIsCorrupted,
};
68 changes: 39 additions & 29 deletions src/pages/iou/request/step/IOURequestStepScan/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,25 +175,34 @@ function IOURequestStepScan({
};

function validateReceipt(file: FileObject) {
const {fileExtension} = FileUtils.splitExtensionFromFileName(file?.name ?? '');
if (
!CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS.includes(fileExtension.toLowerCase() as (typeof CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS)[number])
) {
setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.notAllowedExtension');
return false;
}
return FileUtils.checkIfImageIsCorrupted(file)
.then(() => {
const {fileExtension} = FileUtils.splitExtensionFromFileName(file?.name ?? '');
if (
!CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS.includes(
fileExtension.toLowerCase() as (typeof CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS)[number],
)
) {
setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.notAllowedExtension');
return false;
}

if ((file?.size ?? 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) {
setUploadReceiptError(true, 'attachmentPicker.attachmentTooLarge', 'attachmentPicker.sizeExceeded');
return false;
}
if ((file?.size ?? 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) {
setUploadReceiptError(true, 'attachmentPicker.attachmentTooLarge', 'attachmentPicker.sizeExceeded');
return false;
}

if ((file?.size ?? 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
setUploadReceiptError(true, 'attachmentPicker.attachmentTooSmall', 'attachmentPicker.sizeNotMet');
return false;
}
if ((file?.size ?? 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
setUploadReceiptError(true, 'attachmentPicker.attachmentTooSmall', 'attachmentPicker.sizeNotMet');
return false;
}

return true;
return true;
})
.catch(() => {
setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedImage');
return false;
});
}

const navigateBack = () => {
Expand Down Expand Up @@ -230,21 +239,22 @@ function IOURequestStepScan({
* Sets the Receipt objects and navigates the user to the next page
*/
const setReceiptAndNavigate = (file: FileObject) => {
if (!validateReceipt(file)) {
return;
}

// Store the receipt on the transaction object in Onyx
const source = URL.createObjectURL(file as Blob);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
IOU.setMoneyRequestReceipt(transactionID, source, file.name || '', action !== CONST.IOU.ACTION.EDIT);
validateReceipt(file).then((isFileValid) => {
if (!isFileValid) {
return;
}
// Store the receipt on the transaction object in Onyx
const source = URL.createObjectURL(file as Blob);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
IOU.setMoneyRequestReceipt(transactionID, source, file.name || '', action !== CONST.IOU.ACTION.EDIT);

if (action === CONST.IOU.ACTION.EDIT) {
updateScanAndNavigate(file, source);
return;
}
if (action === CONST.IOU.ACTION.EDIT) {
updateScanAndNavigate(file, source);
return;
}

navigateToConfirmationStep();
navigateToConfirmationStep();
});
};

const setupCameraPermissionsAndCapabilities = (stream: MediaStream) => {
Expand Down
Loading