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 2024-02-07] [$500] IOU - Replacing Receipt with an Invalid File Format Does Not Display an Error Message #28343

Closed
1 of 6 tasks
lanitochka17 opened this issue Sep 27, 2023 · 111 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Sep 27, 2023

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. Login to any account
  2. Send a Money Request to another account, using the Scan Receipt option
  3. Click on the Receipt image to open the preview popup
  4. Click on the “Three dots” icon >> select the “Replace” option
  5. Replace the current image with another image in an unsupported format, such as "webp"
  6. Observe that: there is no error, but when reloading the page, the old image still appears (which indicates that new image is not uploaded)

Expected Result:

In this case, the user has uploaded an invalid image format, so there should be an error message. The file should remain optimistically in Onyx until the user decides to clear the error.

Actual Result:

No error displays.

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.74-2

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Chrome-Desktop.mp4
Safari-Desktop.mov

8123857_img-0539_720

bandicam.2023-09-28.00-58-54-891.mp4

Expensify/Expensify Issue URL:

Issue reported by: @tranvantoan-qn

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695720594688229

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bb8307379994bc8e
  • Upwork Job ID: 1707109060385632256
  • Last Price Increase: 2023-10-18
  • Automatic offers:
    • hoangzinh | Reviewer | 27267319
    • tienifr | Contributor | 27267321
    • tranvantoan-qn | Reporter | 27267322
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 27, 2023
@melvin-bot melvin-bot bot changed the title IOU - Replacing Receipt with an Invalid File Format Does Not Display an Error Message [$500] IOU - Replacing Receipt with an Invalid File Format Does Not Display an Error Message Sep 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 27, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot
Copy link

melvin-bot bot commented Sep 27, 2023

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

@ikevin127
Copy link
Contributor

ikevin127 commented Sep 27, 2023

Proposal - Updated

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

The app allows the user to upload an invalid file format but then shows the old image.

What is the root cause of that problem?

If we want to forbid files with UNALLOWED_EXTENSIONS from being uploaded (front-end only fix, outdated):

The problem stems from the validateReceipt function located in the ReceiptSelector/index.js component:

function validateReceipt(file) {
    const {fileExtension} = FileUtils.splitExtensionFromFileName(lodashGet(file, 'name', ''));
    if (_.contains(CONST.API_ATTACHMENT_VALIDATIONS.UNALLOWED_EXTENSIONS, fileExtension.toLowerCase())) {
        setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.notAllowedExtension');
        return false;
    }
    ...

In this case UNALLOWED_EXTENSION does not exist within CONST.API_ATTACHMENT_VALIDATIONS, therefore no file format is excluded from upload as receipt.

If we want to show an error message and for the file remain optimistically in Onyx until the user decides to clear the error (updated expected result):

  1. The first problem comes from the fact that IOU.replaceReceipt function is currently getting the transaction by using lodashGet(allTransactions, 'transactionID', {}) which is not corrent because allTransactions is the main object which has keys starting with ransactions_{key} format, therefore transaction is always empty.

  2. The second problem is that Onyx failureData is not being populated with errors when the ReplaceReceipt api call fails.

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

Would add UNALLOWED_EXTENSION constant as an array of comma separated strings with the not-allowed formats (TBD).

If we want to show an error message and for the file remain optimistically in Onyx until the user decides to clear the error (updated expected result):

Within IOU.js we should add / change the following:

// Add function that clears transaction error by Id
+ /**
+  * @param {String} transactionID
+  */
+ function clearTransactionError(transactionID){
+     const transaction = lodashGet(allTransactions, `transactions_${transactionID}`, {});
+     Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
+        receipt: transaction.errorsData.receipt,
+        filename: transaction.errorsData.filename,
+        errors: null,
+        errorsData:null
+     });
+ }

/**
 * @param {String} transactionID
 * @param {Object} receipt
 * @param {String} filePath
 */
