Address #101 #114
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
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: public/pyceo#114
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?
Move and add validation to Controller.get_username_from_view(). This addresses #101 and potentially the TODO?
# TODO: share validation logic between CLI and TUI
Would like some clarification/example on what the TODO is reffering to in the cli code as it's only marked as todo in tui code
Address #101to WIP: Address #101Code logic seems pretty organized and reasonable to me 🤔
Seems to match the original Issue description quite well. 👀
I don't see unit tests written or modified for the new changes, so CI should pass.
Let's just make sure it works before merging and deploying this.
Great work Ohm! @o32patel
WIP: Address #101to Address #101Unfortunately this PR already got merged by the time I received a notification for it.
@o32patel could you please address the issues I mentioned below when you get the chance?
@ -54,3 +55,2 @@
# TODO: share validation logic between CLI and TUI
if not username:
self.view.popup('Username must not be empty')
verification_res = validate_username(username)
We should add validation to the CLI too, not just the TUI.
@ -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-]+$")
We don't need the first "+" here. Also, the first character should always be a letter.
@ -268,1 +271,4 @@
return False
def validate_username(username: str) -> Union[str, None]:
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.
Also, I would suggest moving this kind of logic to ceo_common, where it can be shared by both the server and the clients.
@ -0,0 +1,19 @@
import ceo.utils as utils
def test_validate_username():
I don't think this test got run in the CI. Each test file needs to begin with "test_".