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 2022-09-01] [$500] IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt - reported by @dhairyasenjaliya #9928

Closed
kbecciv opened this issue Jul 14, 2022 · 58 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Jul 14, 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 app and login
  2. Tap on Profile icon > Payments
  3. Tap on a payment method
  4. Tap "Make default payment method"

Expected Result:

User is prompted a password and able to view it

Actual Result:

A keyboard opens but prompt cannot be seen whatsoever. User is unable to change default payment method

Workaround:

Unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.1.84.11

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): applausetester+gl3@applause.expensifail.com/uTest123

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5648795_video.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhairyasenjaliya

Slack conversation:

Reported here

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Jul 14, 2022

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

@dhairyasenjaliya
Copy link
Contributor

dhairyasenjaliya commented Jul 15, 2022

I also did report this issue here on slack on Tuesday 12th july :) https://expensify.slack.com/archives/C01GTK53T8Q/p1657600786481579

@mountiny mountiny changed the title IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt reported by @dhairyasenjaliya Jul 15, 2022
@mountiny mountiny changed the title IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt reported by @dhairyasenjaliya IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt - reported by @dhairyasenjaliya Jul 15, 2022
@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Jul 15, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 15, 2022

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

@mountiny
Copy link
Contributor

Thanks for mentioning that @dhairyasenjaliya! I have updated the issue to reflect that.

This can be External issue.

@kadiealexander
Copy link
Contributor

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jul 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 18, 2022

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

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

melvin-bot bot commented Jul 18, 2022

Current assignee @mountiny is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt - reported by @dhairyasenjaliya [$250] IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt - reported by @dhairyasenjaliya Jul 18, 2022
@juhipatel27
Copy link

Using keyboard.dismiss() method we can close the keyboard. when we initialize the payment screen, in useEffect method we can write this code.

@rushatgabhane
Copy link
Member

rushatgabhane commented Jul 19, 2022

@juhipatel27 thanks for your interest in this issue, can you please look at a few exported issues to get a better idea of what we expect from a proposal? https://github.com/Expensify/App/issues?page=2&q=is%3Aopen+is%3Aissue+label%3AExported

  • Where exactly are you proposing these changes
  • Why is keyboard opening in the first place?
  • Why the password screen isn't opening?

@juhipatel27
Copy link

@rushatgabhane
I have resolved above issues
Added code for manage keyboard height

final_keyboard_bug.mov

@mountiny
Copy link
Contributor

@juhipatel27 Very glad to see you have fixed the problem 🎉

However, as Rushat mentioned, we have an automated and standardized way how to handle the proposals, which helps us to stay efficient and on top of things. I would recommend checking out some previous jobs, which already got a contributor assigned to see how they posted the proposals. There is also this part in our contributor guidelines which you might find useful.

Please, also share the exact changes in code you had to make to fix this issue. You can see from all the other issues and contributors that we won't just take the code and runaway, no need to worry about that :)

Thank you!

@juhipatel27
Copy link

juhipatel27 commented Jul 19, 2022

Proposal:

I added code in passwordPopover.js

  1. Added height in state

this.state = { password: '', height: 150, };

  1. Added keyboard listener for get height

`componentDidMount() {
        this.keyboardDidShowListener = Keyboard.addListener('keyboardDidShow', this.keyboardDidShow.bind(this));
    }

keyboardDidShow(e) {
    if (this.state.height - e.endCoordinates.height !== 150) {
        this.setState({height: this.state.height + e.endCoordinates.height});
    }
}`

3.Added view height dynamically

  <View
        style={[
                        styles.m5,
                        {height: this.state.height},
                        !this.props.isSmallScreenWidth ? styles.sidebarPopover : '',
                    ]}
                >

Screenshot 2022-07-19 at 3 48 46 PM
Screenshot 2022-07-19 at 3 48 57 PM

Thanks.

CC: @rushatgabhane @mountiny

@mountiny
Copy link
Contributor

@juhipatel27 Thank you! I will now wait for @rushatgabhane to give it a look, but for future, please, make sure the first word of the comment is proposal, then our webhook will actually make this issue Daily which helps us prioritize tasks and also will increase chances we will review this proposal sooner than later :) Thank you!

@mountiny mountiny added Daily KSv2 and removed Weekly KSv2 labels Jul 19, 2022
@rushatgabhane
Copy link
Member

@juhipatel27 can you think of a simpler/better way to do this? Maybe we could use KeyboardAvoidingView?

@melvin-bot
Copy link

melvin-bot bot commented Aug 5, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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.

2 similar comments
@melvin-bot
Copy link

melvin-bot bot commented Aug 5, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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
Copy link

melvin-bot bot commented Aug 5, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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 melvin-bot bot added the Overdue label Aug 10, 2022
@kadiealexander
Copy link
Contributor

@mountiny any thoughts on the regression warning above?

@melvin-bot melvin-bot bot removed the Overdue label Aug 11, 2022
@mountiny
Copy link
Contributor

@kadiealexander nothing, we haven't merged anything for this issue so we cannot have regression here, we are working on improving this message logic

@dhairyasenjaliya
Copy link
Contributor

dhairyasenjaliya commented Aug 11, 2022

@kadiealexander actually this PR will solve another bug as well so I have mentioned that issue here but somehow melvin consider as regression 🤦

@mountiny
Copy link
Contributor

Yeah, Melvin is dumb, they see a link to an issue, they act 🤦

@kadiealexander
Copy link
Contributor

LOL c'mon Melvin!! Thanks for the update team.

@mountiny mountiny added the Reviewing Has a PR in review label Aug 11, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 18, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION 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 production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

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.

@rushatgabhane
Copy link
Member

PR is moving forward

@rushatgabhane
Copy link
Member

PR is on staging

@melvin-bot
Copy link

melvin-bot bot commented Aug 24, 2022

⚠️ 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 melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 25, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.89-4 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 2022-09-01. 🎊

@melvin-bot melvin-bot bot changed the title [$500] IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt - reported by @dhairyasenjaliya [HOLD for payment 2022-09-01] [$500] IOS/Android - Payments - Making payment method default will have keyboard overlapping password prompt - reported by @dhairyasenjaliya Aug 25, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 1, 2022
@kadiealexander
Copy link
Contributor

@rushatgabhane and @dhairyasenjaliya please apply here so I can issue payment: https://www.upwork.com/jobs/~01fd74bc9f792f9604

@dhairyasenjaliya
Copy link
Contributor

@kadiealexander applied and also link for reporting issue?

@kadiealexander
Copy link
Contributor

kadiealexander commented Sep 1, 2022

Thanks @dhairyasenjaliya, I'll add the reporting payment as a bonus on this contract.

@kadiealexander
Copy link
Contributor

@rushatgabhane and @dhairyasenjaliya please accept the offers and I'll pay this out.

@dhairyasenjaliya
Copy link
Contributor

Done @kadiealexander

@kadiealexander
Copy link
Contributor

Thanks team! Everyone is paid.

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 Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants