-
Notifications
You must be signed in to change notification settings - Fork 43
Bug fix when deleting user from admin interface #1797
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
base: main
Are you sure you want to change the base?
Conversation
gateway/tests/fixtures/fixtures.json
Outdated
| "permissions": [45] | ||
| } | ||
| }, | ||
| { | ||
| "model": "auth.group", | ||
| "pk": 101, | ||
| "fields": { | ||
| "name": "viewer", | ||
| "permissions": [64] | ||
| "permissions": [44] |
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.
Funny, the database population when running tests does include the allauth tables. So, when I removed the dependencies, fewer records are created and the primary keys changed.
|
Note regarding the failure in |
|
If someone tests this PR before me please be aware that this PR it is a bit special. It's not so important that all the tests passed if not that this PR can be executed in a database previously created without errors. |
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.
If someone tests this PR before me please be aware that this PR it is a bit special. It's not so important that all the tests passed if not that this PR can be executed in a database previously created without errors.
What would you recommend as the best path for testing this change @Tansito? Reviewing the code in my case wouldn't help much without having the ability to test it.
|
I would say the correct way to test it would be:
|
I can do this test too. But I feel it's a little overkill... I mean, what's so special about this PR compared to any other PR that includes database migrations? If there's nothing special, should we repeat all these steps to test every PR that has a database migration? Don't get me wrong, I just want to know if we're dealing with a potential error that requires these manual tests. And if so, automate them. |
|
I think that @avilches raises a valid point, but on my side I can say that I ran the manual test and everything seemed to work, jobs ran, migrations seemed to be applied successfully. |
|
What it makes this PR different from another migration is that other migrations are auto-generated but this one specifically is our first manual database migration (I think it was manually created by you @avilches , at least, correct me if it was not like that). It's not that I don't trust on your skills Alberto, don't get me wrong, but I wouldn't consider the same implications an autogenerated migration that a migration created manually. That is my reason. |
# Conflicts: # gateway/main/settings.py # gateway/tests/fixtures/fixtures.json
|
Tested in local with Postgres (docker-compose)
8. Tried again to run a jub. It worked.
The PR is safe to merge @Tansito |




Summary
Error when a user is deleted in the Django admin interface:
From @Tansito:
This PR removes all these dependencies, disabled them from the
INSTALLED_APPSand update the configuration settings and test to ensure everything works as expected.