Skip to content

Correct user token endpoints #212

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.

Sign up for GitHub

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

Jump to bottom
Merged
pallavi2209 merged 3 commits into from
Jan 24, 2017
Merged

Correct user token endpoints #212

pallavi2209 merged 3 commits into from
Jan 24, 2017

Conversation

@pallavi2209
Copy link
Contributor

@pallavi2209 pallavi2209 commented Jan 23, 2017

GET /v1/users/{key}/tokens now correctly returns tokens of given user, instead of all tokens
GET /v1/users/{key}/tokens/{tokenName} now correctly returns given token, instead of first token

@coveralls
Copy link

coveralls commented Jan 23, 2017

Coverage Status

Coverage increased (+0.08%) to 85.443% when pulling 996f50e on correct-get-user-tokens into 0710813 on master.

@coveralls
Copy link

coveralls commented Jan 23, 2017

Coverage Status

Coverage decreased (-0.02%) to 85.344% when pulling 26b811b on correct-get-user-tokens into 0710813 on master.

pallavi2209
pallavi2209 commented Jan 23, 2017
View reviewed changes
api/v1/controllers/userTokens.js
err.key = user + ', ' + tokenName;
throw err;
}

Copy link
Contributor Author

@pallavi2209 pallavi2209 Jan 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Let me add some tests for this check as well.

@pallavi2209 pallavi2209 changed the title correct user token endpoints DO NOT MERGE: correct user token endpoints Jan 23, 2017
@pallavi2209 pallavi2209 force-pushed the correct-get-user-tokens branch from 26b811b to 932a63e Compare January 23, 2017 21:45
@coveralls
Copy link

coveralls commented Jan 23, 2017

Coverage Status

Coverage decreased (-0.02%) to 85.344% when pulling 8e4d13d on correct-get-user-tokens into 90ce19c on master.

@pallavi2209 pallavi2209 force-pushed the correct-get-user-tokens branch 2 times, most recently from 0c2ac28 to f9739e7 Compare January 23, 2017 21:54
@coveralls
Copy link

coveralls commented Jan 23, 2017

Coverage Status

Coverage increased (+0.2%) to 85.607% when pulling f9739e7 on correct-get-user-tokens into 90ce19c on master.

@coveralls
Copy link

coveralls commented Jan 23, 2017

Coverage Status

Coverage increased (+0.2%) to 85.607% when pulling f9739e7 on correct-get-user-tokens into 90ce19c on master.

@pallavi2209 pallavi2209 changed the title DO NOT MERGE: correct user token endpoints Correct user token endpoints Jan 23, 2017
@pallavi2209
Copy link
Contributor Author

pallavi2209 commented Jan 23, 2017

@shriramshankar @iamigo PR open for comments.

iamigo
iamigo suggested changes Jan 23, 2017
View reviewed changes
api/v1/controllers/userTokens.js Outdated
];
whr[cnstnts.SEQ_OR][0]['user.name'][cnstnts.SEQ_LIKE] = user;
whr[cnstnts.SEQ_OR][1]['user.id'][cnstnts.SEQ_LIKE] = user;
if (u.looksLikeId()) {
Copy link
Contributor

@iamigo iamigo Jan 23, 2017

Choose a reason for hiding this comment

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

if (u.looksLikeId(userNameOrId)) {

Copy link
Contributor Author

@pallavi2209 pallavi2209 Jan 24, 2017

Choose a reason for hiding this comment

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

186059

@coveralls
Copy link

coveralls commented Jan 24, 2017

Coverage Status

Coverage increased (+0.3%) to 85.672% when pulling e851847 on correct-get-user-tokens into 90ce19c on master.

@pallavi2209
Copy link
Contributor Author

pallavi2209 commented Jan 24, 2017

@iamigo Corrected the uuid check.

shriramshankar
shriramshankar reviewed Jan 24, 2017
View reviewed changes
api/v1/controllers/userTokens.js
// need to use '$table.field$' for association fields
whr.where = { '$User.id$': {} };
whr.where['$User.id$'] = userNameOrId;
} else {
Copy link
Contributor

@shriramshankar shriramshankar Jan 24, 2017

Choose a reason for hiding this comment

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

aren't the where clauses defined something like this ?
where : {
User: {
id: "id"
}
}

Copy link
Contributor Author

@pallavi2209 pallavi2209 Jan 24, 2017

Choose a reason for hiding this comment

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

I don't think this is working given User is an association of token table.
{ where: { User: { id: 'a568a893-0571-42b9-aa55-ca44e1308346' } } } gives me 'val.replace is not a function' error.

@coveralls
Copy link

coveralls commented Jan 24, 2017

Coverage Status

Coverage increased (+0.3%) to 85.822% when pulling e73fc5e on correct-get-user-tokens into eb96a8a on master.

iamigo
iamigo reviewed Jan 24, 2017
View reviewed changes
api/v1/controllers/userTokens.js
// need to use '$table.field$' for association fields
whr.where = { '$User.id$': {} };
whr.where['$User.id$'][cnstnts.SEQ_LIKE] = userNameOrId;
whr.where['$User.id$'] = userNameOrId;
Copy link
Contributor

@iamigo iamigo Jan 24, 2017

Choose a reason for hiding this comment

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

Doing it this way instead of using $ilike means that we would no longer return a record with id 6da84a2a-f4b7-4ff1-b97b-56ce92501361 if we put 6da84a2a-F4B7-4ff1-b97b-56ce92501361 in the url because of uppercase/lowercase mismatch. I'm OK with that, and it's better for db performance, but then we should document that record ids in a url are case sensitive.

@pallavi2209 pallavi2209 force-pushed the correct-get-user-tokens branch from e73fc5e to aff0b84 Compare January 24, 2017 22:51
@coveralls
Copy link

coveralls commented Jan 24, 2017

Coverage Status

Coverage increased (+0.3%) to 85.822% when pulling aff0b84 on correct-get-user-tokens into dfcf439 on master.

@pallavi2209
Copy link
Contributor Author

pallavi2209 commented Jan 24, 2017

@iamigo Added documentation in swagger yaml.

@pallavi2209 pallavi2209 merged commit edfafac into master Jan 24, 2017
@pallavi2209 pallavi2209 deleted the correct-get-user-tokens branch January 24, 2017 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@iamigo iamigo iamigo approved these changes

@shriramshankar shriramshankar shriramshankar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pallavi2209 @coveralls @iamigo @shriramshankar