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

v3 Bugfix: Return value in MSSQL soft-delete #6916

Merged
merged 1 commit into from Nov 30, 2016

Conversation

6 participants
@lumaxis
Contributor

lumaxis commented Nov 25, 2016

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-mssql pass with this change (including linting)?
  • Does your issue contain a link to existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Have you added an entry under Future in the changelog?

Description of change

This fixes an issue where undefined was returned when using a soft-delete on MSSQL because the query type of the UPDATE query that ran the soft-delete was not properly set.

@lumaxis lumaxis force-pushed the lumaxis:bugfix/mssql-soft-delete-return branch from 5f240f7 to 9e28020 Nov 25, 2016

@mention-bot

This comment has been minimized.

mention-bot commented Nov 25, 2016

@lumaxis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @felixfbecker, @janmeier and @cbauerme to be potential reviewers.

@lumaxis lumaxis changed the base branch from master to v3 Nov 25, 2016

@lumaxis

This comment has been minimized.

Contributor

lumaxis commented Nov 25, 2016

I accidentally opened this against the wrong base branch first. Anything I can do to trigger CI again?

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 25, 2016

Push an empty commit or open a new pr :-)

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Nov 25, 2016

git commit --amend --no-edit
git push --force
Fix soft delete not returning affected rows on MSSQL
Because the query type was not properly set when doing a soft delete, Sequelize tried to format the result in the wrong format: https://github.com/sequelize/sequelize/blob/cf232e76b46b7cb2fc98c455cc2010d520ca592d/lib/dialects/mssql/query.js#L186

@lumaxis lumaxis force-pushed the lumaxis:bugfix/mssql-soft-delete-return branch from 9e28020 to f329852 Nov 25, 2016

@codecov-io

This comment has been minimized.

codecov-io commented Nov 25, 2016

Current coverage is 93.88% (diff: 100%)

Merging #6916 into v3 will increase coverage by <.01%

Powered by Codecov. Last update cf232e7...f329852

@lumaxis

This comment has been minimized.

Contributor

lumaxis commented Nov 25, 2016

@janmeier @felixfbecker Thanks! Looking good 🙂

@lumaxis lumaxis changed the title from Bugfix: mssql soft delete return to Bugfix: Return value in MSSQL soft-delete Nov 25, 2016

@lumaxis

This comment has been minimized.

Contributor

lumaxis commented Nov 29, 2016

@janmeier @felixfbecker What do you think about this change?

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Nov 29, 2016

PR to master?

@lumaxis lumaxis changed the title from Bugfix: Return value in MSSQL soft-delete to v3 Bugfix: Return value in MSSQL soft-delete Nov 30, 2016

@lumaxis lumaxis referenced this pull request Nov 30, 2016

Merged

Bugfix: Return value in MSSQL soft-delete #6930

4 of 5 tasks complete
@lumaxis

This comment has been minimized.

Contributor

lumaxis commented Nov 30, 2016

@felixfbecker Right, sorry! I don't use v4 personally but just opened #6930 which hopefully passes CI soon to 🙂

@sushantdhiman sushantdhiman merged commit 00cd521 into sequelize:v3 Nov 30, 2016

4 checks passed

codecov/patch 100% of diff hit (target 93.88%)
Details
codecov/project 93.88% (+<.01%) compared to cf232e7
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman

This comment has been minimized.

Member

sushantdhiman commented Nov 30, 2016

Master fix merged at #6930

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