torchbox / wagtail-footnotes

MIT License
20 stars 18 forks source link

New migration needed for Wagtail 6.2 #71

Closed willbarton closed 1 month ago

willbarton commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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