jbbqqf opened a new pull request, #39992: URL: https://github.com/apache/superset/pull/39992
### SUMMARY Wraps the user-facing "You need access to the following tables…" message in `SupersetSecurityManager.get_table_access_error_msg` with `flask_babel.gettext` (imported eager as `__`) so the message becomes translatable. Before this change the function returned a raw f-string, so Babel never extracted the msgid and the message remained English regardless of the request locale. Fixes apache/superset#38268 — *SQLlab - Table access error message is not translated* ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF Not applicable — the change is a code-side i18n wrapping. The visible effect (a translated message) only appears once translators populate the new msgid via the standard `scripts/translations/babel_update.sh` flow. ### TESTING INSTRUCTIONS #### Reproduce BEFORE/AFTER yourself (copy-paste) ```bash # --- one-time setup --- git clone https://github.com/apache/superset.git /tmp/repro && cd /tmp/repro python -m venv .venv && source .venv/bin/activate pip install -e '.[test]' # --- BEFORE (origin/master) --- git checkout origin/master # Use the regression test from the PR branch to demonstrate divergence: git fetch https://github.com/jbbqqf/superset.git feat/38268-i18n-table-access-error-msg git checkout FETCH_HEAD -- tests/unit_tests/security/manager_test.py pytest tests/unit_tests/security/manager_test.py::test_get_table_access_error_msg_is_translated -x # Expected: FAIL — `mocker.patch('superset.security.manager.__', ...)` raises # AttributeError because `__` does not exist in the module on master # (the message was a raw f-string). # --- AFTER (this PR) --- git checkout FETCH_HEAD pytest tests/unit_tests/security/manager_test.py::test_get_table_access_error_msg_is_translated -x # Expected: PASS — `__` is the imported gettext alias, the test patches it # and confirms it is called once with the expected msgid and # `tables` substitution. # Also verify the surrounding tests still pass with the new single-line format: pytest tests/unit_tests/security/manager_test.py -k "raise_for_access" -x # Expected: PASS. ``` #### What I ran locally - `python -m py_compile superset/security/manager.py tests/unit_tests/security/manager_test.py` → OK on both files. - `ruff check superset/security/manager.py tests/unit_tests/security/manager_test.py` → All checks passed. - The regression test `test_get_table_access_error_msg_is_translated` is designed to fail on master (no `__` symbol to patch) and pass on this branch (the gettext alias exists and is invoked exactly once with the expected msgid). - Two pre-existing tests asserted the old f-string verbatim with embedded newline + 12 spaces of indentation: - `tests/unit_tests/security/manager_test.py:402-406` (`test_raise_for_access_query_default_schema`) - `tests/unit_tests/security/manager_test.py:1106-1119` (`test_raise_for_access_query_with_catalog`) - Both updated to assert the new single-line format produced by the gettext template. #### Translation extraction After this lands, maintainers should run `scripts/translations/babel_update.sh` to extract the new msgid `"You need access to the following tables: %(tables)s, ..."` into the `.pot` template and `messages.po` files for each locale. This PR does not regenerate translation files; that is an out-of-band release task. ### ADDITIONAL INFORMATION | # | Scenario | Input | Expected | Verified by | |---|----------|-------|----------|-------------| | 1 | English locale | denied table `public.ab_user` | message returns English string with backticked table | `test_raise_for_access_query_default_schema` | | 2 | Multiple denied tables | set with two tables | tables joined with `, ` and substituted into `%(tables)s` | new `test_get_table_access_error_msg_is_translated` | | 3 | Translation hook present | gettext patched | `__` invoked exactly once with `%(tables)s` msgid and `tables` kwarg | new `test_get_table_access_error_msg_is_translated` | - [x] Has associated issue: #38268 - [ ] Required feature flags: - [ ] Changes UI (the user-facing string format goes from multi-line padded to single-line, but the visible behavior in error toasts is identical word-for-word) - [ ] Includes DB Migration - [ ] Introduces new feature or API - [ ] Removes existing feature or API #### Risk / blast radius The format change is from a multi-line f-string with 12 spaces of leading whitespace on the second line to a single-line string. Any downstream code that exact-matches the old whitespace would need updating. A grep across `superset/` and `tests/` found: - `tests/unit_tests/security/manager_test.py` — three assertions; updated. - `tests/unit_tests/extensions/test_sqlalchemy.py:307,324` — these test sites construct the message themselves to mock a security_manager raise; they don't call `get_table_access_error_msg`, so they continue to exercise their own injected message and remain green. No other production code parses or depends on the exact whitespace of the message. ```release-note The "You need access to the following tables…" SQL Lab access-denied error message is now translatable. ``` --- *PR drafted with assistance from Claude Code. The change was reviewed manually against apache/superset's source and matches the i18n pattern used elsewhere in the codebase (e.g. `superset/sql_lab.py`, `superset/connectors/sqla/models.py`). The reproducer block above is the same one used during development.* -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
