Allow changing model attributes in the beforeSave and beforeUpdate callbacks of updateAttributes #102

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@kennu

kennu commented Jul 10, 2012

This patch tracks what model attributes are changed inside the beforeSave() and beforeUpdate() callbacks, when the model is updated by updateAttributes(). It then updates also them to the database backend. Previously any changes would be just lost.

RailwayJS's auto-generated controller code uses updateAttributes() to save modified items. So without this patch, adding a beforeSave() or beforeUpdate() callback (to modify some attributes) has no effect, which is pretty confusing.

I believe that by applying this patch the functionality becomes similar to how Ruby on Rails works in this situation.

@kennu

This comment has been minimized.

Show comment
Hide comment
@kennu

kennu Jul 10, 2012

Added similar support for beforeCreate().

kennu commented Jul 10, 2012

Added similar support for beforeCreate().

+ // Remember what was dirty before the callbacks
+ var wasAlreadyDirty = {}
+ Object.keys(inst).forEach(function (k) {
+ if (!(k !== 'id' && !inst.constructor.schema.definitions[inst.constructor.modelName].properties[k]) && inst.propertyChanged(k)) {

This comment has been minimized.

@anatoliychakkaev

anatoliychakkaev Jul 11, 2012

Collaborator

This condition is really hard to read. it definitely should be simplified.

@anatoliychakkaev

anatoliychakkaev Jul 11, 2012

Collaborator

This condition is really hard to read. it definitely should be simplified.

inst.trigger('save', function (saveDone) {
inst.trigger('update', function (done) {
Object.keys(data).forEach(function (key) {
data[key] = inst[key];
});
+ // Check if any properties were modified during the callbacks
+ Object.keys(inst).forEach(function (k) {
+ if (!wasAlreadyDirty[k] && !(k !== 'id' && !inst.constructor.schema.definitions[inst.constructor.modelName].properties[k]) && inst.propertyChanged(k)) {

This comment has been minimized.

@anatoliychakkaev

anatoliychakkaev Jul 11, 2012

Collaborator

This condition is really hard to read. it definitely should be simplified.

@anatoliychakkaev

anatoliychakkaev Jul 11, 2012

Collaborator

This condition is really hard to read. it definitely should be simplified.

This comment has been minimized.

@kennu

kennu Jul 11, 2012

It's basically copied from another place in the existing code ;-). I don't know the code well enough to say what can be left out.

@kennu

kennu Jul 11, 2012

It's basically copied from another place in the existing code ;-). I don't know the code well enough to say what can be left out.

This comment has been minimized.

@kennu

kennu Jul 11, 2012

Do you think it would make sense to add a model function like "isModelProperty", which would basically return

(k === 'id' || this.constructor.schema.definitions[this.constructor.modelName].properties[k])

? I believe the purpose is just to limit the looping of the property keys to those that are actual database fields.

@kennu

kennu Jul 11, 2012

Do you think it would make sense to add a model function like "isModelProperty", which would basically return

(k === 'id' || this.constructor.schema.definitions[this.constructor.modelName].properties[k])

? I believe the purpose is just to limit the looping of the property keys to those that are actual database fields.

@@ -585,12 +587,26 @@ AbstractClass.prototype.updateAttributes = function updateAttributes(data, cb) {
});
function update() {
+ // Remember what was dirty before the callbacks

This comment has been minimized.

@anatoliychakkaev

anatoliychakkaev Jul 11, 2012

Collaborator

I'm wondering what if attribute was dirty and also changed in hook? I think better solution would be passing additional argument to hook and modify it instead of object itself. What do you think?

@anatoliychakkaev

anatoliychakkaev Jul 11, 2012

Collaborator

I'm wondering what if attribute was dirty and also changed in hook? I think better solution would be passing additional argument to hook and modify it instead of object itself. What do you think?

This comment has been minimized.

@kennu

kennu Jul 11, 2012

That is not how Rails works, so it would be very unintuitive.

@kennu

kennu Jul 11, 2012

That is not how Rails works, so it would be very unintuitive.

@1602

This comment has been minimized.

Show comment
Hide comment
@1602

1602 Apr 15, 2013

Owner

Implemented now.

Owner

1602 commented Apr 15, 2013

Implemented now.

@1602 1602 closed this Apr 15, 2013

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