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: add missing fields to 'FindOrCreateType' #12338

Merged
merged 2 commits into from Jun 7, 2020

Conversation

sonirico
Copy link
Contributor

@sonirico sonirico commented Jun 3, 2020

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 the description below contain a link to an 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)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

This PR includes some typings needed to provide clarification in regards to the usage of findOrCreate functionality, thus the FindOrCreateOptions interface has been altered to include the fields string array to whitelist which fields should be persisted as explained in #12325 . Tests are not passing, as well as in master upstream.

@sonirico sonirico changed the title fix: add missing fields to 'FindOrCreateType fix: add missing fields to 'FindOrCreateType' Jun 3, 2020
@codecov
Copy link

@codecov codecov bot commented Jun 3, 2020

Codecov Report

Merging #12338 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12338   +/-   ##
=======================================
  Coverage   96.40%   96.40%           
=======================================
  Files          95       95           
  Lines        9116     9116           
=======================================
  Hits         8788     8788           
  Misses        328      328           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f367191...ab074ca. Read the comment docs.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Jun 4, 2020

@sushantdhiman sushantdhiman added the status: awaiting response label Jun 4, 2020
@sonirico
Copy link
Contributor Author

@sonirico sonirico commented Jun 4, 2020

@sushantdhiman I've added a handful of tests 👍

}
})

if (! created) {
Copy link
Contributor

@sushantdhiman sushantdhiman Jun 5, 2020

Choose a reason for hiding this comment

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

These tests are supposed to only test syntax, not actually doing any db operations. I don't think these conditions or checks should be included in this code.

Just add some calls to <Model>.findOrCreate to assert correct typings or move these to e2e if you prefer that

Copy link
Contributor

@SimonSchick SimonSchick Jun 6, 2020

Choose a reason for hiding this comment

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

Yes these tests are not actually executed, they are merely type checked.

Copy link
Contributor Author

@sonirico sonirico Jun 6, 2020

Choose a reason for hiding this comment

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

Ok, I have added only the typings because I have seen enough tests for findOrCreate in the integrations folder. Thank you for your guidance!

types/test/model.ts Outdated Show resolved Hide resolved
@sushantdhiman sushantdhiman merged commit 72925cf into sequelize:master Jun 7, 2020
0 of 2 checks passed
@sushantdhiman sushantdhiman removed the status: awaiting response label Jun 7, 2020
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.

None yet

3 participants