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

New user gets stuck on loading after clicking on Set Password button - reported by @adeel0202 #6890

Closed
mvtglobally opened this issue Dec 23, 2021 · 47 comments
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Dec 23, 2021

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. Navigate to app
  2. create new account
  3. Go to email and click on link to validate account
  4. Set password

Steps 2

  1. Navigate to app
  2. create new account
  3. Go to email and click on link to validate account
  4. Set invalid password and hit Set password
  5. Set valid password and hit Set password

Expected Result:

User should be able to proceed

Actual Result:

User is unable to proceed, spinner displayed

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • mWeb
  • Android

Version Number: 1.1.23-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

21-12-22-22-51-45.mp4

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

View all open jobs on GitHub

@MelvinBot
Copy link

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

@mvtglobally mvtglobally added the DeployBlockerCash This issue or pull request should block deployment label Dec 23, 2021
@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Dec 23, 2021
@OSBotify
Copy link
Contributor

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

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

@mvtglobally
Copy link
Author

Adding DeloyBlocker label as we were not able to reproduce in PROD. Issue is not consistently repro across devices

@chiragsalian
Copy link
Contributor

Hmm i just tried this on iOS app and it worked fine.
Then as per the steps i tried it on iPhone mWeb and it also worked fine. I'm unsure of the step "Set invalid password and hit Set password", if you put an invalid password it will show you an error and hence you shouldn't be able to click on set password right.
Incorrect password error,
image

Passwords don't match error
image

Once I set the correct password and click submit it logs me in just fine,
image

One thing i noticed in your video is that the URL is production URL. I think atm we don't provide staging validation links so once you get the validation link you have to modify it to be staging.new.expensify.com instead of new.expensify.com. Also maybe try to close the app completely and try again, maybe there is some persistent data behaving weirdly. Can you re-test and let me know.

@adeel0202
Copy link
Contributor

adeel0202 commented Dec 23, 2021

@chiragsalian I'm facing this issue always and in all browsers. I even modified the link to staging.new.expensify.com as you mentioned, still facing the same issue.

staging.mp4
live.mp4

@chiragsalian
Copy link
Contributor

Well @adeel0202, you got the error on staging and production which could imply this issue is not a blocker. Could you check your network tab to see if you can get some clues like if the network request is stalling or returning an error? It does seem to work fine for me on browser.

147286590-d0b54cca-deb5-4418-8ae8-a0f773c08028.mp4

@mvtglobally, can you confirm if you are facing the issue only on staging and not production once again?

@mvtglobally
Copy link
Author

@chiragsalian only 1 tester was able to reproduce this issue on our side and same tester can't reproduce it on production.
I personally wasn't able to reproduce.
We added deploy blocker just because same tester could repro in staging and could not repro in Prod

@anthony-hull
Copy link
Contributor

I think this is a duplicate of this issue: #6389

@chiragsalian
Copy link
Contributor

@mvtglobally, can you ask the tester to do the following,

  1. Go to https://staging.new.expensify.com/.
  2. Open dev tools (option+cmd+i) on mac, then go to Application -> Local Storage -> https://staging.new.expensify.com/ -> Right click and clear.
  3. Refresh the page, create a new account.
  4. Wait for the email with magic link. Once they get the email copy the magic link URL.
  5. Paste in a browser, dont press enter yet. Instead update the URL so that its staging.new.expensify.com and not new.expensify.com. Press enter.
  6. Confirm setting the password works.

@adeel0202
Copy link
Contributor

@chiragsalian please have a look.

test.mp4

@chiragsalian
Copy link
Contributor

Nice, thanks for the complete video. I was able to reproduce the same on staging. It's interesting that the network requests completed just fine too. Odd.
I haven't tried the same steps on production so I'll try that next and dig deeper into the code.

@chiragsalian
Copy link
Contributor

Yup, looks like it failed on production as well for me for the exact same steps. I suspect it has something to do with the order of events where i.e.,

  • If i clear local storage then create the account and then use the magic link all in the same browser it works fine.
  • But if i attempt to create the account, then clear local storage and then use the magic link it fails on staging and production for me.

Clearer steps to reproduce issue:

  1. Enter a new expensify account.
  2. Go to dev tools and clear local storage.
  3. From the email open the link in either staging newDot or newDot.
  4. Enter password, click set password. 💥 infinite loop.

I suspect its because localStorage has no account details that was saved when we were creating an account. Removing blocker label since the same steps gives me an issue on production as well.

