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

Wagtail 5.1 upgrades and test workflow update #3

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Conversation

katdom13
Copy link

@katdom13 katdom13 commented Oct 4, 2023

Upstream PR: labd#227

Copy link
Collaborator

@nickmoreton nickmoreton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve this but left a question. Thanks

Comment on lines +48 to +49
# 'wagtail_modeladmin', # if Wagtail >=5.1; Don't repeat if it's there already
'wagtail.contrib.modeladmin', # if Wagtail <5.1; Don't repeat if it's there already
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this means, sorry.

Comment on lines +32 to +33
# "wagtail_modeladmin", # if Wagtail >=5.1; Don't repeat if it's there already
"wagtail.contrib.modeladmin", # if Wagtail <5.1; Don't repeat if it's there already
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nickmoreton ,
I based it on Jazzband's update here:
https://github.com/jazzband/wagtailmenus/pull/461/files#diff-e285c1b9901cd202e89d9f37e3249a6fee524a65428895e1a45fd062cc0f9acaR22-R23

Which is a resolution for this Wagtail 5.1 upgrade consideration: wagtail.contrib.modeladmin is deprecated

I updated the PR to actually replace the installed app depending on the Wagtail version.
b00c2b7

@ArnarTumi
Copy link

Hey, any chance that this PR and PR#2 could be merged? If not, any chance that you could add the line from #2 to this PR @katdom13 so that we could use this branch as temporary solution for WT5.1 until it will be merged?

Copy link

@Stormheg Stormheg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that this is not actually compatible with Wagtail 5.1!

The otp_form.html template includes an SVG logo that was deleted in wagtail/wagtail@ecb2126

I've done a quick 'n dirty fix on my fork here: techonomydev@7fe52ba but you may want to create proper fix because my fix breaks support for Wagtail 5.0 and older.

@katdom13
Copy link
Author

Please note that this is not actually compatible with Wagtail 5.1!

The otp_form.html template includes an SVG logo that was deleted in wagtail/wagtail@ecb2126

I've done a quick 'n dirty fix on my fork here: techonomydev@7fe52ba but you may want to create proper fix because my fix breaks support for Wagtail 5.0 and older.

Hi @Stormheg ,
Thanks for checking! How about this workaround? 695027b

@ArnarTumi ,
I have merged the other PR.

Thanks!

@Stormheg
Copy link

Your fix looks good @katdom13 👍

What would be even better is if you could add a test that checks if the otp form page renders okay without crashing. A smoke test.

That way we will be alerted if Wagtail changes one if its internal template includes again. What do you think?

@nickmoreton
Copy link
Collaborator

Hey, any chance that this PR and PR#2 could be merged? If not, any chance that you could add the line from #2 to this PR @katdom13 so that we could use this branch as temporary solution for WT5.1 until it will be merged?

@ArnarTumi This has been rebased now on the maintainers master branch so "wagtailadmin_sprite", has been added in.

@nickmoreton
Copy link
Collaborator

@katdom13 All good thanks and tested locally using the sandbox

@katdom13
Copy link
Author

@Stormheg , @nickmoreton ,
Will be adding test in another PR. Merging this for now.

@katdom13 katdom13 merged commit 45f0206 into master Nov 10, 2023
4 checks passed
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 this pull request may close these issues.

4 participants