Client Accounts.forgotPassword and .verifyEmail pass error in callback #7117
Conversation
Failing this:
|
My fault. Rebuilding. |
Added in History.md. |
Pinging @stubailo who reviewed the old PR. |
Needs rebasing |
Yeah, will do this weekend. |
God slow Internet connection... Will try if I have a good connection. |
I would feel much better about these changes if we continued throwing the exception when there is no |
@laosb to follow up on @benjamn's comment: although this is a well established pattern in this file, it seems really wrong to silently return from these functions when the OPTIONAL callback is not provided and the arguments are incorrect. Can we please add something to this file like reportError(error, callback) {
if (callback) {
callback(error);
} else {
throw error;
}
} And replace all the instances of callback && callback(...);
return; With return reportError(callback, ...); |
Oh yeah of course. |
@laosb were you planning on looking at this? |
@tmeasday Sorry for the delay! Tests running. |
No problem @laosb ! |
@tmeasday Code fixed but seems that tests are not suitable for this update. Can you give me some hints on modifying tests? |
@@ -118,8 +125,7 @@ Accounts.createUser = function (options, callback) { | |||
if (typeof options.password !== 'string') | |||
throw new Error("options.password must be a string"); |
tmeasday
Jul 4, 2016
Contributor
Should we use reportError
on this line too?
Should we use reportError
on this line too?
tmeasday
Jul 4, 2016
Contributor
(Maybe not, we just call check
in other places)
(Maybe not, we just call check
in other places)
@@ -410,6 +410,13 @@ if (Meteor.isClient) (function () { | |||
"email", [ | |||
createUserStep, | |||
logoutStep, | |||
// Create user error without callback should not throw error | |||
function (test, expect) { |
tmeasday
Jul 4, 2016
Contributor
Why did you add this new step? I think other tests cover this situation, no? Plus it fails because the password is empty.
Why did you add this new step? I think other tests cover this situation, no? Plus it fails because the password is empty.
@@ -468,6 +475,14 @@ if (Meteor.isClient) (function () { | |||
test.equal(Meteor.user().username, self.username); | |||
})); | |||
}, | |||
// change password with bad old password, but no callback so no error | |||
function (test, expect) { | |||
Accounts.changePassword('wrong', 'doesntmatter'); |
tmeasday
Jul 4, 2016
Contributor
These should now throw an error right?
These should now throw an error right?
}, | ||
// forgotPassword called on client with blank email, no callback so no error | ||
function (test, expect) { | ||
Accounts.forgotPassword({ email: this.email }); |
tmeasday
Jul 4, 2016
Contributor
Again, this is no longer true.
Again, this is no longer true.
@laosb I made some comments. It looks mostly like we had / you added tests that are now failing because the behaviour is stricter than before. We should just fix those tests to expect errors to be thrown (using |
Not sure about what's happening on circle ci. @tmeasday |
@laosb I would probably rebase this against |
@abernix Yeah I think we should do that. Is it possible that the CircleCI results are related to outdate? |
That's just the first step to solving CI bugs – especially when it seems that everything is failing in the way it is, you might have been half-way between commits or something when this was originally branched. |
Looks like there's one failing test left @laosb! |
@tmeasday Yeah, but I don't know how to fix it. The code does throws an error according to the log, but the test suite complains about no error was thrown. |
Oops, my fault of using |
@laosb I would try rebasing your commits -- something like
|
Ok, just rebased! |
According to the log, it did threw an error but the test suites didn't seen it. @tmeasday can you take a look whether I did something wrong? |
@laosb the reasons that test is failing is that the error is thrown in a callback (i.e. after the server returns a result of the method call). There's no way on the client to have this thrown as an error you can I think it's OK in this case to not include a test for that behaviour. I've removed the test and I'm merging the PR! |
Closing in favour of #7695 |
Move #7117 entry to v.NEXT in History.md
Issue #5664
Previous PR #5681
This rebased @mishavee 's work. Thank you @mishavee !