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

dev/core#4409 Use translation for default language, if exists #26232

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 16, 2023

Overview

There is an assumption in the code that for the default language a translation is not used.

However, the features around draft versions are all set up around translations, so it becomes necessary to create translations even for the default language. This change ensures they are used if they exist.

Before

It is possible to create a translation for the site default language, and the presence of the draft / approval flow makes this a good thing to do. However, the render function will ignore it

After

If it has been created it will be used. Note that the check for 'any' of the given language is primarily about performance, not functionality - everything falls back to the message template if there is no translation.

Technical Details

Although we have a lot of complexity in the translation framework on the rendering side I think assuming that where stuff is configured people configured it to be used is a good underlying principle.

Comments

https://lab.civicrm.org/dev/core/-/issues/4409

@civibot
Copy link

civibot bot commented May 16, 2023

(Standard links)

@civibot civibot bot added the master label May 16, 2023
@eileenmcnaughton eileenmcnaughton force-pushed the trans_locale branch 2 times, most recently from 8fdac5b to f77d934 Compare May 16, 2023 06:46
@eileenmcnaughton
Copy link
Contributor Author

test this please

wmfgerrit pushed a commit to wikimedia/wikimedia-fundraising-crm that referenced this pull request May 22, 2023
Includes civicrm/civicrm-core#26232

This also includes removing the renderQA magic. While I kinda like this
it was superceded by the MessageAdmin UI and actually I suspect no
users really ever used it. Keeping the code keeps the data-translate
path alive & increases our complexity

Bug: T325698
Change-Id: I36eecec156ca3f80bc68a2df8b96ecb70ce41b83
@eileenmcnaughton eileenmcnaughton changed the title Use translation for default language, if exists dev/core#4409 Use translation for default language, if exists Jul 5, 2023
There is an assumption in the code that for the default language a translation is not used.

However, the features around draft versions are all set up around
translations, so it becomes necessary to create translations
even for the default language. This change ensures they are
used if they exist.

Although we have a lot of complexity in the translation framework
on the rendering side I think assuming that where stuff is configured
people configured it to be used is a good underlying principle.
@totten
Copy link
Member

totten commented Jul 7, 2023

@eileenmcnaughton - OK, so the general idea of defining a localized template for the default locale seems OK to me. There's an edge-case to clarify, but I'm OK with it as-is.

For r-run, I defined templates for these locales (each with a different subject):

Screenshot from 2023-07-06 16-08-19

Then I chose a contact and repeatedly registered for an event (but with different values for the recipient's preferred_language). Here are the messages selected:

# Recipient Preferred Selected (Before) Selected (After)
1 null "Standard" "English (United States)"
2 en_US "Standard" "English (United States)"
3 en_AU "English (United States)" "English (United States)"
4 fr_CA "French (Canada)" "French (Canada)"
5 fr_FR "French (Canada)" "French (Canada)"
6 de_CH "Standard" "Standard"

Drilling down on a few scenarios:

  • #1: This is not entirely obvious, but I think the change is pretty fair -- the recipient's preferred_language is null, so it's reasonable to apply the site-default.
  • #2: Definitely an improvement with the patch.
  • #6: Here, I'm a bit more conflicted.
    • Looking from the POV of a GUI-admin (who edits user-locales and template-translations), this seems pretty sensible. (If the user-preferred-locale doesn't come close to any of the available-locales, then use the "Standard". Otherwise... why does "Standard" even exist? Why call it "Standard" if it's never used?)
    • Looking from the POV of your original goal/problem (ie wanting a draft workflow for the baseline template), this seems wrong.

I think this is mergable because the modified scenarios (#1+#2) are reasonable. But for meeting your overall goal, some other work may be needed. One possibly-simple approach:

  • Change the behavior of #6 so that it uses the site-default (same as #1). And...
  • Update the message_admin UI to re-style the "Standard" translation, showing that it's now inert. This might be strikethrough or faded text or icon. Maybe change the word "Standard".

@eileenmcnaughton
Copy link
Contributor Author

Hmm - yeah I thought 6 was winding up back on the site-language because of the changes I had to make to the test - oh - but that is a partials vs no partials. I wonder why that isn't winding up on en_US in sceanrio 6

@totten totten merged commit fa596b4 into civicrm:master Jul 7, 2023
@totten totten deleted the trans_locale branch July 7, 2023 04:23
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 11, 2023
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants