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

Fix #5 Prevent MigrationAutoDetector removes the user unique email constraint #6

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

trottomv
Copy link
Contributor

@trottomv trottomv commented Nov 18, 2023

Prevent MigrationAutoDetector removes the user unique email constraint.

After various tests and checks, it seems to me that the control on the presence of migration is a bit redundant and, indeed, could create other problems. So, I limited myself to adding a test and uncommenting the already existing line that acts on original_attrs["constraint"] without introducing the hypothesized control. Am I missing something?
Because it should be enough that "unique_user_email" is in the INSTALLED_APPS and avoid checking the presence of migrations.

@@ -22,5 +22,4 @@ def ready(self):
),
]
User._meta.constraints = User.Meta.constraints
# ... as long as original_attrs is not updated.
# User._meta.original_attrs["constraints"] = User.Meta.constraints
User._meta.original_attrs["constraints"] = User.Meta.constraints
Copy link
Owner

@carltongibson carltongibson Nov 18, 2023

Choose a reason for hiding this comment

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

I think that this will cause the auto-detector to create an auth migration when unique_user_email is installed but the migration is not run. ( maybe it won't... — I put this together interactively, so maybe the end result isn't the same as the half-way point…)

The comment you removed here was part of the longer comment beginning on ln12.

I think we should have tests for both that case and the issue here (which is that an auth migration is created after the unique_user_email migration is run).

Copy link
Owner

Choose a reason for hiding this comment

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

Ah ha! It' looks like it doesn't 🕺

% ./manage.py showmigrations
admin
 [ ] 0001_initial
 [ ] 0002_logentry_remove_auto_add
 [ ] 0003_logentry_add_action_flag_choices
auth
 [ ] 0001_initial
 [ ] 0002_alter_permission_name_max_length
 [ ] 0003_alter_user_email_max_length
 [ ] 0004_alter_user_username_opts
 [ ] 0005_alter_user_last_login_null
 [ ] 0006_require_contenttypes_0002
 [ ] 0007_alter_validators_add_error_messages
 [ ] 0008_alter_user_username_max_length
 [ ] 0009_alter_user_last_name_max_length
 [ ] 0010_alter_group_name_max_length
 [ ] 0011_update_proxy_permissions
 [ ] 0012_alter_user_first_name_max_length
contenttypes
 [ ] 0001_initial
 [ ] 0002_remove_content_type_name
sessions
 [ ] 0001_initial
unique_user_email
 [ ] 0001_initial
% ./manage.py makemigrations auth
No changes detected in app 'auth'

OK, let me have a play over the weekend, and I'll get this out.

Thanks @trottomv 🎁

Copy link
Contributor Author

@trottomv trottomv Nov 18, 2023

Choose a reason for hiding this comment

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

for completenes:

(.venv) $ python manage.py showmigrations
admin
 [X] 0001_initial
 [X] 0002_logentry_remove_auto_add
 [X] 0003_logentry_add_action_flag_choices
auth
 [X] 0001_initial
 [X] 0002_alter_permission_name_max_length
 [X] 0003_alter_user_email_max_length
 [X] 0004_alter_user_username_opts
 [X] 0005_alter_user_last_login_null
 [X] 0006_require_contenttypes_0002
 [X] 0007_alter_validators_add_error_messages
 [X] 0008_alter_user_username_max_length
 [X] 0009_alter_user_last_name_max_length
 [X] 0010_alter_group_name_max_length
 [X] 0011_update_proxy_permissions
 [X] 0012_alter_user_first_name_max_length
contenttypes
 [X] 0001_initial
 [X] 0002_remove_content_type_name
sessions
 [X] 0001_initial
unique_user_email
 [ ] 0001_initial
(.venv) $ python manage.py makemigrations
No changes detected

@trottomv trottomv changed the title Fix #5 Fix #5 Prevent MigrationAutoDetector removes the user unique email constraint Nov 18, 2023
Copy link
Owner

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, let's have this. Thanks @trottomv 🎁

@carltongibson carltongibson merged commit f1285df into carltongibson:main Nov 20, 2023
1 check passed
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.

2 participants