Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relaxed account regex per ENG-83 #302

Merged
merged 2 commits into from
Jul 29, 2021

Conversation

DanielHHowell
Copy link
Contributor

Modified organization regex for better or worse, along with associated tests

@linear
Copy link

linear bot commented Jul 28, 2021

ENG-84 Update servoX account validation

ENG-83 Relax requirement on account ids

Currently we require a string that looks like an account name. This is enforced in servoX, which is also used in the live emulator.

Proposed validation: ^[\w\.-]{5,50}$

Expression we may want: ^(?!-)([A-Za-z0-9-.]+){5,50}

Once we agree on a validation, this should be applied in several places (see subtasks). Also, we should make sure all existing accounts match the new regex (or else servo upgrade may brick an app).

Copy link
Contributor

@linkous8 linkous8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice have a comment documenting the concerns we are constraining against but I don't think we should block for that (esp since this is likely to change in the future). Approved

Copy link

@mjdeclerck mjdeclerck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provisional approval. Can you please add the validation tests that were performed either in the PR comments or in ENG-83? I want to make sure that along with the inline documentation in the code is visible for future work/revisions. Thanks.

@linkous8 linkous8 self-requested a review July 29, 2021 20:23
Copy link
Contributor

@linkous8 linkous8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DanielHHowell DanielHHowell merged commit d4e999a into main Jul 29, 2021
@DanielHHowell DanielHHowell deleted the daniel/eng-84-update-servox-account-validation branch July 29, 2021 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants