Skip to content

Change set verificationToken on User.confirm - #2426

Closed
JonForest wants to merge 1 commit into
strongloop:masterfrom
JonForest:fix/2424-verificationToken-not-reset-on-confirm
Closed

Change set verificationToken on User.confirm#2426
JonForest wants to merge 1 commit into
strongloop:masterfrom
JonForest:fix/2424-verificationToken-not-reset-on-confirm

Conversation

@JonForest

Copy link
Copy Markdown

This addresses issue #2424

Simple change to setting the verificationToken to null, instead of undefined. undefined causes the field to not be updated via the Postgres adapter.

Note: the tests have a single failure for me at the moment, but they're also failing on master branch.

Use null instead of undefined
@slnode

slnode commented Jun 10, 2016

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@davidcheung

Copy link
Copy Markdown
Contributor

@slnode ok to test

@davidcheung

Copy link
Copy Markdown
Contributor

perhaps we should add a test case to prevent regression, not sure where we can add the test case tho,
@Amir-61 any clues? this is a user model behavior that is ignored before connectors ignore undefined upon instance.save()

@Amir-61

Amir-61 commented Jun 14, 2016

Copy link
Copy Markdown
Member

@Amir-61 any clues? this is a user model behavior that is ignored before connectors ignore undefined upon instance.save()

I'm not sure if I understand you correctly. Is this the same question you are asking in slack or it is different?

@amir: do u know if only SQL connectors ignore values that are undefined (upon ​_update_​)? or its both noSQL and SQL
something like user.update({name:undefined, lastname:'cheung'});
then it would ignore name

It is an expected behaviour see: https://github.com/strongloop/loopback-datasource-juggler/blob/058d9d46c09f249dd7aa14a09e3097aae8a7a21b/test/manipulation.test.js#L515-L526
Also when you see they are removing undefined in dao.js it means it applies to all connectors not just NoSQL DBs or SQL DBs...

@davidcheung

Copy link
Copy Markdown
Contributor

Thanks @Amir-61 was trying to make sure the issue happens in all connectors, so the unit test could be added in loopback itself, if it was connector specific(which its not) maybe we would have needed to test on specific connector.

@JonForest can you add a unit test to prevent regression, perhaps in this file

it('Verify a user\'s email address with custom token generator', function(done) {

@davidcheung davidcheung mentioned this pull request Jun 15, 2016
@davidcheung

Copy link
Copy Markdown
Contributor

related #2440

@JonForest

Copy link
Copy Markdown
Author

Will take a look at that unit test this weekend.

@davidcheung

Copy link
Copy Markdown
Contributor

@JonForest #2440 is basically a duplicate task, I would actually like to propose to close this and follow up there

@JonForest

Copy link
Copy Markdown
Author

Works for me! As long as it gets fixed, all good.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants