Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

beforeCreate hooks does not appear to work #72

Closed
lforge opened this Issue Apr 14, 2012 · 14 comments

Comments

Projects
None yet
5 participants
Contributor

lforge commented Apr 14, 2012

This issue is migrated from the Google group....

I have the following in the model/facility.js file:

Facility.beforeCreate = createTimeStamp;

function createTimeStamp(done) {
  this.date_created = (new Date()) - 0;
  this.date_updated = (new Date()) - 0;
  console.log('JKL: Inside createTimeStamp');
  done();

};

I can see that the trigger is fired, i.e. the "JKL: Inside create TimeStamp" is showing in the console. But there is no value in the db table stored when I click on the Create Facility button. But all the other data columns have data except the date_created and date_updated column. So it either appears that the beforeCreate hook is fired, but the data are override in the ORM level.

I have added a console.log message in my sqlite3.js file in the adapter and in the AbstractClass.create() to see the data being passed into these routines and these two columns have no data in both cases.

I am using the scaffolding controller code. So it is using the .create() method to create the data in the database.

My _form.ejs for these two fields are "hidden", i.e. I am using the following:

<div class="controls">
     <%- form.input("date_updated", {type: "hidden"}) %>
</div>

But they exist in the html page generated. I have also tried to display them and in both cases, the date_created and date_updated field in the db have null value in it.

But the beforeUpdate hook works normally.

Let me know if this is something that I miss or something the framework will need to be updated. Thanks.

@lforge lforge closed this Apr 15, 2012

@lforge lforge reopened this Apr 15, 2012

Contributor

lforge commented Apr 15, 2012

Is there any update on this?

tanema commented Apr 21, 2012

I am seeing this issue as well

User.beforeCreate = function(next){
this.salt = crypto.createHmac('sha1', (new Date().getTime()) +' ').digest('hex');
this.password = crypto.createHmac('sha1', this.salt).update(this.password).digest('hex');
next();
}

but when I check the db the password is still plain text, if I do the hook as beforeSave it is fine though. But I can't use that because if I do one single callback of beforeSave the first save does not give true when I check the propertyChanged('password').

Contributor

lforge commented Apr 22, 2012

In my case, beforeSave only works when I update a record. When I create a new record, beforeSave does not appear to fire neither.

But beforeSave won't work because I don't want to update my date_created time stamp in this case when this is just an update operation.

It will be nice that this is fixed. I have verified that my hook is fired. But the data are not passed to the abstract_class.js and hence the db adapter class.

Contributor

lforge commented May 3, 2012

Any update on this?

by5739 commented May 19, 2012

i meet this issue as well, i found this page from google and found it is unresolved...
at last, i found how to resolve...

in abstract-class.js, we can change like this:

AbstractClass.create = function (data, callback) {

if (stillConnecting(this.schema, this, arguments)) return;

var modelName = this.modelName;

if (typeof data === 'function') {
    callback = data;
    data = {};
}

if (typeof callback !== 'function') {
    callback = function () {};
}

var obj = null;
// if we come from save
if (data instanceof AbstractClass && !data.id) {
    obj = data;
    data = obj.toObject(true);//<-- you can comment this statement
    this.prototype._initProperties.call(obj, data, false);
    create();
} else {
    obj = new this(data);
    data = obj.toObject(true);//<-- you can comment this statement

    // validation required
    obj.isValid(function (valid) {
        if (!valid) {
            callback(new Error('Validation error'), obj);
        } else {
            create();
        }
    });
}

function create() {
    obj.trigger('create', function (done) {
        var data = this.toObject(true);//<-- add this statement here, so _adapter can get the latest data
        this._adapter().create(modelName, data, function (err, id) {
            if (id) {
                defineReadonlyProp(obj, 'id', id);
                addToCache(this.constructor, obj);
            }
            done.call(this, function () {
                if (callback) {
                    callback(err, obj);
                }
            });
        }.bind(this));
    });
}

};

Contributor

lforge commented May 23, 2012

@by5739

Yes, you did it! That line fixes the issue. Thanks!

@lforge lforge closed this May 23, 2012

tanema commented May 23, 2012

has this been submitted for a pull request I do not see it in the current version of the source.

giano commented Jun 26, 2012

Please somebody reopen this. It's still present and the by5739 patch works well! Make a pull please!

@lforge lforge reopened this Jun 29, 2012

Contributor

lforge commented Jun 29, 2012

@by5739 , would you mind making a pull for this please? If not, please send me some more details of yourself such as your name, etc so that we can give you credit for the fix in the pull request. If you pick up the fix from somewhere, please provide reference so that we can give credit the proper original owner of the fix.

I can try to issue a pull request on your behalf. I am kind of new to github. But I will try to make this work. Thanks.

by5739 commented Jul 2, 2012

@lforge , thank you! I am a beginner of git, i have pull this issue, but i don't know is it enough? see from this: #87

Contributor

lforge commented Jul 4, 2012

@by5739,

What a pull request mean is that you fork the original code, make your change, then you can push your code back to your own github repository, then you can send the original author a pull request so that the original author can incorporate your change. Hope I explain this correctly. Like I said, I am kind of new to this. Anyway, I will check into this this weekend and see if I can make a pull request on this. Thanks.

by5739 commented Jul 4, 2012

@lforge Thank you for telling me how to do that, hope you to make a pull for this, Thanks

lforge added a commit to lforge/jugglingdb that referenced this issue Jul 9, 2012

Contributor

lforge commented Jul 9, 2012

@by5739

Just issue a pull request for this. Let's see if this makes it to the master version. Thanks.

anatoliychakkaev pushed a commit that referenced this issue Jul 11, 2012

Merge pull request #101 from lforge/master
Fix for issue #72 beforeCreate trigger not firing properly

by5739 commented Jul 18, 2012

Good!

@1602 1602 closed this Jan 22, 2013

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