Conversation
ADR 0020 asks for the static schema's display_name/description fields to go through OEP-58 translation. Standard extraction tools can't see them because they live in YAML, not Python or JavaScript source. Add a management command that discovers every schema resource (reusing the already-merged SchemaDiscovery step), parses its YAML directly, and writes a generated Python module with one pgettext() call per field, for i18n_tool extract (Django's makemessages) to pick up. pgettext is used instead of plain gettext because ADR 0020 requires a field's display name and description to stay separate messages even when they're identical text; the context string also carries the field's stable id, so the same one-word name on two different roles stays separate too. Wired into the extract_translations Makefile target, ahead of the existing i18n_tool extract step. This only covers the extraction/translation-source side (ADR 0020 points 1-2). Routing the translated strings back out through an API (points 3-4) isn't buildable yet: the schema loader/model layer that would read these fields at runtime, and the role/permission catalog API itself, are both still open PRs (openedx#475-480, openedx#508), not the already-merged discovery step this extraction reuses. Left a comment on openedx-authz#432 with that split and will file a follow-up once those land. This doesn't touch the schema load/validate/compile pipeline at all (intentionally): it only needs raw display_name/description fields, not resolved role extensions or conflict handling, so it stays usable and testable independent of that still-unmerged pipeline.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @efortish! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
The management command had zero test coverage, which dropped overall project coverage and failed the codecov check on this PR. Add tests that invoke it through call_command for both the happy path (writes a valid generated module) and the failure path (a CommandError on extraction failure), and add the one remaining uncovered branch in translation.py: an empty YAML file parsing to None instead of a dict.
|
I'm not fully familiar with the usual Translation strings extraction process on other repos, but should it be run on a CI step? should that CI step be implemented here to call the make extract_translations automatically? |
rodmgwgu
left a comment
rodmgwgu
left a comment
There was a problem hiding this comment.
Thanks for this work, looking good, just added some improvements and notes.
Also, can we add an end-to-end test for the translation extraction process? Perhaps a test that runs extract_schema_translations -> makemessages and asserts a catalog entry like msgctxt "role.display_name:course_admin" / msgid "Course Admin".
|
|
||
| This generated module is never imported or executed: its only purpose is to be | ||
| valid Python source for ``makemessages`` to scan. At runtime, the API (once it | ||
| exists, see openedx-authz#432's own note that this depends on the still-unmerged |
There was a problem hiding this comment.
Nit: I think these comments on PR dependencies shouldn't be kept in code to be merged, only on PR comments, at this will become stale as soon as these are merged.
| ) | ||
|
|
||
| messages: list[TranslatableMessage] = [] | ||
| for category in document.get("permission_categories") or []: |
There was a problem hiding this comment.
On the for loops here, if for some reason the collections are malformed, it's possible that we may end up raising an exception different than SchemaTranslationExtractionError.
For example, if a file has permissions: foo , it will iterate over the characters and permission.get('namespace', '') will fail with an AttributeError.
We should catch these cases and make sure we always rise a SchemaTranslationExtractionError.
| if value is None: | ||
| if optional: | ||
| continue | ||
| value = "" |
There was a problem hiding this comment.
If we fall into this case, this will result in a pgettext(ctx, "") in the generated module, Potentially repeated several times if we have more than one missing field. Not sure what this would cause overall to the translation process.
Perhaps we should validate for this, if not here, on the render step?
| if optional: | ||
| continue | ||
| value = "" | ||
| messages.append(TranslatableMessage(context=f"{field_kind}.{field_name}:{stable_id}", message=str(value))) |
There was a problem hiding this comment.
If for some reason stable_id is missing or "", this would result in potentially duplicated context ids. I think we should validate and error out if this happens.
3 participants
Description
ADR 0020 asks for the static schema's
display_name/descriptionfields to go through OEP-58 translation. Standard extraction tools can't see them because they live in YAML, not Python or JavaScript source.Adds a management command (
extract_schema_translations) that discovers every schema resource (reusing the already-mergedSchemaDiscoverystep from #474), parses its YAML directly, and writes a generated Python module with onepgettext()call per translatable field, fori18n_tool extract(Django'smakemessages) to pick up.pgettextinstead of plaingettextbecause ADR 0020 requires a field's display name and description to stay separate messages even when they're identical text; the context string also carries the field's stable id, so the same one-word name on two different roles (e.g. "Admin") stays separate too. Wired into theextract_translationsMakefile target, ahead of the existingi18n_tool extractstep.Scope: this only covers extraction (ADR 0020 points 1-2)
Routing the translated strings back out through an API (points 3-4 of openedx-authz#432) isn't buildable yet: the schema loader/model layer that would read these fields at runtime, and the role/permission catalog API itself, are both still open PRs/ADRs (#475, #476, #477, #478, #508), not the already-merged discovery step this extraction reuses. Left a comment on #432 with that split; will file a follow-up issue for points 3-4 once the loader and catalog API are further along.
This deliberately doesn't touch the schema load/validate/compile pipeline at all: it only needs raw
display_name/descriptionfields, not resolved role extensions or conflict handling, so it stays usable and testable independent of that still-unmerged pipeline, and won't need rework when it lands with whatever shape it ends up with.Closes openedx-authz#432 (partially, see scope note above)