Skip to content

Fix verificationToken bug #2440

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.

Sign up for GitHub

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

Merged
loay merged 1 commit into from
Jun 17, 2016
Merged

Fix verificationToken bug #2440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/models/user.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ module.exports = function(User) {
fn(err);
} else {
if (user && user.verificationToken === token) {
user.verificationToken = undefined;
user.verificationToken = null;
user.emailVerified = true;
user.save(function(err) {
if (err) {
Expand Down
15 changes: 15 additions & 0 deletions test/user.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,6 +1418,21 @@ describe('User', function() {
}, done);
});

it('sets verificationToken to null after confirmation', function(done) {
testConfirm(function(result, done) {
User.confirm(result.uid, result.token, false, function(err) {
if (err) return done(err);

// Verify by loading user data stored in the datasource
User.findById(result.uid, function(err, user) {
if (err) return done(err);
expect(user).to.have.property('verificationToken', null);
done();
});
});
}, done);
});

it('Should report 302 when redirect url is set', function(done) {

@bajtos bajtos Jun 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bajtos bajtos Jun 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line comment is misplaced because I submitted after pushing the fixup commit. Sorry for the confusion.

@loay loay Jun 17, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

testConfirm(function(result, done) {
request(app)
Expand Down