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

Translatable ModelAdmin #550

Merged
merged 13 commits into from
May 3, 2022

Conversation

dinoperovic
Copy link
Contributor

This PR adds support for translating pages and models registered using Wagtail's modeladmin.
It mostly re-uses the snippets approach and exposes the TranslatableModelAdmin class.

Translatable ModelAdmin has been a requirement in my personal projects and I have been using a similar setup that works pretty well.

Fixes #291.

A less intrusive approach that I was considering is to release a separate wagtail-localize-modeladmin package. In
this case the snippet views can be "patched" by placing the app above wagtail.snippets and handling redirects to modeladmin instances when applicable.

Curious to hear your thoughts and plans for supporting ModelAdmin localization.

@dinoperovic dinoperovic changed the title Translatable ModeAdmin Translatable ModelAdmin Apr 1, 2022
@zerolab
Copy link
Collaborator

zerolab commented Apr 1, 2022

Thank you for this @dinoperovic
Had a cursory look and it feels the views side of things (or part of it) could be addressed by wagtail/wagtail#7857. Can you double check?

Will aim to get some review time next week.

@dinoperovic
Copy link
Contributor Author

@zerolab Yeah I've missed that one. It looks like this could replace some parts of view logic. The views would still need to be overridden to display translation pages etc. Also, to support the previous Wagtail versions. It would be a good idea to align with the approach in the wagtail/wagtail#7857 once it's accepted.

I can take a closer look next week.

@zerolab
Copy link
Collaborator

zerolab commented Apr 2, 2022

cc @kaedroho (in the context of modeladmin/snippets thinking)

@codecov-commenter
Copy link

Codecov Report

Merging #550 (abbac64) into main (8278ab9) will increase coverage by 0.25%.
The diff coverage is 95.89%.

@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
+ Coverage   91.41%   91.66%   +0.25%     
==========================================
  Files          37       42       +5     
  Lines        3263     3457     +194     
  Branches      529      557      +28     
==========================================
+ Hits         2983     3169     +186     
- Misses        161      165       +4     
- Partials      119      123       +4     
Impacted Files Coverage Δ
wagtail_localize/wagtail_hooks.py 80.11% <ø> (ø)
wagtail_localize/models.py 95.85% <33.33%> (-0.28%) ⬇️
wagtail_localize/views/edit_translation.py 86.55% <33.33%> (-0.68%) ⬇️
wagtail_localize/modeladmin/views.py 97.53% <97.53%> (ø)
wagtail_localize/modeladmin/__init__.py 100.00% <100.00%> (ø)
wagtail_localize/modeladmin/helpers.py 100.00% <100.00%> (ø)
wagtail_localize/modeladmin/options.py 100.00% <100.00%> (ø)
wagtail_localize/test/models.py 97.56% <100.00%> (+0.02%) ⬆️
wagtail_localize/test/wagtail_hooks.py 100.00% <100.00%> (ø)
wagtail_localize/views/submit_translations.py 94.76% <100.00%> (+0.68%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8278ab9...abbac64. Read the comment docs.

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Hey @dinoperovic,

This is a lot of excellent work, thank you.
Left a number of questions and comments.

Additionally, this could really use some documentation

wagtail_localize/modeladmin/helpers.py Outdated Show resolved Hide resolved
wagtail_localize/modeladmin/options.py Outdated Show resolved Hide resolved
wagtail_localize/modeladmin/views.py Show resolved Hide resolved
wagtail_localize/modeladmin/views.py Show resolved Hide resolved
wagtail_localize/views/edit_translation.py Outdated Show resolved Hide resolved
wagtail_localize/views/submit_translations.py Outdated Show resolved Hide resolved
wagtail_localize/models.py Show resolved Hide resolved
@dinoperovic
Copy link
Contributor Author

Hey @zerolab,

I've made the requested changes, still missing the documentation which I will look to add.

- used named placeholders for string interpolation
- removed re-export of wagtail modeladmin
- added more comments in modeladmin views
- refactored modeladmin as an optional separate sub-app
- moved modeladmin submit view to modeladmin.views
- moved tests to modeladmin/tests.py
@dinoperovic dinoperovic force-pushed the feature/translatable-modeladmin branch from fc0f36f to 6005400 Compare April 23, 2022 13:03
Copy link
Contributor

@kaedroho kaedroho left a comment

Choose a reason for hiding this comment

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

This is excellent work, thanks! Just one minor thing I wanted to question

@dinoperovic dinoperovic requested a review from kaedroho April 25, 2022 10:54
@zerolab zerolab merged commit 51d98a2 into wagtail:main May 3, 2022
@zerolab
Copy link
Collaborator

zerolab commented May 3, 2022

Merged. Thank you @dinoperovic. Will make a new release sometime towards the end of the week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModelAdmin doesn't have Locale
4 participants