Implement "Add user setting to choose between words based random alias and uuid alias" #5
Conversation
The codebase used dataclass library which is Py3.7+ only
To visually show the order of migrations
| class="btn btn-success">Random Alias | ||
| </button> | ||
| </form> | ||
| <div class="btn-group" role="group"> |
There was a problem hiding this comment.
the indentation doesn't look right here, maybe your IDE mixes up space and tab?
| <div class="small-text mb-3">Choose how to create your email alias by default</div> | ||
| <form method="post" class="form-inline"> | ||
| <input type="hidden" name="form-name" value="change-alias-generator"> | ||
| <select class="custom-select mr-sm-2" name="alias-generator-scheme" id="alias-generator-scheme"> |
There was a problem hiding this comment.
the id="alias-generator-scheme" can be removed here I think
app/models.py
Outdated
| class User(db.Model, ModelMixin, UserMixin): | ||
| __tablename__ = "users" | ||
| email = db.Column(db.String(128), unique=True, nullable=False) | ||
| salt = db.Column(db.String(128), nullable=False) | ||
| password = db.Column(db.String(128), nullable=False) | ||
| name = db.Column(db.String(128), nullable=False) | ||
| is_admin = db.Column(db.Boolean, nullable=False, default=False) | ||
| alias_generator = db.Column(db.Integer, nullable=True, default=AliasGeneratorEnum.word.value) |
There was a problem hiding this comment.
Maybe we can use nullable=False here? A field is easier to work with when it's non-nullable, that's why fields are generally non-nullable. Exception made for the ones that null value makes sense like profile_picture_id, custom_domain_id.
app/models.py
Outdated
| def generate_email() -> str: | ||
| """generate an email address that does not exist before""" | ||
| random_email = random_words() + "@" + EMAIL_DOMAIN | ||
| def generate_email(scheme: int = 1, in_hex: bool = False) -> str: |
There was a problem hiding this comment.
scheme: int = 1 could be replaced by scheme: int = AliasGeneratorEnum.word. The reason for that is enum should be used instead of its value.
|
Overall the PR looks good! I have checkout it locally and tested, the feature works well 👍. I just left some comments, some might be a bit "nitpicking" as I try to minimize the code change as much as possible ... Don't hesitate to use |
No description provided.