public
/
pyceo
24
1
Fork 4
Code Issues 26 Pull Requests 6 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
o32patel commented 2024-01-15 18:32:55 -05:00
Owner
Copy Link

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

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`
o32patel added 1 commit 2024-01-15 18:32:56 -05:00
o32patel commented 2024-01-15 18:34:09 -05:00
Author
Owner
Copy Link

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

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
o32patel changed title from Address #101 to WIP: Address #101 2024-01-15 18:34:15 -05:00
n4chung commented 2024-01-15 18:36:25 -05:00
Owner
Copy Link

Code logic seems pretty organized and reasonable to me 🤔

Seems to match the original Issue description quite well. 👀

Code logic seems pretty organized and reasonable to me 🤔 Seems to match the original Issue description quite well. 👀
n4chung commented 2024-01-15 18:37:44 -05:00
Owner
Copy Link

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.

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.
n4chung approved these changes 2024-01-15 18:38:50 -05:00
n4chung left a comment
Owner
Copy Link

Great work Ohm! @o32patel

Great work Ohm! @o32patel
o32patel added 1 commit 2024-01-15 18:59:19 -05:00
continuous-integration/drone/pr Build is passing Details
a24bc21e01 lint
o32patel added 1 commit 2024-01-15 19:36:25 -05:00
continuous-integration/drone/pr Build is passing Details
4596312afa add tests for username validation
o32patel changed title from WIP: Address #101 to Address #101 2024-01-15 19:40:58 -05:00
o32patel merged commit f06ccdc3f9 into master 2024-01-15 20:01:47 -05:00
o32patel deleted branch 101-username-validation 2024-01-15 20:01:47 -05:00
merenber reviewed 2024-01-15 22:35:30 -05:00
merenber left a comment
Owner
Copy Link

Unfortunately 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?

Unfortunately 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?
ceo/tui/controllers/Controller.py
@ -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)
merenber commented 2024-01-15 22:34:22 -05:00
Owner
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.
ceo/utils.py
@ -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
Owner
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.
ceo/utils.py
@ -268,1 +271,4 @@
return False
def validate_username(username: str) -> Union[str, None]:
merenber commented 2024-01-15 22:32:17 -05:00
Owner
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
Owner
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.
tests/ceo/utils.py
@ -0,0 +1,19 @@
import ceo.utils as utils
def test_validate_username():
merenber commented 2024-01-15 22:28:31 -05:00
Owner
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_".
Sign in to join this conversation.
Reviewers
No reviewers
n4chung
Labels
Clear labels   
priority
high
High priority

  
priority
low
Low priority

  
priority
medium
Medium priority

  
priority
very high
Very high priority

  
BUG

   
Feature

   
High Priority

   
Low Priority

   
Medium Priority
No Label
priority
high
priority
low
priority
medium
priority
very high
BUG
Feature
High Priority
Low Priority
Medium Priority
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#114
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?