Open izarutskaya opened 3 weeks ago
melvin-bot[bot]
commented
3 weeks ago Triggered auto assignment to @VictoriaExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
izarutskaya
commented
3 weeks ago We think this issue might be related to the #collect project.
Krishna2323
commented
3 weeks ago Scan - No error pop-up and scan expense with corrupted PDF can be submitted via QAB
The pdf error is detected when it is rendered on the confirmation page but in case of QAB, the confirmation page is skipped and we don't get the error alert. This also happens with for protected pdf's. https://github.com/Expensify/App/blob/bce61c07a103a7d65f3c5f7cde88a133034e5fbb/src/components/MoneyRequestConfirmationList.tsx#L1115-L1128
If we want to validate the pdf on scan page then we would need to make few changes.
const [pdfFile, setPdfFile] = useState<null | FileObject>(null);setReceiptAndNavigate function, first we should accept a new parameter to check if the pdf is already validated, then check if the file is a pdf file or not and if so then call setPdfFile(file); to set the file and return from setReceiptAndNavigate. const setReceiptAndNavigate = (file: FileObject, isPdfValidated?: boolean) => {
validateReceipt(file).then((isFileValid) => {
if (!isFileValid) {
return;
}
if (Str.isPDF(file.name ?? '') && !isPdfValidated) {
setPdfFile(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);
if (action === CONST.IOU.ACTION.EDIT) {
updateScanAndNavigate(file, source);
return;
}
navigateToConfirmationStep(file, source);
});
};Update PDFThumbnail to accept a new prop onLoadSuccess and pass it to onLoadSuccess prop of Document.
https://github.com/Expensify/App/blob/1f7ebfcafb7fc97b63d4e5baee16d204bdcb5d8f/src/components/PDFThumbnail/index.tsx#L21
Render the pdf component without displaying it and trigger the alert modals when callbacks triggers.
{pdfFile && (
<View style={{position: 'absolute', opacity: 0}}>
<PDFThumbnail
previewSourceURL={pdfFile.uri ?? ''}
onPassword={() => {
setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment');
}}
onLoadSuccess={() => {
setPdfFile(null);
setReceiptAndNavigate(pdfFile, true);
}}
onLoadError={() => {
setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment');
}}
/>
</View>
)}Note: There are some differences between PDFThumbnail component for native and web to there will be minor changes required accordingly.
In this PR, we added the functionality to detect the corrupted pdf files on confirmation page, so if we go with the new solution then we would need to clean up that component as well.
https://github.com/Expensify/App/assets/85894871/d2a5affc-cb14-4a06-b360-14218fc9e8b0
VictoriaExpensify
commented
3 weeks ago Agree this inconsistency should be fixed
melvin-bot[bot]
commented
3 weeks ago Job added to Upwork: https://www.upwork.com/jobs/~013830a17b8e9e0b20
melvin-bot[bot]
commented
3 weeks ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox (External)
suneox
commented
3 weeks ago @Krishna2323 proposal looks good to me. Solution summary: We will validate the file after it is selected instead of validate on another place
🎀 👀 🎀 C+ reviewed
melvin-bot[bot]
commented
3 weeks ago Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
truph01
commented
3 weeks ago There is inconsistency in submitting corrupted PDF via FAB and QAB. App shows error pop-up and the expense is not submitted when submitting via FAB. App does not show error pop-up and the scan expense is submitted when submitting via QAB.
When creating expense by FAB, after uploading the file, we render the file via this logic: https://github.com/Expensify/App/blob/43cd0f7bbb55b455a57f1996a3a11f66573c58d2/src/components/MoneyRequestConfirmationList.tsx#L1115-L1127 in the confirmation page, so there is an error popup appears in this case.
But when creating expense by QAB, after uploading file, we skip confirmation page, that leads to there is no way to check whether the file is corrupted pdf file.
We need to validate the corrupted PDF file right after user picks the file, rather than rendering it then need to wait until the onLoadError triggers. To do it, we can create a function isCorruptedPdfFile (can be used in both web and native):
import { PDFDocument } from 'pdf-lib';async function isCorruptedPdfFile(file) { try { const arrayBuffer = await file.arrayBuffer(); await PDFDocument.load(arrayBuffer); return false; // PDF is not corrupted } catch (error) { if (error.message.includes('password')) { return false; } return true; } }
export default isCorruptedPdfFile;
then use it in this logic:
https://github.com/Expensify/App/blob/43cd0f7bbb55b455a57f1996a3a11f66573c58d2/src/pages/iou/request/step/IOURequestStepScan/index.tsx#L192if (fileExtension === 'pdf') { return isCorruptedPdfFile(file) .then((isCorruptedPdf: boolean) => { if (isCorruptedPdf) { setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment' ); return false; } return true; }); }
and
https://github.com/Expensify/App/blob/43cd0f7bbb55b455a57f1996a3a11f66573c58d2/src/pages/iou/request/step/IOURequestStepScan/index.native.tsx#L164if (fileExtension === 'pdf') { return isCorruptedPdfFile(file) .then((isCorruptedPdf: boolean) => { if (isCorruptedPdf) { Alert.alert('attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingCorruptedAttachment'); return false; } return true; }); }
- If we want to handle password file, we can create ```isPasswordFile```:import { PDFDocument } from 'pdf-lib';
async function isPasswordFile(file) { try { const arrayBuffer = await file.arrayBuffer(); await PDFDocument.load(arrayBuffer); return false; // PDF is not password protected } catch (error) { if (error.message.includes('password')) { return true; // PDF is password protected } return false; } }
export default isPasswordFile;
- And use it like we use ```isCorruptedPdfFile```
### What alternative solutions did you explore? (Optional)
<!---
ATTN: Contributor+
You are the first line of defense in making sure every proposal has a clear and easily understood problem with a "root cause". Do not approve any proposals that lack a satisfying explanation to the first two prompts. It is CRITICALLY important that we understand the root cause at a minimum even if the solution doesn't directly address it. When we avoid this step we can end up solving the wrong problems entirely or just writing hacks and workarounds.
Instructions for how to review a proposal:
1. Address each contributor proposal one at a time and address each part of the question one at a time e.g. if a solution looks acceptable, but the stated problem is not clear then you should provide feedback and make suggestions to improve each prompt before moving on to the next. Avoid responding to all sections of a proposal at once. Move from one question to the next each time asking the contributor to "Please update your original proposal and tag me again when it's ready for review".
2. Limit excessive conversation and moderate issues to keep them on track. If someone is doing any of the following things please kindly and humbly course-correct them:
- Posting PRs.
- Posting large multi-line diffs (this is basically a PR).
- Skipping any of the required questions.
- Not using the proposal template at all.
- Suggesting that an existing issue is related to the current issue before a problem or root cause has been established.
- Excessively wordy explanations.
3. Choose the first proposal that has a reasonable answer to all the required questions.
-->@suneox Can you help check my proposal above? TY
suneox
commented
3 weeks ago @suneox Can you help check my proposal above? TY
We have another proposal: it also validates files after they are selected without rendering. However, I have some concerns about this approach.
PDFThumbnail on native devices uses native code provided by react-native-pdf@truph01 Could you please clarify some point above? And thank you for your proposal 🌟
truph01
commented
3 weeks ago @suneox Thanks for your feedback.
Can it handle files with passwords and extract error types? Due to scanning receipts also does not support PDF files with passwords.
suneox
commented
3 weeks ago
- I wonder that why do we need to handle password pdf case? I think it is not a scope of this issue
@truph01 We have to make it consistent at this comment
So the scope in this issue also handle password case, due to at confirmation page also validate pdf password.
truph01
commented
3 weeks ago @suneox I think that the "inconsistency" mentioned in this https://github.com/Expensify/App/issues/43645#issuecomment-2167010794 is about:
There is inconsistency in submitting corrupted PDF via FAB and QAB. App shows error pop-up and the expense is not submitted when submitting via FAB. App does not show error pop-up and the scan expense is submitted when submitting via QAB.
suneox
commented
3 weeks ago @suneox I think that the "inconsistency" mentioned in this #43645 (comment) is about:
I believe the scope of this issue should prevent uploading a PDF file with a password. This is because submit expense also prevents such files. When editing, we should also prevent uploading PDF files with passwords. If we don’t do this, the backend won’t be able to scan PDFs with passwords.
@VictoriaExpensify I'd like to confirm that the scope of this issue involves handling corrupted files and files has password
melvin-bot[bot]
commented
3 weeks ago @suneox, @grgia, @VictoriaExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
grgia
commented
2 weeks ago I believe the scope of this issue should prevent uploading a PDF file with a password. This is because submit expense also prevents such files. When editing, we should also prevent uploading PDF files with passwords. If we don’t do this, the backend won’t be able to scan PDFs with passwords.
@suneox I agree that we should make all pdf error cases consistent as part of this issue. Does that answer your question?
truph01
commented
2 weeks ago @grgia There are two types of pdf files we should not allow user to upload: Protected pdf and corrupt pdf. I think the @suneox 's question is:
grgia
commented
2 weeks ago @truph01 we should cover both cases
suneox
commented
2 weeks ago Does that answer your question?
@grgia Yes, that is my question. We have to handle both cases
suneox
commented
2 weeks ago @Krishna2323 Can you update your proposal without rendering PDFThumbnail as an absolute view?
Krishna2323
commented
2 weeks ago @suneox, the pdf's can be detected on web but for native there is no straight forward solution. Invisible rendering seems the easiest and most trustable solution thats why we are validating like that till now.
suneox
commented
2 weeks ago @suneox, the pdf's can be detected on web but for native there is no straight forward solution. Invisible rendering seems the easiest and most trustable solution thats why we are validating like that till now.
@Krishna2323 could you please update your test branch to handle for native? Thanks!
truph01
commented
2 weeks ago @suneox I updated the solution to add the implementing in native platform.
truph01
commented
2 weeks ago @suneox @grgia What do you think about using pdf-lib. Here are a few advantages:
suneox
commented
2 weeks ago @truph01 Thanks for the update.
@suneox I updated the solution to implement it on the native platform.
Based on your updated proposal, I could not find how your proposal extracts an error type since we have to show the error type to the user, and the implementation on the native platform is not detailed.
@suneox @grgia What do you think about using pdf-lib? Here are a few advantages:
I think there is no reason to add a new library to handle this issue because:
truph01
commented
2 weeks ago @suneox
I could not find how your proposal extracts an error type
isCorruptedPdfFile to check if the file is corrupted pdf or not. If true, call:
setUploadReceiptError(
true,
'attachmentPicker.attachmentError',
'attachmentPicker.errorWhileSelectingCorruptedAttachment'
);that will display the errror popup.
suneox
commented
2 weeks ago On web, I create a function isCorruptedPdfFile to check if the file is corrupted pdf or not. If true, call:
@truph01 The function isCorruptedPdfFile doesn't extract password error type.
suneox
commented
2 weeks ago @truph01 we should cover both cases
@truph01 Could you please review the comment carefully? Thank you!
truph01
commented
2 weeks ago @suneox Thanks for your feedback. I updated proposal to include the password pdf case.
truph01
commented
2 weeks ago Updated proposal to remove the platform-specific logic because the lib pdf-lib can be used in all platform.
Krishna2323
commented
2 weeks ago @suneox, I will update branch for native in few hours.
huult
commented
2 weeks ago Validate PDF file before uploading it to the server.
After select file from AttachmentPicker the current action is edit then we dismiss modal, but the validation file show at step ConfirmationStep
We will handle the validation pdf file and show error message after validateReceipt file. by adding function checkPDFState into src/libs with code bellow for browser and native platform
// checkPDFState/types.ts
type PDFState = {
canOpen: boolean;
error?: Error;
errorMessage?: 'password' | 'unknown';
}
// checkPDFState/index.ts
import * as pdfjs from 'pdfjs-dist';
import type PDFState from './types';
const checkPDFState = async (source: { url: string }): Promise<PDFState> => {
try {
await pdfjs.getDocument(source).promise;
console.log(`___________ Success ___________`);
return {canOpen: true};
} catch (e: unknown) {
const error = e as Error;
console.log(`___________ Error ___________`, error);
if (error?.message?.match(/password/i)) {
return {canOpen: false, errorMessage: 'password', error};
}
return {canOpen: false, errorMessage: 'unknown', error};
}
}
export default checkPDFState;
// checkPDFState/index.native.ts
import PdfManager from 'react-native-pdf/PdfManager';
import type PDFState from './types';
const checkPDFState = async (source: { url: string }): Promise<PDFState> => {
const path = unescape(source.url.replace(/file:\/\//i, ''));
try {
await PdfManager.loadFile(path);
console.log(`___________ Success ___________`);
return { canOpen: true };
} catch (e: unknown) {
const error = e as Error;
console.log(`___________ Error ___________`, error);
if (error?.message?.match(/password/i)) {
return { canOpen: false, errorMessage: 'password', error };
}
return { canOpen: false, errorMessage: 'unknown', error };
}
}
Then apply the function checkPDFState after validateReceipt
IOURequestStepScan/index.tsx
const setReceiptAndNavigate = (file: FileObject) => {
validateReceipt(file).then(async (isFileValid) => {
if (!isFileValid) {
return;
}
// Store the receipt on the transaction object in Onyx
const source = URL.createObjectURL(file as Blob);
+ if(isPDF(file)) {
+ const {errorMessage} = await checkPDFState({url: source});
+ if(errorMessage) {
+ if(errorMessage === 'password') {
+ setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.protectedPDFNotSupported');
+ return;
+ }
+ setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingAttachment');
+ }
+ }
....IOURequestStepScan/index.native.tsx
const setReceiptAndNavigate = async (file: FileObject) => {
if (!validateReceipt(file)) {
return;
}
+ if(isPDF(file)) {
+ const { errorMessage } = await checkPDFState({ url: file?.uri });
+ if (errorMessage) {
+ if (errorMessage === 'password') {
+ Alert.alert(translate('attachmentPicker.attachmentError'), translate('attachmentPicker.protectedPDFNotSupported'));
+ return;
+ }
+ Alert.alert(translate('attachmentPicker.attachmentError'), translate('attachmentPicker.errorWhileSelectingAttachment'));
+ }
+ }
Thank @huult proposal I will review the new proposal and other proposals that have been updated today.
suneox
commented
2 weeks ago @truph01 I don't think we need to add a new library. You can check out this comment for the reason.
I will update the branch for native in a few hours.
@Krishna2323 I'm still waiting your update the test branch.
@huult Your proposal is a potential solution that only use the existing library, but in your condition only return for a password case, is it correct? Could you also please share your test branch? Thanks.
+ if(errorMessage) {
+ if(errorMessage === 'password') {
+ setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.protectedPDFNotSupported');
- return;
+ }
+ setUploadReceiptError(true, 'attachmentPicker.attachmentError', 'attachmentPicker.errorWhileSelectingAttachment');
+ }@suneox, test branch updated.
huult
commented
2 weeks ago Hi @suneox , Firstly, I would like to thank you for reviewing my proposal.
@huult Your proposal is a potential solution that only use the existing library, but in your condition only return for a password case, is it correct? Could you also please share your test branch? Thanks.
@suneo You're right. I have updated the condition, and here it is test branch
suneox
commented
2 weeks ago @suneox, test branch updated.
@Krishna2323 your provided branch doesn't work on android
https://github.com/Expensify/App/assets/11959869/79133ab3-71f5-4049-9b2a-ef3433276025
You're right. I have updated the condition, and here it is test branch
@huult your provided branch only works on web
https://github.com/Expensify/App/assets/11959869/b8c3ead7-e53c-49bc-9f78-94fcb59d534b
Krishna2323
commented
2 weeks ago @suneox, i will check and update in few hours, maybe there is some minor issue because the error detection works on confirmation page on android.
suneox
commented
2 weeks ago @Krishna2323 @huult Have you got a plan to update your proposal handle issue on android native?
huult
commented
2 weeks ago @suneox I don't have plans to make updates for Android native, as the Android native platform does not support exporting functions to load PDF files.
Krishna2323
commented
2 weeks ago @suneox, test branch updated to fix the issue on android.
suneox
commented
2 weeks ago melvin-bot[bot]
commented
1 week ago @suneox @grgia @VictoriaExpensify 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]
commented
1 week ago @suneox, @grgia, @VictoriaExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
melvin-bot[bot]
commented
1 week ago 📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
melvin-bot[bot]
commented
1 week ago 📣 @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 📖
grgia
commented
1 week ago Thank you @suneox, all yours @Krishna2323 !
Krishna2323
commented
1 week ago Will raise PR within 2 days.
suneox
commented
6 days ago @Krishna2323 still working on PR
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.82-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4622999 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team
Action Performed:
Expected Result:
App will show error pop-up and the scan expense is not submitted.
Actual Result:
There is inconsistency in submitting corrupted PDF via FAB and QAB. App shows error pop-up and the expense is not submitted when submitting via FAB. App does not show error pop-up and the scan expense is submitted when submitting via QAB.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
https://github.com/Expensify/App/assets/115492554/ba369c96-6013-4809-8851-a61680c25490
LARGE PDF FILE corrupted.pdf
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @suneox