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

i18n - extracting new, pulling from tx #9912

Merged
merged 2 commits into from
Nov 28, 2023
Merged

i18n - extracting new, pulling from tx #9912

merged 2 commits into from
Nov 28, 2023

Conversation

Griffin-Sullivan
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the Legacy JS PRs making changes in the legacy Javascript stack label Nov 21, 2023
ekohl
ekohl requested changes Nov 23, 2023
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

To pull new updates I also needed to change this line:

foreman/locale/Makefile

Line 41 in b26b09f

tx pull -f

It needs to be cd .. && tx pull -f. This appears to be something that changed between transifex-client and tx.

bundler.d/test.rb Outdated Show resolved Hide resolved
locale/foreman.pot
@@ -15392,7 +15492,7 @@ msgstr ""

#:
#: ../webpack/assets/javascripts/react_app/components/HostDetails/DetailsCard/PowerStatus/actions.js:13
msgid "Power has been set to \"%s\" successfully"
msgid "Power has been set to \\\"%s\\\" successfully"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is correct.

Copy link
Member

Choose a reason for hiding this comment

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

So it's this line:

foreman/webpack/assets/javascripts/react_app/components/HostDetails/DetailsCard/PowerStatus/actions.js

Line 13 in 38e2c55

__('Power has been set to "%s" successfully'),

There are no backslashes in there, so I question where this come from.

I don't know if this really fixing an escaping issue that already existing, or if there's some duplicate escaping bug that was introduced somewhere. I'll try to dig further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little confused what to look into here. The original string uses single quotes and double quotes on the inside. Why are they both getting swapped to double quotes?

Copy link
Member

Choose a reason for hiding this comment

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

webhippie/gettext_i18n_rails_js@77743bc suggests this is intentional, but it would be nice to test it out end to end.

I see there was a French translation for the old string, so you can try to update this PR with the newly translated string, wait for Packit to be built and try it out in the UI.

It's just a few strings, so I won't hold it back on this, but if we can figure this out while Pulp is getting unblocked that would be nice.

Copy link
Contributor Author

@Griffin-Sullivan Griffin-Sullivan Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused. Does this PR not already update the string for other languages? If not, what do I need to do? I've never tested other languages in Satellite since we've never had to do it on QE.

Copy link
Member

Choose a reason for hiding this comment

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

Yes and no. This PR does multiple things.

It updates the template (.pot). This is the file with source strings. They're never supposed to be translated, but instead are used to update the .po files which exist for each language. So the resulting .po files are a combination of the .pot file as well as the translations from Transifex.

The problem is that Transifex pulls its strings from git develop, so it can't know about updated strings until this is merged. Since source string changed, translators can't have translated it, so it's empty.

Perhaps at some point we can be smarter in finding these minimally changed strings, but in practice I'd expect it's rare. How do you know it's really the same?

Does that help clarify it a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok that makes sense

Griffin-Sullivan
Griffin-Sullivan commented Nov 27, 2023
View reviewed changes
locale/fr/foreman.po
@@ -7356,8 +7356,8 @@ msgstr "État d’alimentation"
msgid "Power a Virtual Machine"
msgstr "Démarrer une Machine Virtuelle"

msgid "Power has been set to \"%s\" successfully"
msgstr "L’alimentation a pu être définie sur \"%s\""
msgid "Power has been set to \\\"%s\\\" successfully"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekohl I made this change. Is this what you were saying to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did I do this right? Copr failed at

pl/foreman.po:11990: 'msgid' and 'msgstr' entries do not both end with '\n'

Copy link
Member

Choose a reason for hiding this comment

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

@ekohl I made this change. Is this what you were saying to do?

This is indeed what I meant.

pl/foreman.po:11990: 'msgid' and 'msgstr' entries do not both end with '\n'

That's a warning from a different file. I left a comment there (that I updated it).

ekohl
ekohl reviewed Nov 28, 2023
View reviewed changes
locale/pl/foreman.po Outdated
"* globalny\n"
"* konfigurowany\n"
"* zbudowany\n"
msgstr "Typem statusu może być:\\n* globalny\\n* konfigurowany\\n* zbudowany\\n"
Copy link
Member

Choose a reason for hiding this comment

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

This is the line that breaks it. I've fixed it up in Transifex so I expect it to work if you pull again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok pulling now

@Griffin-Sullivan
Copy link
Contributor Author

@ekohl I'm not sure how to interpret that failure for Copr

@ekohl
Build packit against rhel-8-86_64
d246256 
This matches what we use in foreman-packaging.
@ekohl
Copy link
Member

ekohl commented Nov 28, 2023

Looks like a networking failure, but it does highlight a difference: we build against centos-stream-8 while regular packaging builds against rhel-8. I've opened #9931 but for now I think we can retrigger is

/packit build

@ekohl
Copy link
Member

ekohl commented Nov 28, 2023

/packit build

1 similar comment
@Griffin-Sullivan
Copy link
Contributor Author

/packit build

@ekohl ekohl merged commit f3a73bb into theforeman:develop Nov 28, 2023
11 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl approved these changes

Assignees
No one assigned
Labels
Legacy JS PRs making changes in the legacy Javascript stack Waiting on contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Griffin-Sullivan @ekohl