Fix verificationToken bug - #2440
Conversation
|
@bajtos PTAL |
|
would also fix #2424 |
| }, done); | ||
| }); | ||
|
|
||
| it('Should report 302 when redirect url is set', function(done) { |
There was a problem hiding this comment.
You are calling done twice, see L1440. More importantly, this test passes even when common/models/user.json is not fixed.
Never trust a test you haven't seen fail.
When I added console logs around done() calls, only the second done on L1440 was called. The event handler for confirmed even is never triggered. I think that's because you are adding the listener long time after confirm() method was invoked.
I am proposing to simplify this test and call User.confirm directly - the behaviour you are fixing is independent of the way how the method is invoked (from code or via REST API).
As I was playing with the test code to make it work correctly, I figured out I can as well commit my changes - see fc90538. This fixup commit should be rebased away before merging the PR.
There was a problem hiding this comment.
The line comment is misplaced because I submitted after pushing the fixup commit. Sorry for the confusion.
There was a problem hiding this comment.
That's weird. I tested when it was set to undefined and it failed .. It must have been something else.
Anyways, your changes LGTM. I tested with undefined and it failed then it passed with null.
Thanks.
|
@loay I added a commit fixing the test, PTAL. If my changes LGTY and CI builds passes, then you can go ahead, squash commits and land the fix. Please back-port this fix to 2.x, don't forget to include a reference to this PR in the commit message in 2.x. |
3 participants
connect to #414