-
Notifications
You must be signed in to change notification settings - Fork 19
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
Allow an admin to create a new project #120
Conversation
|
||
try: | ||
admin_repo.create_project(project_name, is_microsetta) | ||
except: # noqa |
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.
Yes qa! What exception does it throw? You can take the unique constraint violation code out of the other repo classes.
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.
Also, wrapping psycopg2 errors in RepoException and re-throwing them in the repo layer will automatically trigger 422s to be returned, which is convenient when you need to specify an error message close to the source
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.
Ah okay so dont need to wrap this business in a try/except then right?
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.
If its specifically a RepoException, it will turn into a 422 from the registered handler in server.py. If its a different type of exception underneath, it'll do whatever is registered, or 500. That lets you 404 from the repo layer with NotFound or throw 422s with RepoException. You still have to catch the psycopg2 unique violations explicitly, but I do that at the repo layer and reraise them as RepoExceptions usually.
@@ -0,0 +1,10 @@ | |||
ALTER TABLE barcodes.project ADD COLUMN is_microsetta VARCHAR; |
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.
Why VARCHAR with Yes/No rather than the Postgres BOOLEAN type? Then we wouldn't need to think about capitalization, which has burned us in a previous "Yes/no" field.
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.
Not an issue, happy to address
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 really dislike the is_microsetta Yes/No field without further justification. I'd much prefer either a boolean or renaming to "microsetta" and explicitly enumerating all possible values. Whether or not the 422s are fired from exceptions is totally optional.
As the subject says. This PR also scopes to denote whether is project is to be considered a microsetta project.