Address #101 #114
|
@ -1,6 +1,7 @@
|
|||
from abc import ABC
|
||||
|
||||
import ceo.tui.utils as utils
|
||||
from ...utils import validate_username
|
||||
|
||||
|
||||
# NOTE: one controller can control multiple views,
|
||||
|
@ -52,8 +53,9 @@ class Controller(ABC):
|
|||
def get_username_from_view(self):
|
||||
username = self.view.username_edit.edit_text
|
||||
# TODO: share validation logic between CLI and TUI
|
||||
if not username:
|
||||
self.view.popup('Username must not be empty')
|
||||
verification_res = validate_username(username)
|
||||
|
||||
if verification_res:
|
||||
self.view.popup(verification_res)
|
||||
raise Controller.InvalidInput()
|
||||
return username
|
||||
|
||||
|
|
13
ceo/utils.py
|
@ -1,9 +1,10 @@
|
|||
import functools
|
||||
import json
|
||||
import os
|
||||
from typing import List, Dict, Tuple, Callable
|
||||
from typing import List, Dict, Tuple, Callable, Union
|
||||
|
||||
import requests
|
||||
import re
|
||||
from zope import component
|
||||
|
||||
from .StreamResponseHandler import StreamResponseHandler
|
||||
|
@ -11,6 +12,8 @@ from ceo_common.interfaces import IHTTPClient, IConfig
|
|||
from ceo_common.model import Term
|
||||
from ceod.transactions.members import AddMemberTransaction
|
||||
|
||||
VALID_USERNAME_RE = re.compile(r"^[a-z0-9]+[a-z0-9-]+$")
|
||||
merenber
commented
We don't need the first "+" here. Also, the first character should always be a letter. We don't need the first "+" here. Also, the first character should always be a letter.
|
||||
|
||||
|
||||
def http_request(method: str, path: str, **kwargs) -> requests.Response:
|
||||
client = component.getUtility(IHTTPClient)
|
||||
|
@ -266,3 +269,11 @@ From other CSC machines you can connect using
|
|||
return True
|
||||
except PermissionError:
|
||||
return False
|
||||
|
||||
|
||||
def validate_username(username: str) -> Union[str, None]:
|
||||
merenber
commented
This is a nitpick, but it's not clear from the name of this function, nor from its signature, what it's actually doing. Does it raise an Exception if the username isn't valid? Does it return the original username if there's no error? etc. I would suggest renaming it to something like "username_is_valid" and return a bool instead. Alternatively, create a new dataclass type called "UsernameValidationResult" or "UsernameValidationError" and return that instead. This is a nitpick, but it's not clear from the name of this function, nor from its signature, what it's actually doing. Does it raise an Exception if the username isn't valid? Does it return the original username if there's no error? etc.
I would suggest renaming it to something like "username_is_valid" and return a bool instead. Alternatively, create a new dataclass type called "UsernameValidationResult" or "UsernameValidationError" and return that instead.
merenber
commented
Also, I would suggest moving this kind of logic to ceo_common, where it can be shared by both the server and the clients. Also, I would suggest moving this kind of logic to ceo_common, where it can be shared by both the server and the clients.
|
||||
if not username:
|
||||
return 'Username must not be empty'
|
||||
if not VALID_USERNAME_RE.fullmatch(username):
|
||||
return 'Username is invalid'
|
||||
return None
|
||||
|
|
|
@ -0,0 +1,19 @@
|
|||
import ceo.utils as utils
|
||||
|
||||
|
||||
def test_validate_username():
|
||||
merenber
commented
I don't think this test got run in the CI. Each test file needs to begin with "test_". I don't think this test got run in the CI. Each test file needs to begin with "test_".
|
||||
assert utils.validate_username('') == 'Username must not be empty'
|
||||
assert utils.validate_username('-failure') == 'Username is invalid'
|
||||
assert utils.validate_username('35 - joe') == 'Username is invalid'
|
||||
assert utils.validate_username('35 -joe') == 'Username is invalid'
|
||||
assert utils.validate_username('35- joe') == 'Username is invalid'
|
||||
assert utils.validate_username('35$joe') == 'Username is invalid'
|
||||
assert utils.validate_username(' 35joe') == 'Username is invalid'
|
||||
assert utils.validate_username('35 joe') == 'Username is invalid'
|
||||
assert utils.validate_username('35joe ') == 'Username is invalid'
|
||||
assert utils.validate_username('joe!') == 'Username is invalid'
|
||||
assert utils.validate_username('e45jong') is None
|
||||
assert utils.validate_username('joe-35') is None
|
||||
assert utils.validate_username('joe35-') is None
|
||||
assert utils.validate_username('35joe-') is None
|
||||
assert utils.validate_username('35-joe') is None
|
We should add validation to the CLI too, not just the TUI.