Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.43k stars 2.8k forks source link

[HOLD for payment 2024-08-01] [$250] Close account - Unable to close unverified account #44114

Closed lanitochka17 closed 2 months ago

lanitochka17 commented 3 months 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: 1.4.86-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4652683 Email or phone of affected tester (no customers): natnael.expensify+0a89fdndafadf@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Sign up with a new account
  2. Go to Settings > Security > Close account
  3. Enter default email > click 'Close account'

Expected Result:

Account should be closed

Actual Result:

Account is not closed

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/b5f791f2-1678-4d12-ba37-8c7e46b66070

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e5a38773a3b72e10
  • Upwork Job ID: 1804323493948615973
  • Last Price Increase: 2024-07-13
  • Automatic offers:
    • mkhutornyi | Reviewer | 103155489
Issue OwnerCurrent Issue Owner: @kevinksullivan / @isabelastisser
melvin-bot[bot] commented 3 months ago

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

lanitochka17 commented 3 months ago

@isabelastisser FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

cretadn22 commented 3 months ago

Proposal

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

VBA Link does not sign user in when redirected

What is the root cause of that problem?

the App only call CLOSE_ACCOUNT api. this is the response from BE

Ảnh chụp Màn hình 2024-06-21 lúc 03 34 36

the BE does not remove the session on Onyx

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

https://github.com/Expensify/App/blob/ed5000756ad29315d9de996a042c8fea11225952/src/libs/actions/User.ts#L100

We could use signOutAndRedirectToSignIn function to reset the token and navigate to the sign-in page.

What alternative solutions did you explore? (Optional)

we could also remove the session from optimistic data.

bernhardoj commented 3 months ago

Proposal

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

Unable to close unverified account. The request completes but the user isn't logged out.

What is the root cause of that problem?

When the request is completed, there is already a clear onyx command to clear the onyx so the user is logged out. However, when we receive the response, doesClientNeedToBeUpdated returns true which will call OnyxUpdates.saveUpdateInformation which triggers GetMissingOnyxMessages but fails because the account is already closed, so the response onyx data is never applied. https://github.com/Expensify/App/blob/633e708dfe3d2aba47dbb2091ba1f0f8683bfaaf/src/libs/Middleware/SaveResponseInOnyx.ts#L28-L33

The reason doesClientNeedToBeUpdated returns true is because the lastUpdateIDAppliedToClient is smaller than the previousUpdateID response from CloseAccount. https://github.com/Expensify/App/blob/633e708dfe3d2aba47dbb2091ba1f0f8683bfaaf/src/libs/actions/OnyxUpdates.ts#L162-L169

You can see that previousUpdateID is the lastUpdateID of the previous request.

image

But for the CloseAccount response, the previousUpdateID is bigger than the previous request lastUpdateID, so the app detected there is missing onyx data.

image

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

Fix it on the BE so the correct previousUpdateID is returned or if we want to fix this on the FE, we can add CloseAccount request to the requestsToIgnoreLastUpdateID list because we don't care if there is missing data when closing an account. https://github.com/Expensify/App/blob/633e708dfe3d2aba47dbb2091ba1f0f8683bfaaf/src/libs/Middleware/SaveResponseInOnyx.ts#L6-L8

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

isabelastisser commented 3 months ago

@mkhutornyi please review the proposals above when you have a chance. Thanks!

jasham commented 3 months ago

Is this bug is still open or assigned to someone else ?

melvin-bot[bot] commented 3 months ago

📣 @jasham! 📣 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>
jasham commented 3 months ago

Contributor details Your Expensify account email: jasham009@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/jasham4ryou

melvin-bot[bot] commented 3 months ago

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

jasham commented 3 months ago

Is this bug is still open or assigned to someone else ?

isabelastisser commented 3 months ago

@mkhutornyi, please review the proposals above!

@jasham, this is still open.

isabelastisser commented 3 months ago

@mkhutornyi I DM'd you for visibility. Thanks!

mkhutornyi commented 3 months ago

reviewing

mkhutornyi commented 3 months ago

Pulling engineer for review since it's possible to fix in BE 🎀👀🎀

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @AndrewGable, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

isabelastisser commented 3 months ago

Bump @AndrewGable. Thanks!

AndrewGable commented 3 months ago

@isabelastisser - Did you confirm we do want to allow closing of unverified accounts?

isabelastisser commented 3 months ago

@AndrewGable, nope! I will post this to BZ now. Thanks!

isabelastisser commented 3 months ago

Asked here.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

@AndrewGable, @isabelastisser, @mkhutornyi Eep! 4 days overdue now. Issues have feelings too...

mkhutornyi commented 3 months ago

Not overdue

melvin-bot[bot] commented 3 months ago

@AndrewGable @isabelastisser @mkhutornyi 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!

melvin-bot[bot] commented 3 months ago

@AndrewGable, @isabelastisser, @mkhutornyi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 3 months ago

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

isabelastisser commented 2 months ago

Asking in product:

https://expensify.slack.com/archives/C03U7DCU4/p1720453236924149

isabelastisser commented 2 months ago

@AndrewGable, from the discussion:

I think you should be able to close the account and then if the real email owner ever tried to create an Expensify account it would have the effect of reopening the account, the same as would happen with a verified account

what do you think?

melvin-bot[bot] commented 2 months ago

@AndrewGable, @isabelastisser, @mkhutornyi Still overdue 6 days?! Let's take care of this!

isabelastisser commented 2 months ago

Bump @AndrewGable. Thanks!

AndrewGable commented 2 months ago

I agree with Jenna 👍

isabelastisser commented 2 months ago

Thanks, @AndrewGable ! Should I keep this external? Unsure based on @mkhutornyi's comment:

https://github.com/Expensify/App/issues/44114#issuecomment-2190003156

melvin-bot[bot] commented 2 months ago

@AndrewGable, @isabelastisser, @mkhutornyi 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

isabelastisser commented 2 months ago

Bump @AndrewGable

https://github.com/Expensify/App/issues/44114#issuecomment-2221845948

AndrewGable commented 2 months ago

Sure we can keep external, I am OOO for the next week and a half so let's find someone else to push this along. Thanks!

melvin-bot[bot] commented 2 months ago

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

isabelastisser commented 2 months ago

@mkhutornyi, we are keeping this external so let's pick a proposal! Thanks!

melvin-bot[bot] commented 2 months ago

@isabelastisser, @mkhutornyi Whoops! This issue is 2 days overdue. Let's get this updated quick!

isabelastisser commented 2 months ago

Bump @mkhutornyi, please provide an update! I DM'd you for visibility. Thanks!

mkhutornyi commented 2 months ago

Given that we're good to fix this in FE, @bernhardoj's proposal looks good to me. 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @MariaHCD, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

MariaHCD commented 2 months ago

The FE fix to add to the requestsToIgnoreLastUpdateID list make sense to me 👍🏼

melvin-bot[bot] commented 2 months ago

📣 @mkhutornyi 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

bernhardoj commented 2 months ago

PR is ready

cc: @mkhutornyi

melvin-bot[bot] commented 2 months ago

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

isabelastisser commented 2 months ago

I will be OOO tomorrow and next week, so I am reassigning this until I return on July 29. Thanks, @kevinksullivan!

Status: Waiting for PR to merge.

melvin-bot[bot] commented 2 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 2 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.11-5 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-08-01. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 2 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed: