[$250] Attachment - Error not displayed when trying to upload corrupted pdf file to chat

lanitochka17 commented 3 weeks ago

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

Open app
Navigate to any chat
Tap on plus button
Select Add attachment option
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
[x] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/690d2123-aa87-4708-ad99-d78604532fda

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

melvin-bot[bot] commented 3 weeks ago

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

github-actions[bot] commented 3 weeks ago

:wave: 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:

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

srikarparsi commented 3 weeks ago

Making this external to get some eyes

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

srikarparsi commented 3 weeks ago

This PR is related to the attachment modal

srikarparsi commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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 commented 3 weeks ago

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

https://github.com/user-attachments/assets/50bcdf35-f6e4-43ca-9dfa-c1fda762d64d

melvin-bot[bot] commented 3 weeks ago

📣 @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:

Make sure you've read and understood the contributing guidelines.
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.
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.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

muratti32 commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

dominictb commented 2 weeks ago

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?

When the PDF file encountered an error because it is corrupted file, we close the modal: https://github.com/Expensify/App/blob/6d70b10a35a0c0c1d045c29d602ee8e9ecfc7d30/src/components/AttachmentModal.tsx#L566

hence its onModalHide is called, which call: https://github.com/Expensify/App/blob/6d70b10a35a0c0c1d045c29d602ee8e9ecfc7d30/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L301 hence the keyboard is displayed.

Then we show the error modal: https://github.com/Expensify/App/blob/6d70b10a35a0c0c1d045c29d602ee8e9ecfc7d30/src/components/AttachmentModal.tsx#L499 but now that modal is displayed behind the keyboard.

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:

https://github.com/Expensify/App/blob/6d70b10a35a0c0c1d045c29d602ee8e9ecfc7d30/src/components/AttachmentModal.tsx#L495 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().

And in here: https://github.com/Expensify/App/blob/6d70b10a35a0c0c1d045c29d602ee8e9ecfc7d30/src/components/AttachmentModal.tsx#L614 add a new prop
```
                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: https://github.com/Expensify/App/blob/6d70b10a35a0c0c1d045c29d602ee8e9ecfc7d30/src/components/AttachmentModal.tsx#L498
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)

melvin-bot[bot] commented 2 weeks ago

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

Christinadobrzyn commented 2 weeks ago

I'm going to be ooo 9/4-9/10, so I'm going to add a BZ teammate to watch this while I'm away.

@alexpensify - we are reviewing proposals so there shouldn't be much to do at this time. TY!

thesahindia commented 2 weeks ago

I wasn't able to repro this. Is this still reproducible?

dominictb commented 2 weeks ago

@thesahindia I still can reproduce in IOS native:

https://github.com/user-attachments/assets/577a1a78-6364-487c-869b-11f3a2c85dc6

melvin-bot[bot] commented 2 weeks ago

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

thesahindia commented 2 weeks ago

@thesahindia I still can reproduce in IOS native:

Screen.Recording.2024-09-05.at.18.27.13.mov

Could you please share the ios version?

dominictb commented 2 weeks ago

I still can reproduce in IOS native:

I tested it in the latest main branch

thesahindia commented 1 week ago

@dominictb, I am asking about the ios simulator version.

alexpensify commented 1 week ago

@dominictb can you please share the iOS simulator version? Thanks!

dominictb commented 1 week ago

I am using ios 17.4

alexpensify commented 1 week ago

@thesahindia, can we continue the review process, or do we need to start a discussion in the Open Source Slack room? Thanks for the update!

melvin-bot[bot] commented 1 week ago

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

Christinadobrzyn commented 1 week ago

@dominictb can you attach the corrupted file that you tested with so we can see if that helps reproduce the issue?

dominictb commented 1 week ago

This one corrupted.pdf

alexpensify commented 1 week ago

@Christinadobrzyn is back, so I'm going to exit this GH party.

Christinadobrzyn commented 1 week ago

Ah thanks @dominictb i can reproduce this issue when trying to upload corrupted.pdf. I added that file to the OP testing steps. @thesahindia do you think we should move forward with this?

melvin-bot[bot] commented 1 week ago

@srikarparsi @Christinadobrzyn @thesahindia 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!

thesahindia commented 1 week ago

I was able to repro it! The simulator was connected to hardware keyboard and because of that the keyboard behaviour wasn't same as the real device.

thesahindia commented 1 week ago

@dominictb, instead of sharing the code changes, would you mind explaining the solution in plain english as mentioned in the proposal template. There should be an explanation in the proposal about what changes we need and what they do.

dominictb commented 1 week ago

@thesahindia I updated PR to add the explanation for each steps in my solution.

thesahindia commented 1 week ago

@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).

https://github.com/user-attachments/assets/0eaafc2a-ceba-4cb1-89bc-94acea5ac355

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

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

thesahindia commented 1 week ago

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 commented 6 days ago

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.

https://github.com/user-attachments/assets/cb5b3f43-2438-4c71-a2a3-eea35779578d

thesahindia commented 6 days ago

Let's assign @dominictb.

alexpensify commented 6 days ago

@srikarparsi, we need your review; thanks!

Christinadobrzyn commented 3 days ago

Nudged @srikarparsi for a review when online!

srikarparsi commented 3 days ago

Sorry for the delay, assigning @dominictb

melvin-bot[bot] commented 3 days ago

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

Expensify / App