Support wagtail42 #51

nickmoreton · 2023-02-13T16:54:45Z

This updates the package for Wagtail 4.2

Drops: support for Wagtail < 4.1

For completeness I have merged in the following previous work:

and adjust the import paths to allow different wagtail versions

I removed the upper limits on wagtail and django versions.

victoriachan

Hi Nick, there are a couple of mentions of wagtail 3 in Readme and setup to remove. They might have got merged in when you merged the other MRs. Also when testing locally, I got this error: ValueError: Related model 'wagtailcore.pagerevision' cannot be resolved. Looks like one of the migration files needs updating.

victoriachan · 2023-02-27T11:47:48Z

README.md

@@ -18,6 +18,10 @@ Key features:

 ## Usage

+**The code examples below assume you are using Wagtail v3.0+** If you need information for an earlier version of Wagtail see the [Release notes](https://docs.wagtail.org/en/stable/releases/upgrading.html) for guidance.


Aren't we dropping support for Wagtail < 4.1 ?

Resolved in 391be2e

victoriachan · 2023-02-27T11:50:36Z

setup.py

@@ -33,22 +33,25 @@
        "Programming Language :: Python :: 3.7",
        "Programming Language :: Python :: 3.8",
        "Programming Language :: Python :: 3.9",
+        "Programming Language :: Python :: 3.10",


"Programming Language :: Python :: 3.11", as well?

Resolved in 391be2e

victoriachan · 2023-02-27T11:52:00Z

setup.py

        "Framework :: Django",
-        "Framework :: Django :: 2.2",
        "Framework :: Django :: 3.0",


Remove support for Django 3.0 (for Wagtail 4.1 and above).

Resolved in 391be2e

victoriachan · 2023-02-27T11:57:50Z

setup.py

        "Framework :: Wagtail",
        "Framework :: Wagtail :: 2",
+        "Framework :: Wagtail :: 3.0",


We're dropping support for Wagtail < 4.1? And need to add Wagtail 4.1 and 4.2.

Looks like something got confused here when merging in previous changes for Wagtail 4.0?

Resolved in 391be2e

victoriachan · 2023-02-27T12:09:04Z

setup.py

    ],
    install_requires=[
-        "Django>=2.2,<3.3",
-        "Wagtail>=2.11,<2.17",
+        "Wagtail>=4.1",


Should we limit to < 4.3 since we are not sure if it will be compatible?

I used wagtailmedia package as a guide for this

victoriachan · 2023-02-27T12:09:27Z

tox.ini

-    wa215: wagtail>=2.15rc1,<2.16
-    wa216: wagtail>=2.16.1,<2.17
+    wa41: wagtail>=4.1,<4.2
+    wa42: wagtail>=4.2,<5.0


Wonder if we should make this more fine grain, e.g. wagtail>=4.2,<4.3 so if wagtail 4.3 comes out and is incompatible with the existing code here, the test will not suddenly fail before we can update this to be compatible with it.

Resolved in 391be2e

victoriachan · 2023-02-27T12:11:52Z

wagtail_ab_testing/__init__.py

-default_app_config = "wagtail_ab_testing.apps.WagtailAbTestingAppConfig"
+from django import VERSION as DJANGO_VERSION
+
+if DJANGO_VERSION < (3, 2):


Wagtail 4.1 supports Django 3.2, 4.0, 4.1 only?

Resolved in 391be2e

victoriachan · 2023-02-27T12:14:16Z

wagtail_ab_testing/test/__init__.py

@@ -1 +1,4 @@
-default_app_config = "wagtail_ab_testing.test.apps.WagtailAbTestingTestAppConfig"
+from django import VERSION as DJANGO_VERSION


Ditto above. Wagtail 4.1 supports Django 3.2, 4.0, 4.1 only.

Resolved in 391be2e

Wagtail 4.0 changes this model name, so we need to update the migration to use the new name.

nickmoreton · 2023-03-02T12:50:25Z

Also when testing locally, I got this error: ValueError: Related model 'wagtailcore.pagerevision' cannot be resolved. Looks like one of the migration files needs updating.

Thanks this is now resolved in: 3c63ca4

nickmoreton · 2023-03-02T12:52:26Z

setup.py

-        "Framework :: Wagtail :: 2",
-        "Framework :: Wagtail :: 3.0",
-        "Framework :: Wagtail :: 4.0",
+        "Framework :: Wagtail :: 4",


This wasn't right either. I tend to follow the lead from wagtailmedia package

nickmoreton · 2023-03-02T12:56:21Z

Thanks @victoriachan sorry for missing so much here. They should all be resolved now.

nickmoreton · 2023-03-02T13:45:20Z

wagtail_ab_testing/models.py

@@ -77,7 +73,7 @@ class AbTest(models.Model):
    page = models.ForeignKey('wagtailcore.Page', on_delete=models.CASCADE, related_name='ab_tests')
    name = models.CharField(max_length=255)
    hypothesis = models.TextField(blank=True)
-    variant_revision = models.ForeignKey(get_revision_model(), on_delete=models.CASCADE, related_name='+')
+    variant_revision = models.ForeignKey('wagtailcore.Revision', on_delete=models.CASCADE, related_name='+')


Actually there was more that could be done here + a new migration.

This should fix sites that have the package already installed but the treatment_revision foreign key needs to be re-mapped as well.

I'm having second thoughts about this change. It probably needs more work to update the variant_revision fields. Like rebuild_references_index command in wagtail.

I agree with you @nickmoreton that this is not enough to migrate to the Revision model. I'm investigating the best way to go about this.

I've implemented a fix for this in #52 by squashing migrations. See the explanation in the PR description.

pymike00 · 2023-03-24T19:47:49Z

Thank you for this, @nickmoreton.

Much appreciated, gonna give it a try!

Do you actually work at Torchbox?

I wonder, with all due respect to the repo's owner and their time, why wasn't this PR even considered for merging?

Did they abandon the project?

Best,
M.

mwesterhof · 2023-04-04T14:17:17Z

i'm with @pymike00 on this one, would love to know if there's been any changes regarding the maintenance of this project. We're currently investigating options for adding A/B testing support to some wagtail projects, but the lack of support for newer WT versions is quite a deal breaker for us.

Is this something either of you knows the answer to, @kaedroho or @victoriachan ?

Stormheg

Wow, I can see quite the effort went into this. Thanks so much @nickmoreton ❤️

From a code-only perspective, almost all changes make sense to me... but I'm having difficulty testing this PR because I don't have an existing project (+ database) that uses this package to test this with. Because of this, I'm not going to approve this PR right now. Further (extensive) testing is required to make sure all works like expected.

Especially the migration from PageRevision to Revision is something that warrants further investigation to ensure it works for existing databases and new databases. Having an existing database to test with would be invaluable in testing.

I further suggest that we drop Python 3.7 support. Python 3.7 will be end-of-life next month.

Stormheg · 2023-05-21T14:35:14Z

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## [Unreleased]

+- Wagtail 4.2 compatibility
+- Old PR's for Wagtail 3.0 and Wagtail 4.0 are merged here


Suggested change

- Old PR's for Wagtail 3.0 and Wagtail 4.0 are merged here

Nitpick: this information is irrelevant for end-users (developers) using this package. The changelog should be kept concise.

Stormheg · 2023-05-21T14:37:58Z

README.md

@@ -18,6 +18,8 @@ Key features:

 ## Usage

+Wagtail A/B Testing works with Django 3.2+, Wagtail 4.1+ on Python 3.7+ environments.


Suggested change

Wagtail A/B Testing works with Django 3.2+, Wagtail 4.1+ on Python 3.7+ environments.

Wagtail A/B Testing works with Django 3.2+, Wagtail 4.1+ on Python 3.8+ environments.

Suggestion: let's drop official 3.7 support. Python 3.7 is almost 5 years old and will stop receiving updates next month (source)

Stormheg · 2023-05-21T14:38:27Z

setup.py

@@ -33,22 +33,23 @@
        "Programming Language :: Python :: 3.7",


Suggested change

"Programming Language :: Python :: 3.7",

(see above comment about dropping 3.7 support)

Stormheg · 2023-05-21T14:42:30Z

tox.ini

@@ -2,7 +2,9 @@
 skipsdist = True
 usedevelop = True

-envlist = py{37,38,39}-dj{22,30,32,main}-wa{211,215,216,main}-{sqlite,postgres}
+envlist = 
+    py{37,38,39,310}-dj{32,40,41}-wa{41,42}-{sqlite,postgres}


Suggested change

py{37,38,39,310}-dj{32,40,41}-wa{41,42}-{sqlite,postgres}

py{38,39,310}-dj{32,40,41}-wa{41,42}-{sqlite,postgres}

(see comment about dropping Python 3.7 support)

Stormheg · 2023-05-21T14:42:41Z

tox.ini

@@ -17,21 +19,23 @@ commands = coverage run testmanage.py test --deprecation all
 basepython =
    py37: python3.7


Suggested change

py37: python3.7

Stormheg · 2023-05-21T14:51:15Z

wagtail_ab_testing/test/urls.py

+    path("documents/", include(wagtaildocs_urls)),
+    path('abtestingapi/', include(ab_testing_api, namespace='ab_testing_api')),
+    path('abtesting/', include(ab_testing_urls, namespace='wagtail_ab_testing')),


Note: there is some inconsistent quoting here. Would be great if we could adopt a formatter like Black to enforce consistency.

This also what we should be doing according to the Python Package Maintenance guidelines (Continuous Integration section).

Not blocking, but something to consider to further improve this package in the future 🙂

Stormheg · 2023-05-21T14:53:04Z

wagtail_ab_testing/test/wagtail_hooks.py

-
-from wagtail.core import hooks
+from wagtail import hooks
+    


Suggested change

Stormheg · 2023-05-21T18:02:23Z

wagtail_ab_testing/models.py

@@ -77,7 +73,7 @@ class AbTest(models.Model):
    page = models.ForeignKey('wagtailcore.Page', on_delete=models.CASCADE, related_name='ab_tests')
    name = models.CharField(max_length=255)
    hypothesis = models.TextField(blank=True)
-    variant_revision = models.ForeignKey(get_revision_model(), on_delete=models.CASCADE, related_name='+')
+    variant_revision = models.ForeignKey('wagtailcore.Revision', on_delete=models.CASCADE, related_name='+')


I agree with you @nickmoreton that this is not enough to migrate to the Revision model. I'm investigating the best way to go about this.

nitinb99 · 2023-05-28T09:55:04Z

We are thrilled about incorporating the wagtail-ab-testing package into our project. However, since it doesn't currently support version 4.2, we are unable to use it. We appreciate the effort you have put into adding support for the 4.2 version. We hope that this pull request gets merged swiftly, and we eagerly await its integration. Thank you to @nickmoreton, @Stormheg and @victoriachan for your contributions.

Stormheg · 2023-06-18T14:44:43Z

Hey all,

I've spent a couple more hours on this and have come up with a revised PR (#52) that includes a fix for the PageRevision to Revision migration. This fix should work for new projects and projects already wagtail ab testing.

I don't think there are any other obstacles that prevent supporting Wagtail 4.1 and 4.2, hope my PR is merged soon 🤞

Stormheg · 2023-10-05T10:24:53Z

superseded by #52

nickmoreton added 24 commits May 17, 2022 22:19

Run wagtail updatemodulepaths

b2570ac

and adjust the import paths to allow different wagtail versions

don't pass the request to render_html in Wagtail >=3.0

7057e05

Use conditional to get comparison for Wagtail 3.0

4270456

Use conditional to check if the request is passed in addition to context

ee84fde

Django 4+ compatibility, use path in place of url

df77c21

Refactor render_html method

6395e06

Refactor is_shown method

36aaa15

Tody up method calls

8ec3e85

Adjust testmanage.py

143a64a

Bump django and wagtail versions

81430cd

Update README and CHANGELOG

f967dd6

Alter imports for project consistency

6de0840

Change to use version number checks

8965b34

Quiet WAGTAILADMIN_BASE_URL warning during tests

5630045

Be more consistent checking the wagtail version

3d437d4

Allow wagtail 4.0

2b631da

I removed the upper limits on wagtail and django versions.

Initial Wagtail 4 changes

4d3be79

Drop support for Wagtail < 2.15

8944ecb

Cant test django 4.0 & 4.1 on python 3.7

e9e443e

Remove unused import

e3a1fef

Merge remote-tracking branch 'origin/wagtail-4.0' into support/wagtail42

bf6d44f

Update dependencies for Wagtail > 4.1

f55646b

Update changelog

681399c

Drop compatibility code for Wagtail < 4.1

9190291

nickmoreton changed the title ~~WIP: Support wagtail42~~ Support wagtail42 Feb 13, 2023

victoriachan suggested changes Feb 27, 2023

View reviewed changes

nickmoreton added 2 commits March 2, 2023 12:39

Code review changes

391be2e

Swap migration to use revision instead of pagerevision

3c63ca4

Wagtail 4.0 changes this model name, so we need to update the migration to use the new name.

nickmoreton commented Mar 2, 2023

View reviewed changes

Extra work for the PageRevision change

c10d3aa

nickmoreton commented Mar 2, 2023

View reviewed changes

Stormheg reviewed May 21, 2023

View reviewed changes

Stormheg mentioned this pull request Jun 18, 2023

Wagtail 4.1 and 4.2 support (revised) #52

Merged

Stormheg closed this Oct 5, 2023

		@@ -18,6 +18,10 @@ Key features:

		## Usage

		The code examples below assume you are using Wagtail v3.0+ If you need information for an earlier version of Wagtail see the [Release notes](https://docs.wagtail.org/en/stable/releases/upgrading.html) for guidance.

		@@ -1 +1,4 @@
		default_app_config = "wagtail_ab_testing.test.apps.WagtailAbTestingTestAppConfig"
		from django import VERSION as DJANGO_VERSION

		@@ -18,6 +18,8 @@ Key features:

		## Usage

		Wagtail A/B Testing works with Django 3.2+, Wagtail 4.1+ on Python 3.7+ environments.

	py{37,38,39,310}-dj{32,40,41}-wa{41,42}-{sqlite,postgres}
	py{38,39,310}-dj{32,40,41}-wa{41,42}-{sqlite,postgres}

		@@ -17,21 +19,23 @@ commands = coverage run testmanage.py test --deprecation all
		basepython =
		py37: python3.7

Support wagtail42 #51

Support wagtail42 #51

Conversation

nickmoreton commented Feb 13, 2023 • edited Loading

victoriachan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickmoreton Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickmoreton Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickmoreton commented Mar 2, 2023

Choose a reason for hiding this comment

nickmoreton commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pymike00 commented Mar 24, 2023

mwesterhof commented Apr 4, 2023

Stormheg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitinb99 commented May 28, 2023

Stormheg commented Jun 18, 2023

Stormheg commented Oct 5, 2023

nickmoreton commented Feb 13, 2023 •

edited

Loading

nickmoreton Mar 2, 2023 •

edited

Loading

nickmoreton Mar 2, 2023 •

edited

Loading