Skip to content
This repository has been archived by the owner on Jul 17, 2020. It is now read-only.

(#157) add a 'you signed up with your google account' email #343

Merged
merged 1 commit into from
Dec 16, 2017

Conversation

ronaldblanco
Copy link
Contributor

Closes the Issue #157 add a 'you signed up with your google account' email.
All changes were locally tested and lint checked.

@jspaine
Copy link
Contributor

jspaine commented Dec 10, 2017

Hi, it looks good! This one was a bit complicated though because I'm not sure the email needs to be editable, it should just say something like 'you appear to have signed up with google, try logging in with google. if you have any problems send us a message at [email protected]' and can just have default translations (when we get to that). Or what do you think?

Another part of this issue was that the server users password controller should just send a 200 regardless, and the client should show 'an email has been sent to email@address', so the route can't be used to figure out registered users email addresses. You don't have to do this too, I was a bit late for the discussion on the issue but thought it was worth mentioning.

@ronaldblanco
Copy link
Contributor Author

Hello @jspaine; I do not understand what do you mean with: "can just have default translations (when we get to that). Or what do you think?" I did search in the other emails and I do not see any translations options implemented. It is something new do you want me to code?

@jspaine
Copy link
Contributor

jspaine commented Dec 11, 2017

Hi, I just meant that this email is so simple it's not really worth being editable, and there could just be a standard one. But it makes the code a bit more complicated to make it not editable. I guess we shouldn't worry about it for now and leave it editable.

I'll leave some suggestions in the code, but the context of this is a user enters their email in the forgot password form but they signed up with google so don't have a password and the password reset email wouldn't be appropriate. So I don't think the place you're sending the email from is right, look for how the forgot password form is handled on the server.

@@ -54,7 +54,8 @@ export const pageIdentifiers = {
CUSTOMER_UPDATED: 'customer-update',
DONATION_RECEIVED: 'donation-received',
DONATION_RECEIPT: 'donation-receipt',
PASSWORD_RESET: 'password-reset'
PASSWORD_RESET: 'password-reset',
GOOGLE_SINGUP: 'google-singup'

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

identifier: pageIdentifiers.GOOGLE_SINGUP,
title: 'Google Sined Up',
type: pageTypes.EMAIL,
subject: '<p>The User Sined Up with Google!</p>',

This comment was marked as off-topic.

@ronaldblanco
Copy link
Contributor Author

Hey @jspaine take a look at this.

@ronaldblanco
Copy link
Contributor Author

ronaldblanco commented Dec 12, 2017

Check now.
When you think everything it is right I will squash.

@@ -49,5 +49,7 @@ export default async function sendEmail(
path: '/v3/mail/send',
body: mail.toJSON()
})
// console.log(toEmail + '->' + content)
// console.log(content)

This comment was marked as off-topic.

This comment was marked as off-topic.

@@ -29,6 +29,7 @@ export const forgot = async function(req, res) {
if (!user) {
throw new ValidationError({email: 'No account with that email has been found'})
} else if (user.provider !== 'local') {
if (user.provider === 'google') await mailer.sendPasswordGoogle(user)

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@ronaldblanco
Copy link
Contributor Author

Check now, if it is ok I will clean the code and squash.

@@ -29,9 +29,10 @@ export const forgot = async function(req, res) {
if (!user) {
throw new ValidationError({email: 'No account with that email has been found'})

This comment was marked as off-topic.

message: 'It seems like you signed up using your ' + user.provider + ' account'
})
})*/
} else {
user.resetPasswordToken = token
user.resetPasswordExpires = Date.now() + 3600000 // 1 hour

This comment was marked as off-topic.

@ronaldblanco
Copy link
Contributor Author

Check now!

@jspaine
Copy link
Contributor

jspaine commented Dec 13, 2017

Try to think through it more, the if...else's can be made a bit simpler now. We only want to send an email if there's a user.

mailer.sendPasswordReset needs the password reset url too.

@ronaldblanco
Copy link
Contributor Author

And now??

@jspaine
Copy link
Contributor

jspaine commented Dec 14, 2017

Yeah that looks good.

@ronaldblanco
Copy link
Contributor Author

It should be ready!
Let me know.

@jspaine
Copy link
Contributor

jspaine commented Dec 16, 2017

Thanks, looks good!

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

Successfully merging this pull request may close these issues.

2 participants