-
Notifications
You must be signed in to change notification settings - Fork 601
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
feat: migration from 2.2 -> 3.2 -> 4.2 #2413
Conversation
note: unsure if "valid"
Somewhere along the way the email duplication validation from allauth became wonky causing pythondotorg/users/tests/test_views.py Line 233 in 9b4dedd
It seems to be my last failing test, barring i didnt do anything 'bad' by 'fixing' the failing tests post-migration 😅 edit: this was a change in allauth 0.52.0, but disabling it would be a negative since i believe it was added for increased security. I can add ACCOUNT_PREVENT_ENUMERATION = False
"""Added to ``django-allauth==0.52.0``. Set during the Django 2.2 -> 4.2 migration.""" into settings.py def test_user_duplicate_username_email(self):
"""Test that a user can't be created with a duplicate username or email or both."""
settings.ACCOUNT_PREVENT_ENUMERATION = False it prevents people from bruteforcing to see which emails are registered |
I think this is ready for someone to make fun of me now 🤣, all tests are passing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work 💪🏻 I noted a few things. FWIW, I only went though everything you changed (I haven't looked at what you may have missed). Also note that I'm not too familiar with the codebase, but I went through a few Django upgrades, I hope these coments are of use...
@@ -3,6 +3,6 @@ gunicorn==19.9.0 | |||
raven==6.10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's an old one 😄 Not sure it supports Django 4.2, but maybe it's fine... Would be nice to see if we can switch to its successor, sentry-sdk.
Cool, this really looks good! I didn't see your PR yesterday and started working a bit on the Django update by myself. I also didn't notice that Another thing I did different was trying to update all packages in base and dev-requirements.txt. And except |
The test fails because allauth wont tell you whether an email already exists or not to mitigate against email enumeration. In 0.52.0 there was this setting added, but since 0.55.0 it has no longer an effect because it didn't really work from the start. But even if no error message is shown to the user, an error message is sent via email to the original user of the email address. What I did was splitting up the test into two to make sure the error message is sent. |
Solid work @JacobCoffee !! A few things you might consider...
@ambv Your review, please. |
This comment was marked as resolved.
This comment was marked as resolved.
@ewdurbin is this looking okay so far? If so I can try to finish it up - but I wanted to make sure we wanted to move forward here before continuing anymore work. |
@JacobCoffee Yes, overall this is looking very promising. I was able to get things stood up in a test environment with a clone of the production DB without any obvious errors. |
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
In favor of #2520 |
What
*-requirements.txt
, and bumping to the latest available non-breaking version to give "breathing room"Status
Todo
Does the scope of this include cleaning up the BigAutoField warnings?Closes