jazzband / django-two-factor-auth

Complete Two-Factor Authentication for Django providing the easiest integration into most Django projects.
MIT License
1.71k stars 448 forks source link

Fix name of replaced migration #642

Closed youtux closed 1 year ago

youtux commented 1 year ago

Description

In https://github.com/jazzband/django-two-factor-auth/pull/624, the squashed migration declares that it replaces twofactor, while the correct name is two_factor.

This will result in new installs to always execute two_factor.0001_squashed_0008_delete_phonedevice, while they should instead execute only phonenumber.0001_squashed_0001_initial.

How Has This Been Tested?

I run tests on different DB states to verify that the behaviour is correct. In each test, I did the following operations:

Test results

Click here to expand ### New install #### Before Note that we apply `two_factor.0001_squashed_0008_delete_phonedevice`, which is redundant. ``` ❯ python manage.py migrate ... Running migrations: Applying two_factor.0001_squashed_0008_delete_phonedevice... OK Applying phonenumber.0001_squashed_0001_initial... OK ``` #### After Only the `phonenumber.0001_squashed_0001_initial` is applied, as it should. ``` ❯ python manage.py migrate ... Running migrations: Applying phonenumber.0001_squashed_0001_initial... OK ``` ### Existing install, DB state up to version 1.14.0 #### Before ``` ❯ python manage.py migrate ... Running migrations: Applying two_factor.0008_delete_phonedevice... OK Applying phonenumber.0001_squashed_0001_initial... OK ``` #### After ``` ❯ python manage.py migrate ... Running migrations: Applying two_factor.0008_delete_phonedevice... OK Applying phonenumber.0001_squashed_0001_initial... OK ``` ### Existing install, DB state up to version 1.15.0 Note that in both cases, although it says "no migrations to apply", in the `django_migrations` table a new row with the `two_factor.0001_squashed_0008_delete_phonedevice` migration is added. #### Before ``` ❯ python manage.py migrate ... Running migrations: No migrations to apply. ``` #### After ``` ❯ python manage.py migrate ... Running migrations: No migrations to apply. ``` ### Existing install, DB state up to version 1.15.1 No changes #### Before ``` ❯ python manage.py migrate ... Running migrations: No migrations to apply. ``` #### After ``` ❯ python manage.py migrate ... Running migrations: No migrations to apply. ``` ### Existing install, DB state up to version 1.15.2 No changes #### Before ``` ❯ python manage.py migrate ... Running migrations: No migrations to apply. ``` #### After ``` ❯ python manage.py migrate ... Running migrations: No migrations to apply. ``` ### Existing install, DB state up to version 1.15.3 No changes #### Before ``` ❯ python manage.py migrate ... Running migrations: No migrations to apply. ``` #### After ``` ❯ python manage.py migrate ... Running migrations: No migrations to apply. ```

Types of changes

Checklist:

codecov[bot] commented 1 year ago

Codecov Report

Merging #642 (117f7ea) into master (19e02f5) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #642   +/-   ##
=======================================
  Coverage   95.47%   95.47%           
=======================================
  Files          75       75           
  Lines        3204     3204           
  Branches      359      359           
=======================================
  Hits         3059     3059           
  Misses        116      116           
  Partials       29       29           

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

youtux commented 1 year ago

No problem. Would you mind making a new PYPI release with this fix?

youtux commented 1 year ago

Hey @claudep, gentle ping about releasing a new version with this fix, as it's preventing me from upgrading from the latest version.

claudep commented 1 year ago

Sure, will do at appropriate time. However my volunteer time is not infinite :smile:

youtux commented 1 year ago

We’re in the same club :)

Kind regards, Alessio On 11 Aug 2023 at 09:55 +0200, Claude Paroz @.***>, wrote:

Sure, will do at appropriate time. However my volunteer time is not infinite 😄 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

andersk commented 1 year ago

Even with this fix, new installs are still executing two_factor.0001_squashed_0008_delete_phonedevice, just now on the second migrate operation rather than the first.

Reproduction from an empty directory and environment:

$ cat > myapp_settings.py <<EOF
DATABASES = {
    "default": {
        "ENGINE": "django.db.backends.sqlite3",
        "NAME": "sqlite3.db",
    }
}
INSTALLED_APPS = [
    "django.contrib.auth",
    "django.contrib.contenttypes",
    "two_factor",
    "two_factor.plugins.phonenumber",
]
EOF

$ pip install django django-two-factor-auth[phonenumberslite]
…
Successfully installed asgiref-3.7.2 django-4.2.4 django-formtools-2.4.1 django-otp-1.2.2 django-phonenumber-field-6.4.0 django-two-factor-auth-1.15.4 phonenumberslite-8.13.19 pypng-0.20220715.0 qrcode-7.4.2 sqlparse-0.4.4 typing-extensions-4.7.1

$ python -m django migrate --settings=myapp_settings
Operations to perform:
  Apply all migrations: auth, contenttypes, phonenumber
Running migrations:
  Applying contenttypes.0001_initial... OK
  Applying contenttypes.0002_remove_content_type_name... OK
  Applying auth.0001_initial... OK
  Applying auth.0002_alter_permission_name_max_length... OK
  Applying auth.0003_alter_user_email_max_length... OK
  Applying auth.0004_alter_user_username_opts... OK
  Applying auth.0005_alter_user_last_login_null... OK
  Applying auth.0006_require_contenttypes_0002... OK
  Applying auth.0007_alter_validators_add_error_messages... OK
  Applying auth.0008_alter_user_username_max_length... OK
  Applying auth.0009_alter_user_last_name_max_length... OK
  Applying auth.0010_alter_group_name_max_length... OK
  Applying auth.0011_update_proxy_permissions... OK
  Applying auth.0012_alter_user_first_name_max_length... OK
  Applying phonenumber.0001_squashed_0001_initial... OK

$ python -m django migrate --settings=myapp_settings
Operations to perform:
  Apply all migrations: auth, contenttypes, phonenumber, two_factor
Running migrations:
  Applying two_factor.0001_squashed_0008_delete_phonedevice... OK
andersk commented 1 year ago

Even with this fix, new installs are still executing two_factor.0001_squashed_0008_delete_phonedevice, just now on the second migrate operation rather than the first.

649 seems to fix that.

youtux commented 1 year ago

Even with this fix, new installs are still executing two_factor.0001_squashed_0008_delete_phonedevice, just now on the second migrate operation rather than the first.

Ah thank you for pointing that out, I didn't test running 2 times the migrate.py command (I mean, why would I, right?)