-
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
[HOLD for payment 2024-03-14] [$500] [MEDIUM] Smartscan: Show PDF receipt thumbnails #31432
Comments
Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new. |
Job added to Upwork: https://www.upwork.com/jobs/~01f90fbd2f6a7c49f2 |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @0xmiroslav ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Show PDF receipt thumbnails What is the root cause of that problem?We're getting thumbnail for receipt here Line 31 in 69a3358
We do not have the logic to get the thumbnail of pdf -> Default image is shown What changes do you think we should make in order to solve the problem?We can lean on the source to get the thumbnail for pdf. Here's the sample source
What alternative solutions did you explore? (Optional)BE will return the thumbnail and use it to shown on FE side ResultScreen.Recording.2023-11-16.at.18.56.52.mov |
@0xmiroslav can you review this please? Also, @tienifr confirming in the results we're solving for all the thumbnails noted in the OP? |
ProposalPlease re-state the problem that we are trying to solve in this issue.We want to show PDF thumbnail in the process of uploading pdf receipt. What is the root cause of that problem?The root cause of this issue is that, currently, we don't have an implementation of previewing PDF source by thumbnail. The PDF source here means expensify source like Currently, uploading PDF attachment from chat can be previewed before upload and the PDF thumbnail is only available after it's been successfully uploaded and the backend will create an image source url for the PDF, like, Intuitively, we should be able to preview the PDF thumbnail before uploading it to backend because we're able to preview PDF pages before uploading. The new feature to have a What changes do you think we should make in order to solve the problem?We can add an implementation of We can also update On native platform, we can make use of the This solution will need more effort to implement. What alternative solutions did you explore? (Optional) |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@0xmiroslav bump! |
Same. @0xmiroslav can you review these proposals today, please? They've been hanging out for 2 weeks. |
reviewing today |
Perfect, thanks. |
So for the avoidance of doubt and @eh2077 benefit, the below are the final set. (PP-PDF specific)
(Generic)
|
This is wrong:
Should be:
|
Lol, got it. Updated. |
@trjExpensify, @s77rt, @luacmartins, @CherylWalsh, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
PR has been deployed to staging 1 hour ago |
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. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.48-0 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-03-14. 🎊 For reference, here are some details about the assignees on this issue: |
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:
|
Regression Test Proposal
|
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 :) |
Payment SummaryBugZero Checklist (@trjExpensify)
|
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. |
@trjExpensify We had a regression #37852. Should the payment here be $375 in that case? |
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. 👍 |
@trjExpensify Accepted! Thank you! |
Paid both of you, thanks! |
Coming from here. CC: @sobitneupane @pradeepmdk @Gonals
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: 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: #30747 (comment)
Action Performed:
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?
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
The text was updated successfully, but these errors were encountered: