Fix bug 1380356: Add ability to use localization as source lang for translation #1474
Conversation
|
Nice work so far! The settings page will need a slight redesign, but for now I suggest we move the Preferred source locale selector under the Select your homepage selector - it's a similar UX and it's hard to predict which of the features will be used more often. I would also change the label to "Select preferred source locale" and add the default value to the selector: "Default project locale". You'll need to update the |
|
I made progress and am now able to save the preferred source locale. However, I have not been able to track down why when a selection is made it changes the selection for both items. This is only on the UI as the user's selections are being properly saved in the models. I tried giving unique class names to the elements, but that did not alter the behavior. If you have a moment to take a look and offer suggestions on this I would appreciate it. In the meantime, I'll continue to work on it as well as move forward with the next step of using this setting to determine which strings to render in the translation panel. |
There was a problem hiding this comment.
Nice work!
-
To fix the problem with selectors, please look into the
pontoon/teams/static/js/team_selector.jsfile (sorry for misleading you), which handles the actual menu selector. You'll need to make sure that the element we update is withing the selector by prepending something like$(this).parents(".locale-selector").... -
When I select a source locale and save it, the /settings page errors after a reload:
'None' has no attribute 'serialize' -
There's no way to select "Default Project Locale" once you select another locale. Unlike what I said initially when you were creating a menu, the behaviour (and the model) of the preferred locale selector should be exactly the same as for the homepage selector.
Thank you!
I just realized this myself 😞
OK, I originally had appended it but then since it is a Foreignkey was pretty sure it would cause an issue. Should I change the model for |
Sadly, yes.
I'll leave that to you. If possible, remove the migration and add a new one, and before you do so, make sure you unapply the current migration. |
|
All seems to be working as expected now on the first parts of this task. Please let me know if there are any changes or adjustments needed. Working on the next step now, or after I fix that failing test anyway 😃 |
|
So, I actually don't know why that test failed. All tests pass locally, and I'm not entirely sure what the error message is...... |
|
My attempt to access the data in the preferred_source_locale field on the frontend has not been successful (it comes back as Of course, if I'm completely off track then if you could please provide information on how to go about accessing the data I would appreciate it. |
There was a problem hiding this comment.
Nice work @abowler2! All the issues pointed out in #1474 (review) are addressed. I left a few notes.
In b67fd68 I don't see getUserPreferredLocale ever being called. But I'm not even sure we need to make that call from the translate app? Regarding step #3 in the bug we might be good by just changing the backend code:
https://github.com/mozilla/pontoon/blob/master/pontoon/base/views.py#L260
https://github.com/mozilla/pontoon/blob/master/pontoon/base/models.py
I had been using a console.log() in the Translation.js file to show all the values in the However, as you stated that I may not even need to use the call in the translate app, it might not matter. |
There was a problem hiding this comment.
Nice work! The selected preferred source locale appears in the UI!
I've left a few notes on how to speed up the code and get rid of that JS error.
Once that's done we'll address the issue with plurals.
I also realized we'll need to fix how Machinery works, because we only store en-US strings as source in Translation Memory. We'll need to always pass en-US via get-entities and use them in Machinery.
pontoon/base/views.py
Outdated
| @@ -999,6 +1005,7 @@ def user_data(request): | |||
| 'force_suggestions': user.profile.force_suggestions, | |||
| }, | |||
| 'preferred_locales': user.profile.sorted_locales_codes, | |||
| 'preferred_source_locale': user.profile.preferred_source_locale, | |||
There was a problem hiding this comment.
No, we don't need that. That was from when I was trying to access the preferred_source_locale from the frontend. I removed it.
pontoon/base/models.py
Outdated
| for entity in entities: | ||
| translation_array = [] | ||
| string = entity.string | ||
| string_plural = entity.string_plural |
There was a problem hiding this comment.
String is a generic term, also used in translations. Let's call it original instead.
There was a problem hiding this comment.
Nit: I'd also add a blank line between translation_array and string.
pontoon/base/models.py
Outdated
|
|
||
| if entity.string_plural == "": | ||
| if string_plural == "": |
There was a problem hiding this comment.
Nit: '' (single quotes).
pontoon/base/models.py
Outdated
| translation = entity.get_active_translation().serialize() | ||
| translation_array.append(translation) | ||
| if preferred_source_locale != '' and entity.alternative_originals: | ||
| string = entity.alternative_originals[0].string |
There was a problem hiding this comment.
Now to handle plurals, we should actually move this block below the if string_plural == '' - else block and then use the last available plural form for the pluralized string:
if string_plural != '':
string_plural = entity.alternative_originals[-1].stringThere was a problem hiding this comment.
This works for all the files I have in my Test project except the amo.po file. It works when only changing original to the alternative_original but the plural form causes an error with the JSON. ("The response content is not JSON-compatible")
When I try to simulate the query in the shell I get an AssertionError: Negative indexing is not supported.
There was a problem hiding this comment.
Hm, I'm not able to reproduce your issues.
What do you mean by "changing original to the alternative_original"?
I've made the following change:
diff --git a/pontoon/base/models.py b/pontoon/base/models.py
index 105a600a7..820741e54 100644
--- a/pontoon/base/models.py
+++ b/pontoon/base/models.py
@@ -2621,6 +2621,11 @@ class Entity(DirtyFieldsMixin, models.Model):
translation = entity.get_active_translation(plural_form).serialize()
translation_array.append(translation)
+ if preferred_source_locale != '' and entity.alternative_originals:
+ string = entity.alternative_originals[0].string
+ if string_plural != '':
+ string_plural = entity.alternative_originals[-1].string
+
entities_array.append({
'pk': entity.pk,
'original': string,There was a problem hiding this comment.
Ahh, OK. I had them set up as stand-alone if statements instead of nesting the plurals if statement. It is working fine now ~ Thank you.
|
Made requested updates and fixed tests. Working on how to set Machinery to use en-US original now. Let me know if there is anything in particular I need to know to complete this. |
|
Very nice work, thanks! To fix Machinery, I think you'll need to replace |
|
That seems to have worked! Thanks Let me know if anything further is needed. |
abowler2
changed the title
pontoon/base/models.py
Outdated
| @@ -2628,6 +2628,7 @@ def map_entities(cls, locale, preferred_source_locale, entities, visible_entitie | |||
| 'pk': entity.pk, | |||
| 'original': original, | |||
| 'original_plural': original_plural, | |||
| 'machineryOriginal': entity.string, | |||
There was a problem hiding this comment.
Nit: we use snake_case in python, so machinery_original.
|
I'll definitely work on showing the original source in the Locals tab, and let you know if I have questions. |
mathjazz
changed the title
I pushed what I have so far for this....but I just want to say right off that while it is working...it's pretty ugly 🤣
I think that sums up the current issues. TIA 😄 |
There was a problem hiding this comment.
@abowler2 Thanks for looking into it, but I suggest we postpone this to a followup PR. I'll file a bug describing how to implement passing original source string into other locales panel. It requires one other refactor, and to avoid making too big PRs, I suggest we do it separately.
Please revert your last commit and address a few more nits I found and we're good.
pontoon/base/forms.py
Outdated
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super(UserPreferredSourceLocaleForm, self).__init__(*args, **kwargs) | ||
| preferred_source_locales = list(Locale.objects.all().values_list('code', 'name')) |
There was a problem hiding this comment.
Nit: We should actually call this all_locales, same as above.
pontoon/base/models.py
Outdated
| @@ -2205,6 +2208,20 @@ def prefetch_active_translations(self, locale): | |||
| ) | |||
| ) | |||
|
|
|||
| def prefetch_alternative_originals(self, code): | |||
| """ | |||
| Prefetch active translations for given preferred source locale. | |||
pontoon/base/views.py
Outdated
| locale, | ||
| preferred_source_locale, | ||
| entities_to_map, | ||
| visible_entities |
There was a problem hiding this comment.
Nit: please add comma at the end of the line. That code style allows us to add/remove parameters by making 1-line diffs.
pontoon/contributors/views.py
Outdated
| """Save preferred source locale """ | ||
| form = forms.UserPreferredSourceLocaleForm( | ||
| request.POST, | ||
| instance=request.user.profile |
There was a problem hiding this comment.
Nit: comma at the end of the line.
|
Bug filed: You can also split it in two patches if you want (refactor + displaying source string). |
This reverts commit f18672c.
…ozilla#1474) * Add ability for users to select a preferred source locale * Whenever translation to that locale are available, use it as source in string list and source string panel * Keep using original string as the source for Machinery * Remove `marked` and `marked_plural` from Entity data
…ozilla#1474) * Add ability for users to select a preferred source locale * Whenever translation to that locale are available, use it as source in string list and source string panel * Keep using original string as the source for Machinery * Remove `marked` and `marked_plural` from Entity data
Issue: Some localization communities have a hard time to directly translate from English, and prefer to do their work based on an existing translation.
This fix is working to add a Preferred Source Locale selection to allow users to select existing translations to work from.
Current status: I have added the new field to the UserProfile model and added the new setting to the UI of the Settings page.
However, the form is not functioning properly. It is currently rendering the correct selections in the menu, but any change made effects both the
Custom Homepageselector and thePreferred source localeselector. Selections are not being saved in the model field forpreferred_source_locale(the custom_homepage setting still saves when changes are made with the custom homepage selector).If you could take a look at my current progress and help me to find my missteps or where I need to go from here that would be appreciated.