-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(model): create instance with many-to-many association with extra column (#10034) #10050
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
Conversation
if (attr === include.association.foreignKey || attr === include.association.otherKey) { | ||
continue; | ||
} | ||
values[attr] = instance[include.association.through.model.name][attr]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if instance[include.association.through.model.name][attr]
is defined only then we should set values[attr]
lib/model.js
Outdated
if (include.association.through.model.rawAttributes[attr]._autoGenerated) { | ||
continue; | ||
} | ||
if (attr === include.association.foreignKey || attr === include.association.otherKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both if statements can be folded into single check?
b7c01a4
to
d22c974
Compare
Codecov Report
@@ Coverage Diff @@
## master #10050 +/- ##
==========================================
+ Coverage 96.3% 96.3% +<.01%
==========================================
Files 63 63
Lines 9410 9415 +5
==========================================
+ Hits 9062 9067 +5
Misses 348 348
Continue to review full report at Codecov.
|
Thanks for the review. I fixed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, a few more changes and this should be good to go
expect(foo.bars[0].foobar).to.not.equal(null); | ||
expect(foo.bars[0].foobar.baz).to.equal('baz...'); | ||
}, () => { | ||
expect.fail(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect.fail
is not required promise rejection will bubble up
}).then(() => { | ||
return Foo.findOne({ include: Bar }); | ||
}).then(foo => { | ||
expect(foo.name).to.equal('foo...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please repeat these checks for data returned just after create as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense.
d22c974
to
2178bb7
Compare
Thanks @tsasaki609 👍 |
Thank you very much @tsasaki609, excellent work! 😁 |
Pull Request check-list
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
fix #10034
I tried to include extra attributes in the condition processing the scope.