Skip to content

fix #337 #341

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

Merged
merged 2 commits into from
Jun 14, 2021
Merged

fix #337 #341

merged 2 commits into from
Jun 14, 2021

Conversation

Comment on lines 522 to 523
const workPeriods = await WorkPeriod.findAndCountAll(queryCriteria)
const total = await WorkPeriod.count({ where: filter })
Copy link
Contributor

Choose a reason for hiding this comment

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

@xxcxy I didn't know such a method exists. I wonder why you didn't use the output from this method and call await WorkPeriod.count({ where: filter }) anyway below?

Can we just call findAndCountAll?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strange, I forgot to change this. I tried findAndCountAll, the count returned by this method is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

@maxceem @xxcxy
Fyi, findAndCountAll is safe when we don't have include option in query. Otherwise it gives wrong count number and becomes complex to solve.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @eisbilir. I think than it’s better not use it, because if we use it one place then new developer can copy paste it to a new place with include and would not notice that it works incorrectly there.

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@xxcxy looks good.

@maxceem maxceem merged commit bf00b25 into topcoder-platform:dev Jun 14, 2021
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.

3 participants