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

Unable to delete/remove my Paypal.me username - Reported by @mananjadhav #6944

Closed
mvtglobally opened this issue Dec 29, 2021 · 22 comments
Closed
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@mvtglobally
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open app
  2. navigate to Settings > Payments > PayPal.ne
  3. Try to delete existing account

Expected Result:

User should have an option to delete PayPal account

Actual Result:

User has no option to delete PayPal account

Workaround:

Unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.1.23-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screen Shot 2021-12-29 at 4 06 13 PM

Expensify/Expensify Issue URL:
Issue reported by: @mananjadhav
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1640701527159200

View all open jobs on GitHub

@MelvinBot
Copy link

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

@nkuoch nkuoch added Weekly KSv2 External Added to denote the issue can be worked on by a contributor and removed Daily KSv2 labels Dec 30, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Dec 30, 2021
@nkuoch
Copy link
Contributor

nkuoch commented Dec 30, 2021

We should just allow to update it to empty, like we do on web, instead of saying invalid.
image

@nkuoch nkuoch added Weekly KSv2 Daily KSv2 and removed Daily KSv2 Weekly KSv2 labels Dec 30, 2021
@jliexpensify
Copy link
Contributor

Hi @nkuoch - just confirming: are we getting a Contributor to take care of it? Does an Upworks job need to be created?

@mananjadhav
Copy link
Collaborator

Proposed Solution:

While the solution is to just add a length check in AddPaypalMePage.

const isValid = this.state.payPalMeUsername.length === 0 || ValidationUtils.isValidPaypalUsername(this.state.payPalMeUsername);

I feel we should show some kind of feedback to let user know that the Papyal.me username will be unset.

  1. If the value was set by default and then we make it blank, change the button text, "Unset Paypal Account" or words like Remove, Delete
  2. Show a confirm dialog box when making it blank, "This will unset/remove your Paypal.me account. Are you sure"

cc - @nkuoch

@nkuoch
Copy link
Contributor

nkuoch commented Dec 31, 2021

@jliexpensify Yes, I tagged it external so it can be worked on by a contributor via Upworks.

@mananjadhav Hmmm ... I'll let @shawnborton gives his opinion on it.

@shawnborton
Copy link
Contributor

I don't think we need to do anything crazy here. Maybe once the empty input is saved, we can just show a quick growl that says "PayPal.me username has been removed" or something like that?

@parasharrajat
Copy link
Member

I agree with Shawn if we want to go this path then we can just show an alert.

Another approach would be to show a delete button to remove the PayPal account instead of leaving this to the user as he would have to guess or find out that clearing the input will remove his PayPal account.

instead of leaving this to the user as he would have to guess or find out that clearing the input will remove his PayPal account

this won't be user-friendly.

@mananjadhav
Copy link
Collaborator

@parasharrajat +1 for Delete button.

@jliexpensify jliexpensify self-assigned this Jan 4, 2022
@jliexpensify
Copy link
Contributor

Posted!

Internal - https://www.upwork.com/ab/applicants/1478215885127532544/job-details
External - https://www.upwork.com/jobs/~01d8e808cb5f2ab235

@nkuoch - done! No need to remove the Contributor Manager (i.e. me) next time: they can stay assigned to the issue for the duration of it.

I'll update the Upworks job name and GH issue once @shawnborton weighs in with the button vs growl debate.

@shawnborton
Copy link
Contributor

I don't think we need a delete button. I think it will be obvious for the user that if they clear that input, they are removing their PayPal.me login. They are essentially taking voluntary action to remove their own username, so I think that will be good enough.

@mananjadhav
Copy link
Collaborator

Okay I’ll update the proposal.

@mananjadhav
Copy link
Collaborator

mananjadhav commented Jan 4, 2022

Updated Proposal

Extending the proposal from #6944 (comment).

const isValid = _.isEmpty(this.state.payPalMeUsername) || ValidationUtils.isValidPaypalUsername(this.state.payPalMeUsername);

