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

feat: atlas pull and push scripts | FC-55 #422

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

Amr-Nash
Copy link

@Amr-Nash Amr-Nash commented May 3, 2024

Description

This pull request provides the CI scripts and developer tooling for the Atlas Translations Management Design
proposal related to OEP-58.

Testing instructions

Testing the pull_translations:

make ATLAS_OPTIONS='--repository=Zeit-Labs/openedx-translations --revision=fc_55_sample' pull_translations
When the above command is executed:
  1. The None-English translations will be pulled from openedx-translations repo into I18N/ directory.
  2. Then each language file will be split and distributed to the modules one by one
  3. In the end, the pulled translations will be removed and only the split translations can be found in the modules' directories
If the script is working as intended:
  1. The above command should not produce errors when executed.
  2. The pulled translations should be correctly distributed among the modules in the expected paths.
  3. The I18N directory should not exist after the script has been executed.
  4. The app should have all its translations in place and can now be deployed.

Testing the extract_translations:

make extract_translations
When the above command is executed:
  1. The English translations from all en.lproj/Localizable.strings files in all modules will be extracted
  2. The keys of each translation entry will be updated to contain the name of the module followed by a dot e.g. "Module_name.OLD_KEY_of_the_entry".
  3. Then all of the updated entries will be put in one file: I18N/en.lproj/Localizable.strings
If this script is working as intended:
  1. The above command should not produce errors when executed.
  2. Every entry from the English translation of each module should exist in I18N/en.lproj/Localizable.strings with the 3) corresponding comment, if present, placed before it.
  3. The keys in I18N should contain the name of the module from which the entry originated, as specified.
  4. The values and comments should be identical to the original ones.

Status

We'll merge to this pull request into this branch multiple pull requests. We'll keep this open until we're ready for review.

This pull request is useful to watch progress.

Q and A

1. When are source strings extracted? what do the files look like?

  • An automated action will be developed to extract the English sources if they are changed in the IOS repo, then the action will push the sources to the translations repo.
  • The sources will be in one file: I18N/en.lproj/Localizable.strings and it will look like this:
"Module_one.FIRST.ENTRY.KEY.IN.MODULE.ONE" = "English Translation for the first entry in module one.";
"Module_one.SECOND.ENTRY.KEY.IN.MODULE.ONE" = "English Translation for the second entry in module one.";
.
.
"Module_two.FIRST.ENTRY.KEY.IN.MODULE.TWO" = "Translation for the first entry in module two";
.
.

2. Does anything happen to those files before they are added to openedx-translations

The only extracted file is the English sources from the modules in the IOS repo as it is the only translation that lives in that repo.
TBC

3. When translation files are pulled via atlas what do the files look like?

When the translation files are pulled via atlas the files' structure is going to look like the structure below:

I18N/
----ar.lproj/
--------Localizable.strings
----uk.lproj/
--------Localizable.strings
----es.lproj/
--------Localizable.strings

As an example the es.lproj/Localiztion.strings would look like:
I18N/es.lproj/Localizable.strings

"Module_one.FIRST.ENTRY.KEY.IN.MODULE.ONE" = "Traducción al inglés de la primera entrada del módulo uno.";
"Module_one.SECOND.ENTRY.KEY.IN.MODULE.ONE" = "Traducción al inglés de la segunda entrada del módulo uno.";
.
.
"Module_two.FIRST.ENTRY.KEY.IN.MODULE.TWO" = "Traducción de la primera entrada del módulo dos.";
.
.

4) What is done to those files after atlas pull to make them work in the app build process?

After atlas pull is completed, all language translation files are retrieved, similar to the process described in the previous question. Subsequently, a Python script is executed to process each file. This script iterates through each translation file, splits it, removes the module name from the keys, and organizes each entry into its corresponding module.

For instance, the I18N/es.lproj/Localizable.strings file is split into two separate files:

  1. Module_one/Module_one/es.lproj/Localizable.strings
  2. Module_two/Module_two/es.lproj/Localizable.strings

Inside Module_one/Module_one/es.lproj/Localizable.strings, the content would resemble:

"FIRST.ENTRY.KEY.IN.MODULE.ONE" = "English translation of the first entry in module one.";
"SECOND.ENTRY.KEY.IN.MODULE.ONE" = "English translation of the second entry in module one.";
.
.
.

Similarly, inside Module_two/Module_two/es.lproj/Localizable.strings, it would appear as:

"FIRST.ENTRY.KEY.IN.MODULE.TWO" = "English translation of the first entry in module two.";
.
.
.

Once the Python script completes its task and the files are split, the make script proceeds to remove the entire I18N directory and its contents.

5) Why is this needed?

This new process is being introduced to have the best combination of Developer Experience and Translator Experience.

The best experience for Translators requires combining source strings into as few transifex resources as possible. The best experience for Engineers requires splitting translation source files to fit within the modular architecture.

6) What should live in openedx translations?

Translations that were done by the open-edx translators are going to be stored in openedx translations.

7) What should live in atlas?

Atlas is the tool that we are using to pull the translations from openedx translations; therefor, no translations are stored in atlas.

8) What should live in the openedx-app-ios repo?

In the IOS app repo only the English sources are going to be stored.

