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

Drop CIText & update Django #237

Merged
merged 5 commits into from
Feb 13, 2022
Merged

Drop CIText & update Django #237

merged 5 commits into from
Feb 13, 2022

Conversation

jchristgit
Copy link
Collaborator

@jchristgit jchristgit commented Feb 11, 2022

Move from CIText to plain char fields.

The created database migration updates the existing field (without incurring
any data loss, so this can be run safely) and creates the new database index
preventing the creation of team and users whose name only differs in their
case.

Closes #189.

Fixes #235. @thebeanogamer you need to take care when rolling this out because
we may need to adjust existing entries in the database before PostgreSQL will
accept the migration.

@jchristgit jchristgit added the enhancement New feature or request label Feb 11, 2022
@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #237 (ffa24b0) into master (3dd84ca) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   94.41%   94.44%   +0.02%     
==========================================
  Files         107      107              
  Lines        4767     4791      +24     
  Branches      272      273       +1     
==========================================
+ Hits         4501     4525      +24     
  Misses        223      223              
  Partials       43       43              
Impacted Files Coverage Δ
src/authentication/basic_auth.py 100.00% <100.00%> (ø)
src/authentication/tests.py 100.00% <100.00%> (ø)
src/backend/signals.py 100.00% <100.00%> (ø)
src/member/models.py 100.00% <100.00%> (ø)
src/member/tests.py 100.00% <100.00%> (ø)
src/team/models.py 100.00% <100.00%> (ø)
src/team/serializers.py 100.00% <100.00%> (ø)
src/team/tests.py 100.00% <100.00%> (ø)

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 3dd84ca...ffa24b0. Read the comment docs.

@jchristgit
Copy link
Collaborator Author

Note: We tried to squash the migrations in a way that renders the extension obsolete for new installations, but currently we run into circular dependency errors due to a team having an owner (a member) and a member having a team.

https://docs.djangoproject.com/en/4.0/topics/migrations/#squashing-migrations has some documentation on how to deal with this, but we haven't quite gotten it to work just yet.

Copy link
Contributor

@0xAda 0xAda left a comment

Choose a reason for hiding this comment

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

Looks good, just needs exception handling on register

"""A differently-cased but otherwise same username should not be allowed registration."""

self.user.username = self.admin_user.username.upper()
self.assertRaises(IntegrityError, self.user.save)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to teams, this should 400 instead of raising an Exception

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@david-cooke this is testing the database saving part though, not the endpoint. Or do you mean that the endpoint is missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we handle it on registration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There isn't really any entry point for error handling there from what I gather, so I would vote that we merge it as-is right now and wait for Sentry to throw up 🛷

Copy link
Member

@thebeanogamer thebeanogamer left a comment

Choose a reason for hiding this comment

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

We have one username over 32 chars on all our deployments, let's just add a migration to truncate those.

Also, is it worth having a test for trying to register a user/team longer than 32 chars?

@jchristgit
Copy link
Collaborator Author

We have one username over 32 chars on all our deployments, let's just add a migration to truncate those.

No. I am not a MySQL database. I will not be writing a migration that incurs data loss.

@jchristgit
Copy link
Collaborator Author

@thebeanogamer per your request, the migration has been altered to introduce data loss.

jchristgit and others added 4 commits February 13, 2022 17:44
`django-clacks` was removed temporarily until we figure out why poetry
won't lock the dependencies with clacks included.
A unique constraint is created on the database in order to prevent
duplicate signups using a name that only differs in casing for both
users as well as teams. Two test cases are added to verify this
behaviour.
Co-authored-by: Daniel Milnes <thebeanogamer@gmail.com>
Copy link
Member

@thebeanogamer thebeanogamer left a comment

Choose a reason for hiding this comment

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

I think this is fine

@thebeanogamer thebeanogamer enabled auto-merge (squash) February 13, 2022 22:30
@thebeanogamer thebeanogamer merged commit 2d492a6 into master Feb 13, 2022
@thebeanogamer thebeanogamer deleted the drop-citext branch February 13, 2022 23:19
0xAda pushed a commit that referenced this pull request Sep 26, 2024
* Update to Django 4.0

`django-clacks` was removed temporarily until we figure out why poetry
won't lock the dependencies with clacks included.

* Drop CI text fields in favour of charfields

A unique constraint is created on the database in order to prevent
duplicate signups using a name that only differs in casing for both
users as well as teams. Two test cases are added to verify this
behaviour.

* Re-add django-clacks

* Truncate data in migration

Co-authored-by: Daniel Milnes <thebeanogamer@gmail.com>

* Reject usernames with a length > 36

Co-authored-by: Daniel Milnes <thebeanogamer@gmail.com>
Co-authored-by: David Cooke <me@dave.lc>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maximum Username Length Remove CIText dependency
3 participants