[HOLD for payment 2024-03-14] [$500] [MEDIUM] Smartscan: Show PDF receipt thumbnails

trjExpensify commented 10 months ago

Coming from here. CC: @sobitneupane @pradeepmdk @Gonals

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.3.98-5 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: @trjExpensify Slack conversation: https://github.com/Expensify/App/pull/30747#issuecomment-1802321587

Action Performed:

Request Money > Scan > Upload a PDF receipt > choose a participant
Observe there isn't a preview of the PDF receipt in the thumbnail on the confirmation page
Click confirm to create the request
Observe there isn't a preview of the PDF receipt in the thumbnail on the report preview component in the chat
Click the report preview component
Observe there isn't a preview of the PDF receipt in the thumbnail on the request preview component in the expense/iouReport
Click the request preview component
Observe there isn't a preview of the PDF receipt in the thumbnail on the receipt preview component in the request view

Expected Result:

PDF receipts should show a preview in the relevant thumbnails throughout the app.

P.s this also includes in the split preview component, so let's make sure it works there.

Actual Result:

Generic "empty" thumbnail previews are displayed for PDF receipts.

Workaround:

Yes, they can tap the thumbnail to see the PDF but they'd really not know to do that.

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

Confirmation screen:

Report preview on the chat:

Request preview on the expense/iouReport:

Receipt preview on the request:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01f90fbd2f6a7c49f2
Upwork Job ID: 1725113274629943296
Last Price Increase: 2023-11-23

melvin-bot[bot] commented 8 months ago

Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.

dylanexpensify commented 8 months ago

@eh2077 mind giving an ETA on when you'll have the PR raised?

eh2077 commented 8 months ago

@dylanexpensify PR will be ready today

eh2077 commented 8 months ago

Hi team,

As C+ @s77rt pointed out during PR reviewing, there're two issues need to clarify the expected behaviour

Issue 1: Uploading a password protected pdf results in a non-ending prompt unless you type the right pasword

See videos in https://github.com/Expensify/App/pull/35255#issuecomment-1913714726

See discussion https://github.com/Expensify/App/issues/31432#issuecomment-1904201073 and https://github.com/Expensify/App/issues/31432#issuecomment-1905771607

Issue 2: The local preview do not match the one returned by the server

See videos in https://github.com/Expensify/App/pull/35255#issuecomment-1913713879

I feel it's hard to have the resolution of local PDF thumbnail same as the one returned by backend. See https://github.com/Expensify/App/issues/31432#issuecomment-1839330922 about the backend implementation.

cc @trjExpensify @luacmartins

eh2077 commented 8 months ago

@trjExpensify Maybe you can try to refresh the page using Chrome if videos are not loading at the first time.

trjExpensify commented 8 months ago

@eh2077 what clarification questions do you have with regard to the expected behaviour?

eh2077 commented 8 months ago

@trjExpensify For the case of uploading protected PDF, the expected behaviour is to show an alert modal right? As you mentioned earlier

Got it. If we don't allow password protected PDFs for receipt upload, we should be preventing that with the "file type not supported" alert modal. Remind me, do they have a special extension?

eh2077 commented 8 months ago

For the second issue which is about the different resolution between local thumbnail and the one returned by backend. Is it acceptable to have the differences?

trjExpensify commented 8 months ago

@trjExpensify For the case of uploading protected PDF, the expected behaviour is to show an alert modal right? As you mentioned earlier

Yeah, I was asking as I would have thought we could throw the file type error. Uploading a password protected PDF for receipt transcription is pointless, as we can't read any information from it, so I figured we might as well just prevent it.

trjExpensify commented 8 months ago

For the second issue which is about the different resolution between local thumbnail and the one returned by backend. Is it acceptable to have the differences?

I'll defer to @luacmartins on this one. Ideally they are the same if we can do it.

luacmartins commented 8 months ago

@eh2077 why is it challenging to match the resolution?

eh2077 commented 7 months ago

@eh2077 why is it challenging to match the resolution?

@luacmartins Please find this https://github.com/Expensify/App/pull/35255#issuecomment-1929872013 FYI.

youssef-lr commented 7 months ago

