Skip to content
This repository has been archived by the owner. It is now read-only.

Return `200` for all logout requests #42

Merged
merged 10 commits into from Apr 30, 2014
Merged

Conversation

@amargherio
Copy link
Contributor

amargherio commented Apr 24, 2014

This commit should address #33 (/logout never needs to return 401). I'm still kinda new to Javascript, but I'd really like to be able to contribute to this project. If any changes need to be made to this PR, please let me know!

As an aside - should the requests be returning 201 or 200? 201 indicates that a new resource has been created while 200 just indicates success.

amargherio added 2 commits Apr 24, 2014
… since the session no longer exists if the verifyToken() comes back bad, there's no reason to return a client error response.
…dify test cases to fit new status code returns.
@cheddar
Copy link
Contributor

cheddar commented Apr 24, 2014

You are right about 201/200. I had originally said 201, just because I am used to POST requests returning 201, but this is a 200 case.

if (result.statuscode == 200) {
userService.deleteToken(result.token, function (err, result) {
userService.deleteToken(result.token, function(err, result) {
res.send(result.statuscode);

This comment has been minimized.

Copy link
@cheddar

cheddar Apr 24, 2014

Contributor

It's possible that this code also generates a non-200 response. We also are not logging errors. Can you add some lines to log an error if it exists (there should be a log object available in the userapi.js file already) and also to log if we get a non-200 from the deleteToken call?

Then just switch this to a blanket

res.send(200);
return next();
@amargherio
Copy link
Contributor Author

amargherio commented Apr 24, 2014

Will do. I also wanted to look into adding some sad path test cases for the status codes returned from logouts, although I don't know that there would be a significant benefit from having them.

@cheddar
Copy link
Contributor

cheddar commented Apr 24, 2014

More test cases are rarely a bad thing :).

Btw, I forgot to mention, but welcome and thanks for the contribution!

There's gonna be one bit of administrative thing that we are going to have to do before we can accept the contribution as well: get agreement to a CLA. Here's the canned text for that ;).

Before I can merge the pull request, we need you to agree to our CLA. You can find the CLA at http://tidepool-org.github.io/TidepoolCLA.pdf.

If you are working on your own behalf and agree to the CLA, responding here with a comment of "I agree to the terms of Tidepool Project's Contributor License Agreement as it exists at http://tidepool-org.github.io/TidepoolCLA.pdf on " is sufficient. If your contributions are on behalf of a company, can you have your company fill out the corporate CLA and submit it to us?

@jh-bate
Copy link
Contributor

jh-bate commented Apr 24, 2014

Hi Adam,

just as Eric said welcome and thanks for thanking the time to help us out.

Cheers
Jamie

On Fri, Apr 25, 2014 at 6:57 AM, cheddar notifications@github.com wrote:

More test cases are rarely a bad thing :).

Btw, I forgot to mention, but welcome and thanks for the contribution!

There's gonna be one bit of administrative thing that we are going to have
to do before we can accept the contribution as well: get agreement to a
CLA. Here's the canned text for that ;).

Before I can merge the pull request, we need you to agree to our CLA. You
can find the CLA at http://tidepool-org.github.io/TidepoolCLA.pdf.

If you are working on your own behalf and agree to the CLA, responding
here with a comment of "I agree to the terms of Tidepool Project's
Contributor License Agreement as it exists at
http://tidepool-org.github.io/TidepoolCLA.pdf on " is sufficient. If your
contributions are on behalf of a company, can you have your company fill
out the corporate CLA and submit it to us?

Reply to this email directly or view it on GitHubhttps://github.com//pull/42#issuecomment-41318454
.

@amargherio
Copy link
Contributor Author

amargherio commented Apr 24, 2014

Fair enough. It might take me a bit to get some good test cases setup (never done pure Javascript development before - coming from a Java/Scala/Go/Ruby-ish background) but I'll see what I can do.

Thank you for the warm welcome! I'm actually a T1 diabetic, so something like this hits close to home for me. I've been considering giving a Dexcom a try, and with that, thinking about tooling I could put in place to help me manage the data side a bit better. After hearing about this, I decided I wanted to try to contribute as much as I can.

As for the CLA, since I'm working as an individual - I agree to the terms of Tidepool Project's Contributor License Agreement as it exists as http://tidepool-org.github.io/TidepoolCLA.pdf on April 24, 2014.

I've also been in and out of IRC as MarioEIU.

@cheddar
Copy link
Contributor

cheddar commented Apr 24, 2014

I come from the same background (actually, a lot more Java than anything else ;)), also T1D (most of us are), no worries on the time. If you want, I'd merge it just with the changes to logging and making it consistent. Can leave the unit tests for a follow-on. That's always a slippery-slope though ;).

If you think you'll have time for the unit tests, great, I'll wait. If you think that something might get in the way, then let's just get the functionality in and leave the unit tests.

Great to have you!

@@ -452,16 +452,22 @@ module.exports = function (envConfig, userService) {
};

var logout = function (req, res, next) {
verifyToken(req, function (err, result) {
verifyToken(req, function(err, result) {
if (err) {

This comment has been minimized.

Copy link
@cheddar

cheddar Apr 27, 2014

Contributor

I don't remember if the rest of the code here uses truthyness, but in general I think we should try to stay away from it as much as possible

err != null

Is what should be here instead.

verifyToken(req, function (err, result) {
verifyToken(req, function(err, result) {
if (err) {
log.error(err);

This comment has been minimized.

Copy link
@cheddar

cheddar Apr 27, 2014

Contributor

Can you add some text here and switch to warn? Sorry I didn't specify that before. Something like

log.warn(err, 'Error verifying token on logout');
log.error(err);
}
if (result.statuscode != 200) {
log.warn('Received non-200 status code from deleteToken');

This comment has been minimized.

Copy link
@cheddar

cheddar Apr 27, 2014

Contributor

Can you make this include the status code too?

log.warn('Received non-200 status code[%s] from deleteToken', result.statuscode);
verifyToken(req, function (err, result) {
verifyToken(req, function(err, result) {
if (err != null) {
log.warn('Unable to verify token. Error: [%s]', err);

This comment has been minimized.

Copy link
@cheddar

cheddar Apr 29, 2014

Contributor

Much like log4j, et al, our logger understands error objects and handles them specially, it's actually best to put the err first, as in

log.warn(err, 'Unable to verify token.');

This comment has been minimized.

Copy link
@cheddar

cheddar Apr 29, 2014

Contributor

If you change this line to what I have and fix it for the deleteToken call too, I'll go ahead on merge. Sorry for the back and forth.

@cheddar
Copy link
Contributor

cheddar commented Apr 29, 2014

@amargherio One very last little bit of stuff. Sorry the logger stuff wasn't that explicit (the lack of function signatures makes it more difficult...)

Also, when you push the next bit of code, could you comment on the PR? It took me a while to realize that you had done more even though you had actually asked about it in chat...

@amargherio
Copy link
Contributor Author

amargherio commented Apr 30, 2014

@cheddar thanks for the additional comments on the commits. that's my inexperience in javascript shining through.

this most recent commit should pass the travis build.

cheddar added a commit that referenced this pull request Apr 30, 2014
Return `200` for all logout requests
@cheddar cheddar merged commit d25a875 into tidepool-org:master Apr 30, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.