public
/
pyceo
27
1
Fork 3
Code Issues 25 Pull Requests 5 Projects Releases 1 Wiki Activity

Address #101 #114

Merged
o32patel merged 3 commits from 101-username-validation into master 2024-01-15 20:01:47 -05:00
Conversation 3 Commits 3 Files Changed 3 +4 -2
2 changed files with 4 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit a24bc21e01 - Show all commits

1
ceo/tui/controllers/Controller.py
View File

@ -3,6 +3,7 @@ from abc import ABC
import ceo.tui.utils as utils
from ...utils import validate_username
# NOTE: one controller can control multiple views,
# but each view must have exactly one controller
class Controller(ABC):

5
ceo/utils.py
View File

@ -14,6 +14,7 @@ from ceod.transactions.members import AddMemberTransaction
VALID_USERNAME_RE = re.compile(r"^[a-z0-9]+[a-z0-9-]+$")
merenber commented 2024-01-15 22:26:06 -05:00
Review
Copy Link

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)
cfg = component.getUtility(IConfig)
@ -269,10 +270,10 @@ From other CSC machines you can connect using
except PermissionError:
return False
def validate_username(username: str) -> Union[str, None]:
merenber commented 2024-01-15 22:32:17 -05:00
Review
Copy Link

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 2024-01-15 22:34:02 -05:00
Review
Copy Link

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 'Username is invalid'
return None