TODO

  • Fix the python3-localizable pip package for python3 compatibility
  • Install openedx-atlas in the virtual env
  • Refactor the code into a single Python script to improve code reuse
  • Add CI scripts for make extract_translations
  • Test with the iOS team (Volo and others)
  • Review by Brian Smith

Related issues

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 3, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented May 3, 2024

Thanks for the pull request, @Amr-Nash! 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.

@OmarIthawi
Copy link
Member

Note: CLA has been submitted but needs a day or two to reflect on GitHub.

Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @Amr-Nash, this pull request needs the following:

  • squash the commits
  • use conventional commits
  • rename make combine_translations to make extract_translations

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label May 3, 2024
Amr-Nash added a commit to Zeit-Labs/openedx-app-ios-contrib that referenced this pull request May 6, 2024
The Extract will extract the English language translations from all
modules to the I18N folder.

The pull will pull all of the other languages translations from
transifex to the I18N folder then split them to thier modules.
Amr-Nash added a commit to Zeit-Labs/openedx-app-ios-contrib that referenced this pull request May 7, 2024
The Extract will extract the English language translations from all
modules to the I18N folder.

The pull will pull all of the other languages translations from
transifex to the I18N folder then split them to thier modules.
@Amr-Nash Amr-Nash force-pushed the atlas-scripts branch 4 times, most recently from 5b86d24 to 94bc19a Compare May 8, 2024 15:48
@Amr-Nash Amr-Nash requested a review from OmarIthawi May 9, 2024 12:11
@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label May 9, 2024
Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @Amr-Nash. I've reviewed the README and suggested many edits. Please adopt and revise and let me know when it's ready.

You'll be getting another review regarding the Python code from either me or @iamjazzar today.

.gitignore Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Amr-Nash
Copy link
Author

@OmarIthawi Thank you for your feedback. It is much appreciated and I am waiting for the script review.

Copy link

@iamjazzar iamjazzar left a comment

Choose a reason for hiding this comment

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

I've done an initial review, but I haven't tested the functionality yet. The code appears to be clean, but there are numerous nested for loops and some ambiguous names.

.gitignore Show resolved Hide resolved
i18n_scripts/translation.py Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
@OmarIthawi
Copy link
Member

@iamjazzar

but there are numerous nested for loops

Any suggestion how to improve that?

Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Added some notes.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Posting yesterday's review:

i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
i18n_scripts/translation.py Outdated Show resolved Hide resolved
@Amr-Nash
Copy link
Author

Hello @brian-smith-tcril ,
Oman and Jazzar have reviewed the PR multiple times by now. Could you please give it a look?

Copy link

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

Overall this is looking great! The code seems reasonable, so assuming it has been tested that part has a 👍 from me!

I left a couple suggestions in the README that I hope will make the instructions a little more clear.

Please let me know if you have any questions!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@OmarIthawi
Copy link
Member

Thanks @brian-smith-tcril! Yes, the manual testing part is what we focused on.

@Amr-Nash please address Brian notes and let's squash the code once more.

@Amr-Nash
Copy link
Author

Thanks @brian-smith-tcril for your feedback,
@OmarIthawi I squashed and pushed again. I think it is ready now

@Amr-Nash Amr-Nash marked this pull request as ready for review May 27, 2024 06:55
Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @Amr-Nash! I've tested it and it worked locally.

We need an iOS engineer to pull and compile, but overall I think it's ready for merge.

@OmarIthawi
Copy link
Member

@volodymyr-chekyrta would you mind chekcing this pull request? We'd love a test compile from one of your team so we can continue with the other GitHub Actions pipeline steps.

Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Please add the language renaming as well.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
i18n_scripts/requirements.txt Outdated Show resolved Hide resolved
@RawanMatar89
Copy link

RawanMatar89 commented Jun 6, 2024

after testing the instructions and build the app using Ukraine language on simulator (iPhone 15 pro max) I got

SignIn View Registration view
Simulator Screenshot - iPhone 15 Pro Max - 2024-06-06 at 12 42 11 simulator_screenshot_C14D87CE-2F0C-47B4-9FC7-483BCAC84F4F (1)

Copy link
Contributor

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

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

Tested ✅
Works fine for me 👍

README.md Outdated
Comment on lines 49 to 54
### Testing translations

Until the [pull request #422](https://github.com/openedx/openedx-app-ios/pull/422) is merged, translations needs to be pulled from the testing branch `Zeit-Labs/openedx-translations` repo under `fc_55_sample` branch with the following options:
``` bash
make ATLAS_OPTIONS='--repository=Zeit-Labs/openedx-translations --revision=fc_55_sample' pull_translations
```
Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing this section from the README?
It seems that the PR description is sufficient for testing

@OmarIthawi
Copy link
Member

@Amr-Nash please replace the underscore for iOS.

@Amr-Nash
Copy link
Author

Amr-Nash commented Jun 9, 2024

@OmarIthawi Done, please check.
If everything is ok I'll squash and push again.

Extract will extract the English language resources from all
modules to the I18N folder.

Pull will pull all other languages translations from
the openedx-traslations repository to the
I18N folder then split them to thier modules.
@volodymyr-chekyrta volodymyr-chekyrta merged commit dcb4134 into openedx:develop Jun 12, 2024
3 checks passed
@openedx-webhooks
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants