codeant-ai-for-open-source[bot] commented on code in PR #41780:
URL: https://github.com/apache/superset/pull/41780#discussion_r3565313199
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -156,71 +156,104 @@ def test_bootstrap_does_not_crash_without_recaptcha_key(
assert "RECAPTCHA_PUBLIC_KEY" not in payload["conf"]
-# --- language_pack injection --------------------------------------------
+# --- language pack delivery ----------------------------------------------
#
-# The Jed pack is injected by `common_bootstrap_payload` (outside the
-# memoized `cached_common_bootstrap_data`) using the shared
-# `superset.translations.utils.get_language_pack`. Tests here cover the
-# wrapper to confirm the pack lands on the payload for non-English
-# locales and is None for English.
+# The pack is NOT embedded in the bootstrap payload; spa.html loads it via a
+# content-addressed script tag whose URL comes from
+# `get_language_pack_template_context`. A pack supplied through
+# COMMON_BOOTSTRAP_OVERRIDES_FUNC still rides the payload and suppresses the
+# script tag.
-def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+def test_common_bootstrap_payload_does_not_embed_language_pack(
app_context: None,
) -> None:
- """common.language_pack carries the shared utility's pack for non-en."""
- fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ """The payload stays small: no full pack even for non-English locales."""
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value={"locale": "fr"},
),
- patch(
- "superset.views.base.get_language_pack",
- return_value=fake_pack,
- ) as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] == fake_pack
- mock_get.assert_called_once_with("fr")
+ assert payload["language_pack"] is None
-def test_common_bootstrap_payload_skips_pack_for_english(
+def test_common_bootstrap_payload_preserves_override_pack(
app_context: None,
) -> None:
- """English short-circuits: pack is None and the utility is not called."""
+ """A pack from COMMON_BOOTSTRAP_OVERRIDES_FUNC is passed through."""
+ fake_pack: dict[str, Any] = {"domain": "superset", "locale_data":
{"superset": {}}}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
- return_value={"locale": "en"},
+ return_value={"locale": "fr", "language_pack": fake_pack},
),
- patch("superset.views.base.get_language_pack") as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
- patch("superset.views.base.get_locale", return_value="en"),
+ patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] is None
- mock_get.assert_not_called()
+ assert payload["language_pack"] == fake_pack
def test_common_bootstrap_payload_does_not_mutate_memoized_dict(
app_context: None,
) -> None:
- """Injecting language_pack must not write back into the memoize cache."""
+ """Defaulting language_pack must not write back into the memoize cache."""
cached: dict[str, Any] = {"locale": "fr"}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value=cached,
),
- patch("superset.views.base.get_language_pack", return_value={"x": 1}),
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
common_bootstrap_payload()
assert "language_pack" not in cached
+
+
+def _language_pack_context(locale: str, payload_extra: dict[str, Any]) -> Any:
+ from superset.views.base import get_language_pack_template_context
+
+ with (
+ patch(
+ "superset.views.base.get_language_pack_version",
+ return_value="abc123def456",
+ ),
+ patch(
+ "superset.views.base.url_for",
+ return_value="/language_pack/fr/abc123def456/script.js",
+ ),
+ ):
+ return get_language_pack_template_context({"locale": locale,
**payload_extra})
+
+
+def test_language_pack_template_context_versioned_src_for_non_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("fr", {})
Review Comment:
**Suggestion:** Add an explicit type annotation for this local variable so
the inferred context structure is clear to static type checkers. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a newly added local variable in Python code, and it is not type
annotated even though its value has a clear, annotatable dict-like structure.
That matches the rule requiring type hints for relevant new variables.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=31cadb70dbdb4b23aaa122e5da44ec55&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=31cadb70dbdb4b23aaa122e5da44ec55&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_bootstrap_auth.py
**Line:** 240:240
**Comment:**
*Custom Rule: Add an explicit type annotation for this local variable
so the inferred context structure is clear to static type checkers.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=f4148d8519524faea62d75f95701294eefd29990c989fe731f0dbad2f124b9c4&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=f4148d8519524faea62d75f95701294eefd29990c989fe731f0dbad2f124b9c4&reaction=dislike'>๐</a>
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -156,71 +156,104 @@ def test_bootstrap_does_not_crash_without_recaptcha_key(
assert "RECAPTCHA_PUBLIC_KEY" not in payload["conf"]
-# --- language_pack injection --------------------------------------------
+# --- language pack delivery ----------------------------------------------
#
-# The Jed pack is injected by `common_bootstrap_payload` (outside the
-# memoized `cached_common_bootstrap_data`) using the shared
-# `superset.translations.utils.get_language_pack`. Tests here cover the
-# wrapper to confirm the pack lands on the payload for non-English
-# locales and is None for English.
+# The pack is NOT embedded in the bootstrap payload; spa.html loads it via a
+# content-addressed script tag whose URL comes from
+# `get_language_pack_template_context`. A pack supplied through
+# COMMON_BOOTSTRAP_OVERRIDES_FUNC still rides the payload and suppresses the
+# script tag.
-def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+def test_common_bootstrap_payload_does_not_embed_language_pack(
app_context: None,
) -> None:
- """common.language_pack carries the shared utility's pack for non-en."""
- fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ """The payload stays small: no full pack even for non-English locales."""
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value={"locale": "fr"},
),
- patch(
- "superset.views.base.get_language_pack",
- return_value=fake_pack,
- ) as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] == fake_pack
- mock_get.assert_called_once_with("fr")
+ assert payload["language_pack"] is None
-def test_common_bootstrap_payload_skips_pack_for_english(
+def test_common_bootstrap_payload_preserves_override_pack(
app_context: None,
) -> None:
- """English short-circuits: pack is None and the utility is not called."""
+ """A pack from COMMON_BOOTSTRAP_OVERRIDES_FUNC is passed through."""
+ fake_pack: dict[str, Any] = {"domain": "superset", "locale_data":
{"superset": {}}}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
- return_value={"locale": "en"},
+ return_value={"locale": "fr", "language_pack": fake_pack},
),
- patch("superset.views.base.get_language_pack") as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
- patch("superset.views.base.get_locale", return_value="en"),
+ patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] is None
- mock_get.assert_not_called()
+ assert payload["language_pack"] == fake_pack
def test_common_bootstrap_payload_does_not_mutate_memoized_dict(
app_context: None,
) -> None:
- """Injecting language_pack must not write back into the memoize cache."""
+ """Defaulting language_pack must not write back into the memoize cache."""
cached: dict[str, Any] = {"locale": "fr"}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value=cached,
),
- patch("superset.views.base.get_language_pack", return_value={"x": 1}),
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
common_bootstrap_payload()
assert "language_pack" not in cached
+
+
+def _language_pack_context(locale: str, payload_extra: dict[str, Any]) -> Any:
+ from superset.views.base import get_language_pack_template_context
+
+ with (
+ patch(
+ "superset.views.base.get_language_pack_version",
+ return_value="abc123def456",
+ ),
+ patch(
+ "superset.views.base.url_for",
+ return_value="/language_pack/fr/abc123def456/script.js",
+ ),
+ ):
+ return get_language_pack_template_context({"locale": locale,
**payload_extra})
+
+
+def test_language_pack_template_context_versioned_src_for_non_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("fr", {})
+ assert context == {
+ "language_pack_src": "/language_pack/fr/abc123def456/script.js",
+ "language_pack_inline": False,
+ }
+
+
+def test_language_pack_template_context_none_for_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("en", {})
Review Comment:
**Suggestion:** Add a concrete type hint to this local variable assignment
to comply with required typing for relevant new variables. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This is another newly introduced local variable without an explicit type
hint, and the return value is a structured mapping that can be annotated. The
omission is a real instance of the stated typing rule violation.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=81b72529696046b3aa666e28be4f1c03&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=81b72529696046b3aa666e28be4f1c03&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_bootstrap_auth.py
**Line:** 250:250
**Comment:**
*Custom Rule: Add a concrete type hint to this local variable
assignment to comply with required typing for relevant new variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=ca455560f4cf32e464d5f44500858e9a6b96f05214783122300309257077cf0b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=ca455560f4cf32e464d5f44500858e9a6b96f05214783122300309257077cf0b&reaction=dislike'>๐</a>
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -156,71 +156,104 @@ def test_bootstrap_does_not_crash_without_recaptcha_key(
assert "RECAPTCHA_PUBLIC_KEY" not in payload["conf"]
-# --- language_pack injection --------------------------------------------
+# --- language pack delivery ----------------------------------------------
#
-# The Jed pack is injected by `common_bootstrap_payload` (outside the
-# memoized `cached_common_bootstrap_data`) using the shared
-# `superset.translations.utils.get_language_pack`. Tests here cover the
-# wrapper to confirm the pack lands on the payload for non-English
-# locales and is None for English.
+# The pack is NOT embedded in the bootstrap payload; spa.html loads it via a
+# content-addressed script tag whose URL comes from
+# `get_language_pack_template_context`. A pack supplied through
+# COMMON_BOOTSTRAP_OVERRIDES_FUNC still rides the payload and suppresses the
+# script tag.
-def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+def test_common_bootstrap_payload_does_not_embed_language_pack(
app_context: None,
) -> None:
- """common.language_pack carries the shared utility's pack for non-en."""
- fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ """The payload stays small: no full pack even for non-English locales."""
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value={"locale": "fr"},
),
- patch(
- "superset.views.base.get_language_pack",
- return_value=fake_pack,
- ) as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] == fake_pack
- mock_get.assert_called_once_with("fr")
+ assert payload["language_pack"] is None
-def test_common_bootstrap_payload_skips_pack_for_english(
+def test_common_bootstrap_payload_preserves_override_pack(
app_context: None,
) -> None:
- """English short-circuits: pack is None and the utility is not called."""
+ """A pack from COMMON_BOOTSTRAP_OVERRIDES_FUNC is passed through."""
+ fake_pack: dict[str, Any] = {"domain": "superset", "locale_data":
{"superset": {}}}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
- return_value={"locale": "en"},
+ return_value={"locale": "fr", "language_pack": fake_pack},
),
- patch("superset.views.base.get_language_pack") as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
- patch("superset.views.base.get_locale", return_value="en"),
+ patch("superset.views.base.get_locale", return_value="fr"),
):
- payload = common_bootstrap_payload()
+ payload: dict[str, Any] = common_bootstrap_payload()
- assert payload["language_pack"] is None
- mock_get.assert_not_called()
+ assert payload["language_pack"] == fake_pack
def test_common_bootstrap_payload_does_not_mutate_memoized_dict(
app_context: None,
) -> None:
- """Injecting language_pack must not write back into the memoize cache."""
+ """Defaulting language_pack must not write back into the memoize cache."""
cached: dict[str, Any] = {"locale": "fr"}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value=cached,
),
- patch("superset.views.base.get_language_pack", return_value={"x": 1}),
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
common_bootstrap_payload()
assert "language_pack" not in cached
+
+
+def _language_pack_context(locale: str, payload_extra: dict[str, Any]) -> Any:
+ from superset.views.base import get_language_pack_template_context
+
+ with (
+ patch(
+ "superset.views.base.get_language_pack_version",
+ return_value="abc123def456",
+ ),
+ patch(
+ "superset.views.base.url_for",
+ return_value="/language_pack/fr/abc123def456/script.js",
+ ),
+ ):
+ return get_language_pack_template_context({"locale": locale,
**payload_extra})
+
+
+def test_language_pack_template_context_versioned_src_for_non_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("fr", {})
+ assert context == {
+ "language_pack_src": "/language_pack/fr/abc123def456/script.js",
+ "language_pack_inline": False,
+ }
+
+
+def test_language_pack_template_context_none_for_english(
+ app_context: None,
+) -> None:
+ context = _language_pack_context("en", {})
+ assert context == {"language_pack_src": None, "language_pack_inline":
False}
+
+
+def test_language_pack_template_context_inline_for_override_pack(
+ app_context: None,
+) -> None:
+ """An operator-supplied pack suppresses the script tag; spa.html
inlines."""
+ context = _language_pack_context("fr", {"language_pack": {"domain":
"superset"}})
Review Comment:
**Suggestion:** Provide an explicit type annotation for this variable to
keep new structured values fully typed. [custom_rule]
**Severity Level:** Minor ๐งน
<details>
<summary><b>Why it matters? โญ </b></summary>
This added variable also lacks an explicit type annotation despite holding a
clearly annotatable dictionary-like result. That is consistent with the
repository rule requiring type hints on relevant new variables.
</details>
<details>
<summary><b>Rule source ๐ </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=daabcac9c2384ce5abf31f72785bff7b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=daabcac9c2384ce5abf31f72785bff7b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/views/test_bootstrap_auth.py
**Line:** 258:258
**Comment:**
*Custom Rule: Provide an explicit type annotation for this variable to
keep new structured values fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=f7587f3e96331a48515b05a0db6f4195a77be0b8ea1441db649659f8dbfa4b0d&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=f7587f3e96331a48515b05a0db6f4195a77be0b8ea1441db649659f8dbfa4b0d&reaction=dislike'>๐</a>
--
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]
