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

New migration needed for Wagtail 6.2 #71

Closed
willbarton opened this issue Aug 2, 2024 · 4 comments · Fixed by #72
Closed

New migration needed for Wagtail 6.2 #71

willbarton opened this issue Aug 2, 2024 · 4 comments · Fixed by #72

Comments

@willbarton
Copy link
Contributor

Wagtail 6.2 alters the Locale model on TranslatableMixin, adding a verbose_name. Because Footnote inherits from TranslatableMixin, it needs a new migration generated for Wagtail 6.2.

That's easy enough to generate, and I could do so and then PR the change, but I'm not sure how you all would want to handle the backward-incompatibility that would create with Wagtail < 6.1.

Currently, this is a blocker for our 6.2 upgrade as we have a required check in CI that runs makemigrations --dry-run --check to ensure there are no missing migrations.

@zerolab
Copy link
Member

zerolab commented Aug 2, 2024

I think we could conditionally do

# wagtail_footnotes/models.py
from wagtail import VERSION as WAGTAIL_VERSION

...

if WAGTAIL_VERSION <= (6, 1):
   Footnote._meta.get_field("locale").verbose_name = _("locale")

which should sort this. If you have the time to give it a try, it would fantastic

@willbarton
Copy link
Contributor Author

willbarton commented Aug 5, 2024

Unfortunately that doesn't appear to fool Django's migration model state/audodetector, because it does an independent resolution of model field relations (I believe that happens here) rather than it occurring after evaluation of this models.py file. And then that's compared against the model state constructed from the existing migrations.

Long way to saying, Django still wants to generate a new migration with this operation:

migrations.AlterField(
    model_name='footnote',
    name='locale',
    field=models.ForeignKey(editable=False, on_delete=django.db.models.deletion.PROTECT, related_name='+', to='wagtailcore.locale', verbose_name='locale'),
),

@gasman
Copy link
Member

gasman commented Aug 12, 2024

Could we wrap the AlterField operation in a version check?

class Migration(migrations.Migration):

    dependencies = [...]

    operations = []
    if WAGTAIL_VERSION >= (6, 1):
        operations.append(
            migrations.AlterField(
                model_name='footnote',
                name='locale',
                field=models.ForeignKey(editable=False, on_delete=django.db.models.deletion.PROTECT, related_name='+', to='wagtailcore.locale', verbose_name='locale'),
            )
        )

This way, the final state that Django arrives at from evaluating the migrations will match the real model state, whichever Wagtail version it's run under. And if the verbose_name is indeed the only change to the model, then there'll be no change to the database schema, so it doesn't matter whether this migration gets run under 6.0 or 6.1.

I seem to remember us doing something similar in Wagtail itself, possibly as a result of django-taggit making a similarly minor change in a point release.

willbarton added a commit to cfpb/wagtail-footnotes that referenced this issue Aug 12, 2024
Wagtail 6.2 alters the Locale model on TranslatableMixin, adding a verbose_name. Because Footnote inherits from TranslatableMixin, it needs a new migration generated for Wagtail 6.2.

To maintain compatibility with Wagtail <= 6.1, this change makes the migration a noop. See torchbox#71 for context.

Closes torchbox#71
willbarton added a commit to cfpb/wagtail-footnotes that referenced this issue Aug 13, 2024
Wagtail 6.2 alters the Locale model on TranslatableMixin, adding a verbose_name. Because Footnote inherits from TranslatableMixin, it needs a new migration generated for Wagtail 6.2.

To maintain compatibility with Wagtail <= 6.1, this change makes the migration a noop. See torchbox#71 for context.

Closes torchbox#71
@willbarton
Copy link
Contributor Author

Thanks, @gasman. That works for our needs as a solution. I've opened a PR that does that, #72.

zerolab added a commit that referenced this issue Aug 13, 2024
* Add backwards-compatible locale verbose_name migration

Wagtail 6.2 alters the Locale model on TranslatableMixin, adding a verbose_name. Because Footnote inherits from TranslatableMixin, it needs a new migration generated for Wagtail 6.2.

To maintain compatibility with Wagtail <= 6.1, this change makes the migration a noop. See #71 for context.

* Remove Wagtail 6.0 from test matrix

Co-authored-by: Dan Braghiș <[email protected]>
nickmoreton pushed a commit to torchbox-forks/wagtail-footnotes that referenced this issue Aug 20, 2024
* Add backwards-compatible locale verbose_name migration

Wagtail 6.2 alters the Locale model on TranslatableMixin, adding a verbose_name. Because Footnote inherits from TranslatableMixin, it needs a new migration generated for Wagtail 6.2.

To maintain compatibility with Wagtail <= 6.1, this change makes the migration a noop. See torchbox#71 for context.

* Remove Wagtail 6.0 from test matrix

Co-authored-by: Dan Braghiș <[email protected]>
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 a pull request may close this issue.

3 participants