-
Notifications
You must be signed in to change notification settings - Fork 351
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
Support hosts (e.g. notify) and auth (e.g. noauth) #79
Conversation
| .type('application/json'); | ||
|
|
||
| switch (auth) { | ||
| case 'user': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the team auth case? I suspect this breaks the DropboxTeam client. Did the test coverage not show this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't realize team routes were supported at all. I now see team-routes.js, which apparently didn't get regenerated. (I used grep to make sure that everything in my diff used either "user" or "noauth" auth, but the failure here was that my diff didn't actually regenerate everything.)
No tests from npm test failed... odd. I'll investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had missed a change in generate_routes.py; that's why the team routes weren't changed. I fixed that and fixed up one resulting test failure.
Now I realize there's not a lot of testing around the team endpoints. :-) (Notably, nothing tests that they have an auth header.) That makes me a little nervous about merging this, but I also don't really want to spend a lot of time testing. Is there someone else who can add some tests or at least manually verify that the basics are working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just used ran the example server and tested out the team routes. Looks good. Have you tried out the longpoll route?
| @@ -12,7 +11,7 @@ var buildCustomError = function (error, response) { | |||
| }; | |||
| }; | |||
|
|
|||
| var uploadRequest = function (path, args, accessToken, selectUser) { | |||
| var uploadRequest = function (path, args, auth, host, accessToken, selectUser) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert that auth is user.
|
Just one small comment. Squash and rebase and it should be good to go assuming that you've actually tried this on a longpoll route. |
|
Rebased, tested upload/download (found a typo!), and retested that longpoll actually works. (It does.) |
a6e556b
to
3f3934d
Compare
This makes longpoll work. It fixes #42 and #55.