const isValid = ValidationUtils.isValidPaypalUsername(this.state.payPalMeUsername);
if (!isValid) {
this.setState({payPalMeUsernameError: true});
return;
}
this.setState({payPalMeUsernameError: false});
NameValuePair.set(CONST.NVP.PAYPAL_ME_ADDRESS, this.state.payPalMeUsername, ONYXKEYS.NVP_PAYPAL_ME_ADDRESS);
Growl.show(this.props.translate('addPayPalMePage.growlMessageOnSave'), CONST.GROWL.SUCCESS, 3000);

For the Growl, there are two approaches:

  1. Update addPayPalMePage.growlMessageOnSave as Your PayPal username was successfully updated - Probably generic message for adding, updating and delete.

  2. Check if it is a delete operation, then show deleted message.

    • Add a translation addPayPalMePage.growlMessageOnRemove as Your PayPal username was successfully removed
let growlMessageKey = `addPayPalMePage.growlMessageOnSave`;
if(!_.isEmpty(this.props.payPalMeUsername) && _.isEmpty(this.state.payPalMeUsername)) {
   growlMessageKey = `addPayPalMePage.growlMessageOnRemove`;
}

Growl.show(this.props.translate(growlMessageKey), CONST.GROWL.SUCCESS, 3000);

@parasharrajat
Copy link
Member

This is not yet exported but I guess it is going to be so I am reviewing the proposal.

const isValid = _.isEmpty(this.state.payPalMeUsername) || ValidationUtils.isValidPaypalUsername(this.state.payPalMeUsername);

Adding isEmpty check to isValid could be confusing for someone looking at the code. I think it is better to divide those into two variables. What do you think?

if(this.props.payPalMeUsername && this.state.payPalMeUsername) {
growlMessageKey = addPayPalMePage.growlMessageOnRemove;
}

didn't get this... is this the correct condition?

Also, as a small piece of advice, It is always best to add links to code. It speeds up the proposal review process and makes things clear from the developer's perspective. It could save us some Q&A.

@mananjadhav
Copy link
Collaborator

mananjadhav commented Jan 4, 2022

I think it is better to divide those into two variables. What do you think

Sure can do. Can keep isValid as is and add the isEmpty check to the if condition here.

const isValid = ValidationUtils.isValidPaypalUsername(this.state.payPalMeUsername);
if (!isValid) {

if (!_.isEmpty(this.state.payPalMeUsername) && !isValid) {

didn't get this... is this the correct condition?

Sorry yeah, you're right. I was supposed to add ! but I have updated with _isEmpty check.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Jan 4, 2022

Sorry late here, earlier the implementation was like to remove paypal when we save empty but there was a wrong growl message shown earlier, I raised an issue for this #5399.

@stitesExpensify comment here>

That comment was

I personally think this is a non-issue that will be fixed by a combo of #5418 (which will not let you edit your paypal anymore) and https://github.com/Expensify/Expensify/issues/169126 (which will let you delete your paypal with a button rather than by just emptying it out)

So I think it is better to check with @stitesExpensify before proceeding further.

@stitesExpensify
Copy link
Contributor

👋 I agree with @Santhosh-Sellavel (and past me 😄). I don't think we need to do anything here. Default/Delete will fix this issue very soon ™️

@jliexpensify
Copy link
Contributor

@stitesExpensify - Are you happy for me to close the Upworks job?

Also does @mananjadhav get a bonus for reporting?

@stitesExpensify
Copy link
Contributor

Yes, I think that the upwork job can be closed. WRT payment, since there isn't an exact issue for deleting payment methods on /app (it's only on /expensify) we should pay for the reporting.

I'll create a new issue to prevent any future confusion.

@jliexpensify
Copy link
Contributor

jliexpensify commented Jan 9, 2022

Thanks for confirming @stitesExpensify .

Will close Upworks issue once @mananjadhav accepts my invite so I can pay a $250 bonus for reporting.

@mananjadhav
Copy link
Collaborator

Done @jliexpensify

@jliexpensify
Copy link
Contributor

Paid and closed job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants