diff --git a/.github/workflows/automerge-transifex-app-prs.yml b/.github/workflows/automerge-transifex-app-prs.yml index f368dd54465..5df24a43cb5 100644 --- a/.github/workflows/automerge-transifex-app-prs.yml +++ b/.github/workflows/automerge-transifex-app-prs.yml @@ -15,26 +15,15 @@ jobs: uses: actions/checkout@v3 with: fetch-depth: 0 - - name: merge pull request - uses: nick-fields/retry@v2 - id: mergePR + + - name: Auto-merge pull request env: # secrets can't be used in job conditionals, so we set them to env here TRANSIFEX_APP_ACTOR_NAME: "${{ secrets.TRANSIFEX_APP_ACTOR_NAME }}" TRANSIFEX_APP_ACTOR_ID: "${{ secrets.TRANSIFEX_APP_ACTOR_ID }}" # This token requires Write access to the openedx-translations repo GITHUB_TOKEN: ${{ secrets.EDX_TRANSIFEX_BOT_GITHUB_TOKEN }} - PR_NUMBER: ${{ github.event.number }} if: "${{ github.actor == env.TRANSIFEX_APP_ACTOR_NAME && github.actor_id == env.TRANSIFEX_APP_ACTOR_ID }}" - with: - retry_wait_seconds: 60 - max_attempts: 5 - timeout_minutes: 15 - retry_on: error - command: | - # Attempt to merge the PR - gh pr merge ${{ github.head_ref }} --rebase --auto - - # The `fromdate | todate` are used merge to validate that `mergedAt` isn't null - # therefore verifying that the pull request was merged successfully. - gh pr view "$PR_NUMBER" --json mergedAt --jq '.mergedAt | fromdate | todate' + run: | + # Add the pull request to the merge queue with --rebase commit strategy + gh pr merge ${{ github.head_ref }} --rebase --auto diff --git a/.github/workflows/validate-translation-files.yml b/.github/workflows/validate-translation-files.yml index 79eaa78ab42..c78a62764e3 100644 --- a/.github/workflows/validate-translation-files.yml +++ b/.github/workflows/validate-translation-files.yml @@ -50,7 +50,7 @@ jobs: :warning: There are errors in the translation files: ``` - ${{ steps.validate_translation_files.outputs.VALIDATION_ERRORS }} + ${{ steps.validate_translation_files.outputs.VALIDATION_ERRORS || 'No errors were reported.' }} ``` This comment has been posted by the `validate-translation-files.yml` GitHub Actions workflow. diff --git a/Makefile b/Makefile index 9cb4142edd2..ebd1aa73ec9 100644 --- a/Makefile +++ b/Makefile @@ -37,7 +37,7 @@ test_requirements: ## Installs test.txt requirements test: ## Run scripts tests pytest -v -s --cov=. --cov-report=term-missing --cov-report=html scripts/tests -validate_translation_files: ## Run basic validation to ensure files are compilable +validate_translation_files: ## Run basic validation to ensure translation files are valid python scripts/validate_translation_files.py sync_translations: ## Syncs from the old projects to the new openedx-translations project diff --git a/docs/decisions/0002-mark-fuzzy-entries.rst b/docs/decisions/0002-mark-fuzzy-entries.rst new file mode 100644 index 00000000000..8fe46fbec24 --- /dev/null +++ b/docs/decisions/0002-mark-fuzzy-entries.rst @@ -0,0 +1,126 @@ +Mark invalid entries in po files as fuzzy +######################################### + +Status +****** + +Rejected. + +Context +******* +As of the `OEP-58`_, the Transifex GitHub App is used to sync Translations. +These translations are validated by `LexiQA`_, built-in Transifex quality +checks, and human reviewers. During this synchronization process, the +`Transifex GitHub App`_ creates pull requests to this repository. The +translations in the pull requests are then validated by ``msgfmt`` in CI. + +Problem +******* +There are times when the translations being synchronized fail ``msgfmt`` +validation. This prevents the pull requests from being merged. + +Proposed Design Decision [Rejected] +*********************************** + +Rejection Reason +**************** + +This design decision was rejected because there were multiple issues that +had no feasible solution within the original scope of the `OEP-58`_ +proposal. These issues are listed below: + +- The workflow script edits the translations and effectively hide the + errors, which can be confusing for the translators. Therefore, + a good notification should be + sent to translators to avoid silencing errors. +- Writing to the pull request will not trigger the GitHub Actions workflow, + therefore the pull request cannot be merged unless the commit status + is overridden manually via `GitHub "create a commit status" API`_. +- The solution is ``.po`` file specific. And there's no similar solution for + ``.json``, ``.yaml``, iOS, Android or other translations files. + +Please refer to the original pull request which contains the rejected +implementation: `mark invalid entries as fuzzy | FC-0012`_. + + +Proposal Design +*************** + +A GitHub Actions workflow will be implemented to mark invalid entries in +synchronized ``.po`` files as ``fuzzy``. This will update pull requests +created by the `Transifex GitHub App`_. + +To ensure a safe and reliable workflow, the following workflows will be +combined into one single workflow with multiple jobs: + +#. Run ``python-tests.yml`` to validate the python code +#. Then, run ``validate-translation-files.yml`` which performs the following: + + #. Validate the po-files using ``msgfmt`` + #. Notify the translators about the invalid entries via the preferred + communication channel (Slack, Transifex, GitHub) + #. Edit the po files to mark invalid entries as ``fuzzy``, so it's + excluded from ``.mo`` files + #. Revalidate the files + +#. Commit the updated entries and push to the PR branch +#. Automatically merge the PR + + +Informing the Translators +************************* +Translators can be notified about invalid translations via Slack, Transifex +or GitHub issues depending on the community's preference. + +Pros and Cons +************* + +**Pros** + +* New valid strings would make it into the ``.mo`` files +* There's no need for manual intervention, therefore it's fast and won't + create a backlog of pull requests. +* Rejected strings are easily identifiable by looking in the code, so it's + easy to fix them. +* Translators can be notified about invalid translations via Slack, Transifex, + GitHub depending on the community's preference. + + +**Cons** + +* The workflow script runs and edits the pull request, which can be + confusing for the reviewers. +* Previously valid entries are going to be overwritten with new ``fuzzy`` + strings which will make those entries to be shown in source language + (English). + +Rejected Alternative: Keep pull requests open +********************************************* + +- **Create a Transifex issue only:** It would be possible to identify the + faulty entries and create a Transifex issue via API to the faulty entries. + However, this would require attention from the translators + team, which can take a rather long time therefore creating a backlog of + invalid entries. Additionally, Transifex issues are not understood or used + by most of the Open edX community. + +- **Post errors on Slack only:** Post the errors on Slack and ask the + translators + to fix them. Same as the previous option, not all translators are on Slack. + Additionally, this option would have a slow feedback loop causing the pull + requests backlog to build-up. + +- **Mark faulty entries as unreviewed only:** Use the Transifex API to mark + the the invalid entries as unreviewed, then pull only + reviewed entries into this repository. + This option would require an extensive use the of the Transifex API, + and pull again which can be complex to implement. Additionally, this option + would have a slow feedback loop causing the pull requests backlog to + build-up. + + +.. _OEP-58: https://open-edx-proposals.readthedocs.io/en/latest/architectural-decisions/oep-0058-arch-translations-management.html +.. _LexiQA: https://help.transifex.com/en/articles/6219179-lexiqa +.. _Transifex GitHub App: https://github.com/apps/transifex-integration +.. _GitHub "create a commit status" API: https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status +.. _mark invalid entries as fuzzy | FC-0012: https://github.com/openedx/openedx-translations/pull/1944 diff --git a/scripts/tests/test_validate_translation_files.py b/scripts/tests/test_validate_translation_files.py index 821701d5231..233f6ab9e9e 100644 --- a/scripts/tests/test_validate_translation_files.py +++ b/scripts/tests/test_validate_translation_files.py @@ -3,10 +3,11 @@ """ import os.path +import re from ..validate_translation_files import ( get_translation_files, - main, + validate_translation_files, ) SCRIPT_DIR = os.path.dirname(__file__) @@ -35,7 +36,7 @@ def test_main_on_invalid_files(capsys): Integration test for the `main` function on some invalid files. """ mock_translations_dir = os.path.join(SCRIPT_DIR, 'mock_translations_dir') - exit_code = main(mock_translations_dir) + exit_code = validate_translation_files(mock_translations_dir) out, err = capsys.readouterr() assert 'VALID:' in out, 'Valid files should be printed in stdout' @@ -44,8 +45,7 @@ def test_main_on_invalid_files(capsys): assert 'hi/LC_MESSAGES/django.po' not in out, 'Invalid file should be printed in stderr' assert 'en/LC_MESSAGES/django.po' not in out, 'Source file should not be validated' - assert 'INVALID:' in err - assert 'hi/LC_MESSAGES/django.po' in err + assert re.match(r'INVALID: .*hi/LC_MESSAGES/django.po', err) assert '\'msgstr\' is not a valid Python brace format string, unlike \'msgid\'' in err assert 'FAILURE: Some translations are invalid.' in err @@ -57,7 +57,7 @@ def test_main_on_valid_files(capsys): Integration test for the `main` function but only for the Arabic translations which is valid. """ mock_translations_dir = os.path.join(SCRIPT_DIR, 'mock_translations_dir/demo-xblock/conf/locale/ar') - exit_code = main(mock_translations_dir) + exit_code = validate_translation_files(mock_translations_dir) out, err = capsys.readouterr() assert 'VALID:' in out, 'Valid files should be printed in stdout' diff --git a/scripts/validate_translation_files.py b/scripts/validate_translation_files.py index 8d334e26656..0e51482d8a0 100644 --- a/scripts/validate_translation_files.py +++ b/scripts/validate_translation_files.py @@ -1,7 +1,15 @@ -import sys +""" +Validate translation files using GNU gettext `msgfmt` command. + +This script is used to validate translation files in the Open edX platform and mark +invalid entries as fuzzy. +""" + +import argparse import os import os.path import subprocess +import sys def get_translation_files(translation_directory): @@ -39,7 +47,9 @@ def validate_translation_file(po_file): } -def main(translations_dir='translations'): +def validate_translation_files( + translations_dir='translations', +): """ Run GNU gettext `msgfmt` and print errors to stderr. @@ -81,5 +91,14 @@ def main(translations_dir='translations'): return exit_code +def main(): # pragma: no cover + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('--dir', action='store', type=str, default='translations') + args = parser.parse_args() + sys.exit(validate_translation_files( + translations_dir=args.dir, + )) + + if __name__ == '__main__': - sys.exit(main()) # pragma: no cover + main() # pragma: no cover