@anthony-hull, i don't think this is a dupe since your issue talks about enter key. I tested enter key and under the normal working scenario it works fine for me 🤷‍♂️ maybe your local storage was messed up which is why enter key went on an infinite loop?

@chiragsalian chiragsalian added Daily KSv2 Improvement Item broken or needs improvement. and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Dec 28, 2021
@adeel0202
Copy link
Contributor

Well, I'm stuck on loading even if I don't clear local storage. I'm facing this issue every time I create a new account and set up its password 🤷

@chiragsalian
Copy link
Contributor

chiragsalian commented Dec 28, 2021

Hmm thats really odd. I've hit you up on slack and we can maybe further discuss this 1:1 since it could be faster. Maybe even a screen share video chat since im curious to know whats up. Feel free to hit me up on newDot if you prefer that, my email is chirag@expensify.com.

@anthony-hull
Copy link
Contributor

I'm reworking the logic of this page and API calls in this PR:

#6587

The edge case of no account set in local storage when using this page will be covered with a redirect to the login screen.

@anthony-hull
Copy link
Contributor

anthony-hull commented Dec 28, 2021

@anthony-hull, i don't think this is a dupe since your issue talks about enter key. I tested enter key and under the normal working scenario it works fine for me 🤷‍♂️

I think it is and the enter key was a red herring and a coincidence. The enter key and the submit button call the same function so why would they make a difference?

@chiragsalian
Copy link
Contributor

The edge case of no account set in local storage when using this page will be covered with a redirect to the login screen.

Oh thats neat. Yeah have you already implemented that fix? Asking because when i tested your PR it currently went on infinite loop when i followed the test steps to reproduce issue mentioned here.

I think it is and the enter key was a red herring and a coincidence. The enter key and the submit button call the same function so why would they make a difference?

Ah yes, i thought the issue was mostly focused on the enter key not working as expected. Now that i read the issue thread it definitely feels like the same issue as this one.

Since you are reworking on this logic would you like to tackle this issue as well @anthony-hull? Additionally, i was looking at your PR. I believe there is some overlap with this PR wherein its trying to address issues from a malformed or expired token as well.

@anthony-hull
Copy link
Contributor

anthony-hull commented Dec 30, 2021

Oh thats neat. Yeah have you already implemented that fix? Asking because when i tested your PR it currently went on infinite loop when i followed the test steps to reproduce issue mentioned here.

No I haven't yet :) the tests in my PR are my acceptance tests while I develop. I still need to implement that edge case. It's still a draft PR, I only made it into an open one to get allocated an engineer. I should have reverted it to draft once I got allocated a reviewer.

Since you are reworking on this logic would you like to tackle this issue as well @anthony-hull? Additionally, i was looking at your PR. I believe there is some overlap with this PR wherein its trying to address issues from a malformed or expired token as well.

Yes sure, I would be happy to tackle the issue. I'll try your reproduction steps.

Thanks for that link to the PR. It looks like there is overlap. I'll have a think and update my PR/Issue discussion based on this.

@chiragsalian
Copy link
Contributor

chiragsalian commented Dec 30, 2021

Perfect, thanks @anthony-hull.

The edge case of no account set in local storage when using this page will be covered with a redirect to the login screen.

Wrt this statement, so i dug a little deeper today. The code is actually failing not because of missing onyx "account", but because of missing onyx credentials.login over here. But when we click setPassword we first call ValidateEmail which succeeds and even returns the email. So we can actually use this same email to set onyx credentials.login which i think should fix the issue here. Let me know if that is something that still overlaps with your PR because if not then i can push up a fix for it.

@anthony-hull
Copy link
Contributor

I have an issue that if you have already called ValidateEmail, the error doesn't have the email.

Also we have to call changePasswordAndSignIn in the new user UX with an authToken. API.ChangePassword doesn't return the account email so we wouldn't be able to get it from there either.

@anthony-hull
Copy link
Contributor

anthony-hull commented Jan 9, 2022

I have an issue that if you have already called ValidateEmail, the error doesn't have the email.

Also we have to call changePasswordAndSignIn in the new user UX with an authToken. API.ChangePassword doesn't return the account email so we wouldn't be able to get it from there either.

I have resolved this :)

@chiragsalian chiragsalian added the Reviewing Has a PR in review label Jan 10, 2022
@anthony-hull
Copy link
Contributor

The PR linked is ready for review. Just missing some translations and response on required testing documentation!

@anthony-hull
Copy link
Contributor

Waiting on PR review, answer to a question and a backend bug to be confirmed reproduced by the reviewer and fixed

@anthony-hull
Copy link
Contributor

PR merged and deployed to staging

@anthony-hull
Copy link
Contributor

anthony-hull commented Feb 8, 2022

Should this issue be closed and payment issued? the other issue the PR closes was paid today.
@adeel0202

@adeel0202
Copy link
Contributor

@anthony-hull, I'm not compensated for reporting the issue. @chiragsalian, will I be compensated?

@adeel0202
Copy link
Contributor

cc: @mallenexpensify

@mallenexpensify
Copy link
Contributor

Yes @adeel0202 you will be compensated $250, payment details in CONTRIBUTING.md.

If an external contributor other than yourself is hired to work on the issue, you will also be hired for the same job in Upwork. No additional work is needed. If the issue is fixed internally, a dedicated job will be created to hire and pay you after the issue is fixed.
Payment will be made 7 days after code is deployed to production if there are no regressions. If a regression is discovered, payment will be issued 7 days after all regressions are fixed.

@adeel0202
Copy link
Contributor

Thanks @mallenexpensify. I'm waiting to be compensated for reporting the issues since it is fixed in PR 6587 and deployed on production.

@mallenexpensify
Copy link
Contributor

Thanks for the details @adeel0202 , you're right. I was expecting an auto-post to denote the associated PR was deployed to production. I think it didn't happen because there were two issues fixed by one PR (which is rare). I think it's because this issue was pretty-much a duplicate of #6389 Since we didn't clearly call that out in #expensify-open-source (Anthony suggested) we should err on the side of compensating. Can you the invite here please @adeel0202 ?
https://www.upwork.com/jobs/~01b47f9c65a085016f

Please confirm here once you have

@adeel0202
Copy link
Contributor

Thanks @mallenexpensify, I have applied.

@mallenexpensify
Copy link
Contributor

@adeel0202 hired, let me know when you accept the offer and I'll pay

@adeel0202
Copy link
Contributor

@mallenexpensify, accepted.

@mallenexpensify
Copy link
Contributor

Thanks @adeel0202 , paid

@anthony-hull
Copy link
Contributor

Thanks for the details @adeel0202 , you're right. I was expecting an auto-post to denote the associated PR was deployed to production. I think it didn't happen because there were two issues fixed by one PR (which is rare). I think it's because this issue was pretty-much a duplicate of #6389 Since we didn't clearly call that out in #expensify-open-source (Anthony suggested) we should err on the side of compensating. Can you the invite here please @adeel0202 ?

https://www.upwork.com/jobs/~01b47f9c65a085016f

Please confirm here once you have

I didn't get paid for reporting on my version of this issue as the root cause wasn't found on my report. So it's good he got paid for it.

I didn't get paid for fixing this issue either. Should I be compensated?

@anthony-hull
Copy link
Contributor

Also I think part of the issue is this never got the external tag?

@mallenexpensify
Copy link
Contributor

@anthony-hull , based on this post in #6389

Hey, @anthony-hull! Just catching up on the #6890 linked above, it sounds like this issue is the same and the fix will be handled as part of #6587? Let me know

and your reply

Yes you're correct :) I'm just waiting for another PR to merge!

I was assuming the PR fixed both issues, without additional work needing to be done for this issue, is that correct? If so, I don't think you'd be eligible for payment. If the PR needed extra work/code based on this issue, please provide details.

@mallenexpensify mallenexpensify self-assigned this Feb 11, 2022
@mallenexpensify
Copy link
Contributor

mallenexpensify commented Feb 11, 2022

I reopened and assigned to myself. I'm OOO all next week, so it'll take a bit til I reply @anthony-hull

@anthony-hull
Copy link
Contributor

I was working in that area of the code. I made sure this edge case was added to my testing. I tried to cover off all the edge cases I could think of, which can be seen in the long list of edge cases in the testing section, beyond the initial scope of the issue.
The ticket was about improving the UX for expired sign up tokens.

@mallenexpensify
Copy link
Contributor

OK... so, this issue is about getting stuck and other is about improving UX. That makes it sound like this is eligible for compensation. I've created a job for $250, can you apply here https://www.upwork.com/jobs/~01fd4ccce6000a9c6f and comment in this GH once you've done so?

@anthony-hull
Copy link
Contributor

Thank you Matt! I've done so

@mallenexpensify
Copy link
Contributor

Hired @anthony-hull , let me know when you accept and I'll pay ya

@anthony-hull
Copy link
Contributor

Done!

@mallenexpensify
Copy link
Contributor

@anthony-hull , paid ya $250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants