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

Fix isDisabled check for the payments button #3786

Merged
merged 2 commits into from
Jun 30, 2021
Merged

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Jun 29, 2021

Details

See linked issue

Fixed Issues

$ #3609

Tests/QA

  1. Sign into an account. Navigate to the payments page <baseURL>/settings/payments
  2. Set a Paypal username.
  3. Verify that the button is disabled once you've set one, and the input isn't editable
  4. Sign into the same account but on a different client and navigate to the payments page. Verify that the payment button is disabled and the input isn't editable.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Desktop

desktop.mp4

iOS

ios.mp4

Web

web.mp4

@jasperhuangg jasperhuangg self-assigned this Jun 29, 2021
@jasperhuangg jasperhuangg marked this pull request as ready for review June 29, 2021 06:42
@jasperhuangg jasperhuangg requested a review from a team as a code owner June 29, 2021 06:42
@MelvinBot MelvinBot requested review from Julesssss and removed request for a team June 29, 2021 06:43
@Julesssss Julesssss merged commit c555295 into main Jun 30, 2021
@Julesssss Julesssss deleted the jasper-fixPaymentsButton branch June 30, 2021 09:46
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@isagoico
Copy link

isagoico commented Jul 8, 2021

@jasperhuangg @Julesssss I'm a bit confused with the expected result of this PR. In the steps it says:
Verify that the payment button is disabled and the input isn't editable.
There's also this new PR that changes the flow a bit too #3727

So can you verify if the following is correct:

  1. Navigate to settings/payments
  2. Click on "Add new payment method"
  3. Click on PayPal.me
  4. Add a Paypal.me account and save
  5. Click on add new Payment method again
  6. Click on PayPal.me
  7. Verify you can add a new Paypal account

image

Grabando.194.mp4

@roryabraham
Copy link
Contributor

@isagoico That looks correct to me! cc @kevinksullivan Can you confirm?

@roryabraham
Copy link
Contributor

Actually, I'm 99% sure that this is correct, so I'm going to check it off on the deploy checklist.

@kevinksullivan
Copy link
Contributor

kevinksullivan commented Jul 9, 2021

Hm no, there should not be a concept of adding multiple paypal.me usernames for a single user. All we do with a paypal.me login is auto-fill the $ amount and recipient paypal handle when we link a payer out to paypal.me. In other words, we don't let a user choose the specific paypal.me account of the recipient when making the payment.

Ideally we let a user add paypal.me once, and tap that existing username for editing purposes. Otherwise there shouldn't bee an option to add another username. I will create a separate GH for this one to follow up.

@kevinksullivan
Copy link
Contributor

kevinksullivan commented Jul 9, 2021

@jasperhuangg do you know if this PR will allow you to add multiple paypal.me user names? Or, once you have added paypal.me once, and then you add one "again", are we just editing the existing paypal.me login?

GH is here as a follow up.

@Julesssss
Copy link
Contributor

My understanding here is that this PR simple fixed the original functionality:

You should be able to set a PayPal username exactly once.

Ideally we let a user add paypal.me once, and tap that existing username for editing purposes.

I agree that this should be the case, so either that was not originally implemented, or this PR fix missed this requirement.

@jasperhuangg are you able to confirm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants