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 Oct 4][$250] Attachment - Error not displayed when trying to upload corrupted pdf file to chat #48263

Closed
1 of 6 tasks
lanitochka17 opened this issue Aug 29, 2024 · 51 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 29, 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: 9.0.26-1
Reproducible in staging?: Y
Reproducible in production?: N
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): applausetester+omqq1@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

Precondition: user should be Signed In

  1. Open app
  2. Navigate to any chat
  3. Tap on plus button
  4. Select Add attachment option
  5. Upload this corrupted pdf file corrupted.pdf

Expected Result:

"Attachment error" pop-up should be displayed

Actual Result:

Keyboard overlaps the attachment error pop-up and error pop-up is not visible

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

Bug6586335_1724924980382.RPReplay_Final1724924944.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a722d0e57042bb6b
  • Upwork Job ID: 1829184455392581969
  • Last Price Increase: 2024-09-12
  • Automatic offers:
    • dominictb | Contributor | 103991859
Issue OwnerCurrent Issue Owner: @Christinadobrzyn
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

Triggered auto assignment to @srikarparsi (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@srikarparsi
Copy link
Contributor

Making this external to get some eyes

@srikarparsi srikarparsi added the External Added to denote the issue can be worked on by a contributor label Aug 29, 2024
@melvin-bot melvin-bot bot changed the title Attachment - Error not displayed when trying to upload corrupted pdf file to chat [$250] Attachment - Error not displayed when trying to upload corrupted pdf file to chat Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

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

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

melvin-bot bot commented Aug 29, 2024

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

@srikarparsi
Copy link
Contributor

This PR is related to the attachment modal

@srikarparsi
Copy link
Contributor

This seems to only be happening on iOS, tested mWeb chrome and safari and it's not reproducible

@puneetlath puneetlath added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

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

@muratti32
Copy link

muratti32 commented Aug 29, 2024

@srikarparsi It worked correctly on iOS 17.4. Could you please specify the iOS version you encountered issues with?

Screen.Recording.2024-08-29.at.23.43.38.mov

Copy link

melvin-bot bot commented Aug 29, 2024

📣 @muratti32! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@muratti32
Copy link

Contributor details
Your Expensify account email:muratti32@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~0133345a239098ea13

Copy link

melvin-bot bot commented Aug 29, 2024

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@dominictb
Copy link
Contributor

dominictb commented Aug 29, 2024

Edited by proposal-police: This proposal was edited at 2024-08-29 23:59:32 UTC.

Proposal

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

Keyboard overlaps the attachment error pop-up and error pop-up is not visible

What is the root cause of that problem?

hence its onModalHide is called, which call:


hence the keyboard is displayed.

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

  • We need to make sure when the pdf file encountered an error, it will:

Close attachment modal > Open error modal > Call onModalHide function

currently, the order is:

Close attachment modal > Call onModalHide function > Open error modal

  • To do it:

onModalHide();

should be:

                    if (!isPDFLoadError.current) {
                        onModalHide();
                    }

It will make sure if there is a pdf error, we don't call onModalHide, in other words, don't call the function to focus on the composer input restoreKeyboardState().

                    onModalHide={() => {
                        if (isPDFLoadError.current) {
                            isPDFLoadError.current = false;
                            onModalHide?.();
                        }
                    }}

It will call onModalHide?.(), in other words, call the function to focus on the composer restoreKeyboardState().

  • Finally, remove:

    isPDFLoadError.current = false;

  • Because the AttachmentModal is used in a lot of positions in app, so we need to create a new flag prop and only apply the above changes if that flag is true to make sure it does not affect others.

What alternative solutions did you explore? (Optional)

@Christinadobrzyn Christinadobrzyn removed the Bug Something is broken. Auto assigns a BugZero manager. label Sep 3, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 3, 2024
@thesahindia
Copy link
Member

@dominictb's proposal looks good to me!

There is only one issue that the keyboard appears on the screen for a brief moment just after the attachment modal closes. I think it will be hard to fix that (not sure).

Screen.Recording.2024-09-13.at.12.45.52.AM.mov

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 12, 2024

Current assignee @srikarparsi is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@thesahindia
Copy link
Member

There is only one issue that the keyboard appears on the screen for a brief moment just after the attachment modal closes. I think it will be hard to fix that (not sure).

@dominictb, could you look into it and share your thoughts?

@dominictb
Copy link
Contributor

There is only one issue that the keyboard appears on the screen for a brief moment just after the attachment modal closes. I think it will be hard to fix that (not sure).

@thesahindia I cannot reproduce this behavior when applying my solution. However, will try to reproduce it and fix in PR phase.

Screen.Recording.2024-09-13.at.16.02.39.mov

@thesahindia
Copy link
Member

Let's assign @dominictb.

@alexpensify
Copy link
Contributor

@srikarparsi, we need your review; thanks!

@melvin-bot melvin-bot bot added the Overdue label Sep 16, 2024
@Christinadobrzyn
Copy link
Contributor

Nudged @srikarparsi for a review when online!

@melvin-bot melvin-bot bot removed the Overdue label Sep 16, 2024
@srikarparsi
Copy link
Contributor

Sorry for the delay, assigning @dominictb

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 16, 2024
Copy link

melvin-bot bot commented Sep 16, 2024

📣 @dominictb 🎉 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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 17, 2024
@Christinadobrzyn
Copy link
Contributor

just a heads up that I'm going to be ooo until 9/30. I'm not going to assign a BZ teammate since we're working on the PR/discussing the issue. If you need someone before Monday please reach out in Slack for a volunteer

@dominictb
Copy link
Contributor

@Christinadobrzyn PR on production 2 days ago. Should add [HOLD for payment Oct 4].

@Christinadobrzyn Christinadobrzyn changed the title [$250] Attachment - Error not displayed when trying to upload corrupted pdf file to chat [HOLD for payment Oct 4][$250] Attachment - Error not displayed when trying to upload corrupted pdf file to chat Sep 30, 2024
@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Sep 30, 2024

Payouts due:

Do we need a regression test?

@thesahindia
Copy link
Member

(you're paid in NewDot, yes?)

Yep!

@thesahindia
Copy link
Member

Here are the steps for the test case:-

  1. On native app go to any chat
  2. Tap on composer to open the keyboard
  3. Tap on the "+" icon > "Add attachment"
  4. Select a corrupt pdf
  5. Verify you see the error modal and it's not hidden behind the keyboard

@Christinadobrzyn
Copy link
Contributor

Thanks @thesahindia - please submit a payment via NewDot. TY!
@dominictb I've paid you out in Upwork

@Christinadobrzyn Christinadobrzyn added Daily KSv2 and removed Weekly KSv2 labels Oct 4, 2024
@Christinadobrzyn
Copy link
Contributor

I DMd @thesahindia reminding them to request payment. We'll keep it open for a bit longer to make sure @thesahindia sees it.

@thesahindia
Copy link
Member

Thanks for the reminder! I have added it to my list. Feel free to close it.

@garrettmknight
Copy link
Contributor

$250 approved for @thesahindia

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. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
No open projects
Status: No status
Development

No branches or pull requests

9 participants