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

[HOLD for payment 2023-02-03] [$1000] [Image] The attachment PDF preview is not shown offline reported by @Santhosh-Sellavel #12512

Closed
kavimuru opened this issue Nov 7, 2022 · 104 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Nov 7, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open any chat in the new dot
  2. Go offline
  3. Click ➕ in composer -> Add Attachments
  4. Select a PDF attachment

Expected Result:

Should show a preview like we show image preview

Actual Result:

Failed to load error

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.2.24-0
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

200205740-d1d90f4b-358b-46b2-87d0-5ecdc036711e.mov

4

Expensify/Expensify Issue URL:
Issue reported by: @Santhosh-Sellavel
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1667782878782179

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0101e6d9faf1232624
  • Upwork Job ID: 1600152502185627648
  • Last Price Increase: 2022-12-08
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 7, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 7, 2022

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@mallenexpensify mallenexpensify changed the title The attachment PDF preview is not shown offline reported by @Santhosh-Sellavel [Image] The attachment PDF preview is not shown offline reported by @Santhosh-Sellavel Nov 7, 2022
@0xmiroslav
Copy link
Contributor

This issue is reproducible for me on web (both on desktop and on mobile) even when online.

@0xmiroslav
Copy link
Contributor

image

I think this is backend issue.
console error:
image

@mallenexpensify mallenexpensify removed their assignment Nov 9, 2022
@mallenexpensify mallenexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Nov 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

Triggered auto assignment to @davidcardoza (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@mallenexpensify
Copy link
Contributor

I'm heading OOO, reassigning.

@davidcardoza
Copy link
Contributor

I was able to reproduce this. Assigning an engineer to triage - can this be worked on by an external contributor?

@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

Triggered auto assignment to @joelbettner (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@joelbettner
Copy link
Contributor

can this be worked on by an external contributor

Yes, I believe this can be worked on by an external contributor.

@joelbettner joelbettner added the External Added to denote the issue can be worked on by a contributor label Nov 9, 2022
@joelbettner joelbettner removed their assignment Nov 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

Triggered auto assignment to @sketchydroide (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title [Image] The attachment PDF preview is not shown offline reported by @Santhosh-Sellavel [$250] [Image] The attachment PDF preview is not shown offline reported by @Santhosh-Sellavel Nov 9, 2022
@JmillsExpensify
Copy link

cc @youssef-lr I am kind of thinking that we need to have someone holistically approach all these attachment uploads cases. What do you think about us creating one central issue for all the remaining bugs and improvements we need to make? I'm happy to get it started.

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2022
@marcaaron
Copy link
Contributor

I don't think this was a regression was it? There's no real investigation to do. This has just never worked correctly offline.

@marcaaron
Copy link
Contributor

Checked off the first 3 boxes. We should create a test rail step for this though.

@marcaaron
Copy link
Contributor

Think this one is just waiting for payment + regression tests.

@melvin-bot melvin-bot bot removed the Overdue label Feb 6, 2023
@davidcardoza
Copy link
Contributor

payment sent to @aimane-chnaif. @marcaaron is a regression test necessary per your comment here

I don't think this was a regression was it? There's no real investigation to do. This has just never worked correctly offline.

@marcaaron
Copy link
Contributor

Yes, we should create a regression test even though this was not specifically caused by merging another PR.

If I am understanding the BZ checklist instructions here we should. In this case, there was just no specific PR that caused and regression or additional conversation to be had.

I also understand that we changed this recently so it should be responsibility of whoever fixes the issue (which was me 😅) but then BZ is supposed to create the issue to add it to test rail?

Anyway, the steps would be:

  1. Open any chat in New Expensify on Web
  2. Go offline
  3. Click ➕ in composer -> Add Attachments
  4. Select a PDF attachment
  5. Verify a preview of the PDF can be seen

@davidcardoza
Copy link
Contributor

Ya, admittedly, I have yet to run a regression test, it's a bit unclear to me when a regression test should take place or not. Getting a buddy check here - https://expensify.slack.com/archives/C01SKUP7QR0/p1675814496893819

@davidcardoza
Copy link
Contributor

A regression test was added as the previous test was pretty similar to the offline PDF case in this test case:
https://expensify.testrail.io/index.php?/cases/view/1971194&group_by=cases:section_id&group_order=asc&display_deleted_cases=0&group_id=229062

Updated to:

Offline

  1. Open a conversation
  2. Go offline
  3. Click on the plus sign in composer -> Add Attachments
  4. Select a PDF attachment
  5. Verify a preview of the PDF can be seen

@Santhosh-Sellavel
Copy link
Collaborator

@davidcardoza reporting bonus is missed out here!

@davidcardoza
Copy link
Contributor

@Santhosh-Sellavel offer sent.

@Santhosh-Sellavel
Copy link
Collaborator

Thanks @davidcardoza Accepted!

@kbecciv
Copy link

kbecciv commented Mar 17, 2023

Issue is reproductible in build 1.2.87.0

Screenshot_2023-03-17-20-06-08-44_4f9154176b47c00da84e32064abf1c48

@kbecciv kbecciv reopened this Mar 17, 2023
@melvin-bot melvin-bot bot added the Overdue label Mar 17, 2023
@aimane-chnaif
Copy link
Contributor

@kbecciv I think you tried to preview PDF which was already sent as attachment. This should be impossible scenario. How come user can see online data in offline mode before downloading?
This issue is just to preview local PDF before sending attachment.

@melvin-bot melvin-bot bot removed the Overdue label Mar 17, 2023
@kbecciv
Copy link

kbecciv commented Mar 17, 2023

Yes, you are correct, I'm trying  to preview PDF which was already sent as attachment.

@trjExpensify
Copy link
Contributor

Doesn't this one come down to caching on web/desktop, which is being worked on elsewhere?

@aimane-chnaif
Copy link
Contributor

Doesn't this one come down to caching on web/desktop, which is being worked on elsewhere?

I know image cache is being worked already but I don't think PDF cache is being worked anywhere.

@trjExpensify
Copy link
Contributor

Right, but presumably the work we do to support image caching is going to be extended to other file types? I think @thomas-coldwell has it on his radar as a follow-up.

@melvin-bot melvin-bot bot added the Overdue label Mar 27, 2023
@marcaaron
Copy link
Contributor

I'm closing this. The original issue is solved. Not sure what issue we are now experiencing on Android, but let's create a new issue to track it with clearer repro steps.

@kbecciv
Copy link

kbecciv commented Mar 29, 2023

@marcaaron Logged a separate issue here #16694

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests