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

[On Hold #44778] Submit Expense - Inconsistent Error Messages for Password Protected PDFs on Mweb and Android #44091

Closed
2 of 6 tasks
lanitochka17 opened this issue Jun 20, 2024 · 29 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 20, 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: 1.4.86-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): abebemiherat@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

  1. On Android: Sign in with Expensify > FAB > Submit Expense
  2. Add an amount and attach a password-protected PDF
  3. On Mweb: Repeat the above steps
  4. Observe the error messages

Expected Result:

The error message should be consistent for the same password-protected PDF across both platforms

Actual Result:

Different error messages appear for password-protected PDFs on Mweb and Android

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

Add any screenshot/video evidence

Bug6519420_1718889532550.ju.mp4

9!1

19!2

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a71dc36f6e73ac8e
  • Upwork Job ID: 1806742272201940772
  • Last Price Increase: 2024-07-05
Issue OwnerCurrent Issue Owner: @s77rt
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 20, 2024
Copy link

melvin-bot bot commented Jun 20, 2024

Triggered auto assignment to @CortneyOfstad (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.

@lanitochka17
Copy link
Author

@CortneyOfstad FYI 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

@Krishna2323
Copy link
Contributor

Proposal

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

Submit Expense - Inconsistent Error Messages for Password Protected PDFs on Mweb and Android

What is the root cause of that problem?

The error title is wrong attachmentPicker.wrongFileType.

setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.protectedPDFNotSupported');

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

We should be using attachmentPicker.attachmentError.

What alternative solutions did you explore? (Optional)

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Jun 20, 2024

Proposal

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

Inconsistent Error Messages for Password Protected PDFs on Mweb and Android

What is the root cause of that problem?

The root cause is that we are handling errors concerning password protected pdf files for native apps in MoneyRequestConfirmationList. That is because password protected pdf error is not caught by the validation put in IOURequestStepScan for native devices. Hence 'Attachment error' message is displayed because of the default error key attachmentPicker.attachmentError put in MoneyRequestConfirmationList

<ConfirmModal
title={translate('attachmentPicker.attachmentError')}

For web, we handle password protected pdf in IOURequestStepScan.index

if (fileExtension === 'pdf') {
return isPdfFilePasswordProtected(file).then((isProtected: boolean) => {
if (isProtected) {
setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.protectedPDFNotSupported');

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

Change web error message key to attachmentPicker.attachmentError in IOURequestStepScan.index

if (fileExtension === 'pdf') {
return isPdfFilePasswordProtected(file).then((isProtected: boolean) => {
if (isProtected) {
setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.protectedPDFNotSupported');

This will change web error message to Attachment error

What alternative solutions did you explore? (Optional)

Change native platform error message to attachmentPicker.wrongFileType in MoneyRequestConfirmationList

<ConfirmModal
title={translate('attachmentPicker.attachmentError')}

This will change native platforms error message to Invalid file type

@cretadn22
Copy link
Contributor

Proposal

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

Error Messages for Password Protected PDFs on Android is incorrect

What is the root cause of that problem?

On web (only web), we implemented a check to identify Password Protected PDF files and show the correct error message

if (fileExtension === 'pdf') {
return isPdfFilePasswordProtected(file).then((isProtected: boolean) => {
if (isProtected) {
setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.protectedPDFNotSupported');
return false;

However, the isPdfFilePasswordProtected function only operates on the web platform (it is not defined for native). Therefore, if we upload a password-protected PDF file on the native platform, the Scan page will not generate an error. Instead, the error message occurs on the confirmation page on the native

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

Fortunately, we can determine whether the PDF file requires a password by utilizing the onPassword callback of the PDFThumbnail Component on the confirmation page

  1. Need to define new state
    const [invalidAttachmentTitle, setInvalidAttachmentTitle] = useState(translate('attachmentPicker.attachmentError'));
  1. Update this state on onPassword and onLoadError callback
setInvalidAttachmentTitle(translate('attachmentPicker.wrongFileType'));

setInvalidAttachmentTitle(translate('attachmentPicker.attachmentError'));
  1. Using new state
    title={translate('attachmentPicker.attachmentError')}
title={invalidAttachmentTitle}

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Jun 24, 2024
Copy link

melvin-bot bot commented Jun 25, 2024

@CortneyOfstad Huh... This is 4 days overdue. Who can take care of this?

@CortneyOfstad
Copy link
Contributor

Sorry was OoO!

I think this may be more BE (tied to design) so going to get confirmation!

@melvin-bot melvin-bot bot removed the Overdue label Jun 25, 2024
@CortneyOfstad
Copy link
Contributor

Asked here

@CortneyOfstad
Copy link
Contributor

Bumped the thread in design again 👍

@CortneyOfstad
Copy link
Contributor

Alright, confirmed with the Design Team that we want Invalid file type to be the main error across the board!

@CortneyOfstad CortneyOfstad added the External Added to denote the issue can be worked on by a contributor label Jun 28, 2024
@melvin-bot melvin-bot bot changed the title Submit Expense - Inconsistent Error Messages for Password Protected PDFs on Mweb and Android [$250] Submit Expense - Inconsistent Error Messages for Password Protected PDFs on Mweb and Android Jun 28, 2024
Copy link

melvin-bot bot commented Jun 28, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 28, 2024
Copy link

melvin-bot bot commented Jun 28, 2024

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

@s77rt
Copy link
Contributor

s77rt commented Jun 29, 2024

@Krishna2323 @etCoderDysto @cretadn22 Thank you for the proposals. The inconsistency we are having concerns both the error message being different as well as the timing of the verification phase (scan vs confirmation page). Can we add support to pdf verification on the scan step on native to be consist with web and then remove the one in the confirmation page?

@CortneyOfstad
Copy link
Contributor

@Krishna2323 @etCoderDysto @cretadn22 bumps on the above ^^^^ thank you!

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Jul 2, 2024

Hi, @s77rt . We can't use isPdfFilePasswordProtected for iOS and Android since it uses pdfjsLib - a web support only library - to check if a pdf has password. We can create isPdfFilePasswordProtected for native devices that makes use react-native-pdf library. We are already using react-native-pdf(PDF) in PDFView component to render password-protected pdf in attachment carousel in native devices.

What should we do to detect if a pdf is password-protected using react-native-pdf

  1. Generate a random string on every call to isPasswordProtedPDF and pass it to password prop of PDF component. This will trigger onError callback passing error object to it as the password we have passed is wrong.

<PDF
fitPolicy={0}
trustAllCerts={false}
renderActivityIndicator={() => <FullScreenLoadingIndicator style={loadingIndicatorStyles} />}
source={{uri: sourceURL, cache: true, expiration: 864000}}
style={pdfStyles}
onError={handleFailureToLoadPDF}
password={password}
onLoadComplete={finishPDFLoad}

  1. Check if the error is caused by a wrong password passed to a password-protected pdf on onError callback using the same logic used below

const handleFailureToLoadPDF = ((error: Error) => {
if (error.message.match(/password/i)) {

  1. Return true if the above condition holds to be true.

@melvin-bot melvin-bot bot added the Overdue label Jul 2, 2024
@CortneyOfstad
Copy link
Contributor

@s77rt any follow-up on the comment above?

@s77rt
Copy link
Contributor

s77rt commented Jul 3, 2024

@CortneyOfstad I'm on it

@melvin-bot melvin-bot bot removed the Overdue label Jul 3, 2024
@s77rt
Copy link
Contributor

s77rt commented Jul 3, 2024

@etCoderDysto The pdfjsLib does not support rendering on native but I would assume that reading just a buffer of bytes to tell if the file is password protected or not should work fine. Have you tried reading the document?

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Jul 3, 2024

@etCoderDysto The pdfjsLib does not support rendering on native but I would assume that reading just a buffer of bytes to tell if the file is password protected or not should work fine. Have you tried reading the document?

I haven't tried that. I will try it and get back to you.

Copy link

melvin-bot bot commented Jul 4, 2024

@CortneyOfstad @s77rt 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!

@s77rt
Copy link
Contributor

s77rt commented Jul 4, 2024

Still waiting on proposals

Copy link

melvin-bot bot commented Jul 5, 2024

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

@cretadn22
Copy link
Contributor

@s77rt, thank you for your suggestion. Currently, we're using react-native-pdf to detect if a PDF file requires a password in our native setup. However, this method requires rendering the PDF file using the PDF component from react-native-pdf. Upon further investigation into react-native-pdf, I found that it doesn't offer a direct API specifically for checking password protection.

We have two potential solutions:

  1. Integrate another library that handles PDF password detection.
  2. Extend react-native-pdf by implementing native code for both Android and iOS.

Personally, neither of these methods seems justified for addressing such a minor issue. My proposal seems like a workaround, but it effectively resolves the issue without making significant alterations

@s77rt
Copy link
Contributor

s77rt commented Jul 6, 2024

@cretadn22 Your proposal will only change the text error but still keep the inconsistency where on web the validation is done on the scan page while on native it's done on the confirmation page.

I would prefer doing one of the following:

  1. Make the pdfjs library work on native (only for checking if the pdf is password protected and not rendering)
  2. Extend react-native-pdf as you mentioned

@s77rt
Copy link
Contributor

s77rt commented Jul 6, 2024

@cretadn22

Upon further investigation into react-native-pdf, I found that it doesn't offer a direct API specifically for checking password protection.

There is an exported PdfManager that we can use to load the file without rendering.

@Krishna2323
Copy link
Contributor

@s77rt, sorry for delay on this, we are already handling PDF validation here. I think we can close this out.

@s77rt
Copy link
Contributor

s77rt commented Jul 7, 2024

@CortneyOfstad Let hold this on #44778

@CortneyOfstad
Copy link
Contributor

Sounds good — thanks @s77rt!

@melvin-bot melvin-bot bot added the Overdue label Jul 9, 2024
@CortneyOfstad CortneyOfstad changed the title [$250] Submit Expense - Inconsistent Error Messages for Password Protected PDFs on Mweb and Android [On Hold #44778] Submit Expense - Inconsistent Error Messages for Password Protected PDFs on Mweb and Android Jul 9, 2024
@CortneyOfstad CortneyOfstad added Weekly KSv2 and removed Daily KSv2 labels Jul 9, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jul 9, 2024
@CortneyOfstad
Copy link
Contributor

Deployed to production! Going to close 🎉

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. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

6 participants