function replaceReceipt(transactionID, receipt, filePath) {
-    const transaction = lodashGet(allTransactions, 'transactionID', {});
+    const transaction = lodashGet(allTransactions, `transactions_${transactionID}`, {});
    const oldReceipt = lodashGet(transaction, 'receipt', {});

    const optimisticData = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
            value: {
                receipt: {
                    source: filePath,
                    state: CONST.IOU.RECEIPT_STATE.OPEN,
                },
                filename: receipt.name,
            },
        },
    ];

// Populate failureData with error / errorsData
    const failureData = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
            value: {
-                receipt: oldReceipt,
-                filename: transaction.filename,
+                errors: ErrorUtils.getMicroSecondOnyxError(Localize.translateLocal('attachmentPicker.notAllowedExtension')),
+                errorsData: {
+                   receipt: oldReceipt,
+                   filename: transaction.filename,
+                }
            },
        },
    ];

    API.write('ReplaceReceipt', {transactionID, receipt}, {optimisticData, failureData});
}

Within MoneyRequestView.js we should add / change the following:

// Add function that uses the IOU.clearTransactionError() to clear the error.

+    const clearTransactionError = useCallback(() => {
+        IOU.clearTransactionError(transaction.transactionID);
+    }, [transaction]);

{hasReceipt && (
-                <OfflineWithFeedback pendingAction={pendingAction}>
+                <OfflineWithFeedback pendingAction={pendingAction} errors={transaction.errors} onClose={clearTransactionError}>
                    <View style={styles.moneyRequestViewImage}>
                        <ReportActionItemImage
                            thumbnail={receiptURIs.thumbnail}
                            image={receiptURIs.image}
                            enablePreviewModal
                        />
                    </View>
                </OfflineWithFeedback>
            )}

Within OfflineWithFeedback.js we should change the following in order to add padding spacing.ph5 to the error:

{props.shouldShowErrorMessages && hasErrorMessages && (
-                <View style={StyleUtils.combineStyles(styles.offlineFeedback.error, props.errorRowStyles)}>
+                <View style={StyleUtils.combineStyles(styles.offlineFeedback.error, spacing.ph5, props.errorRowStyles)}>
                    <DotIndicatorMessage
                        style={[styles.flex1]}
                        messages={errorMessages}
                        type="error"
                    />
                    <Tooltip text={translate('common.close')}>
                        <PressableWithoutFeedback
                            onPress={props.onClose}
                            style={[styles.touchableButtonImage]}
                            accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
                            accessibilityLabel={translate('common.close')}
                        >
                            <Icon src={Expensicons.Close} />
                        </PressableWithoutFeedback>
                    </Tooltip>
                </View>
            )}

What alternative solutions did you explore? (Optional)

N/A

Videos

web.mp4

@tienifr
Copy link
Contributor

tienifr commented Sep 28, 2023

Proposal

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

IOU - Replacing Receipt with an Invalid File Format Does Not Display an Error Message

