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

Upgrades django to 4.2 #4073

Merged
merged 6 commits into from
Apr 23, 2024
Merged

Upgrades django to 4.2 #4073

merged 6 commits into from
Apr 23, 2024

Conversation

mauromsl
Copy link
Member

@mauromsl mauromsl commented Apr 1, 2024

Updates to make Janeway compatible with Django 4.2
closes #4072

@mauromsl mauromsl marked this pull request as ready for review April 17, 2024 08:40
@mauromsl mauromsl requested a review from joemull April 17, 2024 08:41
Copy link
Member

@joemull joemull left a comment

Choose a reason for hiding this comment

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

I'm a bit unclear on the way the email is transformed or not transformed, and there's one line of code that I am a bit confused by, so just want to make it clear for others as well.

src/core/models.py Outdated Show resolved Hide resolved
email=email,
# Lowercasing the email into the username gets around duplicate
# accounts due to case-sensitivity. We still preserve the original
# casing for the email field and rely on username for CI constraint
Copy link
Member

Choose a reason for hiding this comment

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

I don't entirely follow this comment, so others might have difficulty too. What does the last part mean?

Copy link
Member Author

@mauromsl mauromsl Apr 22, 2024

Choose a reason for hiding this comment

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

I tried to explain exisitng behaviour because I thought it wasn't clear, seems I didn't do that well. The basic idea is that we need a case-sensitive email field, but we need a case-insensitive constraint to avoid accounts being created with variations in case e.g.: m.SaNcHeZ@bbk.ac.uk. The way this is achieved is by setting a unique constraint on username while making it case insensitive. Any ideas on how I can reword it to make it clear?
I'm not very happy with that level of indirection, so perhaps an issue for setting the constraint on email and ditching the username field would be a good one to raise

Copy link
Member

Choose a reason for hiding this comment

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

OK, now I get it, thanks! Maybe something like this?

diff --git a/src/core/models.py b/src/core/models.py
index aba452e2..d480da59 100644
--- a/src/core/models.py
+++ b/src/core/models.py
@@ -196,10 +196,12 @@ class AccountManager(BaseUserManager):
             raise ValueError(f'{email} not a valid email address.')
 
         account = self.model(
+            # The original case of the email is preserved
+            # in the email field
             email=email,
-            # Lowercasing the email into the username gets around duplicate
-            # accounts due to case-sensitivity. We still preserve the original
-            # casing for the email field and rely on username for CI constraint
+            # The email is lowercased in the username field
+            # so that we can perform case-insensitive checking
+            # and avoid creating duplicate accounts
             username=email.lower(),
         )

@joemull joemull assigned mauromsl and unassigned joemull Apr 17, 2024
@mauromsl mauromsl assigned joemull and unassigned mauromsl Apr 22, 2024
@mauromsl mauromsl requested a review from joemull April 22, 2024 19:12
@joemull joemull assigned mauromsl and unassigned joemull Apr 23, 2024
Copy link
Member

@joemull joemull left a comment

Choose a reason for hiding this comment

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

I understand it now, thanks--I put some possible text that is clear to me above. Does it work for you?

@mauromsl mauromsl requested a review from joemull April 23, 2024 14:51
@mauromsl mauromsl assigned joemull and unassigned mauromsl Apr 23, 2024
@joemull joemull requested a review from ajrbyers April 23, 2024 15:03
@joemull joemull assigned ajrbyers and unassigned joemull Apr 23, 2024
@ajrbyers ajrbyers merged commit 3b5aff5 into master Apr 23, 2024
1 check failed
@ajrbyers ajrbyers deleted the 4072-django-4-2 branch April 23, 2024 15:35
@mauromsl mauromsl changed the title 4072 django 4 2 Upgrades django to 4.2 May 2, 2024
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.

Upgrade django version to 4.2
3 participants