search

wagtail / wagtail-localize

Translation plugin for Wagtail CMS
https://wagtail-localize.org/
Other
222 stars 84 forks source link

Allow defining translatable_fields/override_translatable_fields on StructBlock #752

Open freddiemixell opened 8 months ago

freddiemixell commented 8 months ago

This PR is attempting to allow users to mark certain sub-blocks of a struct block to be ignored like UrlBlock.

Example use case: blocks.py

class RichTextBlock(blocks.StructBlock):
    """Rich text block."""

    rich = blocks.RichTextBlock(
        required=True,
        help_text="Add a rich text context",
        features=['h2', 'h3', 'h4', 'h5', 'h6', 'bold', 'italic', 'link', 'ol', 'ul', 'hr', 'superscript', 'subscript', 'strikethrough', 'blockquote', 'code'],
    )

class CustomImageBlock(blocks.StructBlock):
    """Banner image block."""

    image = ImageChooserBlock(required=True, help_text="Add a banner image.")
    description = blocks.TextBlock(required=False, help_text="Add a description for the image.")

    override_translatable_blocks = [
        "description"
    ]

class YouTubeBlock(blocks.StructBlock):
    """YT Video block."""

    video_id = blocks.CharBlock(
        required=True,
        help_text="Add a YouTube video ID"
    )

    override_translatable_blocks = []

class CodeBlock(blocks.StructBlock):
    """Code block."""

    code = blocks.TextBlock(required=True, help_text="Add your code snippet here.")
    language = blocks.ChoiceBlock(choices=[
        ('python', 'Python'),
        ('javascript', 'JavaScript'),
        ('java', 'Java'),
        ('c', 'C'),
    ], required=True, help_text="Select the language of your code snippet.")

    override_translatable_blocks = []

In the above example the first block doesn't define the property so it goes through the code the same as before.

The second block only includes the description field, which technically doesn't matter because the image would be skipped regardless. However, I added it in to illustrate how you could exclude fields by simply not adding them to the list.

The last 2 examples we add the property but with an empty list. That will prevent any blocks from this struct block from being extracted.

Notes: Closes #307

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a3efa8f) 92.60% compared to head (5fb32e5) 92.65%. Report is 13 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #752 +/- ## ========================================== + Coverage 92.60% 92.65% +0.05% ========================================== Files 46 47 +1 Lines 4017 4072 +55 Branches 598 603 +5 ========================================== + Hits 3720 3773 +53 - Misses 175 176 +1 - Partials 122 123 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zerolab commented 8 months ago

Thank you @freddiemixell. Could you please add some tests?

freddiemixell commented 8 months ago

No problem! I begin working on them already but alas the work week came and stole me away lol

I will have some time starting this weekend and I'm off all next week so I will wrap this up then if I don't find during the week.

freddiemixell commented 8 months ago

@zerolab I figured out some intial tests. Let me know if you'd like more or any changes.

Happy holidays! ☃️

freddiemixell commented 8 months ago

This is a solid start @freddiemixell

Can you update

https://github.com/wagtail/wagtail-localize/blob/92c18ab055cbd1de9f039c03278a70fbc5010178/docs/how-to/field-configuration.md?plain=1#L3-L5

and remove the notices, as well as add a section at the end on how to use this? Aside from that, I keep going back to the fact that for regular models we allow defining whether a field is translatable or is synchronised, and that we need to support this for StructBlocks. For example, in your CustomImageBlock we want the description to be translatable, but the image to be synchronised. I am making an assumption here as I am not fully geared to test things locally ('tis Christmas after all), but what happens in that case?

Hey @zerolab,

Thanks for checking this out on Christmas! (Tis Christmas made me lol)

I see what you're saying about the image block and synchronization. I'll investigate if this will cause any issues with synchronized fields today.

If you have any specific tests you're thinking of let me know and I'll try to hook it up, thanks again!

freddiemixell commented 8 months ago

How I'm Testing

How the source test post is structured:

sourceadmin

Scenario 1: Testing ignoring the description struct field

StructBlock:

Expected:

Result:

English Page

1EN

French Page

1FR

Scenario 2: Testing explicitly telling it description should be translated

StructBlock:

Expected:

Result:

English Page

2EN

French Page

2FR

Scenario 3: Testing default/backwards compatibility

StructBlock:

Expected:

Result:

English Page

3EN

French Page

3FR
freddiemixell commented 8 months ago

I'll add that I'm not sure why exactly this is working with the images. That is what I'm going to work on figuring out now so I could really get behind this or find any flaws.

freddiemixell commented 8 months ago

Issue:

When overriding the image of the youtube button in the french version with a picture of the Eiffel Tower I still see the Youtube button.

I believe this means it's breaking the overridable segments somehow.

Update:

I reverted this change a second time, reset everything and now I do see the image block going through so this seems like a problem with this PR still.

Whats actually happening: After testing this a bit further I'm seeing that I was mistaken before. I was getting confused because I had an image field on my blog model and I was mistaking that for the image block. I changed it to another image and that revealed the issue.

translatable_blocks = ['description']

This prevents the images from being overridden, which is sort of enforced by this configuration if you wanted to prevent images from being changed for some reason.

translatable_blocks = ['image']

However, this configuration produces a different scenario where you can keep the same text but allow overriding the image. When I did this I was able to override the image successfully in the French version. Which means this isn't technically a bug but does allow for a little more control over nested images if you need it.

zerolab commented 8 months ago

@freddiemixell the code tests should pretty much follow what you test as a user, manually

zerolab commented 7 months ago

Hey @freddiemixell do you need any help / pointers on tests?

danielfmiranda commented 3 months ago

Hi @zerolab! 👋

I'm Daniel from the Mozilla Foundation, on a team that heavily relies on Wagtail/wagtail-localize for our projects.

We have a stakeholder request that would be directly fulfilled by the work in this PR.

We noticed that your team had previously offered assistance with adding tests, but there hasn't been a response yet.

We were wondering if there are any updates on this PR or an estimated timeline for when it might be merged. This information would help us set the right expectations with our own stakeholders.

Additionally, if there's anything we can do to help move this forward, please let us know!

Thanks 👍

zerolab commented 3 months ago

Hey @danielfmiranda,

It is a difficult one in that I had lots of non-localize work to attend to.

If you/your team have the capacity, a PR that adds tests to this would move things quite a bit. Otherwise, I'll probably realistically get some time during Wagtail Space Netherlands which is mid-June.