Question, we already display previews when uploading a PDF attachment as a comment, why can't we use the same logic to display the PDF receipt's thumbnail?

s77rt commented 7 months ago

@youssef-lr We will be using that same logic however it only works while online since we need to wait for the server's response to get the pdf url (and thumbnail). If we create a scan request offline we need to mimic that behaviour which so far works well but it's not perfect e.g. the thumbnail image is not 100% the same.

eh2077 commented 7 months ago

Hi team, here's the update of PR:

Improving error handling of uploading a protected PDF, see demo in https://github.com/Expensify/App/pull/35255#issuecomment-1944087612

cc @trjExpensify

trjExpensify commented 7 months ago

Commenting in the PR. :)

eh2077 commented 7 months ago

Hi @trjExpensify , can you help to add Waiting for Copy label to help us confirm the new translation of following error message? I was suggested to do so from here.

English: Password protected PDF receipt is not supported.
Spanish: No se admite el recibo en PDF protegido con contraseña.

See the new feature demo below

dylanexpensify commented 7 months ago

done!

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @CherylWalsh (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

trjExpensify commented 7 months ago

@CherylWalsh I'll ultimately defer to you, but in English for the "generic" file type not allowed we have this message:

This is a special case in which a PDF file type is allowed, so it passes our front-end checks. At which point, we figure out it's a password protected PDF and throw the error. We can't SmartScan a PDF we can't access. So I think something like this would be fine and in-keeping with what we have:

Attachment is the wrong type Password protected PDF is not allowed

trjExpensify commented 7 months ago

CC: @iwiznia as I saw you raised this in the thread. Pending the above nod from Cheryl, I think the only portion we need a Spanish translation for then is "Password protected PDF", we already have the rest from the existing file type front-end validation error message.

eh2077 commented 7 months ago

@CherylWalsh Friendly bump https://github.com/Expensify/App/issues/31432#issuecomment-1960648979

CherylWalsh commented 7 months ago

I feel the error message is too aggressive, I don't like "not allowed". It might be standard but I suggest this:

Invalid attachment type. Please avoid password-protected PDFs

dylanexpensify commented 7 months ago

Ooo I like that copy for the error message, Cheryl!

On Tue, Feb 27, 2024 at 10:43 AM Cheryl W @.***> wrote:

I feel the error message is too aggressive, I don't like "not allowed". It might be standard but I suggest this:

Invalid attachment type. Please avoid password-protected PDFs

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/31432#issuecomment-1966268954, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOKVCAEL47HOYYCYRJXIOJ3YVW2EZAVCNFSM6AAAAAA7N7WMW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWGI3DQOJVGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

trjExpensify commented 7 months ago

Can we suggest in the format of the alert modal?

bold title Second copy line

If we're going to make the password-protected PDF one read differently, we should update the front-end validation error for unsupported file types as well so they're consistent. Based on the suggestion that would be:

(PP-PDF)

Invalid attachment type Please avoid password-protected PDFs

AND

(Generic)

Invalid attachment type Please avoid this file type

Yeah, not sure if those are better tbh. 🤔

"File type" is the correct term, I don't think "attachment type" is a term used anywhere commonly to refer to the extension of a file.
Some alternatives to "not allowed" commonly used might be "not permitted" or "not supported", perhaps? It's a pretty transactional error message at the end of the day. Maybe these below?

(PP-PDF specific)

Invalid file type Password protected PDF is not supported

AND

(Generic)

Invalid file type This file type is not supported

CherylWalsh commented 7 months ago

Yes, love this. If not attachment type then file type works. And I like 'not supported' instead of allowed. I know it's only a transactional error message but it does matter.

(PP-PDF specific)

Invalid file type Password-protected PDF is not supported (it's hyphenated)

AND

(Generic)

Invalid file type This file type is not supported

trjExpensify commented 7 months ago

Perfect. So next steps:

@eh2077 can you make sure to include updating the generic file type error message in your PR, so we keep things consistent.
@iwiznia can we get Spanish translations from you for the below, please?

Invalid file type Password-protected PDF is not supported

AND

Invalid file type This file type is not supported

iwiznia commented 7 months ago

This file type is not supported we kind of already have this: https://github.com/Expensify/App/blob/b336cd3b4c258d23071fb47305bc279c7adac477/src/languages/en.ts#L337 and this https://github.com/Expensify/App/blob/b336cd3b4c258d23071fb47305bc279c7adac477/src/languages/en.ts#L445, should we reuse and unify those?

Invalid file type: Tipo de archivo inválido Password-protected PDF is not supported: Los PDFs con password no son compatibles

trjExpensify commented 7 months ago

This file type is not supported we kind of already have this:

Yeah, the idea is to replace This file type is not allowed with This file type is not supported as part of this issue now to align them.

iwiznia commented 7 months ago

ok you already have the text and translation for that

trjExpensify commented 7 months ago

So for the avoidance of doubt and @eh2077 benefit, the below are the final set.

(PP-PDF specific)

Invalid file type Password-protected PDF is not supported

Tipo de archivo inválido Los PDFs con password no son compatibles

(Generic)

Invalid file type This file type is not allowed

Tipo de archivo inválido Este tipo de archivo no es compatible

iwiznia commented 7 months ago

This is wrong:

Este tipo de archivo no son compatibles

Should be:

Este tipo de archivo no es compatible

trjExpensify commented 7 months ago

Lol, got it. Updated.

melvin-bot[bot] commented 6 months ago

@trjExpensify, @s77rt, @luacmartins, @CherylWalsh, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick!

s77rt commented 6 months ago

PR has been deployed to staging 1 hour ago

melvin-bot[bot] commented 6 months ago

⚠️ 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.

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

https://github.com/Expensify/App/pull/35255

If no regressions arise, payment will be issued on 2024-03-14. :confetti_ball:

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

@s77rt requires payment (Needs manual offer from BZ)
@eh2077 requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 6 months ago

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:

[ ] [@s77rt / @eh2077] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@s77rt / @eh2077] 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:
[ ] [@s77rt / @eh2077] 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:
[ ] [@s77rt / @eh2077] Determine if we should create a regression test for this bug.
[ ] [@s77rt / @eh2077] 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:

s77rt commented 6 months ago

The PR that introduced the bug has been identified: Not a a bug. This is a feature request
The offending PR has been commented on: n/a
A discussion in #expensify-bugs has been started: n/a
Determine if we should create a regression test for this bug: Yes

Regression Test Proposal

Go to any chat and request money
Choose Scan tab and upload a pdf file
Verify that you're able to preview the PDF receipt in the confirmation page
Complete the money request process
Verify that you're able to preview the PDF receipt in the chat report
Click on the PDF preview
Verify that you're able to preview the PDF receipt in the IOU report
Click on the PDF preview
Verify that you're able to preview the PDF receipt in the money request report
Click on the PDF preview
Verify that you are displayed the PDF file

eh2077 commented 6 months ago

Hi @trjExpensify

Considering the added time and complexity of completing this task, could we discuss the possibility of a bonus for me and C+ @s77rt ?

We have completed several tasks in this new feature, including displaying local PDFs, detecting password-protected PDFs, and refining translations.

It would be great to have it but it’s also fine if not :)

melvin-bot[bot] commented 6 months ago

Payment Summary

Upwork Job

C+: @s77rt paid $500 via Upwork
Contributor: @eh2077 paid $500 via Upwork

BugZero Checklist (@trjExpensify)

[x] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[x] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1725113274629943296/hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[x] I have verified the payment summary above is correct

trjExpensify commented 6 months ago

Hey @eh2077, thanks for the request. I agree the scope of this issue increased to encompass password-protected PDFs and updating the file type modal. I've added a bonus and sent you both offers for $750.

s77rt commented 6 months ago

@trjExpensify We had a regression https://github.com/Expensify/App/issues/37852. Should the payment here be $375 in that case?

trjExpensify commented 6 months ago

Ah, I didn't see that. Okay cool, so then I'm fine with keeping the bounty at $500 and not reducing for that regression. Once you guys accept the offers, I'll pay a different amount. 👍

s77rt commented 6 months ago

@trjExpensify Accepted! Thank you!

trjExpensify commented 6 months ago

Paid both of you, thanks!

Expensify / App