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

Relax username requirements a little #943

Merged
merged 4 commits into from
Mar 1, 2022

Conversation

ewels
Copy link
Contributor

@ewels ewels commented Feb 28, 2022

Minor changes to username requirements, needs agreement in meeting:

  • Allow . and - in usernames
  • Relax length requirements to 3-30 characters instead of 8-20
  • Tests passing
  • Black formatting
  • Migrations for any changes to the database schema
  • Rebase/merge the dev branch
  • Note in the CHANGELOG

@i-oden i-oden added the could have Wanted or desirable but less important label Feb 28, 2022
@ewels
Copy link
Contributor Author

ewels commented Feb 28, 2022

Not sure why the tests are failing? Seems unrelated to the PR changes I think..

@i-oden
Copy link
Member

i-oden commented Feb 28, 2022

Not sure why the tests are failing? Seems unrelated to the PR changes I think..

Yep, it's the db encryption, have commented out the volumes in the db part of the docker-compose for now and then it works

dds_web/utils.py Outdated Show resolved Hide resolved
@i-oden
Copy link
Member

i-oden commented Feb 28, 2022

Now you should be able to merge dev and then the tests should pass

@ewels
Copy link
Contributor Author

ewels commented Feb 28, 2022

I don't have permissions to merge :)

@i-oden
Copy link
Member

i-oden commented Feb 28, 2022

I meant merge dev into this branch.

@i-oden
Copy link
Member

i-oden commented Feb 28, 2022

So that the docker-compose change is in this too and the tests pass

@i-oden
Copy link
Member

i-oden commented Feb 28, 2022

@ewels Could you also add an entry in the changelog?

@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #943 (ddfae4f) into dev (15ff173) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #943      +/-   ##
==========================================
+ Coverage   87.89%   87.90%   +0.01%     
==========================================
  Files          26       26              
  Lines        2899     2894       -5     
==========================================
- Hits         2548     2544       -4     
+ Misses        351      350       -1     
Impacted Files Coverage Δ
dds_web/api/schemas/user_schemas.py 92.94% <ø> (ø)
dds_web/forms.py 97.36% <ø> (ø)
dds_web/utils.py 76.83% <100.00%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15ff173...ddfae4f. Read the comment docs.

Copy link
Member

@i-oden i-oden left a comment

Choose a reason for hiding this comment

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

Just tested: Do we want to allow usernames containing all or almost all special characters?

@ewels
Copy link
Contributor Author

ewels commented Mar 1, 2022

If in doubt, copy what GitHub does..? 😀

https://github.com/shinnn/github-username-regex#github-username-regex

  • Github username may only contain alphanumeric characters or hyphens.
  • Github username cannot have multiple consecutive hyphens.
  • Github username cannot begin or end with a hyphen.
  • Maximum is 39 characters.

Actually quite restrictive.. Drop-in regex: /^[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$/i

Personally I'm happy with what's in this PR..

@i-oden
Copy link
Member

i-oden commented Mar 1, 2022

@ewels Ok I will add this as a possible future change since it's not a high priority, but I will approve and merge this PR now 👍🏻

@i-oden i-oden merged commit 4a6a629 into ScilifelabDataCentre:dev Mar 1, 2022
@ewels ewels deleted the relax-username-requirements branch March 1, 2022 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could have Wanted or desirable but less important
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants