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

Accounts.forgotPassword and Accounts.verifyEmail throw error instead of pass error in callback #5664

Closed
mishavee opened this Issue Nov 16, 2015 · 8 comments

Comments

Projects
None yet
4 participants
@mishavee

mishavee commented Nov 16, 2015

Where the email address is blank, or the token is blank, the methods throw a new error, but the docs say they return errors in the callback.

The bad thing about this is undocumented behaviour, plus needing two forms of error handling.

Code in: meteor/packages/accounts-password/password_client.js

@stubailo

This comment has been minimized.

Contributor

stubailo commented Nov 17, 2015

I guess there is a bit of distinction of a runtime error that can't be predicted, vs. a programming error where the argument is of the wrong type. In an ideal world, these arguments could be statically checked.

However, I'd definitely accept a PR for returning these errors through the callback instead of throwing them.

@mishavee

This comment has been minimized.

mishavee commented Nov 17, 2015

Thanks @stubailo, that sounds fun. Is there a guide to submitting a pull-request? Which branch do I clone from, and pull-request to? Can I link it to this issue? Is there automated tests to extend?
"Questions, questions, always questions" - I'm looking at other pull requests to see what to do.

@stubailo

This comment has been minimized.

Contributor

stubailo commented Nov 18, 2015

Unfortunately there isn't a great guide about it. But basically:

  1. You branch off of devel and pull request to devel
  2. Run the tests with ./meteor test-packages in the root directory of the repo
  3. The tests should be in the same package as the code you'll be changing, it is also probably a good idea to add more tests about the error handling in this case
  4. Link to the issue by mentioning the issue in the PR description
@mishavee

This comment has been minimized.

mishavee commented Nov 18, 2015

Thanks for the good directions :) I found some similar info in Contributing-to-Meteor but your steps are more useful.

@mishavee

This comment has been minimized.

mishavee commented Nov 19, 2015

Hi @stubailo, I've had fun: cloned the repo, created a new local branch from devel, patched the file, created two tests that pass, and filled in the contributors form. I tried creating a pull request from github desktop, no permission to publish ... same if I try to push the new branch.

Do I need to fork into another github repo just to do a pull request?
Thanks for your time!

@stubailo

This comment has been minimized.

Contributor

stubailo commented Nov 19, 2015

Yes, you can only do a pull request on github from a fork, which is kind of
like a branch but on github.

On Wed, Nov 18, 2015 at 7:44 PM Misha Verplak notifications@github.com
wrote:

Hi @stubailo https://github.com/stubailo, I've had fun: cloned the
repo, created a new local branch from devel, patched the file, created two
tests that pass, and filled in the contributors form. I tried creating a
pull request from github desktop, no permission to publish ... same if I
try to push the new branch.

Do I need to fork into another github repo just to do a pull request?
Thanks for your time!


Reply to this email directly or view it on GitHub
#5664 (comment).

@mishavee

This comment has been minimized.

mishavee commented Nov 19, 2015

Okay I did a fork, added a branch with the changes, created a pull request to devel: #5681
Now it's waiting for the tests to run. Yay, thanks for your help @stubailo!

@zol zol removed the desired label Feb 23, 2016

laosb added a commit that referenced this issue May 25, 2016

laosb added a commit that referenced this issue May 25, 2016

laosb added a commit that referenced this issue Aug 3, 2016

laosb added a commit that referenced this issue Aug 3, 2016

laosb added a commit that referenced this issue Aug 17, 2016

laosb added a commit that referenced this issue Aug 17, 2016

@laosb

This comment has been minimized.

Collaborator

laosb commented Aug 27, 2016

Fixed in #7117!

@laosb laosb closed this Aug 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment