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

Logic to support closed account receiving a link to set password and for unvalidated and closed accounts to get the email the moment they log into ecash #2745

Merged
merged 9 commits into from
May 13, 2021

Conversation

chiragsalian
Copy link
Contributor

@chiragsalian chiragsalian commented May 7, 2021

Details

On HOLD till I get some confirmation in the issue.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/163196

Tests

  1. Make sure you have the code from this web-e and auth PR and have done make etc.
  2. I would recommend testing on a clean slate which makes this easier i.e., go to sql.sh and run delete from notifications;
  3. Go to ecash, click on new chat, enter a new gmail account. Make a note of the email so you don't forgot. Log out.
  4. In ecash log into the email that you entered in the previous step and click continue. It should be a closed account so in the network tab you should see reopenAccount request like so,
    image
  5. Run select * from notifications and confirm you see a "SetCashPassword" template notification.
  6. On dev, notification emails won't get send to gmail domains so you'll need to run
update notifications set json =  json_set(json(notifications.json), '$.to', '<your_expensify_email>');
  1. Edit this line and replace $this->parameters['to'] with your gmail test account otherwise the validate code won't work
  2. In vssh run php /vagrant/Web-Expensify/script/notifyall.php.
  3. Check your email. Click on the link.
  4. Confirm you are redirected to a setpassword screen like so,
    image
  5. Enter a password and confirm it works just fine.

QA Steps

None. Its a little involved with multiple flows and cannot be tested on staging so once this is live I'll ask tom to test.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@chiragsalian chiragsalian self-assigned this May 7, 2021
@chiragsalian chiragsalian changed the title Logic to support closed account receiving a link to set password [HOLD] Logic to support closed account receiving a link to set password May 8, 2021
@chiragsalian chiragsalian requested a review from a team May 10, 2021 20:33
@MelvinBot MelvinBot requested review from AndrewGable and removed request for a team May 10, 2021 20:34
@chiragsalian
Copy link
Contributor Author

Off HOLD since nothing will break if auth and web aren't live yet and the sooner each part goes live the better. But the complete functionality will only work when all 3 pieces are live - cash, web-e and auth.

@chiragsalian chiragsalian changed the title [HOLD] Logic to support closed account receiving a link to set password Logic to support closed account receiving a link to set password and for unvalidated and closed accounts to get the email the moment they log into ecash May 10, 2021
@chiragsalian chiragsalian marked this pull request as ready for review May 10, 2021 21:28
@chiragsalian chiragsalian requested a review from a team as a code owner May 10, 2021 21:28
@MelvinBot MelvinBot requested review from MonilBhavsar and removed request for a team May 10, 2021 21:28
@chiragsalian chiragsalian removed the request for review from MonilBhavsar May 10, 2021 21:28
@chiragsalian
Copy link
Contributor Author

i accidentally used puller bear before putting the PR for review so I removed the latest pullerbear assignee.

@chiragsalian chiragsalian requested a review from Gonals May 10, 2021 23:10
@chiragsalian
Copy link
Contributor Author

Bump @Gonals and @AndrewGable for review

@chiragsalian chiragsalian requested a review from a team May 12, 2021 22:58
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team May 12, 2021 22:58
@chiragsalian
Copy link
Contributor Author

Andrew is OOO for the week so adding another reviewer.

@chiragsalian chiragsalian removed the request for review from AndrewGable May 12, 2021 22:59
@Beamanator Beamanator self-requested a review May 13, 2021 18:07
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One spelling fix & a requested comment improvement; neither are blockers though :D

src/pages/signin/ResendValidationForm.js Outdated Show resolved Hide resolved
*
* @param {String} [login]
*/
function reopenAccount(login = credentials.login) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB; Why are we defaulting login to credentials.login here and in resendValidationLink? Rephrased: When / why would a passed-in login string be different from what's stored in credentials.login?

I believe this has to do with unvalidated / closed accounts (the topic of your PR), but I think it would be useful to make this clear in the function comment

Copy link
Contributor Author

@chiragsalian chiragsalian May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when you enter an email it hits this flow - https://github.com/Expensify/Expensify.cash/pull/2745/files#diff-283c48b7349b0bf7aba66da8891bc8a9f9df70664bd06e7a875f56b19c6c02f9R126 and credentials.login is not set yet that's why we pass the provided email.

But when you click resent link, it hits this flow - https://github.com/Expensify/Expensify.cash/pull/2745/files#diff-c075eec38f8adc854c238b1d4cfeba1ed3e5de0aa740aa78a658ffe3a53814f9R62 and does not need to pass an email since its in credentials.login. Does that make sense?

Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giggidy, LGTM 💪

// Weather or not the account is validated
// Whether or not the account is validated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

¡Excillente!

@Luke9389 Luke9389 merged commit 24d08bf into main May 13, 2021
@Luke9389 Luke9389 deleted the chirag-ecash-reopen-account branch May 13, 2021 19:04
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.43-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.44-0🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

5 participants