Wagtail 4.1 and 4.2 support (revised)

[Stormheg]

This PR supersedes #51, #49 and #48

I took all changes from @nickmoreton's fork and applied a fix to the PageRevision to Revision migration + some formatting fixes. I don't have push access to their fork, hence this new PR. All open comments from #51 should have been addressed in this PR.

I've decided to not drop Python 3.7 support in this PR (I requested this in #51). I'm planning on fixing the CI and will drop support for 3.7 as part of that future PR, just want to get this merged in.

PageRevision to Revision migration

Migrations like this are tricky to do correctly.

I've investigated the best solution for this that works for both existing and fresh databases.

If we look at how PageRevision was changed to Revision in Wagtail 4 (link to migration), we can see PageRevision was renamed Revision using Django's RenameModel migration operation. As a side-effect of this, all foreign keys referencing this model will be updated as well. This includes the ForeignKey to wagtailcore.pagerevision.

With above knowledge in mind, I've been experimenting with how Django handles renaming a model and what happens when migrating existing databases and 'new' databases.

I've discovered that this is very tricky to get right because of the order in which migrations are applied.

If we:

1. Alter the initial migration to create a foreign key to the Revision model instead of PageRevision
2. Change the dependencies list to make sure the initial migration is applied after Wagtail has migrated to the Revision model (like done now; except for the change to dependencies)

The result would be that this would work for new databases but not existing databases because changing the dependencies list will result in inconsistent migration history. If we don't alter the dependencies, we risk our migration being applied before Wagtail's, which would result in an error either way.

If we don't alter the initial migration and only generate an AlterField migration to change the foreign key the result would be that this would work for existing databases but not for new databases. Again, all of this is caused by the order in which migrations are applied.

Summary
We need to change the dependencies of our initial migration to be able to apply it on a fresh database but by doing so we create inconsistent migration history for existing databases.

Solution implemented in this PR
I found the best way to solve this is to leave the initial migration untouched and generate a new migration with an AlterField operation to change the foreign key. This works for existing databases.

To support new installations, I squashed the entire migration history and use Wagtail's Revision model from the start. This avoids the ValueError: Related model 'wagtailcore.pagerevision' cannot be resolved error when migrating the 0001_initial migration, which must continue to reference the old PageRevision model because we cannot alter its dependencies.

Testing process I followed

I tested the migration from Wagtail 2.16 to Wagtail 4.2 by checking out the bakerydemo project @ commit 75eab056f09aef5b3440b1d1ea4c04cfb40414e0, installing wagtail_ab_testing from main and created an AB test for the about page.

Next, I checked out bakerydemo @ commit 8cb4c0a60e12ed0e258a62e6ccc3bcfa46b290e3 and installed wagtail_ab_testing from this branch. I kept the database intact to see if the migration to the Revision model works properly for databases with existing data and history. I'm glad to report this migrates successfully for me with no errors!

I then deleted the database and ran all bakerydemo from scratch to simulate a fresh database. This fresh database only applies the 0001_squashed_0012_abtest_variant_revision migration like expected, with no errors.

[Stormheg]

You can help test this PR by installing wagtail_ab_testing from my fork:

pip install "git+https://github.com/Stormheg/wagtail-ab-testing.git@support/wagtail42#egg=wagtail_ab_testing"

Disclaimer: I would strongly discourage using my fork on production. Don't add this to your requirements.txt. I may change or delete my fork at any time and am not responsible for broken sites.

[Stormheg]

