Partially implement oauth #69
Conversation
There was a problem hiding this comment.
Thanks for getting this started! This is one of the trickiest parts of the solution and there isn't much of a codebase to work with yet, so this can't have been a terribly straightforward PR to put together.
I left some "nit" comments, which are mostly pedantic style things, some questions/suggestions, and some small things I'd like to see changed.
| if !isNotFoundError { | ||
| return nil, result.Error | ||
| } | ||
| return nil, nil |
There was a problem hiding this comment.
Generally if the function did was not successful, it should return some kind of error; returning nil for the error generally implies success. Why do we want to return nil in this case? Maybe there's a compromise that still returns some kind of error here.
There was a problem hiding this comment.
Im not entirely sure what our policy should be here. This is technically not an error as not find a user is perfectly expected. I think the only reason gorm errors is because it attempts to update a pointer and theres no easy to say that 'failed'.
There was a problem hiding this comment.
I'd say that in this function that we call GetUserByTwitterID, I'd expect to find a user. And since we couldn't find a user with that Twitter ID, you could reasonably throw an error. Maybe we can have a UserNotFoundError which we return in both cases where Gorm errors (i.e. gorm.ErrRecordNotFound) and when a record simply doesn't exist.
robotmayo
force-pushed
the
gh10-implement-oauth
branch
from
e2bbeda
to
eba9ca6
Compare
zysim
force-pushed
the
gh10-implement-oauth
branch
from
aba43a8
to
af64402
Compare
Our errors in oauth_test.go are immediately checked for now, so we're sticking to calling em "err".
* Restructured Models, added Models from all PRs * Add Users integration tests * Add Integration Test setup * Add IntegrationTest project * Include .env in build for integration test project * Use `Program.cs` as entry point * Use `Staging` environment for integration tests * db_test now mounts the correct port on the container * Adjust env var for test port (test DB will now share name) * Add Users integration tests * Add Leaderboard Integration Tests * Making the tests work with rebases
This PR mostly address #10
This PR lays down the basic foundation for OAUTH support. Right now it doesn't create or do anything with JWTs because there is currently no standard way to create/manipulate JWTs. I thought about adding that but this PR is already quite large so I wanted to cut scope wherever I could. As noted in the a few of the comments there are somethings that we could use such as a standard JSON out and Error response interface. Something like error codes(and types) would also be helpful at simplifying our error responses and make testing a bit better since we would no longer have to check the human readable strings.
Another noted issue is avoiding talking to gorm/database directly and use functions to fetch and write to the database. Another PR is working on something similar (#60). Im also not super great on the tests as its been a while since I have written production level Go advice in this area would be greatly appreciated.