Skip to content

Snippet translations + translatable fields#6777

Merged
KalobTaulien merged 42 commits intowagtail-localize-2021from
6708-snippet-translations
Jun 10, 2021
Merged

Snippet translations + translatable fields#6777
KalobTaulien merged 42 commits intowagtail-localize-2021from
6708-snippet-translations

Conversation

@KalobTaulien
Copy link
Contributor

@KalobTaulien KalobTaulien commented May 31, 2021

Closes #6708
Related PRs/issues #6703

Acceptance criteria

Note: Don't forget to reference the document from Step 5 #6707

  • Create migrations for all snippets. This document will be very helpful. We need to bootstrap migrations for each model.
    • Make sure all snippet models are translated.
    • Make sure all snippet fields have translatable_fields = [ .. ] set on them
    • Make sure tests still pass. May need to do some adjusting for new language headers in the request object, particularly around redirects.
  • Get Theo to review this (specifically, Theo) to make sure all the fields are the right translation type.

@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj May 31, 2021 19:39 Inactive
@KalobTaulien KalobTaulien marked this pull request as ready for review May 31, 2021 19:46
@KalobTaulien
Copy link
Contributor Author

Locally all the tests pass.

@Pomax I pinged you for code review on this.
@TheoChevalier I pinged you for semi-code review but mostly to make sure I got all the fields right. Your doc was very helpful. I made some notes in the doc too and highlighted some lines for you to take a look at.

@Pomax Pomax force-pushed the wagtail-localize-2021 branch from b9a3842 to f7371d6 Compare May 31, 2021 21:02
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj May 31, 2021 22:24 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj May 31, 2021 22:25 Inactive
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

some minor notes, and a question

objects = NewsQuerySet.as_manager()

class Meta:
class Meta(TranslatableMixin.Meta):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's check if this is actually necessary or not. If we don't need to mark the Meta class as translatable, that saves some migration work. (not much, but "not doing anything we don't need to do is worth not doing" =D)

@TheoChevalier
Copy link
Contributor

I’ve made a few changes to the spreadsheet (all highlighted in orange), mostly answering your questions.
A few fields in snippets are not properly hooked (I’ve highlighted them in red in the Snippets sheet)

Also hit a few errors/have some questions:

PNI General product:

Translating a general product raises:
Capture d’écran 2021-06-01 à 15 48 46

Campaign page

Translating a campaign page that has a petition + donate snippets raises:
Capture d’écran 2021-06-01 à 14 15 12

Youtube Regrets

Regret stories are missing. Is this because it’s a block and not part of the model?

Article pages:

For Text and ID https://docs.google.com/spreadsheets/d/1AU7pxboZoBiVcbTpZO-VUMAhxZwx5mdEH_FBLfo2D-w/edit#gid=0&range=A43 these are fields I’ve found at the bottom of article pages, but I haven’t been able to find them in the model code
Capture d’écran 2021-06-01 à 16 04 55

Homepage

Here too, a lot of the content is not localized, is this because these are blocks and it will come later?
Also it seems every page picker field need to be Translatable, otherwise links will point to /en/…
This affects all page pickers in News you can use, Spotlight, Take action, etc

@TheoChevalier TheoChevalier removed their request for review June 1, 2021 14:49
Co-authored-by: Pomax <pomax@nihongoresources.com>
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj June 1, 2021 15:30 Inactive
KalobTaulien and others added 4 commits June 1, 2021 09:31
Co-authored-by: Pomax <pomax@nihongoresources.com>
Co-authored-by: Pomax <pomax@nihongoresources.com>
Co-authored-by: Pomax <pomax@nihongoresources.com>
Co-authored-by: Pomax <pomax@nihongoresources.com>
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj June 4, 2021 17:20 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj June 4, 2021 17:39 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj June 4, 2021 17:54 Inactive
@KalobTaulien KalobTaulien requested a review from Pomax June 4, 2021 17:59
@mofodevops mofodevops had a problem deploying to foundation-s-6708-snipp-fldhsj June 4, 2021 17:59 Failure
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

looks like snippets at least work now, but we'll need to comb the code on Monday to make sure we caught all of them. We'll also want to fix what looks like a migration numbering issue.

@mofodevops mofodevops had a problem deploying to foundation-s-6708-snipp-fldhsj June 7, 2021 21:56 Failure
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj June 9, 2021 15:23 Inactive
@KalobTaulien KalobTaulien requested a review from Pomax June 9, 2021 15:23
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj June 9, 2021 17:57 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-6708-snipp-fldhsj June 9, 2021 18:58 Inactive
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

Let's get this landed and then deal with the followup as new work.

@KalobTaulien
Copy link
Contributor Author

Ticket made for the campaign page problem. #6851

@KalobTaulien KalobTaulien merged commit 6ad7cb8 into wagtail-localize-2021 Jun 10, 2021
@KalobTaulien KalobTaulien deleted the 6708-snippet-translations branch June 10, 2021 15:57
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