feat: extract translatable strings from the authz schema by efortish · Pull Request #509 · openedx/openedx-authz

efortish · 2026-10-01T00:21:32Z

Description

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.

Adds a management command (extract_schema_translations) that discovers every schema resource (reusing the already-merged SchemaDiscovery step from #474), parses its YAML directly, and writes a generated Python module with one pgettext() call per translatable field, for i18n_tool extract (Django's makemessages) to pick up. pgettext 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 (e.g. "Admin") stays separate too. Wired into the extract_translations Makefile target, ahead of the existing i18n_tool extract step.

Conflicts between files (the same role or permission defined twice) resolve by each document's priority, the same rule engine.schema.compilation uses: higher priority wins, identical definitions merge, and a disagreement at equal priority is a hard error. Extraction re-derives this rule itself instead of depending on the compiler, so the generated catalog matches what the compiled schema would actually serve without coupling extraction to that pipeline's internals.

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 done here. The schema loader/model/compile layer (#475, #476, #477, #478) is merged now, and the role/permission catalog API has its own PR, #510, but that endpoint currently serves the raw YAML text as-is; it doesn't yet look values up through the compiled translation catalog with pgettext. Left a comment on #432 with that split; will file a follow-up issue for points 3-4 once #510 lands.

This deliberately doesn't use the schema load/validate/compile pipeline's typed documents: it only needs raw display_name/description fields, not resolved role extensions, so it stays usable and testable against ad hoc or malformed schema directories too, not just a fully valid one.

Closes openedx-authz#432 (partially, see scope note above)

openedx-webhooks · 2026-10-01T00:21:38Z

Thanks for the pull request, @efortish!

This repository is currently maintained by @openedx/committers-openedx-authz.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where 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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

rodmgwgu · 2026-10-01T18:23:33Z

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

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".

mariajgrimaldi · 2026-10-13T18:46:45Z

+    """A schema resource could not be read or parsed for extraction."""
+
+
+def extract_messages(resources: list[DiscoveredResource] | None = None) -> list[TranslatableMessage]:


I wonder if all these methods could be part of a translatable schema class so we also encapsulate within the class what are the translatable fields.

Went with a lighter-weight version of this: the four sections now share one data table (document key, field_kind label, a function to pull the stable id) instead of repeated per-section code, since behavior is identical across all of them and only those three things differ. Stopped short of a class per section since there's no actual per-section behavior to encapsulate, just data, happy to revisit if that changes.

A lighter version of what I suggested could be having a TranslatableDocument class instead of separate classes for each section. That way, we could encapsulate the extraction process a bit more and make it easier to extend or maintain later on.

I don't want to block this further though.

mariajgrimaldi · 2026-10-13T18:53:04Z

+            # Last one wins on a context collision; in practice this only happens
+            # if two files genuinely define the same stable id, which validation
+            # (a separate step) is responsible for catching.
+            messages[extracted.context] = extracted


What would happen if there are two with the same context? Would this be overriden?

Previously: silently overridden, whichever file sorted last won, which is exactly the bug your test table caught below (the priority-100-beats-priority-200 case). Fixed now: conflicts resolve by priority the same way engine.schema.compilation does (higher wins, identical content merges, equal priority with disagreeing text raises).

Thanks for addressing this! I still wonder if we could just extract all occurrences without resolving priorities. I feel like we're adding more responsibility to the translation process than needed. Ideally, it shouldn't need to know about priorities or how schema compilation works. What do you think?

mariajgrimaldi · 2026-10-13T19:07:16Z

+def _extract_from_resource(resource: DiscoveredResource) -> list[TranslatableMessage]:
+    """Parse one resource's YAML and pull its translatable fields out."""
+    try:
+        contents = resource.read_bytes()
+    except SchemaDiscoveryError as exc:
+        raise SchemaTranslationExtractionError(str(exc)) from exc
+
+    try:
+        document = yaml.safe_load(contents)
+    except yaml.YAMLError as exc:
+        raise SchemaTranslationExtractionError(
+            f"Invalid YAML in {resource.package}:{resource.resource_path}: {exc}"
+        ) from exc


I think this is already done by discovery, no?

openedx-authz/src/openedx_authz/engine/schema/discovery.py

Line 84 in 04ccaa2

def read_bytes(self) -> bytes:

Can we reuse that somehow?

Good catch that these look like the same problem, but they're solving different ones: discovery's sort gives a deterministic order to read files in (so the same directories always produce the same list), it doesn't resolve conflicts between what two files say about the same role/permission. That's now handled explicitly by priority (see the thread below), independent of file order or discovery's sort entirely, so reusing discovery's sort wouldn't actually have fixed the bug you found.

What I wouldn't like is for us to always have to extract from resource each time we need to load the schema.

mariajgrimaldi · 2026-10-13T19:08:14Z

+        messages.extend(_messages_for_entry("permission_category", category.get("id", ""), category))
+    for permission in document.get("permissions") or []:
+        stable_id = f"{permission.get('namespace', '')}.{permission.get('name', '')}"
+        messages.extend(_messages_for_entry("permission", stable_id, permission))
+    for role in document.get("roles") or []:
+        messages.extend(_messages_for_entry("role", role.get("id", ""), role))
+    for extension in document.get("role_extensions") or []:
+        # display_name/description are optional on an extension (it may only
+        # touch permissions or the icon), so only emit the fields it actually
+        # overrides instead of a pair of empty messages for every extension.
+        messages.extend(_messages_for_entry("role_extension", extension.get("role", ""), extension, optional=True))


I think these entries should be part of a class instead. Maybe each part of the schema is a translatable component that knows how to extract itself.

Same as the other thread on this, ended up with a data table (document key, field_kind, stable-id getter) rather than a class per section, since the four sections don't actually behave differently, they just read different fields. Open to a class if that stops being true.

mariajgrimaldi · 2026-10-13T20:10:44Z

I tested the extraction end to end by adding test schemas next to the real schema through OPENEDX_AUTHZ_SCHEMA_DIRECTORIES and running make extract_translations. Then I compared every pgettext() call in the generated module with the resulting django.po.

With the current schema it generates 152 messages, which matches the YAML (17 categories, 48 permissions and 11 roles, each with a display name and a description).

These are the edge cases I tried:

Case	Exit	Result
Same display name on different permissions in the real schema ("View", "Edit", "Delete", "Manage tags")	0	Each one is a separate `.po` entry with its own context, e.g. `permission.display_name:content_libraries.view_library` and `permission.display_name:content_libraries.view_library_team` for "View"
Escaping (quotes, backslash, non-ASCII and RTL text, `%`, `{}`, leading/trailing spaces, long lines, `>-`, `>` and `\|` block scalars, same display name and description)	0	All 24 pairs match the `.po` exactly. `>` and `\|` keep a trailing `\n` in the msgid. Strings with `{...}` get the `python-brace-format` flag
Missing fields (category without `description`, role without `display_name`, permission without `namespace` or `name`)	0	Missing fields show up in the `.po` as `msgid ""` with a context. Contexts end up as `permission.display_name:.no_namespace` and `permission.display_name:test.`
YAML type coercion (`Yes`, `no`, `123`, `1.50`, `~`, a date, a list, a dict)	0	They show up in the `.po` as `"True"`, `"False"`, `"123"`, `"1.5"`, `""`, `"2026-10-13"`, `"['a', 'b']"` and `"{'key': 'value'}"`
Role extensions (only `display_name`, only `description`, both, neither, unknown role)	0	Only the fields an extension overrides show up in the `.po`. The extension for a role that doesn't exist is extracted too
`.yml`, empty, comments-only and `.txt` files	0	`.yml` is discovered and `.txt` is ignored
Same role id twice in one file	0	The second definition replaces the first, without a warning
Same permission (`namespace` + `name`) twice in one file	0	The second definition replaces the first, without a warning
Same role id in two files, one with priority 200 and one with priority 100	0	The file that sorts last by path wins, so the priority 100 file replaces the priority 200 one, without a warning
Same role id with the same text in two files	0	One `.po` entry
A role with the same id as `course_admin` in the real schema	0	The real display name and description are replaced by the test schema's, without a warning. "Course Admin" no longer appears in the `.po`
Two roles without `id`	0	Both get the contexts `role.display_name:` and `role.description:`, and the second replaces the first, without a warning
A role extension with the same id as a role	0	The extension gets its own `role_extension.` context, and both are kept
Control characters (tab, `\r`, `\x04`)	2	The generated module is valid Python, but `makemessages` fails with `CalledProcessError: Command 'django-admin makemessages -l en -v0 -d django' returned non-zero exit status 1`
An entry that isn't a mapping (`roles: [just_a_string]`)	2	`AttributeError: 'str' object has no attribute 'get'`, with no file name
A section written as a mapping instead of a list	2	Same `AttributeError`
Invalid YAML	2	`CommandError: Invalid YAML in openedx_authz:authz/schema/invalid.yaml`

None of the cases with exit 0 print a warning. It's not the end of the world if extraction doesn't validate the schema's shape, since that isn't its responsibility, but it shouldn't fail catastrophically either or generate a valid file without any warning.

I also think duplicates should be handled differently. At this point we don't know what the final schema will look like, so replacing one translation with another could make us lose context.

The test schemas and the script to run them are here if you want to reproduce any case: https://github.com/edunext/openedx-authz/tree/MJG/schema-i18n-extraction/src/openedx_authz/tests/schema/i18n_extraction_cases.

This could be used for the end-to-end testing :))

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.

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.

