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

Issue #5664 Client Accounts.forgotPassword and .verifyEmail pass error in callback #5681

Closed

Conversation

@mishavee
Copy link

@mishavee mishavee commented Nov 19, 2015

To resolve issue #5664.
Accounts.forgotPassword and Accounts.verifyEmail to return error in callback instead of exception.
bugfix in packages/accounts-password/password_client.js
Two additional tests in packages/accounts-password/password_tests.js

if (!options.email)
throw new Error("Must pass options.email");
if (!options.email) {
callback(new Meteor.Error(400, "Must pass options.email"));

This comment has been minimized.

@zimme

zimme Nov 24, 2015
Contributor

callback is optional, you should check that it's available before using it.
Should we throw if there is no callback?

if (!token)
throw new Error("Need to pass token");
if (!token) {
callback(new Meteor.Error(400, "Need to pass token"));

This comment has been minimized.

@zimme

zimme Nov 24, 2015
Contributor

Same as previous comment

@mishavee
Copy link
Author

@mishavee mishavee commented Nov 24, 2015

Good pickup @zimme, I've altered the code to only use the callback if it is passed in.
This applies to forgotPassword, verifyEmail, createUser, changePassword, resetPassword.
I've updated tests to cover this too.

@stubailo
Copy link
Contributor

@stubailo stubailo commented Nov 24, 2015

Hey, this looks great. Unfortunately, I don't have time to look at this in depth right now. I'd suggest one change off the bat, which is that another option would be to throw the type error if a callback is not passed. Otherwise, you'd going to have a function call silently fail with no way for the developer to find out? Or maybe that's the point here, I'm not sure.

@mishavee
Copy link
Author

@mishavee mishavee commented Nov 24, 2015

Thanks for the feedback @stubailo :) I think the point is the error is either of interest to the developer, or ignored ... not great practice, but seems appropriate. I'm happy to have another way!

@@ -116,7 +116,7 @@ Accounts.createUser = function (options, callback) {
if (typeof options.password !== 'string')
throw new Error("options.password must be a string");

This comment has been minimized.

@zimme

zimme Nov 25, 2015
Contributor

Shouldn't this be handled by the callback too?

This comment has been minimized.

@mishavee

mishavee Nov 25, 2015
Author

I think type checking errors need to be thrown as exceptions to get noticed during development, so the developer will realise they're sending in the wrong thing when they test it.
On the other hand, unhandled exceptions in production are ugly for the users.

@tmeasday
Copy link
Contributor

@tmeasday tmeasday commented May 3, 2016

Thanks for this PR, sorry we never got around to merging it. We are currently cleaning up the repository and removing old PRs; if you think this PR still has value and are interested in resurrecting it, we encourage you to re-submit it against current devel.

@tmeasday tmeasday closed this May 3, 2016
laosb added a commit that referenced this pull request May 25, 2016
laosb added a commit that referenced this pull request May 25, 2016
laosb added a commit that referenced this pull request Aug 3, 2016
laosb added a commit that referenced this pull request Aug 3, 2016
laosb added a commit that referenced this pull request Aug 17, 2016
laosb added a commit that referenced this pull request Aug 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants