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 instance.set '1970-01-01' to null field #6839

Merged
merged 1 commit into from Nov 14, 2016

Conversation

6 participants
@neversun
Contributor

neversun commented Nov 11, 2016

Pull Request check-list

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

  • Does npm run test or npm run test-DIALECT 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?
  • Have you added an entry under Future in the changelog?

Description of change

Before this change it is not possible to use instance.set('date', '1970-01-01') when:

  • model has allowNull: true
  • instance retrieved from database and thus date having null as value.
@mention-bot

This comment has been minimized.

mention-bot commented Nov 11, 2016

@neversun, thanks for your PR! By analyzing the history of the files in this pull request, we identified @limianwang, @gutobortolozzo and @sushantdhiman to be potential reviewers.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 11, 2016

Current coverage is 93.85% (diff: 100%)

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

Powered by Codecov. Last update eff3852...2cc2165

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Nov 11, 2016

We need this for master first, otherwise LGTM.

@gutobortolozzo

This comment has been minimized.

Contributor

gutobortolozzo commented Nov 11, 2016

@neversun I would like to put two cents in your PR. I changed your code to

if (originalValue && !(originalValue instanceof Date)) {
  originalValue = new Date(originalValue);
}
if (originalValue && value.getTime() === originalValue.getTime()) {
 return this;
}

and tests looks to work just fine. What do you think about change your code to this shorter version?

Thank you

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Nov 11, 2016

It is not shorter in terms of code branches. You are testing the same condition twice, while the PR only tests it once.

@sushantdhiman sushantdhiman merged commit af3d9c1 into sequelize:v3 Nov 14, 2016

4 checks passed

codecov/patch 100% of diff hit (target 93.85%)
Details
codecov/project 93.85% (+<.01%) compared to eff3852
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment