(#101) Validate usernames across cli & tui #115
No reviewers
Labels
No Label
priority
high
priority
low
priority
medium
priority
very high
BUG
Feature
High Priority
Low Priority
Medium Priority
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: public/pyceo#115
Loading…
Reference in New Issue
No description provided.
Delete Branch "101-username-validation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Current changes should address issues raised by @merenber in #114 excluding #114 (comment) (both CLI and TUI validation)
A few minor comments, but looks great overall. Thank you for taking the initiative to add this feature!
@ -53,1 +56,4 @@
return datetime.datetime.now()
class UsernameValidationResult:
This can be simplified with Python's dataclass decorator:
@ -0,0 +2,4 @@
def test_validate_username():
assert utils.validate_username('').__eq__(utils.UsernameValidationResult(False, 'Username must not be empty'))
Why are we using
__eq__
here? Why not just==
?Since the TUI verifies the usernames for every input, I coped that behaviour over to the CLI parts, should they also be applied to the API? Or is it fine to leave it as only verifiying for new users?
The same username validation logic should be applied to the /members POST endpoint:
https://git.csclub.uwaterloo.ca/public/pyceo/src/branch/master/ceod/api/members.py#L26
I just checked the CI logs, it looks like the tests are failing because some of the tests use underscores in the usernames ... ugh. I think it's OK to allow underscores in the username regex for now. Someone can open a separate PR later to clean that up.
Cool, should be done in
3f751ccece
And everything should be good now
WIP: (#101) Validate usernames across cli & tuito (#101) Validate usernames across cli & tui@ -105,6 +112,11 @@ def print_user_lines(d: Dict):
@members.command(short_help='Get info about a user')
@click.argument('username')
def get(username):
# Verify that the username is valid before requesting data from API
We don't need to validate the username for get/modify/renew/pwreset/delete, because the client will get a 404 if the username is invalid. We only need to validate the username when adding new members.
Should this also be done for the TUI? Currently it validates under Controller.get_username_from_view() which is shared across all/most options
Nah, I think that should be fine. I just didn't want to duplicate code if it wasn't necessary.
Could we also please add a few unit tests for the API (under tests/ceod/api)? Just as a sanity check.
@ -135,6 +135,40 @@ def test_api_create_user_without_forwarding_addresses(cfg, client):
assert data['error'] == "BadRequest: Attribute 'forwarding_addresses' is missing or empty"
def test_api_create_user_without_valid_username(cfg, client):
test_api_create_user_without_valid_username should probably be expanded on with the _ fix mentioned here: #115 (comment)