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

Translation of models with ParentalManyToManyField #564

Merged
merged 1 commit into from
Oct 1, 2023

Conversation

hpoul
Copy link
Contributor

@hpoul hpoul commented May 4, 2022

My try to fix #563
I basically duplicated the code from django-modelcluster copy_cluster: https://github.com/wagtail/django-modelcluster/blob/8666f16eaf23ca98afc160b0a4729864411c0563/modelcluster/models.py#L386-L392

@zerolab zerolab requested a review from kaedroho May 4, 2022 14:59
@zerolab zerolab added this to the 1.2 milestone May 4, 2022
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 @hpoul,

Thank you for this PR. Can you rebase on latest main and re-generate the migration? we reduced the test app migrations to just 0001_initial

This works well to sync the values, but they are not exposed in the edit translation UI which could potentially cause additional confusion

Could you look at handling that too, especially when it comes to Page models. https://github.com/wagtail/wagtail-localize/blob/main/wagtail_localize/views/edit_translation.py#L466-L516 is how that is happening at the moment

@hpoul hpoul force-pushed the parental-many-to-many-fail branch from 4b2dc49 to a1492cc Compare May 6, 2022 18:49
@hpoul
Copy link
Contributor Author

hpoul commented May 6, 2022

@zerolab thanks for taking a look

Can you rebase on latest main and re-generate the migration? we reduced the test app migrations to just 0001_initial

i've now rebased. I'm not sure about the migrations.. i've deleted the old once and recreated a new one.. Was this what you meant, or should it also be part of the 0001_initial migration? 🤔

This works well to sync the values, but they are not exposed in the edit translation UI which could potentially cause additional confusion

Are SynchronizedFields actually exposed in the translation UI? 🤔 Should this simply list the selected IDs or str() of the objects?

Would it be possible to make this as part of a separate PR? This error blocks me from translating some content in my project.. hopefully the last problem 😬

btw. do you know of some easy way to use a forked version as a dependency? The problem is including wagtail-localize as a git dependency doesn't work because it would require manual npm build to generate the static files.. 🤔 I guess I could just commit the generated files into my forked git repo, but that would make merging pretty cumbersome..

@zerolab
Copy link
Collaborator

zerolab commented May 6, 2022

On using a fork.. you cannot really do it because of the FE assets.. best to build it locally and host the built package somewhere you can install from a URL

Will revisit the synchedfield question tomorrow.

@zerolab
Copy link
Collaborator

zerolab commented May 9, 2022

Hey @hpoul,

SynchronisedField, or "overridable segment", does show in the UI. By default it syncs the value from the source, but the value can be overridden. See https://www.wagtail-localize.org/concept/segments/#overridable-segment

@zemogle
Copy link

zemogle commented May 24, 2022

Did this make it into v1.2? I'm having the exact same problem.

@zerolab
Copy link
Collaborator

zerolab commented May 24, 2022

@zemogle this PR is still open, so it did not, I'm afraid

@zerolab zerolab modified the milestones: 1.2, 1.4 - the tidy up release Oct 7, 2022
@zerolab zerolab force-pushed the main branch 3 times, most recently from c7c2117 to 1fc56b0 Compare January 22, 2023 13:46
@lvonlanthen
Copy link

@hpoul I am having the same problem and your fix would be very useful. It has been a while, but what is the state of this PR? Is there a way to help you getting this PR merged? Thanks!

@hpoul
Copy link
Contributor Author

hpoul commented Jun 29, 2023

@lvonlanthen see #564 (review)

This works well to sync the values, but they are not exposed in the edit translation UI which could potentially cause additional confusion

I don't have enough time it understanding in the inner workings of the wagtail admin to fix this.. feel free to take over this PR.. 🤷‍♂️

@hpoul hpoul force-pushed the parental-many-to-many-fail branch 2 times, most recently from afcaf41 to fdf7926 Compare September 30, 2023 10:21
…#563

regenerate migrations (+4 squashed commits)
Squashed commits:
[d65cc37] remove unnecessary migration dependency.
[be6fe90] support copying of fields in ParentalManyToManyField
[fd1a699] failing test for ParentalManyToManyField
[48b512c] testmanage.py makemigrations --merge
@hpoul hpoul force-pushed the parental-many-to-many-fail branch from fdf7926 to 123f06d Compare September 30, 2023 10:28
@hpoul
Copy link
Contributor Author

hpoul commented Sep 30, 2023

I've rebased on main and squashed commits.. is there any chance to get this merged without the UI when it's not synchronized? @zerolab At least it fixes the exception when creating the translation.. I don't see how it would make anything worse..

@codecov-commenter
Copy link

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e2de395) 93.27% compared to head (123f06d) 93.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #564      +/-   ##
==========================================
- Coverage   93.27%   93.23%   -0.04%     
==========================================
  Files          47       47              
  Lines        3908     3918      +10     
  Branches      579      583       +4     
==========================================
+ Hits         3645     3653       +8     
  Misses        154      154              
- Partials      109      111       +2     
Files Coverage Δ
wagtail_localize/test/models.py 98.80% <100.00%> (+0.01%) ⬆️
wagtail_localize/fields.py 91.22% <71.42%> (-1.37%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zerolab zerolab merged commit 9f7a899 into wagtail:main Oct 1, 2023
10 of 13 checks passed
@zerolab
Copy link
Collaborator

zerolab commented Oct 6, 2023

@hpoul now in https://github.com/wagtail/wagtail-localize/releases/tag/v1.6
sorry for the delay!

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