…ction Addresses review feedback on the extraction mechanism: - A malformed section (not a list) or entry (not a mapping) now raises SchemaTranslationExtractionError instead of an AttributeError from deep inside a .get() call on a string or int. - An entry missing its identifying field(s) (a role without id, a permission without namespace/name, etc.) now raises, instead of silently producing duplicate or anonymous contexts. - Conflicts between two documents defining the same role/permission/ category now resolve by each document's priority, the same rule engine.schema.compilation uses: higher priority wins, identical content merges, and a disagreement at equal priority raises. This replaces resolving by file path order, which didn't necessarily match the winner the compiled schema would actually serve. - A display_name/description containing gettext's own msgctxt/msgid separator byte (U+0004) now raises instead of generating a module that's valid Python but breaks msguniq once makemessages runs it. - A missing display_name/description is now skipped for every entry kind, not just role_extensions, instead of generating a useless empty pgettext() call. - The four schema sections (permission_categories, permissions, roles, role_extensions) are now driven by one data table instead of repeated per-section code, since they only differ in three things: the document key, the context's field_kind label, and how to read the entry's stable id. - Added an end-to-end test exercising the real extract_schema_translations -> makemessages pipeline, skipped where the system gettext tools it shells out to aren't installed. Also updates the module docstring and PR description: the schema loader/validate/compile pipeline this module was written to stay independent of is merged now, and the role/permission catalog API has its own PR (openedx#510).

