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

MLIBZ-1614: User Discovery #96

Merged
merged 7 commits into from Jan 18, 2017

Conversation

@thomasconner
Copy link
Contributor

commented Jan 16, 2017

Description

This PR adds the ability to discover users using the SDK.

Changes

  • Override the find() function in the UserStore class to use the user discover api.
  • Add a static find() function to the User class that proxy calls to the UserStore.
  • Default all options to an empty object on the User class. Fixes MLIBZ-1579.
  • Add unit tests for user discovery.
MLIBZ-1614 Add user discover
Override the find function on the UserStore to allow user discovery as described at http://devcenter.kinvey.com/guides/users#lookup. Add a static find function to user class that proxy calls to UserStore.
@codecov-io

This comment has been minimized.

Copy link

commented Jan 16, 2017

Current coverage is 73.55% (diff: 60.00%)

Merging #96 into master will decrease coverage by 0.08%

@@             master        #96   diff @@
==========================================
  Files            75         75          
  Lines          6631       6692    +61   
  Methods         892        900     +8   
  Messages          0          0          
  Branches       1036       1063    +27   
==========================================
+ Hits           4883       4922    +39   
- Misses         1748       1770    +22   
  Partials          0          0          

Powered by Codecov. Last update 92a05c8...5730aaf

// If `options.discover`, use
// [User Discovery](http://devcenter.kinvey.com/guides/users#lookup)
// instead of querying the user namespace directly.
if (options.discover === true) {

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 16, 2017

Member

@thomasconner Why are we overloading the user.find method? On iOS and Xamarin, for example, we have a separate method called User.lookup(). Since the REST API used for lookup is different, I'd prefer if we separated the methods.
Btw, what is find() on users supposed to do? If it's a supported operation, we need unit tests to cover the expected result.

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 16, 2017

Author Contributor

In 1.x the lookup was performed using user.find. I think it makes more sense to separate it out but this is why I did it this way.

As for what a find does on the user collection, I assume it performs a find like it would on any other collection. Again looking at 1.x you will see that it contains code to perform a regular find. https://github.com/Kinvey/js-sdk/blob/1.x/src/core/user.js#L581.

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 16, 2017

Author Contributor

Since this a super call to the implementation for find() on the NetworkStore do we need separate unit tests for the UserStore? The only change would be that the pathname is overridden. We could need 1 unit test to check this.

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 16, 2017

Member

If you agree that separating the two methods them makes sense, let's do it. I think it reduces confusion for the end user, and it's better to be consistent with other v3 SDKs as compared to JS v1.

The unit test on find() was to clearly define what the expected behavior is (if we say find is a supported method on the UserStore). I suspect there's at least some difference in how it behaves on users vs data, but I'm not sure either.

* @param {boolean} [options.discover] Discover users.
* @return {Observable} Observable.
*/
static find(query, options = {}) {

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 16, 2017

Member

Same comment as about find vs lookup. Also a side note - it's probably easier for us to be prescriptive about the methods that devs should use - i.e. we either expose this on UserStore or User. Maintaining both seems redundant and unnecessary overhead.

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 16, 2017

Author Contributor

The UserStore extends the NetworkStore which provides some implementation that would otherwise be duplicated in the User class. However, I think at this point we should just expose the User class that is used to proxy the calls to the UserStore class.

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 16, 2017

Member

That makes sense, as long as we do it consistently for all UserStore calls, and also deprecate the currently documented ones until devs switch over.

MLIBZ-1614 Add lookup API
Add a lookup API function to do user discovery. Remove user discovery from a normal user find.
* @param {Object} [options] Options
* @return {Observable} Observable.
*/
static find(query, options = {}) {

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 18, 2017

Member

Let's remove this until we know what we're expecting.

@tejasranade
Copy link
Member

left a comment

LGTM! Please add a devcenter PR to go with this.

@thomasconner thomasconner merged commit 31335d0 into master Jan 18, 2017

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details
codeclimate no new or fixed issues
Details

@thomasconner thomasconner deleted the MLIBZ-1614_User_Discovery branch Jan 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.