This repository has been archived by the owner on Dec 17, 2018. It is now read-only.
Support $select in get and make sure patch many only returns changed items #32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daffl
commented
Nov 8, 2016
| @@ -20,7 +20,8 @@ function common (app, errors, serviceName = 'people', idProp = 'id') { | |||
| ); | |||
|
|
|||
| it('sets `events` property from options', () => | |||
| expect(app.service(serviceName).events).to.deep.equal([ 'testing' ]) | |||
| expect(app.service(serviceName).events.indexOf('testing')) | |||
| .to.not.equal(-1) | |||
There was a problem hiding this comment.
It's checking for Array index in case we already have existing events (the case e.g. in RethinkDB)
daffl
commented
Nov 8, 2016
| @@ -522,20 +535,23 @@ function common (app, errors, serviceName = 'people', idProp = 'id') { | |||
| }).then(() => service.remove(null, params)); | |||
| }); | |||
|
|
|||
| it('patches multiple even if query changed', () => { | |||
| it('patches multiple, returns correct items', () => { | |||
| const service = app.service(serviceName); | |||
There was a problem hiding this comment.
The current (buggy) implementations make just another find with the changed query which is not correct. What they really need to do is find everything before making the database update and then do another find with the list of all ids from the previous find. This tests covers that.
|
Technically $select should be possible on those methods. I think they are unlikely cases except for update but if we're already here it would make sense to add the tests for them. |
|
If you want to include that in this PR go for it. Otherwise |
daffl
changed the title
|
I added tests for all other methods now as well. |
|
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to feathersjs/feathers#450 and feathersjs-ecosystem/feathers-mongoose#124 (comment)
Closes #25
@feathersjs/core-team Should $select be tested and supported for
patch,update,removeandcreateas well?