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-1604: Update a user causes 401 on subsequent API requests #101

Merged
merged 5 commits into from Jan 18, 2017

Conversation

@thomasconner
Copy link
Contributor

commented Jan 17, 2017

Description

This PR updates the active user data if a PUT request is made to update the user on the backend.

Changes

  • Update active user data with data returned from PUT request made on the active user to the backend.
  • Add unit tests for updating a user.
@tejasranade
Copy link
Member

left a comment

Minor updates requested, but looks good otherwise.

if (isActive) {
return CacheRequest.setActiveUser(this.client, this.data);
.then((data) => {
if (this.isActive() === true) {

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 18, 2017

Member

Redundant ===? :)

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 18, 2017

Author Contributor

Yes. I will remove.

@@ -51,7 +51,7 @@ class UserStore extends NetworkStore {
}

if (!data._id) {
return Promise.ject(new KinveyError('User must have an _id.'));
return Promise.reject(new KinveyError('User must have an _id.'));

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 18, 2017

Member

A test to cover this error handling case would be nice.

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 18, 2017

Author Contributor

A unit test has already been written to cover this case.

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 18, 2017

Member

Oops 😃


return user.update({ email: email })
.then(() => {
expect(user.data).toEqual(responseData);

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 18, 2017

Member

Does the updated user object, in this case, contain the auth token? I'd assume it doesn't. In either case, can we add an assert for the correct behavior?

This comment has been minimized.

Copy link
@thomasconner

thomasconner Jan 18, 2017

Author Contributor

This line is where the authtoken is recreated. This line verifies the active user contains the new data including the new authtoken.

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 18, 2017

Member

Right, if I understand correctly, that's testing the case where you update the active user. What about the other case i.e. a user that's not the active user? Is the token in the response still?

@codecov-io

This comment has been minimized.

Copy link

commented Jan 18, 2017

Current coverage is 74.70% (diff: 75.00%)

Merging #101 into master will increase coverage by 1.07%

@@             master       #101   diff @@
==========================================
  Files            75         75          
  Lines          6631       6631          
  Methods         892        891     -1   
  Messages          0          0          
  Branches       1036       1038     +2   
==========================================
+ Hits           4883       4954    +71   
+ Misses         1748       1677    -71   
  Partials          0          0          

Powered by Codecov. Last update 92a05c8...aeb693a

MLIBZ-1604 Ensure that the authtoken is correct
When updating a non active user, ensure the authtoken (if one is provided) in the response to the PUT request is only saved to the user that is updated and not the active user.
@tejasranade
Copy link
Member

left a comment

LGTM!


return user.update({ email: email })
.then(() => {
expect(user.data).toEqual(responseData);

This comment has been minimized.

Copy link
@tejasranade

tejasranade Jan 18, 2017

Member

Right, if I understand correctly, that's testing the case where you update the active user. What about the other case i.e. a user that's not the active user? Is the token in the response still?

@thomasconner thomasconner merged commit 9bb3fee into master Jan 18, 2017

4 checks passed

codeclimate no new or fixed issues
Details
codecov/patch 75.00% of diff hit (target 73.63%)
Details
codecov/project 74.70% (+1.07%) compared to 92a05c8
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@thomasconner thomasconner deleted the MLIBZ-1604_Update_User 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
5 participants
You can’t perform that action at this time.