efortish · 2026-10-14T19:22:24Z

@rodmgwgu addressing your review:

End-to-end test: added. ExtractSchemaTranslationsEndToEndTests in test_commands.py runs extract_schema_translations against the real schema, feeds the generated module to Django's actual makemessages, and asserts the exact catalog entry you suggested (msgctxt "role.display_name:course_admin" / msgid "Course Admin"). It's skipped where the system gettext tools makemessages shells out to (xgettext/msguniq) aren't installed, which they weren't in my own sandbox either; I pulled in a local copy of those binaries just to verify the test actually passes before pushing it.

CI step for extract_translations: this repo doesn't currently run makemessages/i18n_tool extract as part of CI at all (it's a manual/Transifex-Atlas-driven step elsewhere, same as compile_translations/pull_translations), so I didn't add a dedicated step for it here, that felt like a separate decision for maintainers rather than something to fold into this PR. The new e2e test above does give CI some indirect coverage of the same thing (a real makemessages run against generated output) without a dedicated workflow change.

The other four inline comments (malformed-collection crash, empty-field pgettext, missing stable id, and the stale PR-dependency comment) are fixed too, replied on each thread individually.

efortish · 2026-10-14T19:23:06Z

@mariajgrimaldi thank you for the extremely thorough edge-case pass, this caught a real correctness bug. Going through your table:

The important one: "Same role id in two files, priority 200 vs priority 100 → the priority 100 file wins because it sorts last by path." That's a genuine bug, extraction was resolving conflicts by file path order, which doesn't necessarily match the winner the compiled schema would actually serve. Fixed: conflicts now resolve by each document's priority, mirroring engine.schema.compilation's rule exactly (higher wins, identical content merges, equal priority with disagreeing text raises SchemaTranslationExtractionError). This also fixes the other duplicate-related rows in your table (same role/permission twice in one file, same role id across two files) since they all go through the same resolution now, no special-casing needed.

Control characters (tab, \r, \x04): I reproduced this directly against real makemessages/msguniq rather than guessing. Tab and \r alone are actually fine (just a non-fatal xgettext warning for \r); the fatal msguniq: context separator within string error is specifically \x04, which is gettext's own internal glue byte between msgctxt and msgid, not "control characters" generally. So rather than a broad control-character ban, I added a narrow check for that one byte in both the message text and the stable id, with a clear error instead of a cryptic downstream msguniq crash.

Non-mapping entries / sections (roles: [just_a_string], a section written as a mapping): fixed, raises SchemaTranslationExtractionError naming the resource and the offending key, instead of an AttributeError with no file name.

Missing fields (category without description, etc.): agreed this isn't extraction's job to require. Went further than "don't fail catastrophically" though: a missing field is now skipped outright rather than generating a pgettext(ctx, ""), so there's no empty-string noise in the catalog either.

Invalid YAML: unchanged, still a CommandError/SchemaTranslationExtractionError as before, that part was already working as intended.

I didn't pull in your test schemas branch directly, instead wrote focused unit tests for each scenario against the new behavior (shape validation, missing ids, priority resolution, the \x04 guard) plus the real end-to-end test rodmgwgu asked for. Your branch was genuinely useful for finding all of this though, thank you for building it.

The new end-to-end test in test_commands.py imports polib to parse the generated .po file. It was only ever present transitively through edx-i18n-tools, which is in the dev dependency group, not test, so CI's django52 and quality jobs failed importing test_commands.py with ModuleNotFoundError.

mariajgrimaldi · 2026-10-16T13:44:45Z

@rodmgwgu: to confirm, were you referring to this kind of end to end tests https://github.com/openedx/end-to-end-tests/ or unittests (simulating the extraction process)?

rodmgwgu · 2026-10-16T15:34:20Z

@rodmgwgu: to confirm, were you referring to this kind of end to end tests https://github.com/openedx/end-to-end-tests/ or unittests (simulating the extraction process)?

I was referring to unit tests exercising the extraction process.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 1, 2026

openedx-webhooks added this to Contributions Oct 1, 2026

github-project-automation Bot moved this to Needs Triage in Contributions Oct 1, 2026

efortish mentioned this pull request Oct 1, 2026

Implement extraction and translation of static YAML authz metadata (Internationalization) #432

Open

rodmgwgu requested changes Oct 1, 2026

View reviewed changes

Comment thread src/openedx_authz/engine/schema/translation.py Outdated

Comment thread src/openedx_authz/engine/schema/translation.py Outdated

Comment thread src/openedx_authz/engine/schema/translation.py Outdated

Comment thread src/openedx_authz/engine/schema/translation.py Outdated

mariajgrimaldi requested changes Oct 13, 2026

View reviewed changes

efortish added 3 commits October 14, 2026 13:39

efortish force-pushed the ks/issue-432-schema-i18n-extraction branch from 914a86b to 570592f Compare October 14, 2026 19:21

		"""A schema resource could not be read or parsed for extraction."""


		def extract_messages(resources: list[DiscoveredResource] \| None = None) -> list[TranslatableMessage]:

Conversation

efortish commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Scope: this only covers extraction (ADR 0020 points 1-2)

Uh oh!

openedx-webhooks commented Oct 1, 2026

Uh oh!

rodmgwgu commented Oct 1, 2026

Uh oh!

rodmgwgu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mariajgrimaldi commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efortish commented Oct 14, 2026

Uh oh!

efortish commented Oct 14, 2026

Uh oh!

mariajgrimaldi commented Oct 16, 2026

Uh oh!

rodmgwgu commented Oct 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

efortish commented Oct 1, 2026 •

edited

Loading

mariajgrimaldi commented Oct 13, 2026 •

edited

Loading