Skip to content

use local findOrCreate instead of User.findOrCreate when creating accounts - #12

Open
dpkirchner wants to merge 1 commit into
1602:masterfrom
dpkirchner:master
Open

use local findOrCreate instead of User.findOrCreate when creating accounts#12
dpkirchner wants to merge 1 commit into
1602:masterfrom
dpkirchner:master

Conversation

@dpkirchner

Copy link
Copy Markdown

From what I can tell the strategy modules, such as google.js, expect to be calling lib/user.js's findOrCreate function, but instead they're calling the underlying model findOrCreate (from jugglingdb). This resulted in essentially empty rows being added to the database.

I'm not sure if this is the best solution, but to work around this I renamed findOrCreate in lib/user.js (and all references in strategies/*). Now the database (mongodb in this case) gets the correct information.

@1602

1602 commented May 7, 2013

Copy link
Copy Markdown
Owner

Expected flow was reimplement findOrCreate, so, this may break backwards compatibility.

@dpkirchner

Copy link
Copy Markdown
Author

What do you suggest? I couldn't get the original code to call user.js's
findOrCreate.

@anatoliychakkaev

Copy link
Copy Markdown
Collaborator

Isn't User.findOrCreate = ... in user.js working?

On Tue, May 7, 2013 at 6:55 PM, David Kirchner notifications@github.comwrote:

What do you suggest? I couldn't get the original code to call user.js's
findOrCreate.
On May 6, 2013 11:23 PM, "Anatoliy Chakkaev" notifications@github.com
wrote:

Expected flow was reimplement findOrCreate, so, this may break backwards
compatibility.


Reply to this email directly or view it on GitHub<
https://github.com/1602/compound-passport/pull/12#issuecomment-17525355>
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/12#issuecomment-17547322
.

@dpkirchner

Copy link
Copy Markdown
Author

It wasn't working, no. I may be doing it wrong, though. I created a user model (define('User', function() ...)) and can verify that it was being used. The "if (!u.findOrCreate)" test prevented overwriting the model's method, but removing that test wasn't enough.

@peter-story

Copy link
Copy Markdown
Contributor

@therealdpk, are you implementing User.findOrCreate within the function being exported in app/models/user.js?

module.exports = function (compound, User) {
User.findOrCreate = function (data, done) {
// do something
}
}

@dpkirchner

Copy link
Copy Markdown
Author

I was attempting to use the function that (from what I could tell) was
intended to be used when finding/creating users. Basically, there was no
way (that I could see) that lib/user.js's findOrCreate would ever be
called. It never happened in my testing. If there is a way to get that
findOrCreate called I'd appreciate seeing the backtrace so I could figure
out what I was/am doing wrong.

On Thu, May 30, 2013 at 4:09 PM, peter-story notifications@github.comwrote:

@therealdpk https://github.com/therealdpk, are you implementing
User.findOrCreate within the function being exported in app/models/user.js?

module.exports = function (compound, User) {
User.findOrCreate = function (data, done) {
// do something
}
}


Reply to this email directly or view it on GitHubhttps://github.com//pull/12#issuecomment-18714295
.

David 'dpk' Kirchner

@peter-story

Copy link
Copy Markdown
Contributor

Line 91: if (!u.findOrCreate) {u.findOrCreate = require('./user.js').findOrCreate;}

So this is checking to see if you have the findOrCreate function in your user model. If you don't, then compound-passport loads its own function. However, JugglingDB comes with this function, so that base method will always be found and called.

It seems that the design intended for users to implement their own findOrCreate function in app/models/user.js like I showed above. What I did was copy, paste, and edit the code from compound_passport/lib/users.js.

@alexbaumgertner

Copy link
Copy Markdown
Contributor

@peter-story yep, it seems that this:

 if (!u.findOrCreate) {
        u.findOrCreate = require('./user.js').findOrCreate;
    }

will never happened.

So, I redefined findOrCreate in models#user, and it works :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants