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

Fix gettext plural forms for non-plural locales #2982

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

eemeli
Copy link
Member

@eemeli eemeli commented Oct 10, 2023

Fixes #2744

Includes an SQL data migration to fix the ~100 instances of bad data in the pontoon.mozilla.org DB. Most of those were not actually due directly to the parent issue, but the AMO string Version {0} getting a plural form added to it about half a year ago.

After this lands, previously suggested and auto-rejected translations should be re-submitted. As the migration is non-reversible and I managed to apply it locally before taking note of exactly which ones those may be (in addition to the entities mentioned in #2949), here's at least a query that can list them:

select entity_id, lc.code, tx.string
from base_translation as tx
join base_locale as lc on locale_id = lc.id
join base_entity as ent on entity_id = ent.id
where tx.plural_form is NULL and ent.string_plural != '';

@eemeli eemeli requested a review from mathjazz October 10, 2023 06:34
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Thanks!

Could you elaborate why do we need to re-submit previously suggested and auto-rejected translations?

tx.plural_form IS NULL AND
tx.entity_id = ent.id AND
ent.string_plural != '';
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rewrite this in Django ORM. We use SQL as the last resort - in case the query is impossible or much more complex to write in Django ORM, or for performance reasons.

Something like this should work:

Translation.objects.filter(plural_form__isnull=True).exclude(entity__string_plural="").update(plural_form=0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do, but with the caveat that I'm pretty much out of bandwidth atm for Pontoon work given other deadlines. So if I need to rewrite this, that'll hopefully be sometime next week.

@eemeli
Copy link
Member Author

eemeli commented Oct 10, 2023

Could you elaborate why do we need to re-submit previously suggested and auto-rejected translations?

Because there are only a few of these, and automating that would be more work than doing it manually.

@mathjazz
Copy link
Collaborator

mathjazz commented Oct 10, 2023

Could you elaborate why do we need to re-submit previously suggested and auto-rejected translations?

Because there are only a few of these, and automating that would be more work than doing it manually.

I'm curious what are we actually trying to do with re-submitting these strings. Fix stati and stats?

BTW, the query you posted above finds 106 matches on prod.

@eemeli
Copy link
Member Author

eemeli commented Oct 10, 2023

I'm curious what are we actually trying to do with re-submitting these strings. Fix stati and stats?

Not just that; we want to get these translations approved so that they can be used.

BTW, the query you posted above finds 106 matches on prod.

Yeah, that sounds about right. Most of those should be for the Version {0} string that don't need any action.

@mathjazz
Copy link
Collaborator

Thanks for clarifying.

Leaving the output here for reference:
https://gist.github.com/mathjazz/fc683bf5e5a12ba055e70c483484b049

Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@mathjazz mathjazz merged commit 3e815ff into mozilla:main Oct 23, 2023
8 checks passed
@eemeli eemeli deleted the fix-plural-gettext branch October 30, 2023 13:23
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.

Pontoon fails to approve string translation
2 participants