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.

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

Bugfix for the problems related to eager loading associations and subsequent saving of DAO instances #716

Merged
merged 5 commits into from Jun 24, 2013

Conversation

5 participants
@iamjochem
Contributor

iamjochem commented Jun 19, 2013

this is a fix for the problems outlined in #715, some buster tests now included

iamjochem added some commits Jun 19, 2013

when we validate - just do it on the contained dataValues, and lets a…
…ctually check that we have a rawAttribute definition for each value we attempt to validate (seems that it is possible to set stuff into a doa instance even though the model does not know about a given property)
fix a test - bad model property name used, namely "name" when it shou…
…ld have been "username" (note that this broken test has nothing to do with the changes in this branch :-/)
fix failing test related to SQL generated for bulk insertions - the S…
…QL was correct but the test assumed that the fields were specified in a different order (I think switching from `doa.values` to `dao.dataValues` affected the order of field names in generated SQL)

janmeier added a commit that referenced this pull request Jun 24, 2013

Merge pull request #716 from tandartsnl/bugfix/eagerloading_and_save
Bugfix for the problems related to eager loading associations and subsequent saving of DAO instances

@janmeier janmeier merged commit bb46cd4 into sequelize:master Jun 24, 2013

1 check passed

default The Travis CI build passed
Details

janmeier added a commit that referenced this pull request Jun 24, 2013

@janmeier

This comment has been minimized.

Member

janmeier commented Jun 24, 2013

You sir, are nothing short of awesome! 3 issues closed by merging one PR - thanks :)

@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Jun 25, 2013

:) thanks ... it's nice to have contributions accepted - you guys do a pretty good job running this project.

@kevinludwig

This comment has been minimized.

kevinludwig commented Jul 2, 2013

is this change available in any npm-able version? I did an npm install for 1.7.0-alpha2 (which is the version currently in master) but still seeing the issue.

@durango

This comment has been minimized.

Member

durango commented Jul 4, 2013

@kevinludwig you'll have to npm install from git unfortunately.

@durango durango referenced this pull request Jul 8, 2013

Closed

State of the Union #749

33 of 45 tasks complete
@greghart

This comment has been minimized.

greghart commented Jul 24, 2013

@kevinludwig If you're not able to install from git due to deployment issues, or you just don't like the sound of it, a decent work around is to just .build your include object before saving, eg

saveableObj = Instance.build(includeyObject.values, {isNewRecord:false})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment