feat: standardize translation directory for i18n_tool #341

shadinaif · 2023-03-16T11:21:09Z

feat: standardize translation directory for i18n_tool

This PR prepares the repository to comply with openedx-translations by doing the following:

Move all local translations to conf/locale and convert translations into a symbolic link to conf/locale. The reason behind that is to comply with openedx-translations mechanism that requires translation files to be in conf/locale directory. This fix will avoid the need to modify find commands here
locale link is removed as it is not needed anymore
Make the translation command make extract_translations build a file named django.po instead of text.po. And create a symbolic link from text.po to django.po. This is because openedx-translations requires django.po as the name of the file. And it must be a valid file (not a link) because it uses git add here

Tested on local devstack palm to ensure that everything renders fine
Tested on local devstack palm to ensure that local translation still works

References

This pull request is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.

Check the links above for full information about the overall project.

Internalization is being rearchitected in Open edX Python, XBlock, Micro-frontend, and other projects. There are a number of immediately visible changes:

Remove source and language translations from the repositories, hence no .json, .po or .mo files will be committed into the repos.
Add standardized make extract_translations in all repositories
Push user-facing messages strings into openedx/openedx-translations.
Integrate root repositories with openedx/openedx-atlas to pull translations on build/deploy time

Breaking Changes

One of the primary goals of the project is to avoid breaking changes. If you noticed any suspicious code, please raise your concern. But before that, please know the strategy we're following to avoid breaking changes

For XBlocks:

The standard translation path must be conf/locale. Therefore, we are creating a link from conf/locale to translations so Atlas can find the path without disrupting the current way of having translations locally within the XBlocks
openedx-translations will have a related PR that adds the XBlock to the pipeline. This will not affect the current way of managing/updating translations
At the end of the project, a clear change log will be added and all translations will be handled by Atlas. Thus, the local translation will be removed from the repo within the version bump
A notification for the community will be published to ensure that everyone knows why translations directories are removed from all repos

openedx-webhooks · 2023-03-16T11:21:15Z

Thanks for the pull request, @shadinaif! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

shadinaif · 2023-03-16T11:22:03Z

Makefile

 EXTRACTED_TEXT := $(EXTRACT_DIR)/text.po

 extract_translations: ## extract strings to be translated, outputting .po files
 	cd $(WORKING_DIR) && i18n_tool extract
 	mv $(EXTRACTED_DJANGO) $(EXTRACTED_TEXT)
-	tail -n +20 $(EXTRACTED_DJANGOJS) >> $(EXTRACTED_TEXT)


The plugin has no text in JS files. so collecting is failing

Are you referring to the plugin directory, or to this repository as a whole?

We do have text in the xblock_lti_consumer.js Javascript file. Although we've recently observed that those strings have not been translated in two years; we haven't investigated yet.

Does removing this line mean that those Javascript strings will not be translated?

I've added a fix but then someone fixed it in a another PR https://github.com/openedx/xblock-lti-consumer/pull/353/files#diff-de32ac480ffc073d0eeb9f360281bd32ad2ad166d93821341685c945b414eb3c . So yes, we have JS translations

This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58

MichaelRoytman

Thank you for the pull request. I left a few questions.

I also want to confirm that this won't impact existing translated strings. Those will remain in use until the time comes that openedx-atlas is integrated into this repository. Is that correct?

As I mentioned in the comments, we noticed that our Javascript strings are not being translated properly. It may just be that Transifex hasn't committed (or isn't set up to commit) translations to this repository. But I want to be sure that we can still translate the strings in those files.

I also want to make you aware of the instructions in our README around pulling translations. When will this need to change? I tried to run it and got the following prompt, so I assume it's out of date in favor of the legacy bot. Let me know if I'm mistaken.

(xblock-lti-consumer) mroytman@C02FW0EAML85 xblock-lti-consumer % tx pull -f --mode=reviewed -l en,ar,es_419,fr,he,hi,ko_KR,pt_BR,ru,zh_CN
API token not found. Please provide it and it will be saved in '~/.transifexrc'.
If you don't have an API token, you can generate one in https://www.transifex.com/user/settings/api/
>

MichaelRoytman · 2023-03-16T21:30:25Z

.tx/config

@@ -2,8 +2,7 @@
 host = https://www.transifex.com

 [o:open-edx:p:xblocks:r:lti-consumer]
-file_filter = lti_consumer/translations/<lang>/LC_MESSAGES/text.po
-source_file = lti_consumer/translations/en/LC_MESSAGES/text.po
+file_filter = lti_consumer/conf/locale/<lang>/LC_MESSAGES/text.po


This will still extract translations to this repository. In the grand scheme of the project, is the idea that eventually the source files in this directory will be removed from this repository and will instead be committed to openedx-translations?

Yes, it will extract it to this repo but the destination directory is ignored by .gitignore. daily Action is triggered in openedx-translations to collect translations from all supported repos, so the generated text.po will be copied to openedx-translations during the Action execution

e0d · 2023-03-24T00:20:15Z

Resolved the conversation with an errant click, unresolved and ran that tests.

codecov · 2023-03-24T00:26:18Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (f9dac31) 97.68% compared to head (e4bb599) 97.68%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #341   +/-   ##
=======================================
  Coverage   97.68%   97.68%           
=======================================
  Files          77       77           
  Lines        6187     6187           
=======================================
  Hits         6044     6044           
  Misses        143      143

Flag	Coverage Δ
unittests	`97.68% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mphilbrick211 · 2023-03-24T18:45:36Z

Hi @MichaelRoytman! Would you mind taking a look at this? Thanks!

MichaelRoytman · 2023-03-27T13:05:23Z

Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you.

MichaelRoytman · 2023-03-27T13:05:32Z

Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you.

MichaelRoytman · 2023-03-27T13:06:05Z

Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you.

MichaelRoytman · 2023-03-27T13:07:42Z

Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you.

MichaelRoytman · 2023-03-27T13:08:56Z

Oof. Github is having issues and posted my comment four times. Sorry for tagging you four times! 😅

mphilbrick211 · 2023-03-29T15:07:46Z

Hi, @mphilbrick211! I took a look a few weeks ago and left a few questions for Shadi. If they could address my remaining questions, I'd be happy to re-review. I want to confirm the impact on Javascript string translations and on existing translations in the repository. Thank you.

Hi @shadinaif! Friendly ping on this :)

shadinaif · 2023-03-30T06:22:50Z

Hi @MichaelRoytman and @mphilbrick211 , thank you for your patience on this. I'll push an update and address the issues above very soon. We decided to finish the project without introducing any breaking changes until the last phase of it. I'll add a detailed description to the PR, we're working on a template for that

Thank you!

This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58

shadinaif · 2023-04-14T13:08:14Z

@MichaelRoytman please review now, tested on devstack olive, works fine. We're not changing any behavior in this phase, please see the details in the PR description. Thank you!

This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58

e0d · 2023-04-14T14:35:37Z

@shadinaif Looks like there are some test failures.

shadinaif · 2023-04-15T09:59:14Z

Thank you @e0d . The error is related to a missing token for codecov.io . It's either deleted by someone, or affected by latest Github incident and needs to be re-set https://github.blog/2023-03-23-we-updated-our-rsa-ssh-host-key/

Refs: FC-0012 OEP-58

MichaelRoytman

This looks good to me!

I pinged @Zacharis278 to take a quick look, since he's done some translations works in this repository recently.

Zacharis278

looks good to me!

brian-smith-tcril

All tests passed! Looks like this is good to go!

sarina · 2023-04-24T14:38:44Z

Merging PR on behalf of Shadi, as it has 3 approvals and looks good to me.

openedx-webhooks · 2023-04-24T14:38:54Z

@shadinaif 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 16, 2023

shadinaif commented Mar 16, 2023

View reviewed changes

shadinaif added a commit to Zeit-Labs/openedx-translations that referenced this pull request Mar 16, 2023

feat: Add xblock-lti-consumer to the translation pipeline

04486dc

This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58

shadinaif mentioned this pull request Mar 16, 2023

feat: Add xblock-lti-consumer to the translation pipeline openedx/openedx-translations#151

Merged

MichaelRoytman reviewed Mar 16, 2023

View reviewed changes

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 21, 2023

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 24, 2023

mphilbrick211 requested a review from MichaelRoytman March 24, 2023 18:45

shadinaif force-pushed the shadinaif/FC-0012-OEP-58 branch 2 times, most recently from fcb6336 to eebbd79 Compare April 10, 2023 06:38

shadinaif marked this pull request as draft April 10, 2023 06:38

shadinaif force-pushed the shadinaif/FC-0012-OEP-58 branch from eebbd79 to 4a210a8 Compare April 10, 2023 06:40

shadinaif added a commit to Zeit-Labs/openedx-translations that referenced this pull request Apr 10, 2023

feat: Add xblock-lti-consumer to the translation pipeline

ae14084

This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Apr 11, 2023

shadinaif force-pushed the shadinaif/FC-0012-OEP-58 branch from e667807 to 7d9fa7d Compare April 13, 2023 12:20

shadinaif marked this pull request as ready for review April 14, 2023 13:05

shadinaif added a commit to Zeit-Labs/openedx-translations that referenced this pull request Apr 14, 2023

feat: Add xblock-lti-consumer to the translation pipeline

e696147

This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58

e0d added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Apr 14, 2023

feat: standardize translation directory for i18n_tool

e4bb599

Refs: FC-0012 OEP-58

shadinaif force-pushed the shadinaif/FC-0012-OEP-58 branch from 7d9fa7d to e4bb599 Compare April 17, 2023 07:00

MichaelRoytman approved these changes Apr 19, 2023

View reviewed changes

Zacharis278 approved these changes Apr 19, 2023

View reviewed changes

brian-smith-tcril approved these changes Apr 20, 2023

View reviewed changes

mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 21, 2023

sarina merged commit 5eabfa7 into openedx:master Apr 24, 2023

shadinaif added a commit to Zeit-Labs/openedx-translations that referenced this pull request Apr 25, 2023

feat: Add xblock-lti-consumer to the translation pipeline

726a9fb

This PR needs openedx/xblock-lti-consumer#341 before it's merged. Refs: FC-0012 OEP-58

shadinaif mentioned this pull request May 13, 2023

fix: make extract_translations #371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: standardize translation directory for i18n_tool #341

feat: standardize translation directory for i18n_tool #341

shadinaif commented Mar 16, 2023 •

edited

Loading

openedx-webhooks commented Mar 16, 2023 •

edited

Loading

shadinaif Mar 16, 2023

MichaelRoytman Mar 16, 2023

shadinaif Apr 14, 2023

MichaelRoytman left a comment •

edited

Loading

MichaelRoytman Mar 16, 2023

shadinaif Mar 20, 2023 •

edited

Loading

e0d commented Mar 24, 2023

codecov bot commented Mar 24, 2023 •

edited

Loading

mphilbrick211 commented Mar 24, 2023

MichaelRoytman commented Mar 27, 2023

MichaelRoytman commented Mar 27, 2023

MichaelRoytman commented Mar 27, 2023

MichaelRoytman commented Mar 27, 2023

MichaelRoytman commented Mar 27, 2023

mphilbrick211 commented Mar 29, 2023

shadinaif commented Mar 30, 2023

shadinaif commented Apr 14, 2023

e0d commented Apr 14, 2023

shadinaif commented Apr 15, 2023

MichaelRoytman left a comment

Zacharis278 left a comment

brian-smith-tcril left a comment

sarina commented Apr 24, 2023

openedx-webhooks commented Apr 24, 2023

feat: standardize translation directory for i18n_tool #341

feat: standardize translation directory for i18n_tool #341

Conversation

shadinaif commented Mar 16, 2023 • edited Loading

References

Breaking Changes

openedx-webhooks commented Mar 16, 2023 • edited Loading

shadinaif Mar 16, 2023

Choose a reason for hiding this comment

MichaelRoytman Mar 16, 2023

Choose a reason for hiding this comment

shadinaif Apr 14, 2023

Choose a reason for hiding this comment

MichaelRoytman left a comment • edited Loading

Choose a reason for hiding this comment

MichaelRoytman Mar 16, 2023

Choose a reason for hiding this comment

shadinaif Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

e0d commented Mar 24, 2023

codecov bot commented Mar 24, 2023 • edited Loading

Codecov Report

mphilbrick211 commented Mar 24, 2023

MichaelRoytman commented Mar 27, 2023

MichaelRoytman commented Mar 27, 2023

MichaelRoytman commented Mar 27, 2023

MichaelRoytman commented Mar 27, 2023

MichaelRoytman commented Mar 27, 2023

mphilbrick211 commented Mar 29, 2023

shadinaif commented Mar 30, 2023

shadinaif commented Apr 14, 2023

e0d commented Apr 14, 2023

shadinaif commented Apr 15, 2023

MichaelRoytman left a comment

Choose a reason for hiding this comment

Zacharis278 left a comment

Choose a reason for hiding this comment

brian-smith-tcril left a comment

Choose a reason for hiding this comment

sarina commented Apr 24, 2023

openedx-webhooks commented Apr 24, 2023

shadinaif commented Mar 16, 2023 •

edited

Loading

openedx-webhooks commented Mar 16, 2023 •

edited

Loading

MichaelRoytman left a comment •

edited

Loading

shadinaif Mar 20, 2023 •

edited

Loading

codecov bot commented Mar 24, 2023 •

edited

Loading