-
Notifications
You must be signed in to change notification settings - Fork 76
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: A bug fix, issue 266 #284
fix: A bug fix, issue 266 #284
Conversation
Description: Store the fields that have been ordered by with (and throw if you try to orderBy twice in the same field) instead of just checking if any fields have been ordered by. An orderBy function cannot be called more than once in the same query expression, except on different fields. Fixing issue wovalle#266
Codecov Report
@@ Coverage Diff @@
## master #284 +/- ##
==========================================
+ Coverage 95.46% 95.67% +0.20%
==========================================
Files 26 26
Lines 662 671 +9
Branches 108 112 +4
==========================================
+ Hits 632 642 +10
+ Misses 29 28 -1
Partials 1 1
Continue to review full report at Codecov.
|
src/BaseFirestoreRepository.spec.ts
Outdated
const albumsSubColl = pt.albums; | ||
expect(() => { | ||
albumsSubColl.orderByAscending('releaseDate').orderByDescending('name'); | ||
}).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you assert that calling orderBy* methods not.toThrow()
instead of toBeTruthy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense!
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Just two tiny comments and we can merge this one.
Thanks for the contribution!
src/BaseFirestoreRepository.spec.ts
Outdated
const albumsSubColl = pt.albums; | ||
expect(() => { | ||
albumsSubColl.orderByAscending('releaseDate').orderByDescending('name'); | ||
}).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/BaseFirestoreRepository.spec.ts
Outdated
const albumsSubColl = pt.albums; | ||
expect(() => { | ||
albumsSubColl.orderByAscending('releaseDate').orderByDescending('name'); | ||
}).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add two tests where you call the same orderBy
method twice with different fields, such as:
expect(() => {
albumsSubColl.orderByAscending('releaseDate').orderByAscending('name');
}).not.toThrow();
expect(() => {
albumsSubColl. orderByDescending('releaseDate'). orderByDescending('name');
}).not.toThrow();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Job!
@all-contributors please add @jomendez for code |
I've put up a pull request to add @jomendez! 🎉 |
🎉 This PR is included in version 0.23.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description:
Store the fields that have been ordered by with (and throw if you try to
orderBy twice in the same field) instead of just checking if any fields
have been ordered by.
An orderBy function cannot be called more than once in the same query
expression, except in different fields.
Fixing issue #266