App icon indicating copy to clipboard operation
App copied to clipboard

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

Open izarutskaya opened this issue 1 year ago • 36 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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?

  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] 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-03-30

izarutskaya avatar Feb 28 '24 15:02 izarutskaya

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

melvin-bot[bot] avatar Feb 28 '24 15:02 melvin-bot[bot]

@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 avatar Feb 28 '24 15:02 izarutskaya

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

izarutskaya avatar Feb 28 '24 15:02 izarutskaya

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:

https://github.com/Expensify/App/blob/3e74ddf0d2b408faa270ff243cf333065b177a87/src/components/MoneyRequestConfirmationList.tsx#L627-L634

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:

https://github.com/Expensify/App/assets/77965000/51c44b5c-6dbd-4bd8-885c-360544ba4ede

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.

rayane-d avatar Feb 29 '24 00:02 rayane-d

Triaged

greg-schroeder avatar Mar 02 '24 00:03 greg-schroeder

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

melvin-bot[bot] avatar Mar 02 '24 00:03 melvin-bot[bot]

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

melvin-bot[bot] avatar Mar 02 '24 00:03 melvin-bot[bot]

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. https://github.com/Expensify/App/blob/a5092f5661478dda230204dfaf6be9e2b66e6e27/src/libs/ReceiptUtils.ts#L36

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 <Image/> component.
  2. Pass the error state to getThumbnailAndImageURIs util function.
  3. Before returning thumbnail & image url check for error which is passed. https://github.com/Expensify/App/blob/a5092f5661478dda230204dfaf6be9e2b66e6e27/src/libs/ReceiptUtils.ts#L54 https://github.com/Expensify/App/blob/a5092f5661478dda230204dfaf6be9e2b66e6e27/src/libs/ReceiptUtils.ts#L58
    // 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

https://github.com/Expensify/App/assets/85894871/0d884d71-7467-4406-8686-707d787e9d7f

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. https://github.com/Expensify/App/blob/e9c112a2b89217401dfef3e248fa8e93d4c4e418/src/components/AttachmentPicker/index.native.js#L271-L274

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.

https://github.com/Expensify/App/blob/3e74ddf0d2b408faa270ff243cf333065b177a87/src/components/MoneyRequestConfirmationList.tsx#L627

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

https://github.com/Expensify/App/assets/85894871/e1b1151a-b7d6-4d85-9096-a8458b87f371

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);
   },
);

Krishna2323 avatar Mar 02 '24 01:03 Krishna2323

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.

kmbcook avatar Mar 02 '24 19:03 kmbcook

proposals are in review

situchan avatar Mar 04 '24 14:03 situchan

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

melvin-bot[bot] avatar Mar 07 '24 19:03 melvin-bot[bot]

eta Monday

situchan avatar Mar 08 '24 19:03 situchan

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Mar 09 '24 16:03 melvin-bot[bot]

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

melvin-bot[bot] avatar Mar 12 '24 19:03 melvin-bot[bot]

Proposal review

greg-schroeder avatar Mar 13 '24 10:03 greg-schroeder

@greg-schroeder @situchan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Mar 13 '24 19:03 melvin-bot[bot]

Screenshot 2024-03-14 at 1 36 42 AM

@Expensify/design do we agree to show thumbnail when attached receipt is not previewable (i.e. corrupted image/pdf)?

situchan avatar Mar 13 '24 19:03 situchan

I think showing a fallback image makes sense—but I'm not sure I would show that type of thumbnail. I feel like it kinda makes more sense to show our little fallback image thingy:

CleanShot 2024-03-13 at 14 52 29@2x

What do the other designers think?

dannymcclain avatar Mar 13 '24 19:03 dannymcclain

Yup, I agree with Danny here.

shawnborton avatar Mar 13 '24 20:03 shawnborton

Agree with Shawn who agrees with Danny

dubielzyk-expensify avatar Mar 13 '24 23:03 dubielzyk-expensify

Thanks for feedback. Please update proposals.

situchan avatar Mar 15 '24 21:03 situchan

@situchan, I guess we shouldn't allow users to upload corrupted images when creating a scan request, it will eventually fail. We can hold on this issue if we agree to block users from creating a scan request. In native we can't even upload corrupted images.

Krishna2323 avatar Mar 15 '24 21:03 Krishna2323

@Krishna2323 the expected behavior should be discussed with design team. Why don't we just use this GH if we don't allow corrupted images? I see the the linked issue created just today with no context.

situchan avatar Mar 15 '24 21:03 situchan

Yeah, we can handle that here since we already have context. But I think we should confirm with the design team before we try to hold that issue because it has some differences.

@shawnborton can you pls help

Krishna2323 avatar Mar 15 '24 21:03 Krishna2323

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] avatar Mar 16 '24 16:03 melvin-bot[bot]

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

melvin-bot[bot] avatar Mar 19 '24 19:03 melvin-bot[bot]

@greg-schroeder @situchan this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] avatar Mar 20 '24 19:03 melvin-bot[bot]

We're going to prevent uploading corrupted PDF as scan receipt in #34032. So to be consistent, we should not allow corrupted images. cc: @Expensify/design to confirm the expected behavior.

situchan avatar Mar 22 '24 09:03 situchan

^ is only when we're able to detect if file is corrupted or not. For no-preview files we cannot detect if corrupted or not, I think it still makes sense to show fallback image like this.

situchan avatar Mar 22 '24 09:03 situchan

We're going to prevent uploading corrupted PDF as scan receipt in https://github.com/Expensify/App/issues/34032. So to be consistent, we should not allow corrupted images.

@situchan that makes sense to me.

For no-preview files we cannot detect if corrupted or not, I think it still makes sense to show fallback image like https://github.com/Expensify/App/issues/37435#issuecomment-1995579903.

Agree with this as well!

dannymcclain avatar Mar 22 '24 13:03 dannymcclain