-
Notifications
You must be signed in to change notification settings - Fork 1
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
new manager option to create admin user with prompts #1
Conversation
Or do we want to reference something like this from setup_dev because the only reason you should be inserting roles is if you've cleared the database? |
Would you want to move this into the
|
What do you mean by
|
We modified user creation so that the first user created is the admin. AFAIK this is a pretty standard practice |
I personally prefer option 3 actually, but that's a good idea Dubin. Let's leave this out of flask-base for now and come together on a solution at the end of the semester. |
I'm going to implement option 2. I like option 3 as well, but I don't feel great about having something like this that entrenched in the code when we aren't set on it as a solution for everything. |
Can you guys just give it a quick once over before I merge it, thanks. |
Sure. You should have someone from your team review too! |
@@ -26,6 +26,21 @@ def make_shell_context(): | |||
manager.add_command('db', MigrateCommand) | |||
|
|||
|
|||
def create_default_admin(first_name, last_name, email, password): | |||
if (User.query.filter_by(email=email).first() is not None): |
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.
Nit: parenthesis around statements are not Pythonic
Did flake8 not get mad about 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.
It's actually not a pep8 thing, just a pycharm thing. But you're right, they are redundant.
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.
should be, let's amend pep8
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.
Related: more pythonic would be if not User.query.filter_by(email=email).first():
Whether it's more/less readable, I'll let you decide
Left a few small comments, besides that lgtm |
Thanks guys, I appreciate it. |
@@ -64,6 +64,13 @@ def setup_dev(): | |||
"""Runs the set-up needed for local development.""" | |||
setup_general() | |||
|
|||
admin_email = 'wvr@gmail.com' | |||
if User.query.filter_by(email=admin_email).first() is None: |
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.
@jondubin what he currently has is better than if not User.query.filter_by(email=email).first():
From PEP8:
Comparisons to singletons like None should always be done with 'is' or 'is not', never the equality operators.
Also, beware of writing "if x" when you really mean "if x is not None" -- e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!
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.
setup_dev option in manager creates a hardcoded confirmed admin user if they don't exist
@sandlerben can you take a look. Having to create a function like this might be the tipping point for me in terms of us possibly thinking about another workflow for when someone starts coding.