public
/
pyceo
23
1
Fork 3
Code Issues 26 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 +35 -3
3 changed files with 35 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -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)
merenber commented 2024-01-15 22:34:22 -05:00
Review
Copy Link

We should add validation to the CLI too, not just the TUI.

We should add validation to the CLI too, not just the TUI.
if verification_res:
self.view.popup(verification_res)
raise Controller.InvalidInput()
return username

13
ceo/utils.py
View File

@ -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 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)
@ -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 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 None

19
tests/ceo/utils.py Normal file
View File

@ -0,0 +1,19 @@
import ceo.utils as utils
def test_validate_username():
merenber commented 2024-01-15 22:28:31 -05:00
Review
Copy Link

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