jazzband / wagtailmenus

An app to help you manage and render menus in your Wagtail projects more effectively
MIT License
398 stars 138 forks source link

Migrate from ModelAdmin to Snippets #472

Closed dkirkham closed 8 months ago

dkirkham commented 10 months ago

Fixes #459 for Wagtail >= 5.2.

This fairly large PR removes Wagtail ModelAdmin from the wagtailmenus admin pages, replacing it with Wagtail Snippets.

My approach has been keep as much of the existing wagtailmenus functionality as possible, and absolutely minimise the number of changes to the existing test suite. If functionality changes are desired, these should be discussed and designed accordingly and prepared as separate PRs.

I have also only been targeting Wagtail 5.2 LTS, because earlier versions (particularly 4.1 LTS and 5.1) are supported by existing wagtailmenus releases, and are as a whole only in support for a couple more weeks anyway. That said, the tests appear to work for some earlier Wagtail releases, although I haven't yet validated whether the UI is fully functional. The tests also also work, excepting one HTML markup test, against the current github wagtail#main and that problem may just be a test setup issue.

Given the changes are significant, and I would encourage any users of this package to test it within their overall projects before it is merged or released.

There are a few more things left to do, certainly before release, and the details here may depend on any real-life testing:

MrCordeiro commented 9 months ago

Hey @dkirkham, sorry for the delay in my response.

I had planned to look at this during the weekend, but then got caught up with an unexpected tasks. I'll review your pull request first thing this Saturday. In the meanwhile, I also added @ababic as a reviewer. He's the original author and may offer a much more valuable insight than I could (if he is available, of course).

dkirkham commented 9 months ago

Hi @MrCordeiro, thanks for the feedback and testing. Responding to your questions:

  1. Wagtail 5.2 LTS is out now and for another year, so the aligning timing of this wagtailmenus release with Wagtail 6 (Feb) doesn’t achieve much. What is probably more useful is to see if this new wagtailmenus release can support 5.2 LTS and the forthcoming Wagtail 6. We can test that soon as the RC is imminent.
  2. Sure, while I updated one of the docs, I didn’t check through the detailed documentation to keep the PR more focussed. Would you like me to add these changes to this PR?
vbabiy commented 8 months ago

Any chance we can get this merged and released? Its blocking wagtail 6 upgrades.

schlich commented 8 months ago

Yes I will take a look

dkirkham commented 8 months ago

Sorry all, I've been a bit busy with other things over recent weeks. There is still some documentation to update, and now that Wagtail 6 is out, I'd like to see if we can get compatibility with that and 5.2 LTS. I'll be working on this later today.

dkirkham commented 8 months ago

I've just added Wagtail 6.0 support - there were only a couple of changes as I had expected. I still need to test this within an existing project of mine, which I'll do tomorrow.

@vbabiy, @schlich and others, have you tried this update in your projects, and have any issues arisen? If not, I think we are close to being able to do a release - just some doco to update.

vbabiy commented 8 months ago

I will give this a quick test and let you know.

dkirkham commented 8 months ago

I tested wagtailmenus in one of my projects this evening, using both Wagtail 5.2 and Wagtail 6.0.1, and haven't detected any issues.

I did however encounter an issue during installation, it seems installing from github didn't reliably remove and change all the package files. I found I had to:

If wagtailmenus is the last dependence you have on Wagtail ModelAdmin, then you can also remove it:

I'll start working on the documentation this Wednesday, unless someone else wants to start on it earlier.

vbabiy commented 8 months ago

Worked with no issues for me as well.

schlich commented 8 months ago

i'll take y'alls word on when this is ready to release and i can push the pypi button. @MrCordeiro if you have any reservations make 'em known!

schlich commented 8 months ago

If all we're waiting on is docs i'm gonna go ahead and merge, we can merge the docs when theyre ready

schlich commented 8 months ago

Made a follow-up issue for outstanding documentation ^^ clarifications welcome

schlich commented 8 months ago

Opened a PR for a release candidate at #480