-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[LOW] [Splits] [$500] Mweb/Chrome - Scan request created using corrupt pdf displays incorrect error message #34032
Comments
Job added to Upwork: https://www.upwork.com/jobs/~019826bd315885912e |
Triggered auto assignment to @sonialiap ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan ( |
I can't replicate this on my android mobile (I get the error post upload as expected). Could you please share the file? I believe the receipt is validated here. App/src/pages/iou/ReceiptSelector/index.js Lines 102 to 120 in 393f2be
It relies entirely on the metadata, without checking content. So differences in how the mobile operating system interprets the metadata could lead to these corrupted PDFs getting through.. |
ProposalPlease re-state the problem that we are trying to solve in this issue:Corrupt pdfs can be uploaded as a receipt - and the error message is generic. What is the root cause of that problem?There's no specific pdf checking logic to determine whether the file is corrupted. Currently, we only check for size parameters and required extensions: We check it here: App/src/pages/iou/ReceiptSelector/index.js Lines 102 to 120 in fa533ce
With those parameters: Lines 51 to 60 in fa533ce
What changes do you think we should make in order to solve the problem?Either install front-end checks (render the pdf using 'react-pdf' and 'react-native-pdf'), to check for file corruption. Or adjust the |
ProposalPlease re-state the problem that we are trying to solve in this issue.Mweb/Chrome - Scan request created using corrupt pdf displays incorrect error message What is the root cause of that problem?We are not checking if the PDF file is corrupted/valid file before uploading in the next components: What changes do you think we should make in order to solve the problem?We should validate the PDF before uploading the file in the previously mentioned files and show an error message if PDF is corrupted. Based on our current PDF libraries: App/src/components/PDFView/index.js Line 6 in 0c351a4
Create a function to check the integrity of the file:
Then inside of validateReceipt functions:
Also, we should add this last strings to languages files like: https://github.com/Expensify/App/blob/main/src/languages/en.ts
This could be the result: Valid.PDF.Files.in.Scan.-.test.mp4 |
ProposalPlease re-state the problem that we are trying to solve in this issue.Request Money -Scan - Scan report allows protected PDF upload What is the root cause of that problem?We're not checking the PDF is weather the pdf is valid or not before uploadin it, and when BE scans the pdf for further processing it throws error. What changes do you think we should make in order to solve the problem?We can add a validation on FE to verify if the pdf file is valid or not using the const isPdf = pdfjs.isPdfFile(file.name);
if (isPdf) {
pdfjs
.getDocument(fileSource)
.promise.then(onSuccess)
.catch(onError);
} else {
onSuccess();
} ☝️ here onSucess is the code in below LOCs, and we need to create onError which should call App/src/pages/iou/request/step/IOURequestStepScan/index.js Lines 121 to 144 in a4bdebe
For Native:For native devices, We will try to render the PDF with zero visibilty using PDF component from // fires when when file is picked by user
const setReceiptAndNavigate = (file) => {
if (!validateReceipt(file)) {
return;
}
const {fileExtension} = FileUtils.splitExtensionFromFileName(lodashGet(file, 'name', ''));
if (fileExtension.toLowerCase() === 'pdf') {
setPdfFile(file);
return;
}
onLoadComplete(file);
};
// fires when when pdf is rendered successfully by pdf
const onLoadComplete = (file) => {
// Store the receipt on the transaction object in Onyx
IOU.setMoneyRequestReceipt(transactionID, file.uri, file.name, action !== CONST.IOU.ACTION.EDIT);
if (action === CONST.IOU.ACTION.EDIT) {
updateScanAndNavigate(file, file.uri);
return;
}
navigateToConfirmationStep();
setPdfFile(null);
};
// this renders with hidden visibilty
{pdfFile && (
<Pdf
source={{uri: pdfFile.uri, cache: true}}
onLoadComplete={onLoadComplete}
onError={() => {
Alert.alert(translate('receipt.pdfCorrupted'), translate('receipt.pdfCorruptedDescription'));
setPdfFile(null);
}}
style={styles.invisible}
/>
)} |
@situchan what do you think of the above proposals? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
proposals in review |
@situchan any update on your review? :) |
ProposalPlease re-state the problem that we are trying to solve in this issue.When in money request -> scan, and uploading a pdf file which is corrupted we are allowing user to move through whole flow till we upload file on the backend. We would like to show error as soon as possible to not waste time of users. What is the root cause of that problem?There is no logic implemented in the code for checking if pdfs are corrupted. What changes do you think we should make in order to solve the problem?To accomplish this on the web and desktop we can use library called pdfjs which a web-standard for working with pdfs.
On the native side things are trickier because there is no standard with working with pdfs. After research I couldn't find and solution in react-native world so I decided to go with creating native modules. For android code can look like this
For iOS we don't need any external library we can create function like that
And in our codebase use it here App/src/pages/iou/request/step/IOURequestStepScan/index.native.js Lines 166 to 183 in ce06355
the code would look like that
What alternative solutions did you explore? (Optional)We can change a little bit a UI for the first step in the flow of adding scan while requesting a money and add preview of the document/image and use PDFView component and then if we cant display the preview of the pdf/image we would know that there is something wrong with the file |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@kubabutkiewicz 's proposal LGTM! The native code is really easy to read and understand, so I don't mind us creating a native module to check for valid PDFs |
🎀👀🎀 |
Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@kubabutkiewicz I see that your proposal suggests using a lib for web and building our own code for ios/android. I'm pretty sure that if we go ahead with the new lib request, people are gonna veto it in favor of writing our own for web too. Could you explore writing our own method for web as well? |
@luacmartins actually this lib is already added to our project , so no need to add new one 😄 |
Ah interesting. In that case the proposal looks good then. Let's go ahead with the PR. |
@kubabutkiewicz do you have an ETA for the PR? |
Yes, I think tomorrow will be ready, today was reviewed internally, I will make adjustments and when approved internally I will open to external review |
Hi I noticed on latest main that there is a bug that even when I am uploading pdf which is not corrupted I am getting error that file is corrupted and I think its related to this #40162 PR because of that I cant test newest changes |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
thank you for finding the cause. let's hold on #40471 (review) |
@rushatgabhane you shouldn't hold for #40471 – the mentioned issue was fixed in #40351, which is already merged. @kubabutkiewicz please retry with the latest main. |
Hi, just wanted to inform that I will be OOO from 29.04 to 05.05. |
Issue not reproducible during KI retests. (First week) |
Seems like #40471 (review) may have fixed the issue. Gonna close it since it's no longer reproducible. Please reopen if this is still happening. |
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: 1.4.22
Reproducible in staging?: Y
Reproducible in production?: Y
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Issue found when executing PR #32562
Action Performed:
Expected Result:
Scan request created using corrupt pdf must display correct error message
Actual Result:
Scan request created using corrupt pdf displays incorrect error message " unexpected error while making request"
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6333288_1704467997273.income.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: