CB-230: Migrate OAuth Client code off the ORM. #95
Conversation
ferbncode
force-pushed
the
migrate-oauth
branch
2 times, most recently
from
157e652
to
c9ed69a
Compare
critiquebrainz/db/oauth_client.py
Outdated
|
|
||
|
|
||
| def create(*, user_id, name, desc, website, redirect_uri): | ||
| """Create new OAuth client and generates secret key for it. |
There was a problem hiding this comment.
Should be either "Creates new OAuth client and generates a secret key for it." or "Create new OAuth client and generate a secret key for it."
critiquebrainz/db/oauth_client.py
Outdated
| } | ||
| """ | ||
| client_id = generate_string(20) | ||
| client_secret = generate_string(40) |
There was a problem hiding this comment.
These variables seem unnecessary. Calls to generate_string can be moved into the dictionary below.
I'd also move lengths into "constants" in this module: CLIENT_ID_LENGTH and CLIENT_SECRET_LENGTH.
critiquebrainz/db/oauth_client.py
Outdated
| :user_id, :name, :desc, :website) | ||
| """), client) | ||
|
|
||
| return client |
There was a problem hiding this comment.
Is the return value being used anywhere outside of tests? If not, you can remove it and move dictionary directly into the connection.execute call.
critiquebrainz/db/oauth_client.py
Outdated
| updates.append("name = :name") | ||
| update_data["name"] = name | ||
| if desc is not None: | ||
| updates.append(""" "desc" = :desc""") |
There was a problem hiding this comment.
""" should be used for multi-line strings and docstrings. Here you can use ' instead.
critiquebrainz/db/oauth_client.py
Outdated
| }) | ||
| row = result.fetchone() | ||
| if not row: | ||
| raise db_exceptions.NoDataFoundException("No client with ID: {} found".format(client_id)) |
There was a problem hiding this comment.
Minor thing, but I'd expect text after : to be just the ID. Can't find OAuth client with ID: %(id)s would be a better message, I think.
| website=form.website.data, redirect_uri=form.redirect_uri.data) | ||
| db_oauth_client.update( | ||
| client_id=application["client_id"], | ||
| name=form.name.data, desc=form.desc.data, |
There was a problem hiding this comment.
Don't stack arguments like that. It's easy to miss it if someone is expecting each to be on a new line.
| try: | ||
| application = db_oauth_client.get(client_id) | ||
| except db_exceptions.NoDataFoundException: | ||
| abort(404) |
There was a problem hiding this comment.
raise NotFound and abort(404) give pretty much the same result. Let's just stick with NotFound. Can also pass a message from the "parent" exception.
ferbncode
force-pushed
the
migrate-oauth
branch
from
548fdcc
to
b40018c
Compare
modify db.oauth_client.create and tests and syntax fixes
Added functions and tests to migrate code for
OAuthClientmodel. Also, made changes tofrontendfor using migrated code.