Got tox to run the test suite locally (CI for this project still depends on Travis CI, which has long gone) and all tests pass!

  py38-dj32-wa41-sqlite: OK (27.15=setup[11.04]+cmd[16.11] seconds)
  py38-dj32-wa41-postgres: OK (57.61=setup[10.64]+cmd[46.97] seconds)
  py38-dj32-wa42-sqlite: OK (28.24=setup[10.68]+cmd[17.56] seconds)
  py38-dj32-wa42-postgres: OK (32.85=setup[10.89]+cmd[21.96] seconds)
  py38-dj40-wa41-sqlite: OK (26.73=setup[10.90]+cmd[15.84] seconds)
  py38-dj40-wa41-postgres: OK (32.14=setup[11.04]+cmd[21.11] seconds)
  py38-dj40-wa42-sqlite: OK (27.11=setup[10.99]+cmd[16.13] seconds)
  py38-dj40-wa42-postgres: OK (32.76=setup[11.26]+cmd[21.50] seconds)
  py38-dj41-wa41-sqlite: OK (26.46=setup[10.89]+cmd[15.57] seconds)
  py38-dj41-wa41-postgres: OK (32.69=setup[10.91]+cmd[21.78] seconds)
  py38-dj41-wa42-sqlite: OK (25.58=setup[10.25]+cmd[15.33] seconds)
  py38-dj41-wa42-postgres: OK (42.52=setup[10.76]+cmd[31.76] seconds)
  py39-dj32-wa41-sqlite: OK (29.03=setup[11.57]+cmd[17.46] seconds)
  py39-dj32-wa41-postgres: OK (31.85=setup[11.05]+cmd[20.80] seconds)
  py39-dj32-wa42-sqlite: OK (27.33=setup[10.99]+cmd[16.34] seconds)
  py39-dj32-wa42-postgres: OK (38.13=setup[11.02]+cmd[27.11] seconds)
  py39-dj40-wa41-sqlite: OK (28.62=setup[11.03]+cmd[17.59] seconds)
  py39-dj40-wa41-postgres: OK (33.29=setup[11.16]+cmd[22.13] seconds)
  py39-dj40-wa42-sqlite: OK (27.64=setup[11.11]+cmd[16.53] seconds)
  py39-dj40-wa42-postgres: OK (42.36=setup[10.82]+cmd[31.54] seconds)
  py39-dj41-wa41-sqlite: OK (11.88=setup[0.01]+cmd[11.87] seconds)
  py39-dj41-wa41-postgres: OK (33.51=setup[10.71]+cmd[22.80] seconds)
  py39-dj41-wa42-sqlite: OK (28.60=setup[10.88]+cmd[17.72] seconds)
  py39-dj41-wa42-postgres: OK (33.92=setup[11.04]+cmd[22.88] seconds)
  py310-dj32-wa41-sqlite: OK (31.94=setup[11.91]+cmd[20.03] seconds)
  py310-dj32-wa41-postgres: OK (34.18=setup[11.07]+cmd[23.11] seconds)
  py310-dj32-wa42-sqlite: OK (28.15=setup[11.50]+cmd[16.65] seconds)
  py310-dj32-wa42-postgres: OK (33.59=setup[11.13]+cmd[22.46] seconds)
  py310-dj40-wa41-sqlite: OK (27.31=setup[11.09]+cmd[16.22] seconds)
  py310-dj40-wa41-postgres: OK (37.48=setup[11.33]+cmd[26.15] seconds)
  py310-dj40-wa42-sqlite: OK (28.23=setup[10.75]+cmd[17.48] seconds)
  py310-dj40-wa42-postgres: OK (35.10=setup[11.15]+cmd[23.94] seconds)
  py310-dj41-wa41-sqlite: OK (26.59=setup[10.99]+cmd[15.60] seconds)
  py310-dj41-wa41-postgres: OK (37.34=setup[11.12]+cmd[26.23] seconds)
  py310-dj41-wa42-sqlite: OK (31.22=setup[11.28]+cmd[19.94] seconds)
  py310-dj41-wa42-postgres: OK (33.62=setup[11.12]+cmd[22.50] seconds)
  py311-dj41-wa41-sqlite: OK (35.83=setup[10.81]+cmd[25.02] seconds)
  py311-dj41-wa41-postgres: OK (32.11=setup[10.96]+cmd[21.16] seconds)
  py311-dj41-wa42-sqlite: OK (28.68=setup[11.24]+cmd[17.44] seconds)
  py311-dj41-wa42-postgres: OK (32.43=setup[10.14]+cmd[22.29] seconds)
  py311-dj41-wamain-sqlite: OK (37.34=setup[20.74]+cmd[16.60] seconds)
  py311-dj41-wamain-postgres: OK (43.71=setup[21.79]+cmd[21.93] seconds)
  congratulations :) (1353.53 seconds)
  

[Stormheg]

I just learned about the run_before option and this will probably work even better - no need for magic with squashed migration files.

If I can get run_before working with this PR that would probably be preferable to squashing the migrations. I'll mark this PR as a draft while I investigate.

[harrislapiroff]

Thanks for working on this @Stormheg! Let me know if we can help. I'm eager to get this working for our projects :)

[Stormheg]

Hi @harrislapiroff, thanks for your interest and sorry for my late reply!

I have just added a commit that approaches the migration problem in a different way which I think is more elegant. All this needs is a review 🙌

[SaptakS]

I've decided to not drop Python 3.7 support in this PR

I wonder if it can be dropped now, since Python 3.7 is EOL now. Though if it is complicated to remove, I think we can get this PR merged without the dropping change. I will review the PR as is for now.

[Stormheg]

Hi @SaptakS, we can absolutely drop Python 3.7 but this PR already contains a lot of changes. I did not want to make this PR much larger

[SaptakS]

Makes sense to me.

[SaptakS]

@Stormheg as discussed over call, there's another usecase that I found where the project is already in Wagtail 4.2 and have all the migrations of 4.2 applied, but were not using Wagtail AB Testing package. So when they install the wagtail ab testing (based on this PR) and try to apply migrations, it fails because of the run_before statements. This is because for such projects, the page revision migrations has already been applied and this is a fresh install.

I have tested with 3a557fb and the migrations work fine in the above case with the squashed migrations. I am testing the rest of the project to see if everything works as expected.

[Stormheg]

Thanks for testing this @SaptakS! I will make sure to drop the run_before commit before merging this.

Let me know if you run into any issues. Otherwise, approve the PR and I'll get it merged and a new release out 🚀

[SaptakS]

Rest of the code looks good to me subject to run_before commit is reverted and the squashed migrations are used.

[rgs258]

Anyone hits this in the future, you need to both change your target model back to wagtailcore.pagerevision AND add a run_before:

to='wagtailcore.pagerevision')

and

run_before = [
    ("wagtailcore", "0067_alter_pagerevision_content_json"),
]