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.

Sign up for GitHub

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

Jump to bottom

Use the latest draft for translation (#7754) #7755

Closed
wants to merge 8 commits into from
Closed

Use the latest draft for translation (#7754) #7755

wants to merge 8 commits into from

Conversation

gnomeby
Copy link
Contributor

@gnomeby gnomeby commented Dec 3, 2021 •
edited by zerolab

Fix for #7754

Thanks for contributing to Wagtail! 🎉

Before submitting, please review the contributor guidelines https://docs.wagtail.io/en/latest/contributing/index.html and check the following:

  • Do the tests still pass? (https://docs.wagtail.io/en/latest/contributing/developing.html#testing)
  • Does the code comply with the style guide? (Run make lint from the Wagtail root)
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For front-end changes: Did you test on all of Wagtail’s supported environments?
    • Please list the exact browser and operating system versions you tested.
    • Please list which assistive technologies you tested.
  • For new features: Has the documentation been updated accordingly?

@squash-labs
Copy link

squash-labs bot commented Dec 3, 2021

Manage this branch in Squash

Test this branch here: https://gnomebygnomeby-patch-7754-bxtlm.squash.io

@gnomeby gnomeby mentioned this pull request Dec 3, 2021
Closed
@lb-
Copy link
Member

lb- commented Dec 6, 2021

@gnomeby thanks for taking the time to contribute a fix for #7754 - In principle this looks good but we will get another review from a core team member also.

However, we ask that you take a bit more time to contribute a unit test that covers this scenario with the fix. This way it will be much harder to break this in the future.

@lb- lb- added component:i18n i18n for content created in Wagtail, not the admin UI itself status:Needs Review status:Needs Tests labels Dec 6, 2021
@lb- lb- requested a review from allcaps December 6, 2021 21:25
@gnomeby
Copy link
Contributor Author

gnomeby commented Dec 7, 2021

OK, I'll add them.

@allcaps
Copy link
Member

allcaps commented Jan 20, 2022 •
edited

@gnomeby Thank you for your contribution to Wagtail. The code looks sound. However, I doubt this is the desired behaviour.

Why is copying the last revision more correct than copying the current page? Copying the last revision might come as a surprise to some users. Or am I missing something? Is there a scenario I did not consider? Why is the the latest revision a better source for a translation than the current page?

Revisions are the last 'draft' saved by anyone. That doesn't mean the content is more correct than the current page. The revision might be a rough -and failed- rewrite of the page. That should not be published. Ever. Let alone be translated. I consider the current page as the most correct content. The content most suitable to be translated.

TLDR; The latest revision doesn't have to be 'more correct' than current page.

I'm not 100% sure about my own argumentation, so I'm happy to convinced otherwise.

FYI, simple translation is a perfect starting point for your own translation app. Copy simple_translation from site-packages/wagtail/contrib/simple_translation into your project root and apply your changes. Change installed apps "wagtail.contrib.simple_translation" to "simple_translation". You can take it a step further, add a choice field to the form to ask the user which 'source' to use.

allcaps
allcaps reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@allcaps allcaps left a comment

Choose a reason for hiding this comment

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

I don't think last revision is 'better' content than the current page. A revision might even be a failed draft. Therefore not a suitable source for translations.

@gnomeby
Copy link
Contributor Author

gnomeby commented Jan 20, 2022

@allcaps please take a look to description of issue:
#7754
I'm trying to translate document with only draft revisions.
But for the case when we have published+draft versions you are right, my fix is not suitable and should be improved.

@zerolab zerolab changed the title 7754 Use the latest draft for translation Use the latest draft for translation (#7754) Jan 20, 2022
@gasman
Copy link
Collaborator

gasman commented Jan 20, 2022

I think this might be related to the common "gotcha" with pages that have never been published, seen most recently in #7690 (comment) - if a page has never been published, then the 'live' page record will reflect the state of the page when it was first created and saved for the first time - subsequent 'save draft' actions will only affect page revisions, so it will never move on from that state. In this case, the outcome is less desirable (and more surprising) than selecting the latest draft revision.

Detecting pages that have never been published is a bit of a black art, so I'd suggest that we also select the latest draft for pages that were published and then unpublished. (In these cases, the 'live' page record will be frozen at the last published version, which may be far out of date.)

In other words - when copying a page for translation, I'd suggest the following logic:

  • does the page's real database record have live=True?
    • if yes, copy the page data from the real (live) database record
    • if no, copy the page data from the latest (draft) revision

@gnomeby
Copy link
Contributor Author

gnomeby commented Jan 20, 2022

@gasman agree

@gnomeby
Copy link
Contributor Author

gnomeby commented Mar 17, 2022

I've updated PR using @gasman advice

@chosak chosak mentioned this pull request May 1, 2023
Closed
@lb-
Copy link
Member

lb- commented Aug 5, 2023

There has been some more recent work on how non-saved drafts are treated. This needs some rework and maybe some discussion before it's able to be reviewed again. Flagging as needs work / needs design decision.

@zerolab zerolab requested a review from gasman September 13, 2023 15:44
gasman
gasman approved these changes Nov 8, 2023
View reviewed changes
wagtail/actions/copy_for_translation.py
@@ -146,7 +148,11 @@ def execute(self, skip_permission_checks=False):
self.check(skip_permission_checks=skip_permission_checks)

translated_page = self._copy_for_translation(
self.page, self.locale, self.copy_parents, self.alias, self.exclude_fields
self.page.live and self.page or self.page.get_latest_revision_as_page(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

self.page if self.page.live else self.page.get_latest_revision_as_page() is the more familiar syntax I think, but happy with this logic.

gasman added a commit that referenced this pull request Nov 8, 2023
@gasman @gnomeby
Use the latest draft when copying an unpublished page for translation (…
32c48e1 
…#7755)

Fixes #7754

Co-authored-by: Andrey Nehaychik <andrey.nehaychik@vizor-games.com>
@gasman
Copy link
Collaborator

gasman commented Nov 8, 2023

Rebased and merged in 32c48e1 - thanks @gnomeby!

@gasman gasman closed this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allcaps allcaps allcaps left review comments

@gasman gasman gasman approved these changes

Assignees
No one assigned
Labels
component:i18n i18n for content created in Wagtail, not the admin UI itself status:Needs Design Decision status:Needs Tests status:Needs Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gnomeby @lb- @allcaps @gasman