Skip to content
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

fix: remove options.model overwrite on bulkUpdate #15231

Merged
merged 5 commits into from Nov 7, 2022
Merged

fix: remove options.model overwrite on bulkUpdate #15231

merged 5 commits into from Nov 7, 2022

Conversation

BigsonLvrocha
Copy link
Contributor

@BigsonLvrocha BigsonLvrocha commented Nov 4, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

Closes #11364

This is a way that keeps the current behaviour of bulkUpdate while fixing the unexpected behaviour when calling .update when there are 2 or more models defined for the same table

Todos

  • #1 add test

@BigsonLvrocha BigsonLvrocha changed the title remove options.model overwrite on bulkUpdate fix: remove options.model overwrite on bulkUpdate Nov 4, 2022
@BigsonLvrocha BigsonLvrocha marked this pull request as ready for review November 4, 2022 13:19
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me

ephys
ephys previously requested changes Nov 5, 2022
test/unit/model/update.test.js Outdated Show resolved Hide resolved
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@BigsonLvrocha BigsonLvrocha requested review from WikiRik and ephys and removed request for ephys and WikiRik November 7, 2022 17:09
@BigsonLvrocha
Copy link
Contributor Author

sorry for the span on request, I did not know only one review was requested per pr

@WikiRik WikiRik dismissed ephys’s stale review November 7, 2022 18:54

Issue resolved

@WikiRik
Copy link
Member

WikiRik commented Nov 7, 2022

Thanks for this PR!

@WikiRik WikiRik merged commit c620469 into sequelize:main Nov 7, 2022
@BigsonLvrocha BigsonLvrocha deleted the fix-bulk-update-model-mapping branch November 7, 2022 19:05
@BigsonLvrocha
Copy link
Contributor Author

@WikiRik
Will this change go into v6 too?
What should I do for the fix to get there?

@WikiRik
Copy link
Member

WikiRik commented Nov 7, 2022

Good question! Just open a PR just like this one, but into the v6 branch. We'll review it and then it will land in the next v6 release

WikiRik pushed a commit that referenced this pull request Nov 7, 2022
* fix: remove options.model overwrite on bulkUpdate (#15231)

* fix: remove options.model overwrite on bulkUpdate

* fix: fix typo in test

* fix: make test simpler

Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>

Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
(cherry picked from commit c620469)

* fix: adapt test to v6
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.

Update maps returned instance to the wrong model
3 participants