What is the root cause of that problem?

  1. In
    if (_.contains(CONST.API_ATTACHMENT_VALIDATIONS.UNALLOWED_EXTENSIONS, fileExtension.toLowerCase())) {

we're using UNALLOWED_EXTENSIONS, but it's not in API_ATTACHMENT_VALIDATIONS -> no error message appear

  1. If the image is allowed in FE side, but get errors from BE side, we should show the error message. But in

    App/src/libs/actions/IOU.js

    Lines 2125 to 2127 in 7ada99f

    function replaceReceipt(transactionID, receipt, filePath) {
    const transaction = lodashGet(allTransactions, 'transactionID', {});
    const oldReceipt = lodashGet(transaction, 'receipt', {});

but we're getting transaction by using lodashGet(allTransactions, 'transactionID', {});, and it's wrong because allTransactions is the object and its keys have the transactions_{key} format, so transaction is always empty

and we don't add errors to failureData -> the error message is not shown

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

  1. We should defined UNALLOWED_EXTENSION in API_ATTACHMENT_VALIDATIONS
  2. Change this line to
    const transaction = lodashGet(allTransactions, `transactions_${transactionID}`, {});
  1. Update failureData to store errors and errorsData
value: {
                errors: ErrorUtils.getMicroSecondOnyxError('iou.error.genericEditFailureMessage'),
                errorsData: {
                    receipt: oldReceipt,
                    filename: transaction.filename,
                }
            },
  1. Create the function to clear transaction errors
function clearError(transactionID){
    const transaction = lodashGet(allTransactions, `transactions_${transactionID}`, {});
    Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {errors: null,
        receipt: transaction.errorsData.receipt,
        filename: transaction.errorsData.filename,
        errorsData:null
    },
    );
}
  1. Add errors to OfflineWithFeedback, this props must be the object for now, but we need to render the component so we need to define the new props for OfflineWithFeedback to render the errors as the React component

  2. Add ConfirmModal component that allow users download local file before dismiss the error

  3. When we refresh app, the receipt and filename will be override from BE results (OpenReport API), but not errors. So we need BE fix to return errors: null

What alternative solutions did you explore? (Optional)

None

Result

Screen.Recording.2023-10-09.at.14.32.18.mov

@trjExpensify
Copy link
Contributor

@JmillsExpensify @youssef-lr, how does this one jive with this issue for allowing all file extensions? Seems like we're silently failing in this case.

CC: @AndrewGable @Gonals as well from the receipt upload implementation

@youssef-lr
Copy link
Contributor

youssef-lr commented Sep 28, 2023

@trjExpensify the issue you linked is for attachments. We have some restrictions when it comes to what types of receipts you can upload I think.

@hoangzinh
Copy link
Contributor

hoangzinh commented Sep 28, 2023

@ikevin127 @tienifr Thanks for proposals, everyone. Both of proposals look good. But @tienifr's proposal #28343 (comment) provides a fix for another bug mentioned in step 6 Observe that: there is no error, but when reloading the page. Currently, if we fail to upload a receipt, although BE already returns an error, the App won't revert changes of optimisticData and show the previous receipt until we reload the page.

As such, I suggest we should go with @tienifr's proposal #28343 (comment)

🎀👀🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

Triggered auto assignment to @jasperhuangg, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@trjExpensify
Copy link
Contributor

Currently, if we fail to upload a receipt, although BE already returns an error, the App won't revert changes of optimisticData and show the previous receipt until we reload the page.

Hm, that sounds correct to me. We should show an error message immediately, but we shouldn't revert the receipt until they take action on the error message to dismiss it. For example..

  1. I'm offline on a plane, and decide to do my expenses on the ride home
  2. I upload a receipt for $100 to get paid back from my company, and I throw the receipt away
  3. We optimistically create the request with the receipt
  4. I go back online and the receipt upload fails for some reason
  5. I don't have the receipt anymore, so I download it, and try again

If we just reverted the receipt that failed to upload, a user could very well be out of pocket, so I think we need to be careful with that.

CC: @mountiny @iwiznia

@iwiznia
Copy link
Contributor

iwiznia commented Sep 29, 2023

Yeah I think I agree with that. We need to provide a way for you to get the thing you uploaded

@trjExpensify
Copy link
Contributor

Yeah, so I think that should apply with a failed receipt upload when replacing a receipt here? We display the error message, and when you dismiss it, it reverts back to the original.

@tranvantoan-qn
Copy link

So, does that mean we will now check the file format locally and show an error if it is not a supported file type, instead of sending it to the backend immediately?

@hoangzinh
Copy link
Contributor

Yeah, so I think that should apply with a failed receipt upload when replacing a receipt here? We display the error message, and when you dismiss it, it reverts back to the original.

@trjExpensify, it sounds great to me. Just wanna confirm the scope, do you want to cover above ^ in this GH? (fyi, to fix the current issue, I think we only need to define UNALLOWED_EXTENSION in API_ATTACHMENT_VALIDATIONS as Contributors are suggestings in their proposals).

@melvin-bot melvin-bot bot added the Overdue label Oct 2, 2023
@trjExpensify
Copy link
Contributor

I think we should fix it properly, yeah. Otherwise people will lose receipts that failed to be replaced.

@Gonals might have some insight on the approach for this from the Scan Receipt implementation, as we should be doing the same to give them the option to download it on a failed receipt upload.

@melvin-bot melvin-bot bot removed the Overdue label Oct 2, 2023
@iwiznia
Copy link
Contributor

iwiznia commented Oct 2, 2023

So, does that mean we will now check the file format locally and show an error if it is not a supported file type, instead of sending it to the backend immediately?

To clarify, no, that does not mean that. We might have local validation but this issue is not about that. It is about not losing the file when we sent the request to the server, but the server failed for whatever reason.

@jasperhuangg
Copy link
Contributor

So it sounds like the given proposal that was approved by C+ doesn't entirely fit our requirements. We don't want to lose the optimistic file in Onyx that was uploaded, in case the user wants to redownload the file and try again.

This is still open for proposals!

@hoangzinh
Copy link
Contributor

Cool. @trjExpensify could you help to update the Expected Result section of this issue to reflect what we're expecting to fix here ^? Thanks

@melvin-bot melvin-bot bot added the Overdue label Jan 4, 2024
@trjExpensify
Copy link
Contributor

@hoangzinh we should be unblocked here now! :)

@melvin-bot melvin-bot bot removed the Overdue label Jan 5, 2024
@hoangzinh
Copy link
Contributor

@tienifr let's tackle this issue again 🔥

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 8, 2024
@trjExpensify
Copy link
Contributor

@jasperhuangg over to you for the secondary review!

Copy link

melvin-bot bot commented Jan 31, 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.

@jasperhuangg
Copy link
Contributor

@tienifr let's move forward with your proposal!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 31, 2024
@melvin-bot melvin-bot bot changed the title [$500] IOU - Replacing Receipt with an Invalid File Format Does Not Display an Error Message [HOLD for payment 2024-02-07] [$500] IOU - Replacing Receipt with an Invalid File Format Does Not Display an Error Message Jan 31, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 31, 2024
Copy link

melvin-bot bot commented Jan 31, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jan 31, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.34-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-02-07. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jan 31, 2024

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:

  • [@hoangzinh] The PR that introduced the bug has been identified. Link to the PR:
  • [@hoangzinh] 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:
  • [@hoangzinh] 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:
  • [@hoangzinh] Determine if we should create a regression test for this bug.
  • [@hoangzinh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 7, 2024
@trjExpensify
Copy link
Contributor

👋 checklist time here!

@hoangzinh
Copy link
Contributor

BugZero Checklist:

  • The PR that introduced the bug has been identified. Link to the PR: the root cause is fixed so this PR is just a UI/UX improvement.
  • 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: N/A
  • 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: N/A
  • Determine if we should create a regression test for this bug. YES
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  1. Login to any account
  2. Send a Money Request to another account, using the Scan Receipt option
  3. Click on the Receipt image to open the preview popup
  4. Click on the “Three dots” icon >> select the “Replace” option
  5. Replace the current image with an unsupported file. Ex: [CORRUPT.pdf (https://github.com/Expensify/App/files/13582924/CORRUPT.pdf)
  6. Verify that the error message is shown and users can download the invalid file
  7. Click clear error icon
  8. Verify that the errors are removed, and the previous file is shown

Do we agree 👍 or 👎

@melvin-bot melvin-bot bot added the Overdue label Feb 12, 2024
Copy link

melvin-bot bot commented Feb 13, 2024

@hoangzinh, @trjExpensify, @jasperhuangg, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

@jasperhuangg
Copy link
Contributor

@hoangzinh those tests look good to me, thanks!

@melvin-bot melvin-bot bot removed the Overdue label Feb 13, 2024
@trjExpensify
Copy link
Contributor

Nice, thanks! Applause have a regression test for the failed upload side, so I've created an issue for the replace receipt side.

@trjExpensify
Copy link
Contributor

Summarising payouts as follows:

Settled up, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants