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

[$500] [MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview #37435

Closed
6 tasks done
izarutskaya opened this issue Feb 28, 2024 · 98 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Feb 28, 2024

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


Version Number: v1.4.44-8
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team

Action Performed:

  1. Go to staging.new.expensify.com.
  2. Go to workspace chat.
  3. Go to + > Request money > Scan.
  4. Upload a corrupted file (attached below).
  5. Proceed to confirmation page.

Expected Result:

The confirmation page will show thumbnail for the corrupted file.

Actual Result:

The confirmation page shows blank area for the corrupted file.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6395765_1709133702609!Screenshot_2024-02-27_at_22 59 30
Corrupted file (1)

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ddfcfb2a8a486a23
  • Upwork Job ID: 1763726750178574336
  • Last Price Increase: 2024-04-06
  • Automatic offers:
    • paultsimura | Reviewer | 0
    • Krishna2323 | Contributor | 0
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 28, 2024
Copy link

melvin-bot bot commented Feb 28, 2024

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

@izarutskaya
Copy link
Author

@greg-schroeder I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

@izarutskaya
Copy link
Author

We think that this bug might be related to #wave5-free-submitters
CC @dylanexpensify

@rayane-djouah
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

We are not showing a fallback image when image loading fails.

What is the root cause of that problem?

in the Image component here and here we don't have a mechanism to display a fallback image when image loading fails.

What changes do you think we should make in order to solve the problem?

we should make usage of the onError prop of react-native Image component expo-image Image component.
https://reactnative.dev/docs/image#onerror
https://docs.expo.dev/versions/latest/sdk/image/#onerror
add a prop to our Image component to accept a fallbackSource for the fallback image.
add a new state to the component:

    const [hasImageError, setHasImageError] = useState(false);

add this function:

    const handleImageError = () => {
        setHasImageError(true);
    };

and pass it as onError prop for react-native and expo-image Image components here and here
example:

    return (
        <RNImage
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...forwardedProps}
            source={hasImageError ? fallbackSource : source}
            onError={handleImageError}
        />
    );

based on the hasImageError we determine if we should use fallbackSource or source.

and then, here:

<Image
style={styles.moneyRequestImage}
source={{uri: receiptData?.thumbnail ?? receiptData?.image}}
// AuthToken is required when retrieving the image from the server
// but we don't need it to load the blob:// or file:// image when starting a money request / split bill
// So if we have a thumbnail, it means we're retrieving the image from the server
isAuthTokenRequired={!!receiptData.thumbnail}
/>

we should pass fallbackSource for the receipt.

we should ask design team to design the fallback error image for the receipt.

we can also design a generic fallback error image and use it as a fallbackSource default prop value for Image component.

Result:

for demo, I used @assets/images/eReceiptBGs/eReceiptBG_pink.png image as a fallback image:

fallback.image.Recording.2024-02-29.010427.mp4

What alternative solutions did you explore? (Optional)

we can also use defaultsource prop for react-native Image component and placeholder for expo-image Image component.
https://reactnative.dev/docs/image#defaultsource
https://docs.expo.dev/versions/latest/sdk/image/#placeholder
but the placeholder image will also show when loading.

