-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Email Validation #627
Email Validation #627
Conversation
@flovilmart updated the pull request. |
@drew-gross I've reopened here with a proper rebase on master. |
"" + | ||
"Click here to confirm it:\n" + link; | ||
return sendMail({ | ||
to:user.email, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will have to be user.get('email')
I believe. Tests for the actual adapter is one thing I had punted on in the original PR :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pass the request body there. so that's a plain JS object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man me and @gfosco and @nlutsenko had a big debate about this a couple days ago. Me and @gfosco would rather we give Parse.Object
s to adapters and @nlutsenko would rather we give them raw json. Would you like to weigh in on that discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also cc @lacker, since he had an opinion on this yesterday as well.
I feel quite strongly about not requiring people to use Parse.Objects here, since it provides absolutely no benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In push adapter we pass raw objects for the installations.
Passing Parse.Objects would leave the opportunity to CRUD, do we want to allow that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My standing is that for places where we want to allow CRUD, like in Parse.Cloud things - it makes an absolute sense to do this.
Otherwise, where this is not the intention of the adapter - you can still do it by creating a Parse.Object from JSON object that you get in the adapter, but it's in your best interest to actually not do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My standing is that Parse.Object is a more consistent, more well known, and generally better interface for interacting with data, and that if we want to prevent mutations and/or queries in a particular adapter, we should make an immutable version of Parse.Object instead of changing the entire interface.
For the push adapter I would rather have it accept Parse.Installation but I didn't catch that change before it was merged. We can still consistently use Parse.Objects for future adapters if we decide that is the way we want to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the vote is 2 vs 2 we need a tie breaker.
@flovilmart updated the pull request. |
@@ -24,7 +24,7 @@ describe('Parse.GeoPoint testing', () => { | |||
}); | |||
} | |||
}); | |||
}); | |||
}, 60000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the 60000s for? Tried looking up jasmine docs, they're useful for examples but not much of an api reference..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout on this test. I think this accommodates an old thing, since these tests don't timeout anymore but fail in a flaky way. Meaning - please remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure we have a global 2s timeout for each test already, defined at the top of helper.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove them, those were to bypass the failures that we had for a while in the geo point tests.
@flovilmart updated the pull request. |
1 similar comment
@flovilmart updated the pull request. |
super(); | ||
|
||
if (options && !options.logsFolder) { | ||
throw "FileLoggerAdapter requires logsFolder"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a backwards incompatible change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is, but unlikely to hurt anyone.
in index.js loggerAdapter is defaulted to undefined and logsFolder is the only option.
that would throw only if the user did loggerAdatper: {}
I could remove but it's part of the validation for the adapter loaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a weak justification for breaking compatibility to me. I'd suggest documenting that it's required, and warn if it's not supplied, but not actually fail if it's not supplied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem
@flovilmart updated the pull request. |
1 similar comment
@flovilmart updated the pull request. |
@@ -18,6 +18,10 @@ export class OneSignalPushAdapter extends PushAdapter { | |||
this.validPushTypes = ['ios', 'android']; | |||
this.senderMap = {}; | |||
this.OneSignalConfig = {}; | |||
const { oneSignalAppId, oneSignalApiKey } = pushConfig; | |||
if (!oneSignalAppId || !oneSignalApiKey) { | |||
throw "Trying to initialiazed OneSignalPushAdapter without oneSignalAppId or oneSignalApiKey"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: initialize
@flovilmart updated the pull request. |
// Need direct database access because verification token is not a parse field | ||
return coll.findOne({ | ||
username: username, | ||
_email_reset_token: token, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for compatibility with databases migrated from Parse.com the name needs to be _perishable_token
Sweet, looks like this is getting pretty close to ready. Don't forget to resend the email and set emailVerified to false if the user changes their email. |
Oh right! For the UserController, do you want me to support multiple adapters right away? |
@flovilmart updated the pull request. |
I think we can cross that bridge when we get to it. I'm just expressing my concern :p |
I thought about it too, then I remembered 'easily fixable' :) |
If all files are located at their original location i.e. |
Thank @tanmays Now I get this error: I still have the 'public_html' and it's content and 'views' and it's content inside my parse-server-example directory.. It's like he's not even looking inside my own 'choose_password' file.. |
If you are using There was a bug in v2.1.4 which didn't include these folders with the npm release. You should probably wait for v2.1.5. |
@tanmays Thank you for your help. We will wait for the new release as you suggest. |
Just a heads up. I saw some parse errors with code 100, can't connect to parse API when I was querying from my cloud functions after I set the SERVER_URI in my environment variables. For some reason using the public server url for the serverURL configuration value was causing this. I updated my configuration to use localhost for serverURL and the issue went away. Not sure if this is just a me problem, but if anyone else sees it, try this fix.
|
The public server URL should be a public facing URL if you're not developing, otherwise, you can remove |
The publicServerURL is fine. It was the serverURL that was breaking if it wasn't local host. Just documenting this issue in case anyone else sees it. |
Oh right! Thanks! |
Hello @drew-gross i have a parse server version 2.1.4 so i want to implement user reset password feature using mailgun can you help me with that ? Regards |
Email validation in Parse Server is still experimental. If you want to try it out, you can read this thread to get an idea of how to enable it, but there are no docs yet. |
Hello @drew-gross when the reset user password feature will be enabled ? regards |
Hey @drew-gross |
@432player I got that working by copying the custom files out of my old cloud code and setting the custom pages in my configuration. You could also just download the 2.1.4 branch of parse-server and pull those files out and put them somewhere public. I believe you'll need to follow @tanmays advice about the choose_password file. |
Hey @btate thanks for your input! Can you further explain how to copy the custom files (invalid_link.html, password_reset_success.html, verify_email_success.html,choose_password) into the configuration. |
@432player, you just have to copy those files out of the node module's public_html and view folders and put them somewhere public facing. Then you set the custom page urls in the api configuration.
|
@btate Is there any way to do so in heroku with the same deployment? |
@432player my custom pages are in another web application separate from my services application. So you'll have to figure out where public pages go in your node application using parse-server. I'm not sure where they go. I'd imagine there's just a public_html folder or something that node recognizes. |
@btate Thank you so much! we finally got this working. You were right indeed. |
the html_pages should be visible with 2.1.5. But you can still use the customPages as it's nicer! |
I've set the parse server configuration and made the custom pages publicly accessible. However after sending the form Here's the server config:
In choose_password I only modified this line to: What am I missing here? UPDATE: Had a wrong path in the base var in choose_password. ".heroku.com" instead of ".herokuapp.com". Works fine now. |
@flovilmart after reading this thread, I still have 2 questions:
|
@flovilmart updated the pull request. |
I could successfully enable email verification and reset password by adding required parameters in ParseServer initializer. However, I cannot find how to change the Email Templates (the Text templates to be sent in the email), like "Verification Email Subject", "Verification Email Body", "Password Reset Subject", etc. |
@seonman Afaik there's no way to provide templates for reset and welcome email right now. You should probably open up a separate issue as a feature request. |
@tanmays I have configured email adapter in parse server. How to write parse cloud code for parse server? do we need to have heroku account for parse cloud code? |
@gowridev no need to write cloud code for reset email and verify email functions. For any other custom events you'll probably want to use official mailgun client https://github.com/1lobby/mailgun-js |
Hi @tanmays |
hi @tanmays I am using parse-server-example. |
From #583