@melvin-bot melvin-bot bot added the Overdue label Mar 1, 2024
@greg-schroeder greg-schroeder changed the title Scan - No thumbnail is shown for the corrupted file in receipt preview [MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview Mar 2, 2024
@greg-schroeder greg-schroeder added the External Added to denote the issue can be worked on by a contributor label Mar 2, 2024
@greg-schroeder
Copy link
Contributor

Triaged

@melvin-bot melvin-bot bot changed the title [MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview [$500] [MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview Mar 2, 2024
Copy link

melvin-bot bot commented Mar 2, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01ddfcfb2a8a486a23

@melvin-bot melvin-bot bot added Help Wanted Apply this label when an issue is open to proposals by contributors and removed Overdue labels Mar 2, 2024
Copy link

melvin-bot bot commented Mar 2, 2024

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Mar 2, 2024

Initial proposal ### Proposal

Please re-state the problem that we are trying to solve in this issue.

[MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview

What is the root cause of that problem?

We don't handle image errors in MoneyTemporaryForRefactorRequestConfirmationList & we don't check for errors before returning the thumbnail & image.

function getThumbnailAndImageURIs(transaction: OnyxEntry<Transaction>, receiptPath: string | null = null, receiptFileName: string | null = null): ThumbnailAndImageURI {

What changes do you think we should make in order to solve the problem?

For showing the default svgs like ReceiptGeneric, we need to follow steps below.

  1. Create a state for image error and set that to true when onError triggers on component.
  2. Pass the error state to getThumbnailAndImageURIs util function.
  3. Before returning thumbnail & image url check for error which is passed.
    if (isReceiptImage && typeof path === 'string' && (path.startsWith('blob:') || path.startsWith('file:'))) {

    if (isReceiptImage) {
    // For local files, we won't have a thumbnail yet
    if (isReceiptImage && !error && typeof path === 'string' && (path.startsWith('blob:') || path.startsWith('file:'))) {
        return {thumbnail: null, image: path, isLocalFile: true, filename};
    }

    if (isReceiptImage && !error) {
        return {thumbnail: `${path}.1024.jpg`, image: path, filename};
    }

Result

corruptedImageBug.mp4

Alternatively

We can directly use ReceiptGeneric image as source when image error is true.
source={{uri: imageError ? ReceiptGeneric : receiptThumbnail || receiptImage}}

Proposal

Please re-state the problem that we are trying to solve in this issue.

Android - IOU - Nothing happens when you select corrupted jpg file in SmartScan/no error message

What is the root cause of that problem?

ImageSize.getSize fails to get the size of the image when its corrupted and throws error, but we don't handle it to show alert incase it fails.

RNImage.getSize(fileData.fileCopyUri || fileData.uri, (width, height) => {
fileData.width = width;
fileData.height = height;
return validateAndCompleteAttachmentSelection(fileData);

What changes do you think we should make in order to solve the problem?

We need to use third parameter of ImageSize.getSize which receives a failure callback and handle the error accordingly. Incase of corrupt image it throws unknown image format, we can use it and show corrupt image alert else we will show general alert.

For no-preview files we cannot detect if corrupted or not so we will use the image suggested by design team and pass it to fallbackSource.

ImageSize.getSize(
   fileData.fileCopyUri || fileData.uri,
   (width, height) => {
       fileData.width = width;
       fileData.height = height;
       return validateAndCompleteAttachmentSelection(fileData);
   },
   (error) => {
       if (error.message === 'unknown image format') {
           showImageCorruptionAlert();
       } else {
           showGeneralAlert();
       }
   },
);

If we want to block users from adding corrupted images on web also we can use ImageSize.getSize and block user and show modal when failure callback is called.

            ImageSize.getSize(
               fileCopy.fileCopyUri || fileCopy.uri,
               () => {},
               (e) => {
                   /// Block users and show message.
               },
           );

This is just a pseudo-code, it will require more changes but approach will be to use RNImage.getSize to check if the image file is corrupted or not. Will only add this check for images by using Str.isImage(fileCopy.fileName || fileCopy.name)

Pseudo code for web
    function checkIfImageIsCorrupted(file) {
        return new Promise((resolve, reject) => {
            if (!Str.isImage(file.name)) {
                resolve(true)
            }
            ImageSize.getSize(file.uri)
                .then((r) => {
                    resolve(true);
                })
                .catch(() => {
                    reject(new Error(`'Error reading file: The file is corrupted`));
                });
        });
    }
    function validateReceipt(file) {
        return checkIfImageIsCorrupted(file)
            .then(() => {
                const {fileExtension} = FileUtils.splitExtensionFromFileName(lodashGet(file, 'name', ''));
                if (!CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS.includes(fileExtension.toLowerCase())) {
                    setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.notAllowedExtension');
                    return false;
                }

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

                if (lodashGet(file, 'size', 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
                    setUploadReceiptError(true, 'attachmentPicker.attachmentTooSmall', 'attachmentPicker.sizeNotMet');
                    return false;
                }
                return true;
            })
            .catch(() => {
                setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedImage');
                return false;
            });
    }
    const setReceiptAndNavigate = (file) => {
        validateReceipt(file).then((isFileValid) => {
            if (!isFileValid) {
                return;
            }
            // Store the receipt on the transaction object in Onyx
            const source = URL.createObjectURL(file);
            IOU.setMoneyRequestReceipt(transactionID, source, file.name, action !== CONST.IOU.ACTION.EDIT);

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

            navigateToConfirmationStep();
        });
    };
Fix for native bug here: /issues/38389
RNImage.getSize(
   fileData.fileCopyUri || fileData.uri,
   (width, height) => {
       fileData.width = width;
       fileData.height = height;
       return validateAndCompleteAttachmentSelection(fileData);
   },
   (error) => {
       validateAndCompleteAttachmentSelection(fileData);
   },
);

Result

corrupt_image_fix.mp4

Alternative

Or simply use fileData (without modifications) when error callback is triggered.

RNImage.getSize(
   fileData.fileCopyUri || fileData.uri,
   (width, height) => {
       fileData.width = width;
       fileData.height = height;
       return validateAndCompleteAttachmentSelection(fileData);
   },
   (error) => {
       validateAndCompleteAttachmentSelection(fileData);
   },
);

@kmbcook
Copy link
Contributor

kmbcook commented Mar 2, 2024

The corrupted file used to create this issue seems to be a very small image, only 16 x 16 pixels. A thumbnail of this image might appear blank, possibly. The corrupted file I am using works as expected. I see a thumbnail of it.

@melvin-bot melvin-bot bot added the Overdue label Mar 4, 2024
@situchan
Copy link
Contributor

situchan commented Mar 4, 2024

proposals are in review

Copy link

melvin-bot bot commented Mar 7, 2024

@greg-schroeder, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@situchan
Copy link
Contributor

situchan commented Mar 8, 2024

eta Monday

@melvin-bot melvin-bot bot removed the Overdue label Mar 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Apr 8, 2024

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer 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 📖

@thienlnam thienlnam changed the title [$500] [MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview [HOLD #37435][$500] [MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview Apr 10, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 10, 2024
Copy link

melvin-bot bot commented Apr 11, 2024

@arosiclair, @paultsimura, @greg-schroeder, @Krishna2323 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@paultsimura
Copy link
Contributor

@Krishna2323 do you have an approximate estimate for the PR?

@melvin-bot melvin-bot bot removed the Overdue label Apr 11, 2024
@Krishna2323
Copy link
Contributor

Will surely raise the PR tomorrow.

@paultsimura
Copy link
Contributor

@greg-schroeder could we please remove the HOLD from the issue title?

@greg-schroeder greg-schroeder added Daily KSv2 and removed Weekly KSv2 labels Apr 15, 2024
@greg-schroeder greg-schroeder changed the title [HOLD #37435][$500] [MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview [$500] [MEDIUM] Scan - No thumbnail is shown for the corrupted file in receipt preview Apr 15, 2024
@greg-schroeder
Copy link
Contributor

greg-schroeder commented Apr 15, 2024

Hold removed, moved back to Daily. PR is in review

Copy link

melvin-bot bot commented Apr 18, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Copy link

melvin-bot bot commented Apr 18, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@paultsimura
Copy link
Contributor

This was deployed to production on April 22, due payment 2024-04-29

@Krishna2323
Copy link
Contributor

@sakluger, gentle reminder for payments.

@Krishna2323
Copy link
Contributor

@sakluger, bump for payments 🙇🏻

@greg-schroeder
Copy link
Contributor

Hmm. Sorry, the automation seems to have failed this one. We generally search for payments by issue title + label. I'll get this taken care of now.

@greg-schroeder
Copy link
Contributor

Done, apologies.

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. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
No open projects
Development

No branches or pull requests